Thread: 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
Attachment
> 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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
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
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