Date: Wed, 10 Jul 2024 11:15:59 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Jelte Fennema-Nio <postgres@jeltef.nl>,
	Robert Haas <robertmhaas@gmail.com>,
	Fujii Masao <masao.fujii@oss.nttdata.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Add a GUC check hook to ensure summarize_wal cannot be enabled
 when wal_level is minimal
Message-ID: <Zo6zv2XGGJJugTOF@nathan>
References: <802ff3d5-fdc7-4b08-9046-2e9fc5fdae0e@oss.nttdata.com>
 <CA+TgmoYPNfgAT3OubjoYSiXTb3-urOjQxT3SCEUCrRBx5g6ihA@mail.gmail.com>
 <CA+TgmoY8DnzWNK_xu4=NTYdxGBGnFucrEpOALnS0awMp-s_pKQ@mail.gmail.com>
 <7c1b48d8-0f08-4733-a4e6-d55f04581db7@oss.nttdata.com>
 <CA+TgmoansG0B8ayQGw9Ozee6Jawz=HFUACpO_4gp-+SFJBxiYw@mail.gmail.com>
 <Zo6YPuvTmRW3b1s7@nathan>
 <CAGECzQQcLYiAdRj5xAm1G43MtpD_rm029ziqrbpqyDdxaCoEQA@mail.gmail.com>
 <1744272.1720624273@sss.pgh.pa.us>
 <Zo6sTje4m0czRor9@nathan>
 <1749638.1720626878@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1749638.1720626878@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/Zo6zv2XGGJJugTOF%40nathan>
Precedence: bulk

On Wed, Jul 10, 2024 at 11:54:38AM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.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