Re: pg_settings.pending_restart not set when line removed
Alexander Kukushkin writes: > IMO is totally wrong, because the actual value didn't change: it was an > empty string in the config and now it remains an empty string due to the > default value in the guc.c I can't get very excited about that. The existing message about "parameter \"%s\" cannot be changed without restarting the server" was emitted without regard to that fine point, and nobody has complained about it. regards, tom lane
Re: pg_settings.pending_restart not set when line removed
Hi Alvaro, On Tue, 27 Jul 2021 at 22:17, Alvaro Herrera wrote: > I tested this -- it works correctly AFAICS. > Nope, IMO it doesn't work correctly. Lets say we have recovery_target = '' in the config: localhost/postgres=# select name, setting, setting is null, pending_restart from pg_settings where name = 'recovery_target'; name │ setting │ ?column? │ pending_restart ─┼─┼──┼─ recovery_target │ │ f│ f (1 row) After that we remove it from the config and call pg_ctl reload. It sets the panding_restart. localhost/postgres=# select name, setting, setting is null, pending_restart from pg_settings where name = 'recovery_target'; name │ setting │ ?column? │ pending_restart ─┼─┼──┼─ recovery_target │ │ f│ t (1 row) IMO is totally wrong, because the actual value didn't change: it was an empty string in the config and now it remains an empty string due to the default value in the guc.c Regards, -- Alexander Kukushkin
Re: pg_settings.pending_restart not set when line removed
On 2021-Jul-27, Tom Lane wrote:
> So maybe like
>
> if (gconf->context < PGC_SIGHUP)
> {
> + /* The removal can't be effective without a restart */
> + gconf->status |= GUC_PENDING_RESTART;
> ereport(elevel,
> (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
Thanks, done that way.
> One thing worth checking is whether the pending-restart flag
> gets cleared again if the DBA undoes the removal and again
> reloads. I think the right thing will happen, but it'd be
> worthwhile to check.
I tested this -- it works correctly AFAICS.
Thanks!
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Al principio era UNIX, y UNIX habló y dijo: "Hello world\n".
No dijo "Hello New Jersey\n", ni "Hello USA\n".
Re: pg_settings.pending_restart not set when line removed
Alvaro Herrera writes:
> On 2021-Jul-27, Tom Lane wrote:
>> Ugh. I think this patch is likely to create more problems than it fixes.
> I doubt that; as I said, the code already behaves in exactly that way
> for closely related operations, so this patch isn't doing anything new.
> Note that that loop this code is modifying only applies to lines that
> are removed from the config file.
Ah ... what's wrong here is some combination of -ENOCAFFEINE and a
not-great explanation on your part. I misread the patch as adding
"error = true" rather than the flag change. I agree that setting
the GUC_PENDING_RESTART flag is fine, because set_config_option
would do so if we reached it. Perhaps you should comment this
along that line? Also, the cases inside set_config_option
uniformly set that flag *before* the ereport not after.
So maybe like
if (gconf->context < PGC_SIGHUP)
{
+ /* The removal can't be effective without a restart */
+ gconf->status |= GUC_PENDING_RESTART;
ereport(elevel,
(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
One thing worth checking is whether the pending-restart flag
gets cleared again if the DBA undoes the removal and again
reloads. I think the right thing will happen, but it'd be
worthwhile to check.
regards, tom lane
Re: pg_settings.pending_restart not set when line removed
On 2021-Jul-27, Tom Lane wrote: > Alvaro Herrera writes: > > You could argue that this is *weird* (why does reading pg_file_settings > > set a flag in global state?) ... but that weirdness is not something > > this patch is introducing. > > Ugh. I think this patch is likely to create more problems than it fixes. I doubt that; as I said, the code already behaves in exactly that way for closely related operations, so this patch isn't doing anything new. Note that that loop this code is modifying only applies to lines that are removed from the config file. > We should be looking to get rid of that flag, not make its behavior even > more complex. Are you proposing to remove the pending_restart column from pg_settings? That seems a step backwards. What I know is that the people behind management interfaces need some way to know if changes to the config need a system restart. Now maybe we want that feature to be implemented in a different way than it currently is. I frankly don't care enough to do that myself. I agree that the current mechanism is weird, but it's going to take more than a one-liner to fix it. The one-liner is only intended to fix a very specific problem. -- Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/ "After a quick R of TFM, all I can say is HOLY CR** THAT IS COOL! PostgreSQL was amazing when I first started using it at 7.2, and I'm continually astounded by learning new features and techniques made available by the continuing work of the development team." Berend Tober, http://archives.postgresql.org/pgsql-hackers/2007-08/msg01009.php
Re: pg_settings.pending_restart not set when line removed
Alvaro Herrera writes: > You could argue that this is *weird* (why does reading pg_file_settings > set a flag in global state?) ... but that weirdness is not something > this patch is introducing. Ugh. I think this patch is likely to create more problems than it fixes. We should be looking to get rid of that flag, not make its behavior even more complex. regards, tom lane
Re: pg_settings.pending_restart not set when line removed
> On 27 Jul 2021, at 01:02, Alvaro Herrera wrote: > I tried the attached patch, which sets GUC_PENDING_RESTART if we're > doing pg_file_settings(). Then any subsequent read of pg_settings will > have the pending_restart flag set. This seems to work correctly, and > consistently with the case where you change a line (without removing it) > in unpatched master. LGTM after testing this with various changes and ways to reload, and +1 for being consistent with changing a line. > You could argue that this is *weird* (why does reading pg_file_settings > set a flag in global state?) ... but that weirdness is not something > this patch is introducing. Agreed. Another unrelated weird issue is that we claim that the config file "contains errors" if the context is < PGC_SIGHUP for restart required settings. It seems a bit misleading to call pending_restart an error since it implies (in my reading) there were syntax errors. But, unrelated to this patch and report (and it's been like that for a long time), just hadn't noticed that before. -- Daniel Gustafsson https://vmware.com/
