Message-ID: <8a668c51-c6af-4744-a978-c6a2ce3b1d83@oss.nttdata.com>
Date: Mon, 15 Jul 2024 11:56:42 +0900
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Add a GUC check hook to ensure summarize_wal cannot be enabled
 when wal_level is minimal
Content-Language: en-US
To: Robert Haas <robertmhaas@gmail.com>
Cc: Nathan Bossart <nathandbossart@gmail.com>,
 pgsql-hackers@lists.postgresql.org
References: <6e30082e-041b-4e31-9633-95a66de76f5d@oss.nttdata.com>
 <ZoVgXuYWbzTLXyRY@nathan>
 <802ff3d5-fdc7-4b08-9046-2e9fc5fdae0e@oss.nttdata.com>
 <CA+TgmoYPNfgAT3OubjoYSiXTb3-urOjQxT3SCEUCrRBx5g6ihA@mail.gmail.com>
 <CA+TgmoY8DnzWNK_xu4=NTYdxGBGnFucrEpOALnS0awMp-s_pKQ@mail.gmail.com>
 <7c1b48d8-0f08-4733-a4e6-d55f04581db7@oss.nttdata.com>
 <CA+TgmoansG0B8ayQGw9Ozee6Jawz=HFUACpO_4gp-+SFJBxiYw@mail.gmail.com>
 <CA+TgmoZ53ADUm2UAGEspMuv-Db5SGtrOXUx=ps6LcGexsf5-fA@mail.gmail.com>
 <e2549085-5246-4edc-bc62-ae19df48acc5@oss.nttdata.com>
 <CA+TgmoYFOWbxk=FP+ZnxwVaKQEce=+0Xvzh0DUC9=YLNHiQW1w@mail.gmail.com>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
In-Reply-To: 
 <CA+TgmoYFOWbxk=FP+ZnxwVaKQEce=+0Xvzh0DUC9=YLNHiQW1w@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/8a668c51-c6af-4744-a978-c6a2ce3b1d83%40oss.nttdata.com>
Precedence: bulk


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