Postgres Pro
Downloads
Home   >   mailing lists

Thread: Reserve prefixes for loaded libraries proposal

Reserve prefixes for loaded libraries proposal

From
Florin Irion
Date:
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

Attachment

Re: Reserve prefixes for loaded libraries proposal

From
Chapman Flack
Date:
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

Re: Reserve prefixes for loaded libraries proposal

From
Florin Irion
Date:


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

From
Florin Irion
Date:
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
Attachment

Re: Reserve prefixes for loaded libraries proposal

From
Michael Paquier
Date:
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
Attachment

Re: Reserve prefixes for loaded libraries proposal

From
Florin Irion
Date:
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 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

Re: Reserve prefixes for loaded libraries proposal

From
Peter Eisentraut
Date:
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.

Re: Reserve prefixes for loaded libraries proposal

From
Florin Irion
Date:
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
Cel: +39 328 5904901
Tel: +39 0574 054953
Via Alvise Cadamosto, 47
59100, Prato, PO
Italia