From: Tom Lane <tgl@sss.pgh.pa.us>
To: peter.eisentraut@enterprisedb.com
cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
 Shinya11.Kato@oss.nttdata.com,
        masao.fujii@oss.nttdata.com, bharath.rupireddyforpostgres@gmail.com,
        daniel@yesql.se, pgsql-hackers@postgresql.org
Subject: Re: Emit a warning if the extension's GUC is set incorrectly
In-reply-to: <1640909.1640638123@sss.pgh.pa.us>
References: <9f5e96ca44891915f1d2736bea9fdf45@oss.nttdata.com>
 <83b33009-3a1b-9230-8f07-2115763c1853@enterprisedb.com>
 <c5660e836dd511549810fd35b12edc3c@oss.nttdata.com>
 <20211221.100834.1297468238807011575.horikyota.ntt@gmail.com>
 <57009.1640107405@sss.pgh.pa.us> <116024.1640111629@sss.pgh.pa.us>
 <1640909.1640638123@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Mon, 27 Dec 2021 15:48:43 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <1902102.1640711137.0@sss.pgh.pa.us>
Date: Tue, 28 Dec 2021 12:06:55 -0500
Message-ID: <1902182.1640711215@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1902182.1640711215%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1902102.1640711137.1@sss.pgh.pa.us>

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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="delete-disallowed-placeholders.patch";
	charset="us-ascii"
Content-ID: <1902102.1640711137.2@sss.pgh.pa.us>
Content-Description: delete-disallowed-placeholders.patch
Content-Transfer-Encoding: quoted-printable

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 =3D 0; i < num_guc_variables; i++)
 	{
 		struct config_generic *var =3D guc_variables[i];
@@ -9384,9 +9389,14 @@ MarkGUCPrefixReserved(const char *className)
 			var->name[classLen] =3D=3D 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 =3D 42;  -- allowed if plpgsql is not loaded ye=
t
-LOAD 'plpgsql';  -- this will now warn about it
-WARNING:  unrecognized configuration parameter "plpgsql.bogus_setting"
-SET plpgsql.extra_foo_warnings =3D false;  -- but now, it's an error
+SET plpgsql.extra_foo_warnings =3D true;  -- allowed if plpgsql is not lo=
aded yet
+LOAD 'plpgsql';  -- this will throw a warning and delete the variable
+WARNING:  invalid configuration parameter name "plpgsql.extra_foo_warning=
s", removing it
+DETAIL:  "plpgsql" is now a reserved prefix.
+SET plpgsql.extra_foo_warnings =3D 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 =3D 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 =3D 42;  -- allowed if plpgsql is not loaded ye=
t
-LOAD 'plpgsql';  -- this will now warn about it
-SET plpgsql.extra_foo_warnings =3D false;  -- but now, it's an error
+SET plpgsql.extra_foo_warnings =3D true;  -- allowed if plpgsql is not lo=
aded yet
+LOAD 'plpgsql';  -- this will throw a warning and delete the variable
+SET plpgsql.extra_foo_warnings =3D true;  -- now, it's an error
 SHOW plpgsql.extra_foo_warnings;
-SET plpgsql.bogus_setting =3D 43;  -- you can still use the pre-existing =
variable
-SHOW plpgsql.bogus_setting;
 =

 --
 -- Test DISCARD TEMP

------- =_aaaaaaaaaa0--