Reserve prefixes for loaded libraries proposal
Hello,
If we set a parameter in the postgresql.conf that the loaded library doesn't
recognize at startup, it throws a warning.
For example if one sets `plpgsql.no_such_setting` for plpgsql:
```
WARNING: unrecognized configuration parameter "plpgsql.no_such_setting"
```
We could also help users get a warning if they set a parameter with the
`SET`
command. I've seen many cases where users make typos and break things badly,
check the following example:
```
postgres=# BEGIN;
BEGIN
postgres=*# SET plpgsql.no_such_setting = false;
SET
postgres=*# -- do critical queries taking into account that
plpgsql.no_such_setting is false;
postgres=*# COMMIT;
COMMIT
```
I propose to make the user aware of such mistakes. I also made the patch
only
to warn the user but still correctly `SET` the parameter so that he is the
one
that chooses if he wants to continue or `ROLLBACK`. I don't know if this
last
part is correct, but at least it doesn't break any previous implementation.
This is what I mean:
```
postgres=# BEGIN;
BEGIN
postgres=*# SET plpgsql.no_such_setting = false;
WARNING: unrecognized configuration parameter "plpgsql.no_such_setting"
DETAIL: "plpgsql" is a reserved prefix.
HINT: If you need to create a custom placeholder use a different prefix.
SET
postgres=*# -- choose to continue or not based on the warning
postgres=*# ROLLBACK or COMMIT
```
The patch I'm attaching is registering the prefix for all the loaded
libraries,
and eventually, it uses them to check if any parameter is recognized,just
as we
do at startup.
Please, let me know what you think.
Cheers,
Florin Irion
Attachments:
reservePrefixWarnUser.patchapplication/octet-stream; name=reservePrefixWarnUser.patchDownload
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 6652a60ec3..7e03074373 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -234,6 +234,8 @@ static bool check_recovery_target_lsn(char **newval, void **extra, GucSource sou
static void assign_recovery_target_lsn(const char *newval, void *extra);
static bool check_primary_slot_name(char **newval, void **extra, GucSource source);
static bool check_default_with_oids(bool *newval, void **extra, GucSource source);
+static void EmitWarningsOnSetPlaceholders(const char *varName);
+static List *reserved_class_prefix = NIL;
/* Private functions in guc-file.l that need to be called from guc.c */
static ConfigVariable *ProcessConfigFileInternal(GucContext context,
@@ -8703,6 +8705,8 @@ ExecSetVariableStmt(VariableSetStmt *stmt, bool isTopLevel)
(superuser() ? PGC_SUSET : PGC_USERSET),
PGC_S_SESSION,
action, true, 0, false);
+
+ EmitWarningsOnSetPlaceholders(stmt->name);
break;
case VAR_SET_MULTI:
@@ -8789,6 +8793,8 @@ ExecSetVariableStmt(VariableSetStmt *stmt, bool isTopLevel)
(superuser() ? PGC_SUSET : PGC_USERSET),
PGC_S_SESSION,
action, true, 0, false);
+
+ EmitWarningsOnSetPlaceholders(stmt->name);
break;
case VAR_RESET_ALL:
ResetAllOptions();
@@ -9275,6 +9281,7 @@ EmitWarningsOnPlaceholders(const char *className)
{
int classLen = strlen(className);
int i;
+ MemoryContext oldcontext;
for (i = 0; i < num_guc_variables; i++)
{
@@ -9290,8 +9297,51 @@ EmitWarningsOnPlaceholders(const char *className)
var->name)));
}
}
+ oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+ reserved_class_prefix = lappend(reserved_class_prefix, pstrdup(className));
+ MemoryContextSwitchTo(oldcontext);
}
+/*
+ * Throw a warning if a user creates a placeholder with
+ * the "SET" command and we don't recognize it.
+ */
+static void
+EmitWarningsOnSetPlaceholders(const char *varName)
+{
+ char *gucQualifierSeparator;
+
+ gucQualifierSeparator = strchr(varName, GUC_QUALIFIER_SEPARATOR);
+
+ if (gucQualifierSeparator)
+ {
+ int classLen = gucQualifierSeparator - varName;
+ ListCell *lc;
+
+ foreach(lc, reserved_class_prefix)
+ {
+ char *rcprefix = (char *) lfirst(lc);
+
+ if (strncmp(varName, rcprefix, classLen) == 0)
+ {
+ for (int i = 0; i < num_guc_variables; i++)
+ {
+ struct config_generic *var = guc_variables[i];
+
+ if ((var->flags & GUC_CUSTOM_PLACEHOLDER) != 0 &&
+ strncmp(varName, var->name, strlen(varName))==0)
+ {
+ ereport(WARNING,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("unrecognized configuration parameter \"%s\"", var->name),
+ errdetail("\"%.*s\" is a reserved prefix.", classLen, var->name),
+ errhint("If you need to create a custom placeholder use a different prefix.")));
+ }
+ }
+ }
+ }
+ }
+}
/*
* SHOW command
diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
index 59da91ff04..e5049bed19 100644
--- a/src/test/regress/expected/guc.out
+++ b/src/test/regress/expected/guc.out
@@ -813,3 +813,24 @@ set default_with_oids to f;
-- Should not allow to set it to true.
set default_with_oids to t;
ERROR: tables declared WITH OIDS are not supported
+-- test SET unrecongnized parameter
+SET foo = FALSE; -- no such setting
+ERROR: unrecognized configuration parameter "foo"
+-- test setting a parameter with a registered prefix (plpgsql)
+SET plpgsql.extra_foo_warnings = FALSE; -- no such setting
+WARNING: unrecognized configuration parameter "plpgsql.extra_foo_warnings"
+DETAIL: "plpgsql" is a reserved prefix.
+HINT: If you need to create a custom placeholder use a different prefix.
+SHOW plpgsql.extra_foo_warnings; -- the parameter is however set
+ plpgsql.extra_foo_warnings
+----------------------------
+ false
+(1 row)
+
+-- cleanup
+RESET foo;
+ERROR: unrecognized configuration parameter "foo"
+RESET plpgsql.extra_foo_warnings;
+WARNING: unrecognized configuration parameter "plpgsql.extra_foo_warnings"
+DETAIL: "plpgsql" is a reserved prefix.
+HINT: If you need to create a custom placeholder use a different prefix.
diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql
index c39c11388d..d9669d6e5a 100644
--- a/src/test/regress/sql/guc.sql
+++ b/src/test/regress/sql/guc.sql
@@ -311,3 +311,13 @@ reset check_function_bodies;
set default_with_oids to f;
-- Should not allow to set it to true.
set default_with_oids to t;
+
+-- test SET unrecongnized parameter
+SET foo = FALSE; -- no such setting
+
+-- test setting a parameter with a registered prefix (plpgsql)
+SET plpgsql.extra_foo_warnings = FALSE; -- no such setting
+SHOW plpgsql.extra_foo_warnings; -- the parameter is however set
+-- cleanup
+RESET foo;
+RESET plpgsql.extra_foo_warnings;
\ No newline at end of file
On 09/30/21 17:54, Florin Irion wrote:
We could also help users get a warning if they set a parameter with the
`SET` command.
This is funny. For years I have been so confident I knew how this worked
that I, obviously, hadn't tried it. :)
My first setting of a made-up variable gets no warning, as I already expected:
postgres=# set plpgsql.no_such_setting = false;
SET
Then as soon as I do the first thing in the session involving plpgsql,
I get the warning for that one:
postgres=# do language plpgsql
postgres-# 'begin delete from pg_class where false; end';
WARNING: unrecognized configuration parameter "plpgsql.no_such_setting"
DO
But then, I have always assumed I would get warnings thereafter:
postgres=# set plpgsql.not_this_one_neither = false;
SET
But no!
So I am in favor of patching this.
Regards,
-Chap
On Thu, 2021-09-30 at 18:26 -0400, Chapman Flack wrote:
On 09/30/21 17:54, Florin Irion wrote:
We could also help users get a warning if they set a parameter with the
`SET` command.So I am in favor of patching this.
+1 on the idea.
Yours,
Laurenz Albe
Il giorno ven 1 ott 2021 alle ore 00:26 Chapman Flack <chap@anastigmatix.net>
ha scritto:
On 09/30/21 17:54, Florin Irion wrote:
We could also help users get a warning if they set a parameter with the
`SET` command.This is funny. For years I have been so confident I knew how this worked
that I, obviously, hadn't tried it. :)My first setting of a made-up variable gets no warning, as I already
expected:
postgres=# set plpgsql.no_such_setting = false;
SETThen as soon as I do the first thing in the session involving plpgsql,
I get the warning for that one:postgres=# do language plpgsql
postgres-# 'begin delete from pg_class where false; end';
WARNING: unrecognized configuration parameter "plpgsql.no_such_setting"
DO
I choose `plpgsql` in my example because perhaps it is best known to the
majority, plpgsql gets loaded when the user first uses it, and doesn't need
to be preloaded at startup.
This proposal will help when we have any extension in the
`shared_preload_libraries`
and the check is only made at postgres start.
However, if one already used plpgsql in a session and then it `SET`s an
unknown parameter
it will not get any warning as the check is made only when it gets loaded
the first time.
```
postgres=# do language plpgsql
'begin delete from pg_class where false; end';
DO
postgres=# set plpgsql.no_such_setting = false;
SET
postgres=# do language plpgsql
'begin delete from pg_class where false; end';
DO
```
With my patch it will be registered and it will throw a warning also in
this case:
```
postgres=# do language plpgsql
postgres-# 'begin delete from pg_class where false; end';
DO
postgres=# set plpgsql.no_such_setting = false;
WARNING: unrecognized configuration parameter "plpgsql.no_such_setting"
DETAIL: "plpgsql" is a reserved prefix.
HINT: If you need to create a custom placeholder use a different prefix.
SET
```
But then, I have always assumed I would get warnings thereafter:
postgres=# set plpgsql.not_this_one_neither = false;
SETBut no!
Exactly.
So I am in favor of patching this.
Regards,
-Chap
Thanks,
Florin Irion
Hi,
I adjusted a bit the code and configured my mail client to
send patch attachments appropiately(hopefully). :)
So here is my second version.
Cheers,
Florin Irion
Attachments:
v2-0001-Reserve-the-prefix-for-each-extension.patchtext/plain; charset=UTF-8; name=v2-0001-Reserve-the-prefix-for-each-extension.patchDownload
From 9a50261faad3a7578dab150f1c05b510285e283c Mon Sep 17 00:00:00 2001
From: Florin Irion <florin.irion@enterprisedb.com>
Date: Thu, 7 Oct 2021 09:51:29 +0200
Subject: [PATCH] Reserve the prefix for each extension
Throw a warning when we `SET` a variable that doesn't exist with
a reserved prefix. This will protect against setting incorrect
configurations for any extension, like we do with the `GUC`s.
Add a regression test that checks the patch using the "plpgsql"
registered prefix.
---
src/backend/utils/misc/guc.c | 49 +++++++++++++++++++++++++++++++
src/test/regress/expected/guc.out | 21 +++++++++++++
src/test/regress/sql/guc.sql | 10 +++++++
3 files changed, 80 insertions(+)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 6652a60ec3..f19310df44 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -234,6 +234,8 @@ static bool check_recovery_target_lsn(char **newval, void **extra, GucSource sou
static void assign_recovery_target_lsn(const char *newval, void *extra);
static bool check_primary_slot_name(char **newval, void **extra, GucSource source);
static bool check_default_with_oids(bool *newval, void **extra, GucSource source);
+static void EmitWarningsOnSetPlaceholders(const char *varName);
+static List *reserved_class_prefix = NIL;
/* Private functions in guc-file.l that need to be called from guc.c */
static ConfigVariable *ProcessConfigFileInternal(GucContext context,
@@ -8703,6 +8705,8 @@ ExecSetVariableStmt(VariableSetStmt *stmt, bool isTopLevel)
(superuser() ? PGC_SUSET : PGC_USERSET),
PGC_S_SESSION,
action, true, 0, false);
+
+ EmitWarningsOnSetPlaceholders(stmt->name);
break;
case VAR_SET_MULTI:
@@ -8789,6 +8793,8 @@ ExecSetVariableStmt(VariableSetStmt *stmt, bool isTopLevel)
(superuser() ? PGC_SUSET : PGC_USERSET),
PGC_S_SESSION,
action, true, 0, false);
+
+ EmitWarningsOnSetPlaceholders(stmt->name);
break;
case VAR_RESET_ALL:
ResetAllOptions();
@@ -9275,6 +9281,7 @@ EmitWarningsOnPlaceholders(const char *className)
{
int classLen = strlen(className);
int i;
+ MemoryContext oldcontext;
for (i = 0; i < num_guc_variables; i++)
{
@@ -9290,8 +9297,50 @@ EmitWarningsOnPlaceholders(const char *className)
var->name)));
}
}
+ oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+ reserved_class_prefix = lappend(reserved_class_prefix, pstrdup(className));
+ MemoryContextSwitchTo(oldcontext);
}
+/*
+ * Throw a warning if a user creates a placeholder with
+ * the "SET" command and we don't recognize it.
+ */
+static void
+EmitWarningsOnSetPlaceholders(const char *varName)
+{
+ char *gucQualifierSeparator = strchr(varName, GUC_QUALIFIER_SEPARATOR);
+
+ if (gucQualifierSeparator)
+ {
+ unsigned int classLen = gucQualifierSeparator - varName;
+ unsigned int varLen = strlen(varName);
+ ListCell *lc;
+
+ foreach(lc, reserved_class_prefix)
+ {
+ char *rcprefix = (char *) lfirst(lc);
+
+ if (strncmp(varName, rcprefix, classLen) == 0)
+ {
+ for (int i = 0; i < num_guc_variables; i++)
+ {
+ struct config_generic *var = guc_variables[i];
+
+ if ((var->flags & GUC_CUSTOM_PLACEHOLDER) != 0 &&
+ strncmp(varName, var->name, varLen)==0)
+ {
+ ereport(WARNING,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("unrecognized configuration parameter \"%s\"", var->name),
+ errdetail("\"%.*s\" is a reserved prefix.", classLen, var->name),
+ errhint("If you need to create a custom placeholder use a different prefix.")));
+ }
+ }
+ }
+ }
+ }
+}
/*
* SHOW command
diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
index 59da91ff04..e5049bed19 100644
--- a/src/test/regress/expected/guc.out
+++ b/src/test/regress/expected/guc.out
@@ -813,3 +813,24 @@ set default_with_oids to f;
-- Should not allow to set it to true.
set default_with_oids to t;
ERROR: tables declared WITH OIDS are not supported
+-- test SET unrecongnized parameter
+SET foo = FALSE; -- no such setting
+ERROR: unrecognized configuration parameter "foo"
+-- test setting a parameter with a registered prefix (plpgsql)
+SET plpgsql.extra_foo_warnings = FALSE; -- no such setting
+WARNING: unrecognized configuration parameter "plpgsql.extra_foo_warnings"
+DETAIL: "plpgsql" is a reserved prefix.
+HINT: If you need to create a custom placeholder use a different prefix.
+SHOW plpgsql.extra_foo_warnings; -- the parameter is however set
+ plpgsql.extra_foo_warnings
+----------------------------
+ false
+(1 row)
+
+-- cleanup
+RESET foo;
+ERROR: unrecognized configuration parameter "foo"
+RESET plpgsql.extra_foo_warnings;
+WARNING: unrecognized configuration parameter "plpgsql.extra_foo_warnings"
+DETAIL: "plpgsql" is a reserved prefix.
+HINT: If you need to create a custom placeholder use a different prefix.
diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql
index c39c11388d..d9669d6e5a 100644
--- a/src/test/regress/sql/guc.sql
+++ b/src/test/regress/sql/guc.sql
@@ -311,3 +311,13 @@ reset check_function_bodies;
set default_with_oids to f;
-- Should not allow to set it to true.
set default_with_oids to t;
+
+-- test SET unrecongnized parameter
+SET foo = FALSE; -- no such setting
+
+-- test setting a parameter with a registered prefix (plpgsql)
+SET plpgsql.extra_foo_warnings = FALSE; -- no such setting
+SHOW plpgsql.extra_foo_warnings; -- the parameter is however set
+-- cleanup
+RESET foo;
+RESET plpgsql.extra_foo_warnings;
\ No newline at end of file
--
2.31.1
On Thu, Sep 30, 2021 at 11:54:04PM +0200, Florin Irion wrote:
We could also help users get a warning if they set a parameter with the
`SET`
command. I've seen many cases where users make typos and break things badly,
check the following example:```
postgres=# BEGIN;
BEGIN
postgres=*# SET plpgsql.no_such_setting = false;
SET
postgres=*# -- do critical queries taking into account that
plpgsql.no_such_setting is false;
postgres=*# COMMIT;
COMMIT
```
Could you give a more concrete example here? What kind of critical
work are you talking about here when using prefixes? Please note that
I am not against the idea of improving the user experience in this
area as that's inconsistent, as you say.
--
Michael
Hi,
Il giorno gio 21 ott 2021 alle ore 08:02 Michael Paquier <
michael@paquier.xyz> ha scritto:
On Thu, Sep 30, 2021 at 11:54:04PM +0200, Florin Irion wrote:
We could also help users get a warning if they set a parameter with the
`SET`
command. I've seen many cases where users make typos and break things
badly,
check the following example:
```
postgres=# BEGIN;
BEGIN
postgres=*# SET plpgsql.no_such_setting = false;
SET
postgres=*# -- do critical queries taking into account that
plpgsql.no_such_setting is false;
postgres=*# COMMIT;
COMMIT
```Could you give a more concrete example here? What kind of critical
work are you talking about here when using prefixes? Please note that
I am not against the idea of improving the user experience in this
area as that's inconsistent, as you say.
--
Michael
Thank you for the interest.
I used `plpgsql` in my example/test because it's something that many of
us know already.
However, for example, pglogical2
<https://github.com/2ndQuadrant/pglogical> has the
`pglogical.conflict_resolution`
configuration parameter that can be set to one of the following:
```
error
apply_remote
keep_local
last_update_wins
first_update_wins
```
You can imagine that conflicting queries could have different outcomes
based on this setting.
IMHO there are other settings like this, in other extensions, that can
be critical.
In any case, even setting something that is not critical could still
be important for a user, one example, `pg_stat_statements.track`.
Cheers,
Florin
--
Florin Irion
www.enterprisedb.com
On 07.10.21 14:03, Florin Irion wrote:
I adjusted a bit the code and configured my mail client to
send patch attachments appropiately(hopefully). :)So here is my second version.
Committed.
I made two notable changes: I renamed the function, since it looked
like EmitWarningsOnPlaceholders() but was doing something not analogous.
Also, you had in your function
strncmp(varName, var->name, varLen)
probably copied from EmitWarningsOnPlaceholders(), but unlike there, we
want to compare the whole string here, and this would potentially do
something wrong if there were a GUC setting that was a substring of the
name of another one.
Committed.
I made two notable changes: I renamed the function, since it looked
like EmitWarningsOnPlaceholders() but was doing something not analogous.
Also, you had in your function
strncmp(varName, var->name, varLen)
probably copied from EmitWarningsOnPlaceholders(), but unlike there, we
want to compare the whole string here, and this would potentially do
something wrong if there were a GUC setting that was a substring of the
name of another one.
Yeah, it makes sense!
Thank you very much!
Thank you for the changes and thank you for committing it!
Cheers,
Florin
--
*Florin Irion*
*www.enterprisedb.com <http://www.enterprisedb.com>*
Cel: +39 328 5904901
Tel: +39 0574 054953
Via Alvise Cadamosto, 47
59100, Prato, PO
Italia