On Mon, Jul 15, 2024 at 01:47:14PM -0500, Nathan Bossart wrote: > On Mon, Jul 15, 2024 at 02:30:42PM -0400, Robert Haas wrote: >> I guess I'm in the group of people who doesn't understand how this can >> possibly work. There's no guarantee about the order in which GUC check >> hooks are called, so you don't know if the value of the other variable >> has already been set to the final value or not, which seems like a >> fatal problem even if the code happens to work correctly as of today. >> Even if you have such a guarantee, you can't prohibit a configuration >> change at pg_ctl reload time: the server can refuse to start in case >> of an invalid configuration, but a running server can't decide to shut >> down or stop working at reload time. > > My understanding is that the correctness of this GUC check hook depends on > wal_level being a PGC_POSTMASTER GUC. The check hook would always return > true during startup, and there'd be an additional cross-check in > PostmasterMain() that would fail startup if necessary. After that point, > we know that wal_level cannot change, so the GUC check hook for > summarize_wal can depend on wal_level. If it fails, my expectation would > be that the server would just ignore that change and continue.
I should also note that since wal_level defaults to "replica", I don't think we even need any extra "always return true on startup" logic. If wal_level is set prior to summarize_wal, the check hook will fail startup as needed. If summarize_wal is set first, the check hook will return true, and we'll fall back on the PostmasterMain() check (that already exists). In short, the original patch [0] seems like it should work in this particular scenario, barring some corner case I haven't discovered. That being said, it's admittedly fragile and probably not a great precedent to set. I've been thinking about some ideas for more generic GUC dependency tooling, but I don't have anything to share yet. [0] https://postgr.es/m/attachment/161852/v1-0001-Prevent-summarize_wal-from-enabling-when-wal_leve.patch -- nathan
