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

On Mon, Jul 15, 2024 at 4:10 PM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
> You don't, but the GUC check hook should always return true when
> summarize_wal is processed first. We'd rely on the PostmasterMain() check
> to fail in that case.

OK, I see. So at startup time, the check hook might or might not catch
a configuration mismatch, but if it doesn't, then the PostmasterMain()
check will fire. Later, we'd have an unsolvable problem if both
wal_level and summarize_wal could change, but since wal_level can't
change after startup time, we only need to check summarize_wal, and it
can rely on the value of wal_level being correct.

TBH, I don't want to do that. I think it's too fragile. It's the sort
of thing that just barely works given the exact behavior of these
particular GUCs, but it relies on a bunch of subtle assumptions which
won't be evident to future readers of the code. People will very
possibly copy this barely-working code into other contexts where it
doesn't work at all, or they'll think the code implementing this is
buggy even if it isn't.

We don't really need that check for correctness here, and it arguably
even prohibits more than necessary - e.g. suppose you crash without
summarizing all the WAL and then restart with wal_level=minimal. It's
perfectly fine to run the summarizer and have it catch up with all
possible pre-crash summarization, but the proposed change would
prohibit it. Granted, the current check would require you to start
with summarize_wal=off and then enable it later to get that done, but
we could remove that check. Or maybe we should leave it alone, but my
point is: if we have a reasonable option to decouple the values of two
GUCs so that the legal values of one don't depend on each other, I
think that is always going to be better and simpler than trying to
implement cross-checks on the values for which we don't have proper
infrastructure.

--
Robert Haas
EDB: http://www.enterprisedb.com