Message-ID: <802ff3d5-fdc7-4b08-9046-2e9fc5fdae0e@oss.nttdata.com>
Date: Thu, 4 Jul 2024 23:35:00 +0900
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Add a GUC check hook to ensure summarize_wal cannot be enabled
 when wal_level is minimal
Content-Language: en-US
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
References: <6e30082e-041b-4e31-9633-95a66de76f5d@oss.nttdata.com>
 <ZoVgXuYWbzTLXyRY@nathan>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
In-Reply-To: <ZoVgXuYWbzTLXyRY@nathan>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/802ff3d5-fdc7-4b08-9046-2e9fc5fdae0e%40oss.nttdata.com>
Precedence: bulk


On 2024/07/03 23:29, Nathan Bossart wrote:
> On Wed, Jul 03, 2024 at 11:08:48PM +0900, Fujii Masao wrote:
>> +/*
>> + * GUC check_hook for summarize_wal
>> + */
>> +bool
>> +check_summarize_wal(bool *newval, void **extra, GucSource source)
>> +{
>> +	if (*newval && wal_level == WAL_LEVEL_MINIMAL)
>> +	{
>> +		GUC_check_errmsg("WAL cannot be summarized when \"wal_level\" is \"minimal\"");
>> +		return false;
>> +	}
>> +	return true;
>> +}
> 
> IME these sorts of GUC hooks that depend on the value of other GUCs tend to
> be quite fragile.  This one might be okay because wal_level defaults to
> 'replica' and because there is an additional check in postmaster.c, but
> that at least deserves a comment.

Yes, I didn't add a cross-check for wal_level and summarize_wal intentionally
because wal_level is a PGC_POSTMASTER option, and incorrect settings of them
are already checked at server startup. However, I agree that adding a comment
about this would be helpful.

BTW, another concern with summarize_wal is that it might not be enough to
just check the summarize_wal and wal_level settings. We also need to
ensure that WAL data generated with wal_level=minimal is not summarized.

For example, if wal_level is changed from minimal to replica or logical,
some old WAL data generated with wal_level=minimal might still exist in pg_wal.
In this case, if summarize_wal is enabled, the settings of wal_level and
summarize_wal is valid, but the started WAL summarizer might summarize
this old WAL data unexpectedly.

I haven't reviewed the code regarding how the WAL summarizer chooses
its starting point, so I'm not sure if there's a real risk of summarizing
WAL data generated with wal_level=minimal.


> This sort of thing comes up enough that perhaps we should add a
> better-supported way to deal with GUCs that depend on each other...

+1 for v18 or later. However, since the reported issue is in v17,
it needs to be addressed without such a improved check mechanism.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION