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

On 2024/07/12 1:16, Robert Haas wrote:
> On Thu, Jul 11, 2024 at 6:51 AM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>> It looks like the fast_forward field in WalSummarizerData is no longer necessary.
>>
>> So far, I haven't found any other issues with the patch.
>
> Thanks for reviewing. Regarding fast_forward, I think I had the idea
> in mind that perhaps it should be exposed by
> pg_get_wal_summarizer_state(),

Understood.

> but I didn't actually implement that.
> Thinking about it again, I think maybe it's fine to just remove it
> from the shared memory state, as this should be a rare scenario in
> practice. What is your opinion?

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. 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...

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.

On 2024/07/12 3:00, Robert Haas wrote:
> On Thu, Jul 11, 2024 at 6:51 AM Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>> So far, I haven't found any other issues with the patch.
>
> Here is a new version that removes the hunks you highlighted and also
> adds a test case.

Thanks for updating the patch! LGTM.

I have one small comment:

+# This test aims to validate that takeing an incremental backup fails when

"takeing" should be "taking"?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION