Re: pg_rewind WAL segments deletion pitfall

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: torikoshia <torikoshia(at)oss(dot)nttdata(dot)com>
Cc: Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org>, bungina(at)gmail(dot)com, cyberdemn(at)gmail(dot)com, pgsql-hackers(at)lists(dot)postgresql(dot)org, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
Subject: Re: pg_rewind WAL segments deletion pitfall
Date: 2023-08-22 05:32:06
Message-ID: ZORIVnxs+lXoEAQG@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-bugs pgsql-hackers

On Fri, Aug 18, 2023 at 03:40:57PM +0900, torikoshia wrote:
> Thanks for the patch, I've marked this as ready-for-committer.
>
> BTW, this issue can be considered a bug, right?
> I think it would be appropriate to provide backpatch.

Hmm, I agree that there is a good argument in back-patching as we have
the WAL files between the redo LSN and the divergence LSN, but
pg_rewind is not smart enough to keep them around. If the archives of
the primary were not able to catch up, the old primary is as good as
kaput, and restore_command won't help here.

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()? Also, I am wondering if we should be smarter
with any potential conflict handling between the source and the
target, rather than just enforcing a FILE_ACTION_NONE for all these
files. In short, could it be better to copy the WAL file from the
source if it exists there?

+ /*
+ * 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.

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

In response to

Responses

Browse pgsql-bugs by date

  From Date Subject
Next Message PG Bug reporting form 2023-08-22 07:11:30 BUG #18063: postgresql-x64-14 service not working
Previous Message Michael Paquier 2023-08-22 01:43:59 Re: cache lookup failed dropping public schema with trgm index

Browse pgsql-hackers by date

  From Date Subject
Next Message David Rowley 2023-08-22 05:36:34 Re: PG 16 draft release notes ready
Previous Message Amit Kapila 2023-08-22 04:45:07 Re: [PoC] pg_upgrade: allow to upgrade publisher node