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

Commit 75d22069e 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
index bff949a40bc4a28f822c503fa6d9417de6a3dcf6..f345eceb5b7b5c19ba47f92915bb98fe3e81e811 100644 (file)
--- 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
index 2433038c3ec492a885138f4262a883c063745fc5..5ad7477f618348b63547e9abc8cf9b8a2409cfb1 100644 (file)
--- 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
index b57758ed273c7a987b756641f0421a01abe63c47..f97f4e44884bb4b2ac531c8fecb760cb6b40634c 100644 (file)
--- 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;