Re: pg_rewind WAL segments deletion pitfall

[Alena Vinter]

Hi,

I noticed some strange behaviour in the keepwal logic.

The keepwal filename is built from the timeline of the segment being
read, which is the *target's* TLI:

    current_tli = xlogreader->seg.ws_tli;
    XLogFileName(xlogfname + sizeof(XLOGDIR), current_tli, ...);
    keepwal_add_entry(xlogfname);

When the source cluster is on a lower timeline than the target, the
rewound node ends up with stray segment from a timeline newer than
the source's, e.g.:

    000000020000000000000002 (rewound)
    000000030000000000000002 (rewound)
    000000040000000000000002 (rewound)
    000000080000000000000002 (kept by keepwal)

I have a small fix that clamps the keepwal TLI to the source's latest
timeline, so each entry refers to a segment the source can actually
hold:

    current_tli = Min(xlogreader->seg.ws_tli, source_tli);

A TAP test (012_leftover_wal_segment.pl) reproduces by bumping the
target's timeline past the divergence point, promoting the standby as
the lower-TLI source, rewinding, and asserting that no segment left in
the target's pg_wal carries a TLI above the source's.  It fails
without the patch and passes with it.

Patch attached.  Thoughts?

012_leftover_wal_segment.pl
Description: Perl program

diff --git a/src/bin/pg_rewind/parsexlog.c b/src/bin/pg_rewind/parsexlog.c
index 023e23b063c..cdb8cc2272d 100644
--- a/src/bin/pg_rewind/parsexlog.c
+++ b/src/bin/pg_rewind/parsexlog.c
@@ -166,6 +166,7 @@ readOneRecord(const char *datadir, XLogRecPtr ptr, int tliIndex,
  */
 void
 findLastCheckpoint(const char *datadir, XLogRecPtr forkptr, int tliIndex,
+				   TimeLineID source_tli,
 				   XLogRecPtr *lastchkptrec, TimeLineID *lastchkpttli,
 				   XLogRecPtr *lastchkptredo, const char *restoreCommand)
 {
@@ -227,8 +228,14 @@ findLastCheckpoint(const char *datadir, XLogRecPtr forkptr, int tliIndex,
 
 			snprintf(xlogfname, MAXFNAMELEN, XLOGDIR "/");
 
-			/* update current values */
-			current_tli = xlogreader->seg.ws_tli;
+			/*
+			 * update current values
+			 *
+			 * These segments belong to the target, which may be on a higher
+			 * TLI than the source. Clamp to the source's latest TLI so that
+			 * keepwal entries refer to segments the source can hold.
+			 */
+			current_tli = Min(xlogreader->seg.ws_tli, source_tli);
 			current_segno = xlogreader->seg.ws_segno;
 
 			XLogFileName(xlogfname + sizeof(XLOGDIR),
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 2e86fd158d0..876061ff61d 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -471,6 +471,7 @@ main(int argc, char **argv)
 	keepwal_init();
 
 	findLastCheckpoint(datadir_target, divergerec, lastcommontliIndex,
+					   source_tli,
 					   &chkptrec, &chkpttli, &chkptredo, restore_command);
 	pg_log_info("rewinding from last common checkpoint at %X/%08X on timeline %u",
 				LSN_FORMAT_ARGS(chkptrec), chkpttli);
diff --git a/src/bin/pg_rewind/pg_rewind.h b/src/bin/pg_rewind/pg_rewind.h
index 9a981f7f246..d205f1f8cf1 100644
--- a/src/bin/pg_rewind/pg_rewind.h
+++ b/src/bin/pg_rewind/pg_rewind.h
@@ -36,7 +36,7 @@ extern void extractPageMap(const char *datadir, XLogRecPtr startpoint,
 						   int tliIndex, XLogRecPtr endpoint,
 						   const char *restoreCommand);
 extern void findLastCheckpoint(const char *datadir, XLogRecPtr forkptr,
-							   int tliIndex,
+							   int tliIndex, TimeLineID source_tli,
 							   XLogRecPtr *lastchkptrec, TimeLineID *lastchkpttli,
 							   XLogRecPtr *lastchkptredo,
 							   const char *restoreCommand);