Re: Allow ALTER SYSTEM SET on unrecognized custom GUCs
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
Re: Allow ALTER SYSTEM SET on unrecognized custom GUCs
> On 17 Oct 2023, at 05:19, Tom Lane 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
Thanks for the answer. The code looks good to me. Thanks, Shihao On Thu, Oct 19, 2023 at 12:00 PM Tom Lane wrote: > shihao zhong 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 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
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 wrote: > On 18/10/2023 12:15, Tom Lane wrote: > > Andrei Lepikhov 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
On 18/10/2023 12:15, Tom Lane wrote: Andrei Lepikhov 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
Andrei Lepikhov 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
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
