Thread

Commits

Allow ALTER SYSTEM to set unrecognized custom GUCs.
- 2d870b4aeff1 17.0 landed

Allow ALTER SYSTEM SET on unrecognized custom GUCs

Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-17T00:19:16Z

Currently we have this odd behavior (for a superuser):

regression=# ALTER SYSTEM SET foo.bar TO 'baz';
ERROR:  unrecognized configuration parameter "foo.bar"
regression=# SET foo.bar TO 'baz';
SET
regression=# ALTER SYSTEM SET foo.bar TO 'baz';
ALTER SYSTEM

That is, you can't ALTER SYSTEM SET a random custom GUC unless there
is already a placeholder GUC for it, because the find_option call in
AlterSystemSetConfigFile fails.  This is surely pretty inconsistent.
Either the first ALTER SYSTEM SET ought to succeed, or the second one
ought to fail too, because we don't have any more knowledge about the
custom GUC than we did before.

In the original discussion about this [1], I initially leaned towards
"they should both fail", but I reconsidered: there doesn't seem to be
any harm in allowing ALTER SYSTEM SET to succeed for any custom GUC
name, as long as you're superuser.

Hence, attached is a patch for that.  Much of it is refactoring to
avoid duplicating the code that checks for a reserved GUC name, which
I think should still be done here --- otherwise, we're losing a lot of
the typo detection that that check was intended to provide.  (That is,
if you have loaded an extension that defines "foo" as a prefix, we
should honor the extension's opinion about whether "foo.bar" is
valid.)  I also fixed the code for GRANT ON PARAMETER so that it
follows the same rules and throws the same errors for invalid cases.

There's a chunk of AlterSystemSetConfigFile that now needs indenting
one more tab stop, but I didn't do that yet for ease of review.

Thoughts?

			regards, tom lane

[1] https://www.postgresql.org/message-id/flat/169746329791.169914.16613647309012285391%40wrigleys.postgresql.org

Re: Allow ALTER SYSTEM SET on unrecognized custom GUCs

Andrei Lepikhov <a.lepikhov@postgrespro.ru> — 2023-10-18T04:55:48Z

On 17/10/2023 07:19, Tom Lane wrote:
> Currently we have this odd behavior (for a superuser):
> 
> regression=# ALTER SYSTEM SET foo.bar TO 'baz';
> ERROR:  unrecognized configuration parameter "foo.bar"
> regression=# SET foo.bar TO 'baz';
> SET
> regression=# ALTER SYSTEM SET foo.bar TO 'baz';
> ALTER SYSTEM
> 
> That is, you can't ALTER SYSTEM SET a random custom GUC unless there
> is already a placeholder GUC for it, because the find_option call in
> AlterSystemSetConfigFile fails.  This is surely pretty inconsistent.
> Either the first ALTER SYSTEM SET ought to succeed, or the second one
> ought to fail too, because we don't have any more knowledge about the
> custom GUC than we did before.
> 
> In the original discussion about this [1], I initially leaned towards
> "they should both fail", but I reconsidered: there doesn't seem to be
> any harm in allowing ALTER SYSTEM SET to succeed for any custom GUC
> name, as long as you're superuser.
> 
> Hence, attached is a patch for that.  Much of it is refactoring to
> avoid duplicating the code that checks for a reserved GUC name, which
> I think should still be done here --- otherwise, we're losing a lot of
> the typo detection that that check was intended to provide.  (That is,
> if you have loaded an extension that defines "foo" as a prefix, we
> should honor the extension's opinion about whether "foo.bar" is
> valid.)  I also fixed the code for GRANT ON PARAMETER so that it
> follows the same rules and throws the same errors for invalid cases.
> 
> There's a chunk of AlterSystemSetConfigFile that now needs indenting
> one more tab stop, but I didn't do that yet for ease of review.
> 
> Thoughts?

I have reviewed this patch. It looks good in general. Now, we can change 
the placeholder value with the SET command and have one more tool (which 
may be unusual) to pass some data through the session.
Keeping away from the reason why DBMS allows such behaviour, I have one 
question:
"SET foo.bar TO 'smth'" can immediately alter the placeholder's value. 
But what is the reason that "ALTER SYSTEM SET foo.bar TO 'smth'" doesn't 
do the same?

-- 
regards,
Andrey Lepikhov
Postgres Professional

Re: Allow ALTER SYSTEM SET on unrecognized custom GUCs

Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-18T05:15:11Z

Andrei Lepikhov <a.lepikhov@postgrespro.ru> writes:
> "SET foo.bar TO 'smth'" can immediately alter the placeholder's value. 
> But what is the reason that "ALTER SYSTEM SET foo.bar TO 'smth'" doesn't 
> do the same?

Because it's not supposed to take effect until you issue a reload
command (and maybe not even then, depending on which GUC we're
talking about).  I certainly think it wouldn't make sense for your
own session to adopt the value ahead of others.

			regards, tom lane

Re: Allow ALTER SYSTEM SET on unrecognized custom GUCs

Andrei Lepikhov <a.lepikhov@postgrespro.ru> — 2023-10-18T05:55:52Z

On 18/10/2023 12:15, Tom Lane wrote:
> Andrei Lepikhov <a.lepikhov@postgrespro.ru> writes:
>> "SET foo.bar TO 'smth'" can immediately alter the placeholder's value.
>> But what is the reason that "ALTER SYSTEM SET foo.bar TO 'smth'" doesn't
>> do the same?
> 
> Because it's not supposed to take effect until you issue a reload
> command (and maybe not even then, depending on which GUC we're
> talking about).  I certainly think it wouldn't make sense for your
> own session to adopt the value ahead of others.

Thanks for the answer.
Introducing the assignable_custom_variable_name can be helpful. The code 
looks good. I think it deserves to be committed - after the indentation 
fix, of course.

-- 
regards,
Andrey Lepikhov
Postgres Professional

Re: Allow ALTER SYSTEM SET on unrecognized custom GUCs

shihao zhong <zhong950419@gmail.com> — 2023-10-19T13:58:05Z

I do like the idea that we should keep the set and the altar system with
the same behavior. But one thing I am worried about is the typo detected
here because I usually make that type of mistake myself. I believe we
should have an extra log to explicitly tell the user this is a `custom
variable` guc.

Btw, another aspect I want to better understand is if the superuser session
called pg_reload_conf with custom variables, does that mean these custom
variables will override the other active transaction's SET command?

Thanks,
Shihao

On Wed, Oct 18, 2023 at 1:59 AM Andrei Lepikhov <a.lepikhov@postgrespro.ru>
wrote:

> On 18/10/2023 12:15, Tom Lane wrote:
> > Andrei Lepikhov <a.lepikhov@postgrespro.ru> writes:
> >> "SET foo.bar TO 'smth'" can immediately alter the placeholder's value.
> >> But what is the reason that "ALTER SYSTEM SET foo.bar TO 'smth'" doesn't
> >> do the same?
> >
> > Because it's not supposed to take effect until you issue a reload
> > command (and maybe not even then, depending on which GUC we're
> > talking about).  I certainly think it wouldn't make sense for your
> > own session to adopt the value ahead of others.
>
> Thanks for the answer.
> Introducing the assignable_custom_variable_name can be helpful. The code
> looks good. I think it deserves to be committed - after the indentation
> fix, of course.
>
> --
> regards,
> Andrey Lepikhov
> Postgres Professional
>
>
>
>

Re: Allow ALTER SYSTEM SET on unrecognized custom GUCs

Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-19T16:00:21Z

shihao zhong <zhong950419@gmail.com> writes:
> I do like the idea that we should keep the set and the altar system with
> the same behavior. But one thing I am worried about is the typo detected
> here because I usually make that type of mistake myself. I believe we
> should have an extra log to explicitly tell the user this is a `custom
> variable` guc.

I don't think there's any chance of getting away with that.  As noted
upthread, a lot of people use placeholder GUCs as a substitute for a
proper session-variable feature.  If we ever get real session variables,
we could start to nudge people away from using placeholders; but right
now too many people would complain about the noise of a warning.

> Btw, another aspect I want to better understand is if the superuser session
> called pg_reload_conf with custom variables, does that mean these custom
> variables will override the other active transaction's SET command?

No, a per-session SET will override a value coming from the config file.
That's independent of whether it's a regular or custom GUC.

			regards, tom lane

Re: Allow ALTER SYSTEM SET on unrecognized custom GUCs

shihao zhong <zhong950419@gmail.com> — 2023-10-19T16:05:44Z

Thanks for the answer. The code looks good to me.

Thanks,
Shihao

On Thu, Oct 19, 2023 at 12:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

> shihao zhong <zhong950419@gmail.com> writes:
> > I do like the idea that we should keep the set and the altar system with
> > the same behavior. But one thing I am worried about is the typo detected
> > here because I usually make that type of mistake myself. I believe we
> > should have an extra log to explicitly tell the user this is a `custom
> > variable` guc.
>
> I don't think there's any chance of getting away with that.  As noted
> upthread, a lot of people use placeholder GUCs as a substitute for a
> proper session-variable feature.  If we ever get real session variables,
> we could start to nudge people away from using placeholders; but right
> now too many people would complain about the noise of a warning.
>
> > Btw, another aspect I want to better understand is if the superuser
> session
> > called pg_reload_conf with custom variables, does that mean these custom
> > variables will override the other active transaction's SET command?
>
> No, a per-session SET will override a value coming from the config file.
> That's independent of whether it's a regular or custom GUC.
>
>                         regards, tom lane
>

Re: Allow ALTER SYSTEM SET on unrecognized custom GUCs

x4mmm@yandex-team.ru — 2023-10-19T17:29:13Z


> On 17 Oct 2023, at 05:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> In the original discussion about this [1], I initially leaned towards
> "they should both fail", but I reconsidered: there doesn't seem to be
> any harm in allowing ALTER SYSTEM SET to succeed for any custom GUC
> name, as long as you're superuser.

+1 for allowing non-existent custom GUCs.
From time to time we have to roll out custom binaries controlled by GUCs that do not exist in normal binaries. Juggling with postgresql.conf would be painful in this case.


Best regards, Andrey Borodin.

Re: Allow ALTER SYSTEM SET on unrecognized custom GUCs

Andrew Dunstan <andrew@dunslane.net> — 2023-10-23T14:19:54Z

On 2023-10-16 Mo 20:19, Tom Lane wrote:
> Currently we have this odd behavior (for a superuser):
>
> regression=# ALTER SYSTEM SET foo.bar TO 'baz';
> ERROR:  unrecognized configuration parameter "foo.bar"
> regression=# SET foo.bar TO 'baz';
> SET
> regression=# ALTER SYSTEM SET foo.bar TO 'baz';
> ALTER SYSTEM
>
> That is, you can't ALTER SYSTEM SET a random custom GUC unless there
> is already a placeholder GUC for it, because the find_option call in
> AlterSystemSetConfigFile fails.  This is surely pretty inconsistent.
> Either the first ALTER SYSTEM SET ought to succeed, or the second one
> ought to fail too, because we don't have any more knowledge about the
> custom GUC than we did before.
>
> In the original discussion about this [1], I initially leaned towards
> "they should both fail", but I reconsidered: there doesn't seem to be
> any harm in allowing ALTER SYSTEM SET to succeed for any custom GUC
> name, as long as you're superuser.
>
> Hence, attached is a patch for that.  Much of it is refactoring to
> avoid duplicating the code that checks for a reserved GUC name, which
> I think should still be done here --- otherwise, we're losing a lot of
> the typo detection that that check was intended to provide.  (That is,
> if you have loaded an extension that defines "foo" as a prefix, we
> should honor the extension's opinion about whether "foo.bar" is
> valid.)  I also fixed the code for GRANT ON PARAMETER so that it
> follows the same rules and throws the same errors for invalid cases.
>
> There's a chunk of AlterSystemSetConfigFile that now needs indenting
> one more tab stop, but I didn't do that yet for ease of review.
>
> Thoughts?
>
> 			


Haven't read the patch but in principle I agree.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com