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

[Florin Irion]

Il lun 21 feb 2022, 20:12 Tom Lane  ha scritto:

> I wrote:
> > Florin Irion  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

>

Re: Reserve prefixes for loaded libraries proposal

[Florin Irion]

Hi,

Il giorno gio 21 ott 2021 alle ore 08:02 Michael Paquier <
mich...@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

Re: Reserve prefixes for loaded libraries proposal

[Florin Irion]

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

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

[Florin Irion]

Il giorno ven 18 feb 2022 alle ore 10:58 Tom Lane  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

Reserve prefixes for loaded libraries proposal

[Florin Irion]

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


reservePrefixWarnUser.patch
Description: Binary data

Re: Reserve prefixes for loaded libraries proposal

[Florin Irion]

Il giorno ven 1 ott 2021 alle ore 00:26 Chapman Flack 
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;
> 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
>

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;
> SET
>
> But no!

Exactly.

> So I am in favor of patching this.
>
> Regards,
> -Chap

Thanks,
Florin Irion

Re: Reserve prefixes for loaded libraries proposal

[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 IrionFrom 9a50261faad3a7578dab150f1c05b510285e283c Mon Sep 17 00:00:00 2001
From: Florin Irion 
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 intclassLen = gucQualifierSeparator - varName;
+   unsigned intvarLen = 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.

pg_create_logical_replication_slot argument incongruency

[Florin Irion]

Hello,

The function `pg_create_logical_replication_slot()`  is documented to have
a `two_phase` argument(note the underscore), but the function instead
requires `twophase`.

```
\df pg_catalog.pg_create_logical_replication_slot
List of functions
-[ RECORD 1
]---+-

Schema  | pg_catalog
Name| pg_create_logical_replication_slot
Result data type| record
Argument data types | slot_name name, plugin name, temporary boolean
DEFAULT false, twophase boolean DEFAULT false, OUT slot_name name, OUT lsn
pg_lsn
Type| func
```

This was introduced in commit 19890a06.

IMHO we should use the documented argument name `two_phase` and change the
function to accept it.

What do you think?

Please, check the attached patch.


Cheers,
Florin
--
*www.enterprisedb.com *


two_phase_slot_v1.patch
Description: Binary data

Re: pg_create_logical_replication_slot argument incongruency

[Florin Irion]

On 20/09/22 03:33, Michael Paquier wrote:
> On Mon, Sep 19, 2022 at 07:02:16PM +0200, Florin Irion wrote:
>> This was introduced in commit 19890a06.
>>
>> IMHO we should use the documented argument name `two_phase` and change the
>> function to accept it.
>>
>> What do you think?
> 
> 19890a0 is included in REL_14_STABLE, and changing an argument name is
> not acceptable in a stable branch as it would imply a catversion
> bump.  Let's change the docs so as we document the parameter as
> "twophase", instead.
> --
> Michael

I understand. 

OK, patch only for the docs attached.

Cheers, 
Florin
www.enterprisedb.comdiff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index f63a04c667..f1a8a9a0ba 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -25891,7 +25891,7 @@ postgres=# SELECT * FROM 
pg_walfile_name_offset(pg_stop_backup());
 
  pg_create_logical_replication_slot
 
-pg_create_logical_replication_slot ( 
slot_name name, 
plugin name , 
temporary boolean, 
two_phase boolean  )
+pg_create_logical_replication_slot ( 
slot_name name, 
plugin name , 
temporary boolean, 
twophase boolean  )
 record
 ( slot_name name,
 lsn pg_lsn )
@@ -25904,7 +25904,7 @@ postgres=# SELECT * FROM 
pg_walfile_name_offset(pg_stop_backup());
 the slot should not be permanently stored to disk and is only meant
 for use by the current session. Temporary slots are also
 released upon any error. The optional fourth parameter,
-two_phase, when set to true, specifies
+twophase, when set to true, specifies
 that the decoding of prepared transactions is enabled for this
 slot. A call to this function has the same effect as the replication
 protocol command CREATE_REPLICATION_SLOT ... 
LOGICAL.

Re: pg_create_logical_replication_slot argument incongruency

[Florin Irion]

Thank you!

Il mar 20 set 2022, 12:29 Michael Paquier  ha scritto:

> On Tue, Sep 20, 2022 at 08:41:56AM +0200, Florin Irion wrote:
> > OK, patch only for the docs attached.
>
> Thanks, applied.
> --
> Michael
>