Обсуждение: Emit a warning if the extension's GUC is set incorrectly

Hi hackers,

If wrong GUCs of auth_delay, pg_trgm, postgres_fdw and sepgsql are 
described in postgresql.conf, a warning is not emitted unlike 
pg_stat_statements, auto_explain and pg_prewarm.
So, I improved it by adding EmitWarningsOnPlaceholders.
An example output is shown below.
---
2021-11-14 18:18:16.486 JST [487067] WARNING:  unrecognized 
configuration parameter "auth_delay.xxx"
---

What do you think?

-- 
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

> On 14 Nov 2021, at 11:03, Shinya Kato <Shinya11.Kato@oss.nttdata.com> wrote:

> If wrong GUCs of auth_delay, pg_trgm, postgres_fdw and sepgsql are described in postgresql.conf, a warning is not
emittedunlike pg_stat_statements, auto_explain and pg_prewarm. 
> So, I improved it by adding EmitWarningsOnPlaceholders.
> An example output is shown below.
> ---
> 2021-11-14 18:18:16.486 JST [487067] WARNING:  unrecognized configuration parameter "auth_delay.xxx"
> ---
>
> What do you think?

Seems reasonable on a quick skim, commit da2c1b8a2 did a similar roundup back
in 2009 but at the time most of these didn't exist (pg_trgm did but didn't have
custom option back then).  There is one additional callsite defining custom
variables in src/pl/tcl/pltcl.c which probably should get this treatment as
well, it would align it with the pl/perl counterpart.

I'll have a closer look and test tomorrow.

--
Daniel Gustafsson        https://vmware.com/

On 2021-11-15 04:50, Daniel Gustafsson wrote:
> Seems reasonable on a quick skim, commit da2c1b8a2 did a similar 
> roundup back
> in 2009 but at the time most of these didn't exist (pg_trgm did but 
> didn't have
> custom option back then).  There is one additional callsite defining 
> custom
> variables in src/pl/tcl/pltcl.c which probably should get this 
> treatment as
> well, it would align it with the pl/perl counterpart.
> 
> I'll have a closer look and test tomorrow.
Thank you for the review!
I have missed src/pl/tcl/pltcl.c, so I created the new patch.

-- 
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Mon, Nov 15, 2021 at 6:33 AM Shinya Kato
<Shinya11.Kato@oss.nttdata.com> wrote:
>
> On 2021-11-15 04:50, Daniel Gustafsson wrote:
> > Seems reasonable on a quick skim, commit da2c1b8a2 did a similar
> > roundup back
> > in 2009 but at the time most of these didn't exist (pg_trgm did but
> > didn't have
> > custom option back then).  There is one additional callsite defining
> > custom
> > variables in src/pl/tcl/pltcl.c which probably should get this
> > treatment as
> > well, it would align it with the pl/perl counterpart.
> >
> > I'll have a closer look and test tomorrow.
> Thank you for the review!
> I have missed src/pl/tcl/pltcl.c, so I created the new patch.

Do we need to add them in the following too?

delay_execution/delay_execution.c
ssl_passphrase_func.c
worker_spi.c

Especially, worker_spi.c is important as it works as a template for
the bg worker code.

I'm not sure if we have "how to create an extension/a bg worker?" in
the core docs, if we have, it is a good idea to make note of using
EmitWarningsOnPlaceholders so that it will not be missed in the future
modules.

I observed an odd behaviour:
1) I set postgres_fdw.XXX = 'I_messed_up_conf_file' in postgresql.conf
2) With EmitWarningsOnPlaceholders("postgres_fdw"); in postgres_fdw
contrib module, I created the extension, have seen the following
warning:
2021-11-15 06:02:31.198 UTC [2018111] WARNING:  unrecognized
configuration parameter "postgres_fdw.XXX"
3) I further did, "alter system set
postgres_fdw.XXX='I_further_messed_up_conf_file';" and also "select
pg_reload_conf();", it silently accepts.

postgres=# create extension postgres_fdw ;
WARNING:  unrecognized configuration parameter "postgres_fdw.XXX"
CREATE EXTENSION
postgres=# alter system set postgres_fdw.XXX='I_further_messed_up_conf_file';
ALTER SYSTEM
postgres=# select pg_reload_conf();
 pg_reload_conf
----------------
 t
(1 row)

My point is, why should the "create extension" succeed with a WARNING
when a wrong parameter related to it is set in the postgresql.conf
file so that we don't see further problems as shown in (3). I think
EmitWarningsOnPlaceholders should emit an error so that the extension
will not get created in the first place if a wrong configuration
parameter is set in the postgresql.conf file. Many times, users will
not have access to server logs so it is good to fail the "create
extension" statement.

Thoughts?

postgres=# create extension postgres_fdw ;
ERROR:  unrecognized configuration parameter "postgres_fdw.XXX"
postgres=#

Regards,
Bharath Rupireddy.

On 2021-11-15 15:14, Bharath Rupireddy wrote:

> Do we need to add them in the following too?
> 
> delay_execution/delay_execution.c
> ssl_passphrase_func.c
> worker_spi.c
> 
> Especially, worker_spi.c is important as it works as a template for
> the bg worker code.

Thank you for pointing this out! I added it.

> I'm not sure if we have "how to create an extension/a bg worker?" in
> the core docs, if we have, it is a good idea to make note of using
> EmitWarningsOnPlaceholders so that it will not be missed in the future
> modules.

I cannot find any such docs, so I add a comment to 
src/backend/utils/misc/guc.c.

> I observed an odd behaviour:
> 1) I set postgres_fdw.XXX = 'I_messed_up_conf_file' in postgresql.conf
> 2) With EmitWarningsOnPlaceholders("postgres_fdw"); in postgres_fdw
> contrib module, I created the extension, have seen the following
> warning:
> 2021-11-15 06:02:31.198 UTC [2018111] WARNING:  unrecognized
> configuration parameter "postgres_fdw.XXX"
> 3) I further did, "alter system set
> postgres_fdw.XXX='I_further_messed_up_conf_file';" and also "select
> pg_reload_conf();", it silently accepts.
> 
> postgres=# create extension postgres_fdw ;
> WARNING:  unrecognized configuration parameter "postgres_fdw.XXX"
> CREATE EXTENSION
> postgres=# alter system set 
> postgres_fdw.XXX='I_further_messed_up_conf_file';
> ALTER SYSTEM
> postgres=# select pg_reload_conf();
>  pg_reload_conf
> ----------------
>  t
> (1 row)
> 
> My point is, why should the "create extension" succeed with a WARNING
> when a wrong parameter related to it is set in the postgresql.conf
> file so that we don't see further problems as shown in (3). I think
> EmitWarningsOnPlaceholders should emit an error so that the extension
> will not get created in the first place if a wrong configuration
> parameter is set in the postgresql.conf file. Many times, users will
> not have access to server logs so it is good to fail the "create
> extension" statement.
> 
> Thoughts?
> 
> postgres=# create extension postgres_fdw ;
> ERROR:  unrecognized configuration parameter "postgres_fdw.XXX"
> postgres=#

I confirmed it too, and I think that's definitely a problem.
I also thought it would be a good idea to emit an ERROR as well as when 
an invalid normal GUC is set.
I didn't change the function name because it would affect many third 
party extensions.

I plan to change to emit an error when an invalid custom GUC is set in 
the SET or ALTER SYSTEM SET commands, but I haven't tackled this yet.
The patch as of now is attached.


-- 
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Tue, Nov 16, 2021 at 2:50 PM Shinya Kato
<Shinya11.Kato@oss.nttdata.com> wrote:
> > I'm not sure if we have "how to create an extension/a bg worker?" in
> > the core docs, if we have, it is a good idea to make note of using
> > EmitWarningsOnPlaceholders so that it will not be missed in the future
> > modules.
>
> I cannot find any such docs, so I add a comment to
> src/backend/utils/misc/guc.c.
>
> > I observed an odd behaviour:
> > 1) I set postgres_fdw.XXX = 'I_messed_up_conf_file' in postgresql.conf
> > 2) With EmitWarningsOnPlaceholders("postgres_fdw"); in postgres_fdw
> > contrib module, I created the extension, have seen the following
> > warning:
> > 2021-11-15 06:02:31.198 UTC [2018111] WARNING:  unrecognized
> > configuration parameter "postgres_fdw.XXX"
> > 3) I further did, "alter system set
> > postgres_fdw.XXX='I_further_messed_up_conf_file';" and also "select
> > pg_reload_conf();", it silently accepts.
> >
> > postgres=# create extension postgres_fdw ;
> > WARNING:  unrecognized configuration parameter "postgres_fdw.XXX"
> > CREATE EXTENSION
> > postgres=# alter system set
> > postgres_fdw.XXX='I_further_messed_up_conf_file';
> > ALTER SYSTEM
> > postgres=# select pg_reload_conf();
> >  pg_reload_conf
> > ----------------
> >  t
> > (1 row)
> >
> > My point is, why should the "create extension" succeed with a WARNING
> > when a wrong parameter related to it is set in the postgresql.conf
> > file so that we don't see further problems as shown in (3). I think
> > EmitWarningsOnPlaceholders should emit an error so that the extension
> > will not get created in the first place if a wrong configuration
> > parameter is set in the postgresql.conf file. Many times, users will
> > not have access to server logs so it is good to fail the "create
> > extension" statement.
> >
> > Thoughts?
> >
> > postgres=# create extension postgres_fdw ;
> > ERROR:  unrecognized configuration parameter "postgres_fdw.XXX"
> > postgres=#
>
> I confirmed it too, and I think that's definitely a problem.
> I also thought it would be a good idea to emit an ERROR as well as when
> an invalid normal GUC is set.
> I didn't change the function name because it would affect many third
> party extensions.

For backward compatibility you can retain the function
EmitWarningsOnPlaceholders as-is and have another similar function
that emits an error:

In guc.c, have something like below:
static void
check_conf_params(const char *className, bool emit_error)
{
   /* have the existing code of EmitWarningsOnPlaceholders here */
            ereport(emit_error ? ERROR : WARNING,
                    (errcode(ERRCODE_UNDEFINED_OBJECT),
                     errmsg("unrecognized configuration parameter \"%s\"",
                            var->name)));
}

void
EmitErrorOnPlaceholders(const char *className)
{
    check_conf_params(className, true);
}

void
EmitWarningsOnPlaceholders(const char *className)
{
    check_conf_params(className, false);
}

And change all the core extensions to use EmitErrorOnPlaceholders.
This way you don't break the backward compatibility of the outside
extensions, if they want they get to choose which behaviour they want
for their extension.

Actually, I didn't quite like the function name
EmitWarningsOnPlaceholders or EmitErrorOnPlaceholders, it could have
been something else. But let's not go that direction of changing the
function name for backward compatibility.

Regards,
Bharath Rupireddy.

Thank you for the review and sorry for the late reply.

On 2021-11-16 19:25, Bharath Rupireddy wrote:
>> > I observed an odd behaviour:
>> > 1) I set postgres_fdw.XXX = 'I_messed_up_conf_file' in postgresql.conf
>> > 2) With EmitWarningsOnPlaceholders("postgres_fdw"); in postgres_fdw
>> > contrib module, I created the extension, have seen the following
>> > warning:
>> > 2021-11-15 06:02:31.198 UTC [2018111] WARNING:  unrecognized
>> > configuration parameter "postgres_fdw.XXX"
>> > 3) I further did, "alter system set
>> > postgres_fdw.XXX='I_further_messed_up_conf_file';" and also "select
>> > pg_reload_conf();", it silently accepts.
>> >
>> > postgres=# create extension postgres_fdw ;
>> > WARNING:  unrecognized configuration parameter "postgres_fdw.XXX"
>> > CREATE EXTENSION
>> > postgres=# alter system set
>> > postgres_fdw.XXX='I_further_messed_up_conf_file';
>> > ALTER SYSTEM
>> > postgres=# select pg_reload_conf();
>> >  pg_reload_conf
>> > ----------------
>> >  t
>> > (1 row)

I have made changes to achieve the above.
However, it also gives me an error when
---
postgres=# SET abc.a TO on;
SET
postgres=# SET abc.b TO on;
2021-12-16 16:22:20.351 JST [2489236] ERROR:  unrecognized configuration 
parameter "abc.a"
2021-12-16 16:22:20.351 JST [2489236] STATEMENT:  SET abc.b TO on;
ERROR:  unrecognized configuration parameter "abc.a"
---

I know that some people do not think this is good.
Therefore, can I leave this problem alone or is there another better 
way?


> For backward compatibility you can retain the function
> EmitWarningsOnPlaceholders as-is and have another similar function
> that emits an error:
> 
> In guc.c, have something like below:
> static void
> check_conf_params(const char *className, bool emit_error)
> {
>    /* have the existing code of EmitWarningsOnPlaceholders here */
>             ereport(emit_error ? ERROR : WARNING,
>                     (errcode(ERRCODE_UNDEFINED_OBJECT),
>                      errmsg("unrecognized configuration parameter 
> \"%s\"",
>                             var->name)));
> }
> 
> void
> EmitErrorOnPlaceholders(const char *className)
> {
>     check_conf_params(className, true);
> }
> 
> void
> EmitWarningsOnPlaceholders(const char *className)
> {
>     check_conf_params(className, false);
> }
> 

Thank you for your advise.
According to [1], we used the same function name, but the warning level 
was INFO.
Therefore, I think it is OK to use the same function name.

[1] 
https://www.postgresql.org/message-id/flat/200901051634.n05GYNr06169%40momjian.us#1d045374f014494e4b40a4862a000723

-- 
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2021/12/16 16:31, Shinya Kato wrote:
> Thank you for the review and sorry for the late reply.
> 
> On 2021-11-16 19:25, Bharath Rupireddy wrote:
>>> > I observed an odd behaviour:
>>> > 1) I set postgres_fdw.XXX = 'I_messed_up_conf_file' in postgresql.conf
>>> > 2) With EmitWarningsOnPlaceholders("postgres_fdw"); in postgres_fdw
>>> > contrib module, I created the extension, have seen the following
>>> > warning:
>>> > 2021-11-15 06:02:31.198 UTC [2018111] WARNING:  unrecognized
>>> > configuration parameter "postgres_fdw.XXX"
>>> > 3) I further did, "alter system set
>>> > postgres_fdw.XXX='I_further_messed_up_conf_file';" and also "select
>>> > pg_reload_conf();", it silently accepts.
>>> >
>>> > postgres=# create extension postgres_fdw ;
>>> > WARNING:  unrecognized configuration parameter "postgres_fdw.XXX"
>>> > CREATE EXTENSION
>>> > postgres=# alter system set
>>> > postgres_fdw.XXX='I_further_messed_up_conf_file';
>>> > ALTER SYSTEM
>>> > postgres=# select pg_reload_conf();
>>> >  pg_reload_conf
>>> > ----------------
>>> >  t
>>> > (1 row)
> 
> I have made changes to achieve the above.

IMO this behavior change is not good. For example, because it seems to break at least the following case. I think that
theseare the valid steps, but with the patch, the server fails to start up at the step #2 because pg_trgm's custom
parametersare treated as invalid ones.
 

1. Add the following two pg_trgm parameters to postgresql.conf
     - pg_trgm.similarity_threshold
     - pg_trgm.strict_word_similarity_threshold

2. Start the server

3. Run "CREATE EXTENSION pg_trgm" and then use pg_trgm


When I compiled PostgreSQL with the patch, I got the following
compilation failure.

guc.c:5453:4: error: implicit declaration of function 'EmitErrorsOnPlaceholders' is invalid in C99
[-Werror,-Wimplicit-function-declaration]
                         EmitErrorsOnPlaceholders(placeholder);
                         ^


-            ereport(WARNING,
+            ereport(ERROR,

I'm still not sure why you were thinking that ERROR is more proper here.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2021-12-17 01:55, Fujii Masao wrote:
> On 2021/12/16 16:31, Shinya Kato wrote:
>> Thank you for the review and sorry for the late reply.
>> 
>> On 2021-11-16 19:25, Bharath Rupireddy wrote:
>>>> > I observed an odd behaviour:
>>>> > 1) I set postgres_fdw.XXX = 'I_messed_up_conf_file' in postgresql.conf
>>>> > 2) With EmitWarningsOnPlaceholders("postgres_fdw"); in postgres_fdw
>>>> > contrib module, I created the extension, have seen the following
>>>> > warning:
>>>> > 2021-11-15 06:02:31.198 UTC [2018111] WARNING:  unrecognized
>>>> > configuration parameter "postgres_fdw.XXX"
>>>> > 3) I further did, "alter system set
>>>> > postgres_fdw.XXX='I_further_messed_up_conf_file';" and also "select
>>>> > pg_reload_conf();", it silently accepts.
>>>> >
>>>> > postgres=# create extension postgres_fdw ;
>>>> > WARNING:  unrecognized configuration parameter "postgres_fdw.XXX"
>>>> > CREATE EXTENSION
>>>> > postgres=# alter system set
>>>> > postgres_fdw.XXX='I_further_messed_up_conf_file';
>>>> > ALTER SYSTEM
>>>> > postgres=# select pg_reload_conf();
>>>> >  pg_reload_conf
>>>> > ----------------
>>>> >  t
>>>> > (1 row)
>> 
>> I have made changes to achieve the above.
> 
> IMO this behavior change is not good. For example, because it seems to
> break at least the following case. I think that these are the valid
> steps, but with the patch, the server fails to start up at the step #2
> because pg_trgm's custom parameters are treated as invalid ones.
> 
> 1. Add the following two pg_trgm parameters to postgresql.conf
>     - pg_trgm.similarity_threshold
>     - pg_trgm.strict_word_similarity_threshold
> 
> 2. Start the server
> 
> 3. Run "CREATE EXTENSION pg_trgm" and then use pg_trgm

It can happen because the placeholder "pg_trgm" is not already 
registered.
I think too this behavior is not good.
I need to consider an another implementation or to allow undefined GUCs 
to be set.


> When I compiled PostgreSQL with the patch, I got the following
> compilation failure.
> 
> guc.c:5453:4: error: implicit declaration of function
> 'EmitErrorsOnPlaceholders' is invalid in C99
> [-Werror,-Wimplicit-function-declaration]
>                         EmitErrorsOnPlaceholders(placeholder);
>                         ^
> 
> 
> -            ereport(WARNING,
> +            ereport(ERROR,

Thanks! There was a correction omission, so I fixed it.


> I'm still not sure why you were thinking that ERROR is more proper 
> here.

Since I get an ERROR when I set the wrong normal GUCs, I thought I 
should also get an ERROR when I set the wrong extension's GUCs.
However, there are likely to be harmful effects, and I may need to think 
again.


For now, I'v attached the patch that fixed the compilation error.

-- 
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2021/12/17 11:25, Shinya Kato wrote:
> For now, I'v attached the patch that fixed the compilation error.

Thanks for updating the patch! I confirmed that the compilation was completed successfully with this new patch. But the
regressiontest failed. You can see that Patch Tester [1] also reported the failure of regression test for your patch.
 

[1]
http://commitfest.cputube.org/

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 17.12.21 03:25, Shinya Kato wrote:
> For now, I'v attached the patch that fixed the compilation error.

I think it would be good if you could split the uncontroversial new 
EmitErrorsOnPlaceholders() calls into a separate patch.  And please add 
an explanation what exactly the rest of the patch changes.

On 2021-12-17 15:42, Peter Eisentraut wrote:
> On 17.12.21 03:25, Shinya Kato wrote:
>> For now, I'v attached the patch that fixed the compilation error.
> 
> I think it would be good if you could split the uncontroversial new
> EmitErrorsOnPlaceholders() calls into a separate patch.  And please
> add an explanation what exactly the rest of the patch changes.

Thank you for the comment!
I splitted the patch.

- v6-01-Add-EmitWarningsOnPlaceholders.patch
We should use EmitWarningsOnPlaceholders when we use 
DefineCustomXXXVariable.
I don't think there is any room for debate.

- v6-0002-Change-error-level-of-EmitWarningsOnPlaceholders.patch
This is a patch to change the error level of EmitWarningsOnPlaceholders 
from WARNING to ERROR.
I think it's OK to emit ERROR as well as when the wrong GUC is set for 
non-extensions, or since it does not behave abnormally, it can be left 
as WARNING.
Thought?

- v6-0003-Emit-error-when-invalid-extensions-GUCs-are-set.patch
This is a patch to emit error when invalid extension's GUCs are set.
No test changes have been made, so the regression test will fail.
I have created a patch, but I don't think this is necessary because of 
the previous discussion.

-- 
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

At Mon, 20 Dec 2021 21:05:23 +0900, Shinya Kato <Shinya11.Kato@oss.nttdata.com> wrote in 
> - v6-01-Add-EmitWarningsOnPlaceholders.patch
> We should use EmitWarningsOnPlaceholders when we use
> DefineCustomXXXVariable.
> I don't think there is any room for debate.

Unfortunately, pltcl.c defines variables both in pltcl and pltclu but
the patch adds the warning only for pltclu.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

At Fri, 17 Dec 2021 11:25:22 +0900, Shinya Kato <Shinya11.Kato@oss.nttdata.com> wrote in 
> On 2021-12-17 01:55, Fujii Masao wrote:
> > I'm still not sure why you were thinking that ERROR is more proper
> > here.
> 
> Since I get an ERROR when I set the wrong normal GUCs, I thought I
> should also get an ERROR when I set the wrong extension's GUCs.
> However, there are likely to be harmful effects, and I may need to
> think again.

The GUC placeholder mechanism is evidently designed so that server
allows to define variables unknown to the server before loading
extension modules.  ERRORing out that variables at the timing is
contradicting the objective for the placeholder mechanism.

Addition to that EmitWarningsOnPlaceholders()'s objective is to warn
for variables that shouldn't be of a *known* namespace.  However, the
patch is intending to check out variable definitions of all namespaces
that are seen in configuration file, but it is undeterminable at that
time whether each of the namespaces is either just wrong or unknown
yet.  And the latter is, as mentioned above, what we are intending to
allow.

As the concusion, I think we cannot rule-out "wrong" namespaces unless
we find any means to distinguish whether a namespace is wrong or
not-yet-defined before loading extension modules.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 2021/12/22 3:33, Tom Lane wrote:
> I wrote:
>> 0003 looks to me like it was superseded by 75d22069e.  I do not
>> particularly like that patch though; it seems both inefficient
>> and ill-designed.  0003 is adding a check in an equally bizarre
>> place.  Why isn't add_placeholder_variable the place to deal
>> with this?  Also, once we know that a prefix is reserved,
>> why don't we throw an error not just a warning for attempts to
>> set some unknown variable under that prefix?  We don't allow
>> you to set unknown non-prefixed variables.
> Concretely, I think we should do the attached, which removes much of
> 75d22069e and does the check at the point of placeholder creation.
> (After looking closer, add_placeholder_variable's caller is the
> function that is responsible for rejecting bad names, not
> add_placeholder_variable itself.)
>
> This also fixes an indisputable bug in 75d22069e, namely that it
> did prefix comparisons incorrectly and would throw warnings for
> cases it shouldn't.  Reserving "plpgsql" shouldn't have the effect
> of reserving "pl" as well.

Thank you for creating the patch.
This is exactly what I wanted to create. It looks good to me.


> I'm tempted to propose that we also rename EmitWarningsOnPlaceholders
> to something like MarkGUCPrefixReserved, to more clearly reflect
> what it does now.  (We could provide the old name as a macro alias
> to avoid breaking extensions needlessly.)

+1


-- 
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

I wrote:
> Concretely, I think we should do the attached, which removes much of
> 75d22069e and does the check at the point of placeholder creation.

I pushed that, and along the way moved the test case to be beside
the existing tests concerning custom GUC names, rather than appended
at the end of guc.sql as it had been.  That turns out to make the
buildfarm very unhappy.  I had not thought to test that change with
"force_parallel_mode = regress"; but with that on, we can see that
the warning (or now error) is reported each time a parallel worker
is launched, if the parent process contains a bogus placeholder.
So that accidentally unveiled another deficiency in the design of
the original patch --- we surely don't want that to happen.

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.

            regards, tom lane

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

Florin Irion <irionr@gmail.com> writes:
> Il giorno ven 18 feb 2022 alle ore 10:58 Tom Lane <tgl@sss.pgh.pa.us> ha
> scritto:
>> 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?

> Thank you for taking care of the bug I introduced with 75d22069e,
> I didn't notice this thread until now.

Yeah, this thread kinda disappeared under the rug during the Christmas
holidays, but we need to decide whether we want to push forward or
leave things at the status quo.

> For what it's worth, I agree with throwing an ERROR if the placeholder is
> unrecognized. Initially, I didn't want to change too much the liberty of
> setting any placeholder, but mainly to not go unnoticed.

I also think that this is probably a good change to make.

One situation where somebody would be unhappy is if they're using GUC
variables as plain custom variables despite them being within the
namespace of an extension they're using.  But that seems to me to be
(a) far-fetched and (b) mighty dangerous, since some new version of the
extension might suddenly start reacting to those variables in ways you
presumably didn't expect.

Perhaps a more plausible use-case is "I need to work with both versions
X and Y of extension E, and Y has setting e.foo while X doesn't --- but
I can just set e.foo unconditionally since X will ignore it".  With this
patch, that could still work, but you'd have to be certain to apply the
setting before loading E.

I don't really see any other use-cases favoring the current behavior.
On balance, eliminating possible mistakes seems like enough of a
benefit to justify disallowing these cases.

            regards, tom lane

I wrote:
> Florin Irion <irionr@gmail.com> writes:
>> For what it's worth, I agree with throwing an ERROR if the placeholder is
>> unrecognized. Initially, I didn't want to change too much the liberty of
>> setting any placeholder, but mainly to not go unnoticed.

> I also think that this is probably a good change to make.

Hearing no objections, done.

            regards, tom lane