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

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/

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]/messages/by-id/200901051634.n05GYNr06169@momjian.us, 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]: /messages/by-id/200901051634.n05GYNr06169@momjian.us
/messages/by-id/200901051634.n05GYNr06169@momjian.us

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

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 regression test failed. You can see that Patch Tester [1]http://commitfest.cputube.org/ also reported the failure of regression test for your patch.

[1]: http://commitfest.cputube.org/
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

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

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

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

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

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

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

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