Thread: pg_settings.pending_restart not set when line removed

Hi

I got a report from Gabriele Bartolini and team that the pg_settings
view does not get the pending_restart flag set when a setting's line is
removed from a file (as opposed to its value changed).

The explanation seems to be that GUC_PENDING_RESTART is set by
set_config_option, but when ProcessConfigFileInternal is called only to
provide data (applySettings=true), then set_config_option is never
called and thus the flag doesn't get set.

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.

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.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Hay dos momentos en la vida de un hombre en los que no debería
especular: cuando puede permitírselo y cuando no puede" (Mark Twain)

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

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