Re: Emit a warning if the extension's GUC is set incorrectly - Mailing list pgsql-hackers

I wrote:
> As a stopgap to turn the farm green again, I am going to revert
> 75d22069e as well as my followup patches.  If we don't want to
> give up on that idea altogether, we have to find some way to
> suppress the chatter from parallel workers.  I wonder whether
> it would be appropriate to go further than we have, and actively
> delete placeholders that turn out to be within an extension's
> reserved namespace.  The core issue here is that workers don't
> necessarily set GUCs and load extensions in the same order that
> their parent did, so if we leave any invalid placeholders behind
> after reserving an extension's prefix, we're risking issues
> during worker start.

Here's a delta patch (meant to be applied after reverting cab5b9ab2)
that does things like that.  It fixes the parallel-mode problem ...
so do we want to tighten things up this much?

            regards, tom lane

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index d239004347..fec535283c 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -9362,8 +9362,8 @@ DefineCustomEnumVariable(const char *name,
 /*
  * Mark the given GUC prefix as "reserved".
  *
- * This prints warnings if there are any existing placeholders matching
- * the prefix, and then prevents new ones from being created.
+ * 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.
  */
@@ -9374,7 +9374,12 @@ MarkGUCPrefixReserved(const char *className)
     int            i;
     MemoryContext oldcontext;

-    /* Check for existing placeholders. */
+    /*
+     * 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];
@@ -9384,9 +9389,14 @@ MarkGUCPrefixReserved(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 *));
         }
     }

diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
index 5ad7477f61..60998ee644 100644
--- a/src/test/regress/expected/guc.out
+++ b/src/test/regress/expected/guc.out
@@ -550,21 +550,15 @@ 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
+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"
-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
 --
diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql
index f97f4e4488..63ab2d9be6 100644
--- a/src/test/regress/sql/guc.sql
+++ b/src/test/regress/sql/guc.sql
@@ -165,12 +165,10 @@ 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
+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;
-SET plpgsql.bogus_setting = 43;  -- you can still use the pre-existing variable
-SHOW plpgsql.bogus_setting;

 --
 -- Test DISCARD TEMP