Reserve prefixes for loaded libraries proposal

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

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

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&gt; 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&gt;*
Cel: +39 328 5904901
Tel: +39 0574 054953
Via Alvise Cadamosto, 47
59100, Prato, PO
Italia