Disallow setting bogus GUCs within an extension's reserved namespace.

Commit 75d22069e tried to throw a warning for setting a custom GUC whose
prefix belongs to a previously-loaded extension, if there is no such GUC
defined by the extension.  But that caused unstable behavior with
parallel workers, because workers don't necessarily load extensions and
GUCs in the same order their leader did.  To make that work safely, we
have to completely disallow the case.  We now actually remove any such
GUCs at the time of initial extension load, and then throw an error not
just a warning if you try to add one later.  While this might create a
compatibility issue for a few people, the improvement in error-detection
capability seems worth it; it's hard to believe that there's any good
use-case for choosing such GUC names.

This also un-reverts 5609cc01c (Rename EmitWarningsOnPlaceholders() to
MarkGUCPrefixReserved()), since that function's old name is now even
more of a misnomer.

Florin Irion and Tom Lane

Discussion: https://postgr.es/m/1902182.1640711215@sss.pgh.pa.us

diff --git a/contrib/auth_delay/auth_delay.c b/contrib/auth_delay/auth_delay.c
index 38f4276db39660dc8551e715671d4c02ebbb285e..6b94d653ea4f86e6685563c7039b9ed841b437d5 100644 (file)
--- a/contrib/auth_delay/auth_delay.c
+++ b/contrib/auth_delay/auth_delay.c
@@ -68,7 +68,7 @@ _PG_init(void)
                            NULL,
                            NULL);
 
-   EmitWarningsOnPlaceholders("auth_delay");
+   MarkGUCPrefixReserved("auth_delay");
 
    /* Install Hooks */
    original_client_auth_hook = ClientAuthentication_hook;

diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c
index 3e09abaecac972d68b605acc34c88eb0300e8faa..d3029f85efe4488e47fe05ca466d722b8f2e2afe 100644 (file)
--- a/contrib/auto_explain/auto_explain.c
+++ b/contrib/auto_explain/auto_explain.c
@@ -231,7 +231,7 @@ _PG_init(void)
                             NULL,
                             NULL);
 
-   EmitWarningsOnPlaceholders("auto_explain");
+   MarkGUCPrefixReserved("auto_explain");
 
    /* Install hooks. */
    prev_ExecutorStart = ExecutorStart_hook;

diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c
index 16ddddccbbc4b427497651f3751b85d4b1d2cfd9..e7efbfb9c34f5d1e3ef230ce56c4198d7ecae721 100644 (file)
--- a/contrib/basic_archive/basic_archive.c
+++ b/contrib/basic_archive/basic_archive.c
@@ -69,7 +69,7 @@ _PG_init(void)
                               0,
                               check_archive_directory, NULL, NULL);
 
-   EmitWarningsOnPlaceholders("basic_archive");
+   MarkGUCPrefixReserved("basic_archive");
 
    basic_archive_context = AllocSetContextCreate(TopMemoryContext,
                                                  "basic_archive",

diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index 1d4d74b171f16debc76e615fcffba18b73c23b62..45e012a63a55d48858145a620457ed33de163a24 100644 (file)
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -137,7 +137,7 @@ _PG_init(void)
                             NULL,
                             NULL);
 
-   EmitWarningsOnPlaceholders("pg_prewarm");
+   MarkGUCPrefixReserved("pg_prewarm");
 
    RequestAddinShmemSpace(MAXALIGN(sizeof(AutoPrewarmSharedState)));
 

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 9d7d0812ac36b9cec44b72a64ae069b92c1878ef..38d92a89cc42388e4427737416959f8cee240b7e 100644 (file)
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -437,7 +437,7 @@ _PG_init(void)
                             NULL,
                             NULL);
 
-   EmitWarningsOnPlaceholders("pg_stat_statements");
+   MarkGUCPrefixReserved("pg_stat_statements");
 
    /*
     * Request additional shared resources.  (These are no-ops if we're not in

diff --git a/contrib/pg_trgm/trgm_op.c b/contrib/pg_trgm/trgm_op.c
index 0407c7dd64472e955c1f465d9f91730807ecd7d4..e9b7981619fb630e7df636b29f4ea5f3700d47d1 100644 (file)
--- a/contrib/pg_trgm/trgm_op.c
+++ b/contrib/pg_trgm/trgm_op.c
@@ -101,7 +101,7 @@ _PG_init(void)
                             NULL,
                             NULL);
 
-   EmitWarningsOnPlaceholders("pg_trgm");
+   MarkGUCPrefixReserved("pg_trgm");
 }
 
 /*

diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
index af38e956e70fcdd14ea031a4624c25fbc0caad1b..2c6b2894b963e7d979cd111b587e863591a82a37 100644 (file)
--- a/contrib/postgres_fdw/option.c
+++ b/contrib/postgres_fdw/option.c
@@ -538,5 +538,5 @@ _PG_init(void)
                               NULL,
                               NULL);
 
-   EmitWarningsOnPlaceholders("postgres_fdw");
+   MarkGUCPrefixReserved("postgres_fdw");
 }

diff --git a/contrib/sepgsql/hooks.c b/contrib/sepgsql/hooks.c
index d71c802106a6e3aed0da0f66cf84d242d14d9b1d..97e61b8043fe7ad93384bf5ab0763423422955f1 100644 (file)
--- a/contrib/sepgsql/hooks.c
+++ b/contrib/sepgsql/hooks.c
@@ -455,7 +455,7 @@ _PG_init(void)
                             NULL,
                             NULL);
 
-   EmitWarningsOnPlaceholders("sepgsql");
+   MarkGUCPrefixReserved("sepgsql");
 
    /* Initialize userspace access vector cache */
    sepgsql_avc_init();

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 01f373815e0a465f298890dc5b5b4118096e7fd9..eaa4bf2c30405c9bf51e79cee67477198262d885 100644 (file)
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -150,6 +150,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;
@@ -5590,18 +5592,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);
        }
    }
 
@@ -9355,15 +9383,26 @@ DefineCustomEnumVariable(const char *name,
 }
 
 /*
+ * Mark the given GUC prefix as "reserved".
+ *
+ * This deletes any existing placeholders matching the prefix,
+ * and then prevents new ones from being created.
  * Extensions should call this after they've defined all of their custom
  * GUCs, to help catch misspelled config-file entries.
  */
 void
-EmitWarningsOnPlaceholders(const char *className)
+MarkGUCPrefixReserved(const char *className)
 {
    int         classLen = strlen(className);
    int         i;
+   MemoryContext oldcontext;
 
+   /*
+    * Check for existing placeholders.  We must actually remove invalid
+    * placeholders, else future parallel worker startups will fail.  (We
+    * don't bother trying to free associated memory, since this shouldn't
+    * happen often.)
+    */
    for (i = 0; i < num_guc_variables; i++)
    {
        struct config_generic *var = guc_variables[i];
@@ -9373,11 +9412,21 @@ EmitWarningsOnPlaceholders(const char *className)
            var->name[classLen] == GUC_QUALIFIER_SEPARATOR)
        {
            ereport(WARNING,
-                   (errcode(ERRCODE_UNDEFINED_OBJECT),
-                    errmsg("unrecognized configuration parameter \"%s\"",
-                           var->name)));
+                   (errcode(ERRCODE_INVALID_NAME),
+                    errmsg("invalid configuration parameter name \"%s\", removing it",
+                           var->name),
+                    errdetail("\"%s\" is now a reserved prefix.",
+                              className)));
+           num_guc_variables--;
+           memmove(&guc_variables[i], &guc_variables[i + 1],
+                   (num_guc_variables - i) * sizeof(struct config_generic *));
        }
    }
+
+   /* And remember the name so we can prevent future mistakes. */
+   oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+   reserved_class_prefix = lappend(reserved_class_prefix, pstrdup(className));
+   MemoryContextSwitchTo(oldcontext);
 }
 
 

diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index f1bfe79feb81bf6ed6bafc81390452e587cdf538..ea774968f07615bf9adcb89e39dbaf527ae26c2d 100644 (file)
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -354,7 +354,10 @@ extern void DefineCustomEnumVariable(const char *name,
                                     GucEnumAssignHook assign_hook,
                                     GucShowHook show_hook);
 
-extern void EmitWarningsOnPlaceholders(const char *className);
+extern void MarkGUCPrefixReserved(const char *className);
+
+/* old name for MarkGUCPrefixReserved, for backwards compatibility: */
+#define EmitWarningsOnPlaceholders(className) MarkGUCPrefixReserved(className)
 
 extern const char *GetConfigOption(const char *name, bool missing_ok,
                                   bool restrict_privileged);

diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index 3f785b1e8d5a522e22004f66367ec3995a07fd43..b5879c294715ba354ab3c112818050433db77b5a 100644 (file)
--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -455,7 +455,7 @@ _PG_init(void)
                               PGC_SUSET, 0,
                               NULL, NULL, NULL);
 
-   EmitWarningsOnPlaceholders("plperl");
+   MarkGUCPrefixReserved("plperl");
 
    /*
     * Create hash tables.

diff --git a/src/pl/plpgsql/src/pl_handler.c b/src/pl/plpgsql/src/pl_handler.c
index b4b85092806268bd4991d8b5907bbd5a246ccdc0..190d286f1c8ddf8a5a0572ca0a4bf8a6e0b31eba 100644 (file)
--- a/src/pl/plpgsql/src/pl_handler.c
+++ b/src/pl/plpgsql/src/pl_handler.c
@@ -197,7 +197,7 @@ _PG_init(void)
                               plpgsql_extra_errors_assign_hook,
                               NULL);
 
-   EmitWarningsOnPlaceholders("plpgsql");
+   MarkGUCPrefixReserved("plpgsql");
 
    plpgsql_HashTableInit();
    RegisterXactCallback(plpgsql_xact_cb, NULL);

diff --git a/src/pl/tcl/pltcl.c b/src/pl/tcl/pltcl.c
index 7c045f4560713af3491e11829cd1c7714649b7fc..ab759833db11128cad4a2d84c849cbaa163bcd38 100644 (file)
--- a/src/pl/tcl/pltcl.c
+++ b/src/pl/tcl/pltcl.c
@@ -474,8 +474,8 @@ _PG_init(void)
                               PGC_SUSET, 0,
                               NULL, NULL, NULL);
 
-   EmitWarningsOnPlaceholders("pltcl");
-   EmitWarningsOnPlaceholders("pltclu");
+   MarkGUCPrefixReserved("pltcl");
+   MarkGUCPrefixReserved("pltclu");
 
    pltcl_pm_init_done = true;
 }

diff --git a/src/test/modules/delay_execution/delay_execution.c b/src/test/modules/delay_execution/delay_execution.c
index ad50383bf8ae8ae8cc89960263be01f5b19fa8f1..cf34e8c2d7d9aefa1f03ba3999d1207a99df02c6 100644 (file)
--- a/src/test/modules/delay_execution/delay_execution.c
+++ b/src/test/modules/delay_execution/delay_execution.c
@@ -91,7 +91,7 @@ _PG_init(void)
                            NULL,
                            NULL);
 
-   EmitWarningsOnPlaceholders("delay_execution");
+   MarkGUCPrefixReserved("delay_execution");
 
    /* Install our hook */
    prev_planner_hook = planner_hook;

diff --git a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c
index 3ba33e501c3c6fedd0b087584525d1af0b6d7b79..7c469fd57e885768429da403b9ecfcf8ed1096ef 100644 (file)
--- a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c
+++ b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c
@@ -49,7 +49,7 @@ _PG_init(void)
                               NULL,
                               NULL);
 
-   EmitWarningsOnPlaceholders("ssl_passphrase");
+   MarkGUCPrefixReserved("ssl_passphrase");
 
    if (ssl_passphrase)
        openssl_tls_init_hook = set_rot13;

diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
index 05ced63780e6b11fd2f6187152900c0ddb52e7cb..48829df29c35a61ac86e8c8bd714d43fe05d56d4 100644 (file)
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -322,7 +322,7 @@ _PG_init(void)
                               0,
                               NULL, NULL, NULL);
 
-   EmitWarningsOnPlaceholders("worker_spi");
+   MarkGUCPrefixReserved("worker_spi");
 
    /* set up common data for all our workers */
    memset(&worker, 0, sizeof(worker));

diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
index 75b6bfbf116ae8d7de77b2aa13da597e871ec03f..3de6404ba5bcfccdbeee2bd843ab9b3a332fd856 100644 (file)
--- a/src/test/regress/expected/guc.out
+++ b/src/test/regress/expected/guc.out
@@ -548,6 +548,17 @@ 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.extra_foo_warnings = true;  -- allowed if plpgsql is not loaded yet
+LOAD 'plpgsql';  -- this will throw a warning and delete the variable
+WARNING:  invalid configuration parameter name "plpgsql.extra_foo_warnings", removing it
+DETAIL:  "plpgsql" is now a reserved prefix.
+SET plpgsql.extra_foo_warnings = true;  -- 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"
 --
 -- Test DISCARD TEMP
 --

diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql
index 3e2819449c26e31eef945c79c0a77207f9752f69..d5db101e4865d7b53f5d0546032083231af08661 100644 (file)
--- a/src/test/regress/sql/guc.sql
+++ b/src/test/regress/sql/guc.sql
@@ -163,6 +163,13 @@ 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.extra_foo_warnings = true;  -- allowed if plpgsql is not loaded yet
+LOAD 'plpgsql';  -- this will throw a warning and delete the variable
+SET plpgsql.extra_foo_warnings = true;  -- now, it's an error
+SHOW plpgsql.extra_foo_warnings;
+
 --
 -- Test DISCARD TEMP
 --