MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Tue, 29 Aug 2023 22:15:51 +0900
From: torikoshia <torikoshia@oss.nttdata.com>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, cyberdemn@gmail.com
Cc: michael@paquier.xyz, pgsql-hackers@postgresql.org, bungina@gmail.com,
 pgsql-hackers@lists.postgresql.org
Subject: Re: pg_rewind WAL segments deletion pitfall
In-Reply-To: <20230824.094538.2107929879048686192.horikyota.ntt@gmail.com>
References: <8b385bb6d5f87e54c1c6333fece0444a@oss.nttdata.com>
 <ZORIVnxs+lXoEAQG@paquier.xyz>
 <CAFh8B=k2DUwruBA3djsMf3b_wfqeN092iqaDcZYJ4PAgnne2KA@mail.gmail.com>
 <20230824.094538.2107929879048686192.horikyota.ntt@gmail.com>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <bb706f8393c5207812268a5a2e3f98d7@oss.nttdata.com>
Archived-At: 
 <https://www.postgresql.org/message-id/bb706f8393c5207812268a5a2e3f98d7%40oss.nttdata.com>
Precedence: bulk

On 2023-08-24 09:45, Kyotaro Horiguchi wrote:
> At Wed, 23 Aug 2023 13:44:52 +0200, Alexander Kukushkin
> <cyberdemn@gmail.com> wrote in
>> On Tue, 22 Aug 2023 at 07:32, Michael Paquier <michael@paquier.xyz> 
>> wrote:
>> > I don't like much this patch.  While it takes correctly advantage of
>> > the backward record read logic from SimpleXLogPageRead() able to
>> > handle correctly timeline jumps, it creates a hidden dependency in the
>> > code between the hash table from filemap.c and the page callback.
>> > Wouldn't it be simpler to build a list of the segment names using the
>> > information from WALOpenSegment and build this list in
>> > findLastCheckpoint()?
>> 
>> I think the first version of the patch more or less did that. Not
>> necessarily a list, but a hash table of WAL file names that we want to
>> keep. But Kyotaro Horiguchi didn't like it and suggested creating 
>> entries
>> in the filemap.c hash table instead.
>> But, I agree, doing it directly from the findLastCheckpoint() makes 
>> the
>> code easier to understand.
> ...
>> > +       /*
>> > +        * Some entries (WAL segments) already have an action assigned
>> > +        * (see SimpleXLogPageRead()).
>> > +        */
>> > +       if (entry->action == FILE_ACTION_UNDECIDED)
>> > +           entry->action = decide_file_action(entry);
>> >
>> > This change makes me a bit uneasy, per se my previous comment with the
>> > additional code dependencies.
>> >
>> 
>> We can revert to the original approach (see
>> v1-0001-pg_rewind-wal-deletion.patch from the very first email) if you 
>> like.
> 
> On the other hand, that approach brings in another source that
> suggests the way that file should be handled. I still think that
> entry->action should be the only source.

+1.
Imaging a case when we come to need decide how to treat files based on 
yet another factor, I feel that a single source of truth is better than 
creating a list or hash for each factor.

> However, it seems I'm in the
> minority here. So I'm not tied to that approach.
> 
>> > I think that this scenario deserves a test case.  If one wants to
>> > emulate a delay in WAL archiving, it is possible to set
>> > archive_command to a command that we know will fail, for instance.
>> >
>> 
>> Yes, I totally agree, it is on our radar, but meanwhile please see the 
>> new
>> version, just to check if I correctly understood your idea.

Thanks for the patch.
I tested v4 patch using the script attached below thread and it has 
successfully finished.

https://www.postgresql.org/message-id/2e75ae22dce9a227c3d47fa6d0ed094a%40oss.nttdata.com


-- 
Regards,

--
Atsushi Torikoshi
NTT DATA Group Corporation