Horton

Thread

Commits

  1. Warning on SET of nonexisting setting with a prefix reserved by an extension

  1. Reserve prefixes for loaded libraries proposal

    Florin Irion <irionr@gmail.com> — 2021-09-30T21:54:04Z

    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

  2. Re: Reserve prefixes for loaded libraries proposal

    Chapman Flack <chap@anastigmatix.net> — 2021-09-30T22:26:08Z

    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

  3. Re: Reserve prefixes for loaded libraries proposal

    Laurenz Albe <laurenz.albe@cybertec.at> — 2021-10-01T04:27:41Z

    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

  4. Re: Reserve prefixes for loaded libraries proposal

    Florin Irion <irionr@gmail.com> — 2021-10-01T07:07:01Z

    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

  5. Re: Reserve prefixes for loaded libraries proposal

    Florin Irion <irionr@gmail.com> — 2021-10-07T12:03:02Z

    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

  6. Re: Reserve prefixes for loaded libraries proposal

    Michael Paquier <michael@paquier.xyz> — 2021-10-21T06:02:44Z

    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

  7. Re: Reserve prefixes for loaded libraries proposal

    Florin Irion <irionr@gmail.com> — 2021-10-22T23:08:21Z

    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

  8. Re: Reserve prefixes for loaded libraries proposal

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-12-01T14:22:39Z

    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.

  9. Re: Reserve prefixes for loaded libraries proposal

    Florin Irion <irionr@gmail.com> — 2021-12-01T17:47:40Z

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