From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: pg_settings.pending_restart not set when line removed
In-reply-to: <202107271429.4m5ngdum7f66@alvherre.pgsql>
References: <202107271429.4m5ngdum7f66@alvherre.pgsql>
Comments: In-reply-to Alvaro Herrera <alvherre@alvh.no-ip.org>
	message dated "Tue, 27 Jul 2021 10:29:32 -0400"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <273673.1627398036.1@sss.pgh.pa.us>
Date: Tue, 27 Jul 2021 11:00:36 -0400
Message-ID: <273674.1627398036@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/273674.1627398036%40sss.pgh.pa.us>
Precedence: bulk

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