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
