From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Add a GUC check hook to ensure summarize_wal cannot be enabled when wal_level is minimal
Date: Thu, 4 Jul 2024 23:35:00 +0900
Message-ID: <802ff3d5-fdc7-4b08-9046-2e9fc5fdae0e@oss.nttdata.com> (raw)
In-Reply-To: <ZoVgXuYWbzTLXyRY@nathan>
References: <6e30082e-041b-4e31-9633-95a66de76f5d@oss.nttdata.com>
<ZoVgXuYWbzTLXyRY@nathan>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: masao.fujii@oss.nttdata.com, nathandbossart@gmail.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
In-Reply-To: <802ff3d5-fdc7-4b08-9046-2e9fc5fdae0e@oss.nttdata.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox