Horton

Thread

Commits

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

  2. Revert changes about warnings/errors for placeholders.

  3. Rename EmitWarningsOnPlaceholders() to MarkGUCPrefixReserved().

  4. Rethink handling of settings with a prefix reserved by an extension.

  5. Add missing EmitWarningsOnPlaceholders() calls.

  6. Add EmitWarningsOnPlaceholders calls to contrib modules that are likely to

  1. Emit a warning if the extension's GUC is set incorrectly

    Shinya Kato <shinya11.kato@oss.nttdata.com> — 2021-11-14T10:03:59Z

    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

  2. Re: Emit a warning if the extension's GUC is set incorrectly

    Daniel Gustafsson <daniel@yesql.se> — 2021-11-14T19:50:56Z

    > 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 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?

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/

  3. Re: Emit a warning if the extension's GUC is set incorrectly

    Shinya Kato <shinya11.kato@oss.nttdata.com> — 2021-11-15T01:03:22Z

    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

  4. Re: Emit a warning if the extension's GUC is set incorrectly

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-11-15T06:14:55Z

    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.

  5. Re: Emit a warning if the extension's GUC is set incorrectly

    Shinya Kato <shinya11.kato@oss.nttdata.com> — 2021-11-16T09:20:33Z

    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

  6. Re: Emit a warning if the extension's GUC is set incorrectly

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-11-16T10:25:13Z

    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.

  7. Re: Emit a warning if the extension's GUC is set incorrectly

    Shinya Kato <shinya11.kato@oss.nttdata.com> — 2021-12-16T07:31:03Z

    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

  8. Re: Emit a warning if the extension's GUC is set incorrectly

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-12-16T16:55:19Z

    
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


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

  9. Re: Emit a warning if the extension's GUC is set incorrectly

    Shinya Kato <shinya11.kato@oss.nttdata.com> — 2021-12-17T02:25:22Z

    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

  10. Re: Emit a warning if the extension's GUC is set incorrectly

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-12-17T05:00:27Z

    
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 regression test 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

  11. Re: Emit a warning if the extension's GUC is set incorrectly

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-12-17T06:42:58Z

    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.

  12. Re: Emit a warning if the extension's GUC is set incorrectly

    Shinya Kato <shinya11.kato@oss.nttdata.com> — 2021-12-20T12:05:23Z

    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

  13. Re: Emit a warning if the extension's GUC is set incorrectly

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-12-21T01:08:34Z

    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

  14. Re: Emit a warning if the extension's GUC is set incorrectly

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-12-21T01:50:07Z

    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

  15. Re: Emit a warning if the extension's GUC is set incorrectly

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-12-21T17:23:25Z

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Mon, 20 Dec 2021 21:05:23 +0900, Shinya Kato <Shinya11.Kato@oss.nttdata.com> wrote in 
>> 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.

Right.  The rest of 0001 looks fine, so pushed with that correction.

I concur that 0002 is unacceptable.  The result of it will be that
a freshly-loaded extension will fail to hook into the system as it
should, because it will fail to complete its initialization.
That will cause user frustration and bug reports.

(BTW, I wonder if EmitWarningsOnPlaceholders should be using LOG
rather than WARNING when it's running in the postmaster.  Use of
WARNING is moderately likely to result in nothing getting printed
at all.)

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.

			regards, tom lane

  16. Re: Emit a warning if the extension's GUC is set incorrectly

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-12-21T18:33:49Z

    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.

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.)

			regards, tom lane

  17. Re: Emit a warning if the extension's GUC is set incorrectly

    Shinya Kato <shinya11.kato@oss.nttdata.com> — 2021-12-22T07:50:34Z

    On 2021-12-22 02:23, Tom Lane wrote:
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
>> At Mon, 20 Dec 2021 21:05:23 +0900, Shinya Kato 
>> <Shinya11.Kato@oss.nttdata.com> wrote in
>>> 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.
> 
> Right.  The rest of 0001 looks fine, so pushed with that correction.
> 
> I concur that 0002 is unacceptable.  The result of it will be that
> a freshly-loaded extension will fail to hook into the system as it
> should, because it will fail to complete its initialization.
> That will cause user frustration and bug reports.
> 
> (BTW, I wonder if EmitWarningsOnPlaceholders should be using LOG
> rather than WARNING when it's running in the postmaster.  Use of
> WARNING is moderately likely to result in nothing getting printed
> at all.)
> 
> 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.
> 
> 			regards, tom lane

Thank you for pushing!
Thank you all for the reviews, I think I will take down 0002 and 0003.

-- 
Regards,

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

  18. Re: Emit a warning if the extension's GUC is set incorrectly

    Shinya Kato <shinya11.kato@oss.nttdata.com> — 2021-12-24T03:29:47Z

    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

  19. Re: Emit a warning if the extension's GUC is set incorrectly

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-12-27T20:48:43Z

    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

  20. Re: Emit a warning if the extension's GUC is set incorrectly

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-12-28T17:06:55Z

    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

  21. Re: Emit a warning if the extension's GUC is set incorrectly

    Florin Irion <irionr@gmail.com> — 2022-02-18T11:47:23Z

    Il giorno ven 18 feb 2022 alle ore 10:58 Tom Lane <tgl@sss.pgh.pa.us> ha
scritto:

> 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 lan
>

Hello,

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

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.

In my humble opinion, I still think that this should go on and disallow
bogus placeholders as we do for postgres unrecognized settings.

I tested your delta patch with and without parallel mode, and, naturally,
it behaves correctly.

My 2 cents.

+1

Cheers,
Florin Irion

  22. Re: Emit a warning if the extension's GUC is set incorrectly

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-18T22:18:21Z

    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

  23. Re: Emit a warning if the extension's GUC is set incorrectly

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-21T19:12:04Z

    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

  24. Re: Emit a warning if the extension's GUC is set incorrectly

    Florin Irion <irionr@gmail.com> — 2022-02-21T21:22:55Z

    Il lun 21 feb 2022, 20:12 Tom Lane <tgl@sss.pgh.pa.us> ha scritto:

> 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
>

Thank you for taking care of this.

Cheers,
Florin

>