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

On Sun, Jul 14, 2024 at 10:56 PM Fujii Masao
<masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
> I don't think it's a rare scenario since summarize_wal can be enabled
> after starting the server with wal_level=minimal. Therefore, I believe
> such a configuration should be prohibited using a GUC check hook,
> as my patch does.

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.

> Alternatively, we should at least report or
> log something when summarize_wal is enabled but fast_forward is also
> enabled, so users can easily detect or investigate this unexpected situation.
> I'm not sure if exposing fast_forward is necessary for that or not...

To be honest, I'm uncomfortable with how much time is passing while we
debate these details. I feel like we need to get these WAL format
changes done sooner rather than later considering beta2 is already out
the door. Other changes like logging messages or not can be debated
once the basic fix is in. Even if they don't happen, nobody will have
a corrupted backup once the basic fix is done. At most they will be
confused about why some backup is failing.

> Regarding pg_get_wal_summarizer_state(), it is documented that
> summarized_lsn indicates the ending LSN of the last WAL summary file
> written to disk. However, with the patch, summarized_lsn advances
> even when fast_forward is enabled. The documentation should be updated,
> or summarized_lsn should be changed so it doesn't advance while
> fast_forward is enabled.

I think we need to advance summarized_lsn to get the proper behavior.
I can update the documentation.

> I have one small comment:
>
> +# This test aims to validate that takeing an incremental backup fails when
>
> "takeing" should be "taking"?

Will fix, thanks.

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