pg_settings.pending_restart not set when line removed

On 27 Jul 2021, at 01:02, Alvaro Herrera <alvherre@alvh.no-ip.org> 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/

Alvaro Herrera <alvherre@alvh.no-ip.org> 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

On 2021-Jul-27, Tom Lane wrote:

Alvaro Herrera <alvherre@alvh.no-ip.org> 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

Alvaro Herrera <alvherre@alvh.no-ip.org> 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

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

Hi Alvaro,

On Tue, 27 Jul 2021 at 22:17, Alvaro Herrera <alvherre@alvh.no-ip.org>
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

Alexander Kukushkin <cyberdemn@gmail.com> 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