From: Tom Lane <tgl@sss.pgh.pa.us>
To: peter.eisentraut@enterprisedb.com
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: Shinya11.Kato@oss.nttdata.com
Cc: masao.fujii@oss.nttdata.com
Cc: bharath.rupireddyforpostgres@gmail.com
Cc: daniel@yesql.se
Cc: pgsql-hackers@postgresql.org
Subject: Re: Emit a warning if the extension's GUC is set incorrectly
Date: Tue, 28 Dec 2021 12:06:55 -0500
Message-ID: <1902182.1640711215@sss.pgh.pa.us> (raw)
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>
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
Attachments:delete-disallowed-placeholders.patch (text/x-diff, 4.1K) downloaddiff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.cindex 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.outindex 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.sqlindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, peter.eisentraut@enterprisedb.com, horikyota.ntt@gmail.com, Shinya11.Kato@oss.nttdata.com, masao.fujii@oss.nttdata.com, bharath.rupireddyforpostgres@gmail.com, daniel@yesql.se
Subject: Re: Emit a warning if the extension's GUC is set incorrectly
In-Reply-To: <1902182.1640711215@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox