Rethink handling of settings with a prefix reserved by an extension.

tglsfdc · tglsfdc · commit 2ed8a8cc5b63 · 2021-12-27T14:35:50.000-05:00
Commit 75d2206 made SET print a warning if you tried to set an unrecognized parameter within namespace previously reserved by an extension. It seems better for that to be an outright error though, for the same reason that we don't let you set unrecognized unqualified parameter names. In any case, the preceding implementation was inefficient and erroneous. Perform the check in a more appropriate spot, and be more careful about prefix-match cases. Discussion: https://postgr.es/m/116024.1640111629@sss.pgh.pa.us
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
@@ -148,6 +148,8 @@ extern bool optimize_bounded_sort;
 
 static int	GUC_check_errcode_value;
 
+static List *reserved_class_prefix = NIL;
+
 /* global variables for check hook support */
 char	   *GUC_check_errmsg_string;
 char	   *GUC_check_errdetail_string;
@@ -235,8 +237,6 @@ static bool check_recovery_target_lsn(char **newval, void **extra, GucSource sou
 static void assign_recovery_target_lsn(const char *newval, void *extra);
 static bool check_primary_slot_name(char **newval, void **extra, GucSource source);
 static bool check_default_with_oids(bool *newval, void **extra, GucSource source);
-static void check_reserved_prefixes(const char *varName);
-static List *reserved_class_prefix = NIL;
 
 /* Private functions in guc-file.l that need to be called from guc.c */
 static ConfigVariable *ProcessConfigFileInternal(GucContext context,
@@ -5569,18 +5569,44 @@ find_option(const char *name, bool create_placeholders, bool skip_errors,
 		 * doesn't contain a separator, don't assume that it was meant to be a
 		 * placeholder.
 		 */
-		if (strchr(name, GUC_QUALIFIER_SEPARATOR) != NULL)
+		const char *sep = strchr(name, GUC_QUALIFIER_SEPARATOR);
+
+		if (sep != NULL)
 		{
-			if (valid_custom_variable_name(name))
-				return add_placeholder_variable(name, elevel);
-			/* A special error message seems desirable here */
-			if (!skip_errors)
-				ereport(elevel,
-						(errcode(ERRCODE_INVALID_NAME),
-						 errmsg("invalid configuration parameter name \"%s\"",
-								name),
-						 errdetail("Custom parameter names must be two or more simple identifiers separated by dots.")));
-			return NULL;
+			size_t		classLen = sep - name;
+			ListCell   *lc;
+
+			/* The name must be syntactically acceptable ... */
+			if (!valid_custom_variable_name(name))
+			{
+				if (!skip_errors)
+					ereport(elevel,
+							(errcode(ERRCODE_INVALID_NAME),
+							 errmsg("invalid configuration parameter name \"%s\"",
+									name),
+							 errdetail("Custom parameter names must be two or more simple identifiers separated by dots.")));
+				return NULL;
+			}
+			/* ... and it must not match any previously-reserved prefix */
+			foreach(lc, reserved_class_prefix)
+			{
+				const char *rcprefix = lfirst(lc);
+
+				if (strlen(rcprefix) == classLen &&
+					strncmp(name, rcprefix, classLen) == 0)
+				{
+					if (!skip_errors)
+						ereport(elevel,
+								(errcode(ERRCODE_INVALID_NAME),
+								 errmsg("invalid configuration parameter name \"%s\"",
+										name),
+								 errdetail("\"%s\" is a reserved prefix.",
+										   rcprefix)));
+					return NULL;
+				}
+			}
+			/* OK, create it */
+			return add_placeholder_variable(name, elevel);
 		}
 	}
 
@@ -8764,7 +8790,6 @@ ExecSetVariableStmt(VariableSetStmt *stmt, bool isTopLevel)
 									 (superuser() ? PGC_SUSET : PGC_USERSET),
 									 PGC_S_SESSION,
 									 action, true, 0, false);
-			check_reserved_prefixes(stmt->name);
 			break;
 		case VAR_SET_MULTI:
 
@@ -8850,8 +8875,6 @@ ExecSetVariableStmt(VariableSetStmt *stmt, bool isTopLevel)
 									 (superuser() ? PGC_SUSET : PGC_USERSET),
 									 PGC_S_SESSION,
 									 action, true, 0, false);
-
-			check_reserved_prefixes(stmt->name);
 			break;
 		case VAR_RESET_ALL:
 			ResetAllOptions();
@@ -9345,8 +9368,9 @@ EmitWarningsOnPlaceholders(const char *className)
 {
 	int			classLen = strlen(className);
 	int			i;
-	MemoryContext	oldcontext;
+	MemoryContext oldcontext;
 
+	/* Check for existing placeholders. */
 	for (i = 0; i < num_guc_variables; i++)
 	{
 		struct config_generic *var = guc_variables[i];
@@ -9362,48 +9386,12 @@ EmitWarningsOnPlaceholders(const char *className)
 		}
 	}
 
+	/* And remember the name so we can prevent future mistakes. */
 	oldcontext = MemoryContextSwitchTo(TopMemoryContext);
 	reserved_class_prefix = lappend(reserved_class_prefix, pstrdup(className));
 	MemoryContextSwitchTo(oldcontext);
 }
 
-/*
- * Check a setting name against prefixes previously reserved by
- * EmitWarningsOnPlaceholders() and throw a warning if matching.
- */
-static void
-check_reserved_prefixes(const char *varName)
-{
-	char	   *sep = strchr(varName, GUC_QUALIFIER_SEPARATOR);
-
-	if (sep)
-	{
-		size_t		classLen = sep - varName;
-		ListCell   *lc;
-
-		foreach(lc, reserved_class_prefix)
-		{
-			char	   *rcprefix = lfirst(lc);
-
-			if (strncmp(varName, rcprefix, classLen) == 0)
-			{
-				for (int i = 0; i < num_guc_variables; i++)
-				{
-					struct config_generic *var = guc_variables[i];
-
-					if ((var->flags & GUC_CUSTOM_PLACEHOLDER) != 0 &&
-						strcmp(varName, var->name) == 0)
-					{
-						ereport(WARNING,
-								(errcode(ERRCODE_UNDEFINED_OBJECT),
-								 errmsg("unrecognized configuration parameter \"%s\"", var->name),
-								 errdetail("\"%.*s\" is a reserved prefix.", (int) classLen, var->name)));
-					}
-				}
-			}
-		}
-	}
-}
 
 /*
  * SHOW command
diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
@@ -548,6 +548,23 @@ ERROR:  invalid configuration parameter name "special.weird name"
 DETAIL:  Custom parameter names must be two or more simple identifiers separated by dots.
 SHOW special."weird name";
 ERROR:  unrecognized configuration parameter "special.weird name"
+-- Check what happens when you try to set a "custom" GUC within the
+-- namespace of an extension.
+SET plpgsql.bogus_setting = 42;  -- allowed if plpgsql is not loaded yet
+LOAD 'plpgsql';  -- this will now warn about it
+WARNING:  unrecognized configuration parameter "plpgsql.bogus_setting"
+SET plpgsql.extra_foo_warnings = false;  -- but now, it's an error
+ERROR:  invalid configuration parameter name "plpgsql.extra_foo_warnings"
+DETAIL:  "plpgsql" is a reserved prefix.
+SHOW plpgsql.extra_foo_warnings;
+ERROR:  unrecognized configuration parameter "plpgsql.extra_foo_warnings"
+SET plpgsql.bogus_setting = 43;  -- you can still use the pre-existing variable
+SHOW plpgsql.bogus_setting;
+ plpgsql.bogus_setting 
+-----------------------
+ 43
+(1 row)
+
 --
 -- Test DISCARD TEMP
 --
@@ -813,22 +830,3 @@ set default_with_oids to f;
 -- Should not allow to set it to true.
 set default_with_oids to t;
 ERROR:  tables declared WITH OIDS are not supported
--- test SET unrecognized parameter
-SET foo = false;  -- no such setting
-ERROR:  unrecognized configuration parameter "foo"
--- test setting a parameter with a registered prefix (plpgsql)
-SET plpgsql.extra_foo_warnings = false;  -- no such setting
-WARNING:  unrecognized configuration parameter "plpgsql.extra_foo_warnings"
-DETAIL:  "plpgsql" is a reserved prefix.
-SHOW plpgsql.extra_foo_warnings;  -- but the parameter is set
- plpgsql.extra_foo_warnings 
-----------------------------
- false
-(1 row)
-
--- cleanup
-RESET foo;
-ERROR:  unrecognized configuration parameter "foo"
-RESET plpgsql.extra_foo_warnings;
-WARNING:  unrecognized configuration parameter "plpgsql.extra_foo_warnings"
-DETAIL:  "plpgsql" is a reserved prefix.
diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql
@@ -163,6 +163,15 @@ SHOW custom."bad-guc";
 SET special."weird name" = 'foo';  -- could be allowed, but we choose not to
 SHOW special."weird name";
 
+-- Check what happens when you try to set a "custom" GUC within the
+-- namespace of an extension.
+SET plpgsql.bogus_setting = 42;  -- allowed if plpgsql is not loaded yet
+LOAD 'plpgsql';  -- this will now warn about it
+SET plpgsql.extra_foo_warnings = false;  -- but now, it's an error
+SHOW plpgsql.extra_foo_warnings;
+SET plpgsql.bogus_setting = 43;  -- you can still use the pre-existing variable
+SHOW plpgsql.bogus_setting;
+
 --
 -- Test DISCARD TEMP
 --
@@ -311,14 +320,3 @@ reset check_function_bodies;
 set default_with_oids to f;
 -- Should not allow to set it to true.
 set default_with_oids to t;
-
--- test SET unrecognized parameter
-SET foo = false;  -- no such setting
-
--- test setting a parameter with a registered prefix (plpgsql)
-SET plpgsql.extra_foo_warnings = false;  -- no such setting
-SHOW plpgsql.extra_foo_warnings;  -- but the parameter is set
-
--- cleanup
-RESET foo;
-RESET plpgsql.extra_foo_warnings;
