From: Shinya Kato <Shinya11.Kato@oss.nttdata.com>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Daniel Gustafsson <daniel@yesql.se>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Emit a warning if the extension's GUC is set incorrectly
Date: Tue, 16 Nov 2021 18:20:33 +0900
Message-ID: <98d354d3b099d8e24c6b653336bd29d7@oss.nttdata.com> (raw)
In-Reply-To: <CALj2ACX9eDm9bPmcZDTGazEVrH+p7jUXfN0miBc_VVze_mjw9A@mail.gmail.com>
References: <524fa2c0a34f34b68fbfa90d0760d515@oss.nttdata.com>
<681A95E0-0CCA-4C66-A12B-CE0D8D5076D7@yesql.se>
<25643eaa81a69416317e85c2fd7d4c0a@oss.nttdata.com>
<CALj2ACX9eDm9bPmcZDTGazEVrH+p7jUXfN0miBc_VVze_mjw9A@mail.gmail.com>
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
Attachments:v3_Add_EmitWarningsOnPlaceholders.patch (text/x-diff, 4.0K) downloaddiff --git a/contrib/auth_delay/auth_delay.c b/contrib/auth_delay/auth_delay.cindex 5820ac328d..d11dd1e416 100644--- a/contrib/auth_delay/auth_delay.c+++ b/contrib/auth_delay/auth_delay.c@@ -67,6 +67,9 @@ _PG_init(void)
NULL,
NULL,
NULL);
++ EmitWarningsOnPlaceholders("auth_delay");+
/* Install Hooks */
original_client_auth_hook = ClientAuthentication_hook;
ClientAuthentication_hook = auth_delay_checks;
diff --git a/contrib/pg_trgm/trgm_op.c b/contrib/pg_trgm/trgm_op.cindex fb38135f7a..0407c7dd64 100644--- a/contrib/pg_trgm/trgm_op.c+++ b/contrib/pg_trgm/trgm_op.c@@ -100,6 +100,8 @@ _PG_init(void)
NULL,
NULL,
NULL);
++ EmitWarningsOnPlaceholders("pg_trgm");
}
/*
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.cindex 48c7417e6e..36555398ec 100644--- a/contrib/postgres_fdw/option.c+++ b/contrib/postgres_fdw/option.c@@ -469,4 +469,6 @@ _PG_init(void)
NULL,
NULL,
NULL);
++ EmitWarningsOnPlaceholders("postgres_fdw");
}
diff --git a/contrib/sepgsql/hooks.c b/contrib/sepgsql/hooks.cindex 19a3ffb7ff..44a09c35a7 100644--- a/contrib/sepgsql/hooks.c+++ b/contrib/sepgsql/hooks.c@@ -455,6 +455,8 @@ _PG_init(void)
NULL,
NULL);
+ EmitWarningsOnPlaceholders("sepgsql");+
/* Initialize userspace access vector cache */
sepgsql_avc_init();
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.cindex e91d5a3cfd..07647a0d84 100644--- a/src/backend/utils/misc/guc.c+++ b/src/backend/utils/misc/guc.c@@ -9322,6 +9322,13 @@ DefineCustomEnumVariable(const char *name,
define_custom_variable(&var->gen);
}
+/*+ * For historical context, this function name is EmitWarningsOnPlaceholders,+ * but it emits an error when an invalid custom GUC is set.+ *+ * If we use DefineCustomXXXVariable, it is recommended that this function is+ * added.+ */
void
EmitWarningsOnPlaceholders(const char *className)
{
@@ -9336,7 +9343,7 @@ EmitWarningsOnPlaceholders(const char *className)
strncmp(className, var->name, classLen) == 0 &&
var->name[classLen] == GUC_QUALIFIER_SEPARATOR)
{
- ereport(WARNING,+ ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("unrecognized configuration parameter \"%s\"",
var->name)));
diff --git a/src/pl/tcl/pltcl.c b/src/pl/tcl/pltcl.cindex e11837559d..1389aa0943 100644--- a/src/pl/tcl/pltcl.c+++ b/src/pl/tcl/pltcl.c@@ -474,6 +474,8 @@ _PG_init(void)
PGC_SUSET, 0,
NULL, NULL, NULL);
+ EmitWarningsOnPlaceholders("pltclu");+
pltcl_pm_init_done = true;
}
diff --git a/src/test/modules/delay_execution/delay_execution.c b/src/test/modules/delay_execution/delay_execution.cindex b3d0841ba8..8ec623ac52 100644--- a/src/test/modules/delay_execution/delay_execution.c+++ b/src/test/modules/delay_execution/delay_execution.c@@ -91,6 +91,8 @@ _PG_init(void)
NULL,
NULL);
+ EmitWarningsOnPlaceholders("delay_execution");+
/* Install our hook */
prev_planner_hook = planner_hook;
planner_hook = delay_execution_planner;
diff --git a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.cindex 6b0a3db104..3ba33e501c 100644--- a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c+++ b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c@@ -48,6 +48,9 @@ _PG_init(void)
NULL,
NULL,
NULL);
++ EmitWarningsOnPlaceholders("ssl_passphrase");+
if (ssl_passphrase)
openssl_tls_init_hook = set_rot13;
}
diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.cindex 0b6246676b..adb02d8cb8 100644--- a/src/test/modules/worker_spi/worker_spi.c+++ b/src/test/modules/worker_spi/worker_spi.c@@ -322,6 +322,8 @@ _PG_init(void)
0,
NULL, NULL, NULL);
+ EmitWarningsOnPlaceholders("worker_spi");+
/* set up common data for all our workers */
memset(&worker, 0, sizeof(worker));
worker.bgw_flags = BGWORKER_SHMEM_ACCESS |
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: Shinya11.Kato@oss.nttdata.com, bharath.rupireddyforpostgres@gmail.com, daniel@yesql.se
Subject: Re: Emit a warning if the extension's GUC is set incorrectly
In-Reply-To: <98d354d3b099d8e24c6b653336bd29d7@oss.nttdata.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox