On 2024/07/12 1:16, Robert Haas wrote:
On Thu, Jul 11, 2024 at 6:51 AM Fujii Masao <masao.fu...@oss.nttdata.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.fu...@oss.nttdata.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
