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
diff --git a/contrib/auth_delay/auth_delay.c b/contrib/auth_delay/auth_delay.c
index 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.c
index 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.c
index 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.c
index 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.c
index e91d5a3cfd..d136ca12ea 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -5431,11 +5431,19 @@ valid_custom_variable_name(const char *name)
{
bool saw_sep = false;
bool name_start = true;
+ int placeholder_length = 0;
for (const char *p = name; *p; p++)
{
+ placeholder_length++;
if (*p == GUC_QUALIFIER_SEPARATOR)
{
+ /* Check invalid placeholder and custom parameter*/
+ char *placeholder = malloc(sizeof(char) * (placeholder_length - 1));
+ strncpy(placeholder, name, placeholder_length - 1);
+ placeholder[placeholder_length -1] = '\0';
+ EmitErrorsOnPlaceholders(placeholder);
+
if (name_start)
return false; /* empty name component */
saw_sep = true;
@@ -9322,6 +9330,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 +9351,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.c
index 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.c
index 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.c
index 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.c
index 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 |
