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

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