Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal

On Wed, Jul 10, 2024 at 11:54:38AM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart(at)gmail(dot)com> writes:
>> I haven't tested it, but from skimming around the code, it looks like
>> ProcessConfigFileInternal() would deduplicate any previous entries in the
>> file prior to applying the values and running the check hooks. Else,
>> reloading a configuration file with multiple startup-only GUC entries could
>> fail, even without bogus GUC check hooks.
>
> While it's been a little while since I actually traced the logic,
> I believe the reason that case doesn't fail is this bit in
> set_config_with_handle, about line 3477 as of HEAD:
>
> case PGC_POSTMASTER:
> if (context == PGC_SIGHUP)
> {
> /*
> * We are re-reading a PGC_POSTMASTER variable from
> * postgresql.conf. We can't change the setting, so we should
> * give a warning if the DBA tries to change it. However,
> * because of variant formats, canonicalization by check
> * hooks, etc, we can't just compare the given string directly
> * to what's stored. Set a flag to check below after we have
> * the final storable value.
> */
> prohibitValueChange = true;
> }
> else if (context != PGC_POSTMASTER)
> // throw "cannot be changed now" error

That's what I thought at first, but then I saw this in
ProcessConfigFileInternal():

/* If it's already marked, then this is a duplicate entry */
if (record->status & GUC_IS_IN_FILE)
{
/*
* Mark the earlier occurrence(s) as dead/ignorable. We could
* avoid the O(N^2) behavior here with some additional state,
* but it seems unlikely to be worth the trouble.
*/
ConfigVariable *pitem;

for (pitem = head; pitem != item; pitem = pitem->next)
{
if (!pitem->ignore &&
strcmp(pitem->name, item->name) == 0)
pitem->ignore = true;
}
}

--
nathan