pg.ddx.io  pgsql-bugs@postgresql.org mailing list archive  

help / color / mirror / Atom feed

From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Georgy Shelkovy <g.shelkovy@arenadata.io>
To: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
Subject: Re: BUG #18575: Sometimes pg_rewind mistakenly assumes that nothing needs to be done.
Date: Fri, 9 Aug 2024 18:26:29 +0300
Message-ID: <fe7af87d-b9bf-481a-902c-089c49aa911d@iki.fi> (raw)
In-Reply-To: <2171536a-8227-4e53-ac47-33b69093b61d@iki.fi>
References: <18575-1b9b3b60e2a480de@postgresql.org>
	<092dd515-b7b4-4fd0-8407-ceca2f02f6ec@iki.fi>
	<3382141723103572@mail.yandex.ru>
	<e08235e3-b216-4bf3-8a9a-8ea819ae105e@iki.fi>
	<188271723109777@mail.yandex.com>
	<3a626a21-319b-44e9-a2b5-94b8a93f9cd8@iki.fi>
	<31111723111906@mail.yandex.com>
	<1760421723118253@mail.yandex.com>
	<2171536a-8227-4e53-ac47-33b69093b61d@iki.fi>

On 08/08/2024 16:07, Heikki Linnakangas wrote:
> So it seems true that rewind is not required in those cases. However, if
> the WAL is already written on the standby's disk, just not replayed yet,
> then when you restart the server, it will replay the WAL from timeline
> 1. That does seem surprising. Perhaps pg_rewind should just update the
> minRecoveryPoint and minRecoveryTLI in control file in that case, to
> force WAL recovery to follow the timeline switch to TLI 2.
> 
> I will try to write a TAP test for the "Bad" and the assertion failure
> case, fix the assertion failure, and test how updating the
> minRecoveryPoint would behave.

I've divided this into three separate issues:

1. I think the assertion in pg_rewind is simply bogus, and we should 
remove it. Attached 0002-Remove-bogus-assertion-in-pg_rewind.patch does 
that, and adds a test case to cover it.

2. Independently of pg_rewind: When you start PostgreSQL, it will first 
try to recover all the WAL it has locally in pg_wal. That goes wrong if 
you have set a recovery target TLI. For example, imagine this situation:

- Recovery target TLI is 2, set explicitly in postgresql.conf
- The switchpoint from TLI 1 to 2 happened at WAL position 0/1510198 
(the switchpoint is found in 00000002.history)
- There is a WAL file 000000010000000000000001 under pg_wal, which 
contains valid WAL up to 0/1590000

When you start the server, it will first recover all the WAL from 
000000010000000000000001, up to 0/1590000. Then it will connect to the 
primary to fetch mor WAL, but it will fail to make any progress because 
it already blew past the switch point.

It's obviously wrong to replay the WAL from timeline 1 beyond the 1->2 
switchpoint, when the recovery target is TLI 2. The attached 
0003-Don-t-read-past-current-TLI-during-archive-recovery.patch fixes 
that. However, the logic to find the right WAL segment file and read the 
WAL is extremely complicated, and I don't feel comfortable that I got 
all the cases right. Review would be highly appreciated.

The patch includes a test case to demonstrate the case, with no 
pg_rewind. It does include one "manual" step to copy a timeline history 
file into pg_wal, marked with XXX, however. So I'm not sure how possible 
this scenario is in production setups .

3. When pg_rewind has nothing to do, the target server is left 
unmodified, in a state such that when you restart it, it will replay all 
the WAL it has locally in pg_wal first, before connecting to the 
primary. Even though the target is a direct ancestor of the source and 
hence it *can* follow the WAL to the source's position without 
rewinding, it doesn't mean that it *will* actually do so.

The attached  changes it so that it updates the control file in that 
case, setting minRecoveryPoint and minRecoveryPointTLI to point to the 
source's current WAL position. That way, when you start it up, it will 
follow the timeline history to reach that point. (This requires fixing 
issue 2, because otherwise it still won't follow the history correctly 
to reach the minRecoveryPointTLI)

To make the test pass, it actually would be sufficient to copy the 
timeline history file into pg_wal (which the patch also does). But 
updating the minRecoveryPoint seems good to ensure that it follows the 
right timeline. Otherwise it relies on the logic at startup to find the 
latest timeline, and that the latest timeline is the one you tried to 
rewind to. I think it would go wrong if there was another 
higher-numbered history file present in pg_wal for some reason.


All in all, I don't feel very confident about all this. The assertion 
seems straightforward, so barring objections I'll commit and backpatch 
that. The timeline-following at startup (issue 2) seems pretty clearly 
wrong, but I'm not sure it's worth the risk to backpatch. Similarly 
issue 3 might not be worth the risk to backpatch, especially if we don't 
also backpatch 2. I would love to hear comments on those.

Georgy, if you have the possibility to test this patches with your repro 
script, that would be highly appreciated.

-- 
Heikki Linnakangas
Neon (https://neon.tech)

Attachments:

  [text/x-patch] 0001-If-a-command-dies-in-a-command_checks_all-test-print.patch (1.1K, ../fe7af87d-b9bf-481a-902c-089c49aa911d@iki.fi/2-0001-If-a-command-dies-in-a-command_checks_all-test-print.patch)
  download | inline diff:
From 278496a655d425313af785a8153d9010044bc4b2 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Fri, 9 Aug 2024 14:13:24 +0300
Subject: [PATCH 1/4] If a command dies in a command_checks_all() test, print
 output

I noticed while writing a test case for an assertion failure in
pg_rewind. On assertion failure, it prints out the assertion to
stderr, but command_checks_all() swallowed it.
---
 src/test/perl/PostgreSQL/Test/Utils.pm | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 022b44ba22..64e6d50621 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -1056,8 +1056,12 @@ sub command_checks_all
 
 	# See http://perldoc.perl.org/perlvar.html#%24CHILD_ERROR
 	my $ret = $?;
-	die "command exited with signal " . ($ret & 127)
-	  if $ret & 127;
+	if ($ret & 127)
+	{
+		diag "stdout: $stdout";
+		diag "stderr: $stderr";
+		die "command exited with signal " . ($ret & 127);
+	}
 	$ret = $ret >> 8;
 
 	# check status
-- 
2.39.2



  [text/x-patch] 0002-Remove-bogus-assertion-in-pg_rewind.patch (5.9K, ../fe7af87d-b9bf-481a-902c-089c49aa911d@iki.fi/3-0002-Remove-bogus-assertion-in-pg_rewind.patch)
  download | inline diff:
From bddbb33dfe6a57373bd76f06d928530603eecd64 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Fri, 9 Aug 2024 13:30:55 +0300
Subject: [PATCH 2/4] Remove bogus assertion in pg_rewind

It's entirely possible that the calculated "point of divergence" is
before the end of WAL on the target, because when we calculate the
point of divergence, we assume that the target's WAL extends to
infinity on its current timeline. It might be more clear to calculate
the end of WAL first, and if the target is a direct ancestor of the
source, consider the end of target WAL as the point of
divergence. However, it doesn't change the result: the target is a
direct ancestor of the source, and doesn't need to be rewound.

Add tests to cover the "no rewind required" cases, both when the
target's end of WAL is exactly at the point of divergence, which is
what the removed assertion assumed (case B in the test), and the case
where the assertion failed (case A).

Discussion: https://www.postgresql.org/message-id/18575-1b9b3b60e2a480de@postgresql.org
---
 src/bin/pg_rewind/pg_rewind.c                 |   7 -
 src/bin/pg_rewind/t/010_target_is_ancestor.pl | 126 ++++++++++++++++++
 2 files changed, 126 insertions(+), 7 deletions(-)
 create mode 100644 src/bin/pg_rewind/t/010_target_is_ancestor.pl

diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 323c35646c..101e92b549 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -434,16 +434,9 @@ main(int argc, char **argv)
 		 * in the target that needs rewinding.
 		 */
 		if (target_wal_endrec > divergerec)
-		{
 			rewind_needed = true;
-		}
 		else
-		{
-			/* the last common checkpoint record must be part of target WAL */
-			Assert(target_wal_endrec == divergerec);
-
 			rewind_needed = false;
-		}
 	}
 
 	if (!rewind_needed)
diff --git a/src/bin/pg_rewind/t/010_target_is_ancestor.pl b/src/bin/pg_rewind/t/010_target_is_ancestor.pl
new file mode 100644
index 0000000000..541581a4e8
--- /dev/null
+++ b/src/bin/pg_rewind/t/010_target_is_ancestor.pl
@@ -0,0 +1,126 @@
+
+# Copyright (c) 2021-2024, PostgreSQL Global Development Group
+
+# Test cases where the target is a direct ancestor of the source.
+#
+# We set up two timelines like this:
+#
+#            -------------- TLI 2 (source)
+#           /
+#  ---A----B-------C------- TLI 1
+#
+#
+# On TLI 1, we take a backup on points A, B and C. We use a server
+# restored from each of those points as the target, and a primary
+# server running on TLI 2 as the source.
+#
+# - A is a direct ancestor of the source, so no rewind is required
+# - B is also a direct ancestor of the source, at exactly the point
+#     where the timelines diverged. No rewind is required.
+# - C is not a direct ancestor, so rewind is required. (This case is
+#   not the focus of these tests, it's here just for completeness.)
+
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+use File::Copy;
+use File::Path qw(rmtree);
+
+my $tmp_folder = PostgreSQL::Test::Utils::tempdir;
+
+my $orig_node = PostgreSQL::Test::Cluster->new('orig_node');
+$orig_node->init(allows_streaming => 1);
+$orig_node->append_conf(
+	'postgresql.conf', qq(
+wal_log_hints = on
+));
+$orig_node->start;
+$orig_node->safe_psql('postgres', 'CREATE TABLE test_tab (t TEXT)');
+$orig_node->safe_psql('postgres',
+	"INSERT INTO test_tab VALUES ('at the beginning')");
+$orig_node->stop;
+$orig_node->backup_fs_cold('backup_A');
+
+$orig_node->start;
+$orig_node->safe_psql('postgres',
+	"INSERT INTO test_tab VALUES ('after backup A')");
+$orig_node->stop;
+$orig_node->backup_fs_cold('backup_B');
+
+# Restore backup B to create Timeline 2
+my $node_2 = PostgreSQL::Test::Cluster->new('node_2');
+$node_2->init_from_backup($orig_node, 'backup_B', has_streaming => 1);
+$node_2->start;
+$node_2->promote;
+$node_2->safe_psql('postgres', "INSERT INTO test_tab VALUES ('in node_2')");
+$node_2->stop;
+
+$orig_node->start;
+$orig_node->safe_psql('postgres',
+	"INSERT INTO test_tab VALUES ('this will be lost on rewind')");
+$orig_node->stop;
+$orig_node->backup_fs_cold('backup_C');
+
+my $source_node = $node_2;
+my $source_pgdata = $source_node->data_dir;
+
+# Create a new node from a backup, and run pg_rewind to rewind it as a
+# follower of $source_node
+sub rewind_from_backup
+{
+	my ($backup, $expected_stderr) = @_;
+
+	my $target_node = PostgreSQL::Test::Cluster->new('restored_' . $backup);
+	$target_node->init_from_backup($orig_node, $backup, has_streaming => 1);
+	my $target_pgdata = $target_node->data_dir;
+
+	# Keep a temporary postgresql.conf or it would be overwritten during the rewind.
+	copy("$target_pgdata/postgresql.conf", "$tmp_folder/postgresql.conf.tmp");
+
+	command_checks_all(
+		[
+			'pg_rewind',
+			"--debug",
+			"--no-sync",
+			"--source-pgdata=$source_pgdata",
+			"--target-pgdata=$target_pgdata"
+		],
+		0,
+		[],
+		[$expected_stderr],
+		"pg_rewind on node restored from $backup");
+
+	# Now move back postgresql.conf with old settings
+	move("$tmp_folder/postgresql.conf.tmp", "$target_pgdata/postgresql.conf");
+
+	# Configure it to connect to the new primary
+	my $node_2_connstr = $node_2->connstr();
+	$target_node->append_conf(
+		'postgresql.conf', qq(
+primary_conninfo = '$node_2_connstr'
+));
+	$target_node->set_standby_mode();
+	$target_node->start;
+
+	$node_2->start;
+	$node_2->wait_for_catchup($target_node->name);
+	my $result =
+	  $target_node->safe_psql('postgres', "SELECT * FROM test_tab");
+	is( $result, qq(at the beginning
+after backup A
+in node_2),
+		"check query after rewind from $backup");
+	$target_node->stop;
+
+	$node_2->stop;
+}
+
+rewind_from_backup('backup_C',
+	qr/pg_rewind: rewinding from last common checkpoint at .* on timeline 1/);
+rewind_from_backup('backup_B', qr/pg_rewind: no rewind required/);
+rewind_from_backup('backup_A', qr/pg_rewind: no rewind required/);
+
+done_testing();
-- 
2.39.2



  [text/x-patch] 0003-Don-t-read-past-current-TLI-during-archive-recovery.patch (11.1K, ../fe7af87d-b9bf-481a-902c-089c49aa911d@iki.fi/4-0003-Don-t-read-past-current-TLI-during-archive-recovery.patch)
  download | inline diff:
From aa57f9f6d21176aab6b3e520b4eb0473b8b4a609 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Fri, 9 Aug 2024 16:50:58 +0300
Subject: [PATCH 3/4] Don't read past current TLI during archive recovery

If you set the recovery target timeline, the recovery would not
correctly follow the timeline history to reach that timeline, if a
local WAL segment had more WAL records on one of the older
timelines. Fix that by not reading WAL past the switchpoint in the
timeline history.

This case came up with pg_rewind: if you call pg_rewind on a server
that is a direct ancestor of the source, it exits with "no rewind
required" and does nothing. However, if the target had already
streamed WAL past the point of divergence, and just hadn't applied it
yet, when you start it up it will follow the original timeline, not
the one you tried to rewind to. This commit alone doesn't fix that,
because the server still won't know that it's supposed to follow the
other timeline. But this at least fixes the similar case when you have
set the recovery target timeline explicitly.

Discussion: https://www.postgresql.org/message-id/18575-1b9b3b60e2a480de%40postgresql.org
---
 src/backend/access/transam/xlogrecovery.c     | 81 +++++++++++++--
 .../recovery/t/044_change_recovery_target.pl  | 98 +++++++++++++++++++
 2 files changed, 171 insertions(+), 8 deletions(-)
 create mode 100644 src/test/recovery/t/044_change_recovery_target.pl

diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
index ad817fbca6..d3d1774d26 100644
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -117,12 +117,20 @@ bool		wal_receiver_create_temp_slot = false;
  * scanning data that was copied from an ancestor timeline when the current
  * file was created.)  During a sequential scan we do not allow this value
  * to decrease.
+ *
+ * curFileUpto: the end of the curFileTLI timeline in the history of the
+ * timeline we're recovering to (ie. recoveryTargetTLI).  We must not read the
+ * currently open file beyond this position, because it contains WAL that is
+ * not part of the history of the timeline we're recovering to.  Upon reaching
+ * that point, we need to open the file from the next timeline in the history
+ * instead.
  */
 RecoveryTargetTimeLineGoal recoveryTargetTimeLineGoal = RECOVERY_TARGET_TIMELINE_LATEST;
 TimeLineID	recoveryTargetTLIRequested = 0;
 TimeLineID	recoveryTargetTLI = 0;
 static List *expectedTLEs;
 static TimeLineID curFileTLI;
+static XLogRecPtr curFileUpto;
 
 /*
  * When ArchiveRecoveryRequested is set, archive recovery was requested,
@@ -3315,6 +3323,7 @@ XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
 	int			emode = private->emode;
 	uint32		targetPageOff;
 	XLogSegNo	targetSegNo PG_USED_FOR_ASSERTS_ONLY;
+	XLogRecPtr	readUpto;
 	int			r;
 
 	XLByteToSeg(targetPagePtr, targetSegNo, wal_segment_size);
@@ -3346,9 +3355,36 @@ XLogPageRead(XLogReaderState *xlogreader, XLogRecPtr targetPagePtr, int reqLen,
 		readSource = XLOG_FROM_ANY;
 	}
 
+retry:
+	/*
+	 * If we have reached the end of the current timeline in the recovery
+	 * target timeline's history, need to switch to the file from the next
+	 * timeline.
+	 *
+	 * Note: The logic and purpose of this is similar to
+	 * XLogReadDetermineTimeline(), but we don't use the end-of-segment to
+	 * determine the next TLI like XLogReadDetermineTimeline() does.  We don't
+	 * need it because XLogFileReadAnyTLI() will try to read from the WAL
+	 * segment with the latest possible TLI.  This is more flexible than
+	 * XLogReadDetermineTimeline(), which makes it more likely that we'll make
+	 * some progress towards the recovery target, even if all the WAL is not
+	 * (yet) available.
+	 */
+	if (curFileUpto != InvalidXLogRecPtr &&
+		curFileUpto <= targetPagePtr + reqLen)
+	{
+		if (readFile >= 0)
+		{
+			close(readFile);
+			readFile = -1;
+		}
+		curFileTLI = tliOfPointInHistory(targetPagePtr + reqLen, expectedTLEs);
+		curFileUpto = tliSwitchPoint(curFileTLI, expectedTLEs, NULL);
+		readSource = XLOG_FROM_ANY;
+	}
+
 	XLByteToSeg(targetPagePtr, readSegNo, wal_segment_size);
 
-retry:
 	/* See if we need to retrieve more data */
 	if (readFile < 0 ||
 		(readSource == XLOG_FROM_STREAM &&
@@ -3382,28 +3418,45 @@ retry:
 		}
 	}
 
+	/*
+	 * If the recovery target TLI changed during WaitForWALToBecomeAvailable,
+	 * the record we're looking for might not be on this file anymore.
+	 */
+	 if (curFileUpto != InvalidXLogRecPtr &&
+		 curFileUpto <= targetPagePtr + reqLen)
+		 goto retry;
+
 	/*
 	 * At this point, we have the right segment open and if we're streaming we
 	 * know the requested record is in it.
 	 */
 	Assert(readFile != -1);
 
+	/*
+	 * If we're recovering through old timelines, make sure we don't read
+	 * beyond the point where we're supposed to switch to next timeline in the
+	 * recovery target's history.
+	 */
+	readUpto = curFileUpto;
+
 	/*
 	 * If the current segment is being streamed from the primary, calculate
 	 * how much of the current page we have received already. We know the
 	 * requested record has been received, but this is for the benefit of
 	 * future calls, to allow quick exit at the top of this function.
 	 */
-	if (readSource == XLOG_FROM_STREAM)
+	if (readSource == XLOG_FROM_STREAM &&
+		(readUpto == InvalidXLogRecPtr || flushedUpto < readUpto))
 	{
-		if (((targetPagePtr) / XLOG_BLCKSZ) != (flushedUpto / XLOG_BLCKSZ))
-			readLen = XLOG_BLCKSZ;
-		else
-			readLen = XLogSegmentOffset(flushedUpto, wal_segment_size) -
-				targetPageOff;
+		readUpto = flushedUpto;
 	}
-	else
+
+	if (readUpto == InvalidXLogRecPtr ||
+		((targetPagePtr) / XLOG_BLCKSZ) != (readUpto / XLOG_BLCKSZ))
 		readLen = XLOG_BLCKSZ;
+	else
+		readLen = XLogSegmentOffset(readUpto, wal_segment_size) -
+			targetPageOff;
 
 	/* Read the requested page */
 	readOff = targetPageOff;
@@ -3773,7 +3826,10 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
 				}
 				/* Reset curFileTLI if random fetch. */
 				if (randAccess)
+				{
 					curFileTLI = 0;
+					curFileUpto = InvalidXLogRecPtr;
+				}
 
 				/*
 				 * Try to restore the file from archive, or read an existing
@@ -3837,6 +3893,9 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
 						XLogRecPtr	ptr;
 						TimeLineID	tli;
 
+						if (!expectedTLEs)
+							expectedTLEs = readTimeLineHistory(recoveryTargetTLI);
+
 						if (fetching_ckpt)
 						{
 							ptr = RedoStartLSN;
@@ -3858,6 +3917,7 @@ WaitForWALToBecomeAvailable(XLogRecPtr RecPtr, bool randAccess,
 									 tli, curFileTLI);
 						}
 						curFileTLI = tli;
+						curFileUpto = tliSwitchPoint(tli, expectedTLEs, NULL);
 						SetInstallXLogFileSegmentActive();
 						RequestXLogStreaming(tli, ptr, PrimaryConnInfo,
 											 PrimarySlotName,
@@ -4184,6 +4244,8 @@ rescanLatestTimeLine(TimeLineID replayTLI, XLogRecPtr replayLSN)
 	 * between the old target and new target in pg_wal.
 	 */
 	restoreTimeLineHistoryFiles(oldtarget + 1, newtarget);
+	if (curFileTLI != 0)
+		curFileUpto = tliSwitchPoint(curFileTLI, expectedTLEs, NULL);
 
 	ereport(LOG,
 			(errmsg("new target timeline is %u",
@@ -4254,6 +4316,9 @@ XLogFileRead(XLogSegNo segno, int emode, TimeLineID tli,
 	{
 		/* Success! */
 		curFileTLI = tli;
+		if (!expectedTLEs)
+			expectedTLEs = readTimeLineHistory(recoveryTargetTLI);
+		curFileUpto = tliSwitchPoint(curFileTLI, expectedTLEs, NULL);
 
 		/* Report recovery progress in PS display */
 		snprintf(activitymsg, sizeof(activitymsg), "recovering %s",
diff --git a/src/test/recovery/t/044_change_recovery_target.pl b/src/test/recovery/t/044_change_recovery_target.pl
new file mode 100644
index 0000000000..99aa126d6b
--- /dev/null
+++ b/src/test/recovery/t/044_change_recovery_target.pl
@@ -0,0 +1,98 @@
+
+# Copyright (c) 2021-2024, PostgreSQL Global Development Group
+
+# Test that recovery server follows the right path through the WAL
+# files towards the recovery target timeline. Consider the following
+# timeline history:
+#
+#    ---S---------------------->P   TLI 1
+#                     \
+#                      -------->    TLI 2
+#
+# Standby has replayed the WALL on TLI up to point S, and is currently
+# stopped. However, it already has all the WAL from TLI 1 locally in
+# pg_wal, because it was restored from th archive or streamed from the
+# primary on TLI 1 earlier. If you now set recovery target timeline to
+# TLI 2, the standby must not replay the WAL it already has on TLI 1,
+# beyond the point where they diverged.
+#
+# This test sets up that scenario, by pausing the replay on the
+# standby.
+
+use strict;
+use warnings FATAL => 'all';
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+use File::Copy;
+
+my $primary1 = PostgreSQL::Test::Cluster->new('primary1');
+$primary1->init(allows_streaming => 1);
+$primary1->start;
+$primary1->backup("bkp");
+
+my $standby = PostgreSQL::Test::Cluster->new('standby');
+$standby->init_from_backup($primary1, "bkp", has_streaming => 1);
+$standby->start;
+
+# Pause the standby
+$standby->safe_psql('postgres', "SELECT pg_wal_replay_pause()");
+
+# Make some changes in the primary. They will be streamed to the
+# standby, but not yet applied.
+$primary1->safe_psql('postgres', 'CREATE TABLE test_tab1 (t TEXT)');
+$primary1->safe_psql('postgres', 'CREATE TABLE test_tab2 (t TEXT)');
+$primary1->safe_psql('postgres',
+	"INSERT INTO test_tab1 VALUES ('both timelines')");
+$primary1->safe_psql('postgres',
+	"INSERT INTO test_tab2 VALUES ('both timelines')");
+
+# Create timeline 2 at this point
+my $primary2 = PostgreSQL::Test::Cluster->new('primary2');
+$primary2->init_from_backup($primary1, "bkp", has_streaming => 1);
+$primary2->start;
+$primary1->wait_for_replay_catchup($primary2);
+$primary2->promote;
+
+# Make some changes on both timelines
+$primary1->safe_psql('postgres',
+	"INSERT INTO test_tab1 VALUES ('TLI 1 only')");
+
+$primary2->safe_psql('postgres',
+	"INSERT INTO test_tab2 VALUES ('TLI 2 only')");
+
+# Stop the standby, and re-point it to TLI 2 (primary2).
+$standby->stop;
+my $primary2_connstr = $primary2->connstr;
+$standby->append_conf('postgresql.conf',
+	"primary_conninfo = '$primary2_connstr'");
+$standby->append_conf('postgresql.conf',
+	"recovery_target_timeline = 2");
+
+# Copy the history file. Otherwise, the standby will just error out
+# with "FATAL: recovery target timeline 2 does not exist"
+#
+# XXX: Perhaps it should try to connect to the primary and fetch the
+# history file from there instead of erroring out. But it doesn't do
+# that today.
+copy($primary2->data_dir . '/pg_wal/00000002.history',
+	$standby->data_dir . '/pg_wal/00000002.history')
+  or BAIL_OUT("could not copy 00000002.history");
+
+# Check that it recovers correctly to TLI 2 when started up.
+$standby->start;
+$primary2->wait_for_replay_catchup($standby);
+
+my $result =
+  $standby->safe_psql('postgres', "SELECT * FROM test_tab1");
+is( $result, qq(both timelines),
+		"changes on TLI 1 are *not* on standby");
+
+$result =
+  $standby->safe_psql('postgres', "SELECT * FROM test_tab2");
+is( $result, qq(both timelines
+TLI 2 only),
+		"all changes on TLI 2 are on the standby");
+
+done_testing();
-- 
2.39.2



  [text/x-patch] 0004-Update-minRecoveryPoint-when-no-rewind-is-required.patch (5.4K, ../fe7af87d-b9bf-481a-902c-089c49aa911d@iki.fi/5-0004-Update-minRecoveryPoint-when-no-rewind-is-required.patch)
  download | inline diff:
From 6d534f6d1f1b8a0a375a57cde9479543efb87f14 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Fri, 9 Aug 2024 18:23:02 +0300
Subject: [PATCH 4/4] Update minRecoveryPoint when no rewind is required

Alternatively, perhaps just copying the history file would be enough?
---
 src/bin/pg_rewind/pg_rewind.c                 | 40 ++++++++++++++++++-
 src/bin/pg_rewind/t/010_target_is_ancestor.pl | 21 ++++++----
 2 files changed, 52 insertions(+), 9 deletions(-)

diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 101e92b549..e2767ba9ec 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -77,6 +77,9 @@ bool		do_sync = true;
 static bool restore_wal = false;
 DataDirSyncMethod sync_method = DATA_DIR_SYNC_METHOD_FSYNC;
 
+static char *sourceHistfile;
+static char *sourceHistfileName;
+
 /* Target history */
 TimeLineHistoryEntry *targetHistory;
 int			targetNentries;
@@ -372,7 +375,7 @@ main(int argc, char **argv)
 	 */
 	if (target_tli == source_tli)
 	{
-		pg_log_info("source and target cluster are on the same timeline");
+		pg_log_info("no rewind required, source and target cluster are on the same timeline");
 		rewind_needed = false;
 		target_wal_endrec = 0;
 	}
@@ -431,12 +434,39 @@ main(int argc, char **argv)
 		/*
 		 * Check for the possibility that the target is in fact a direct
 		 * ancestor of the source. In that case, there is no divergent history
-		 * in the target that needs rewinding.
+		 * in the target that needs rewinding, but we still update the
+		 * target's minRecoveryPoint, so that when you start it up, it will
+		 * recover to that timeline. (Otherwise, if it has more WAL locally in
+		 * pg_wal or available from the archive, it might choose to follow
+		 * some other timeline.)
 		 */
 		if (target_wal_endrec > divergerec)
 			rewind_needed = true;
 		else
+		{
+			ControlFileData ControlFile_new;
+
 			rewind_needed = false;
+
+			memcpy(&ControlFile_new, &ControlFile_target, sizeof(ControlFileData));
+			ControlFile_new.minRecoveryPoint = 	Max(ControlFile_source.minRecoveryPoint,
+													ControlFile_source.checkPoint);
+			ControlFile_new.minRecoveryPointTLI = source_tli;
+			pg_log_info("no rewind required, updating target to source's timeline",
+						LSN_FORMAT_ARGS(ControlFile_new.minRecoveryPoint),
+						ControlFile_new.minRecoveryPointTLI);
+			if (!dry_run)
+				update_controlfile(datadir_target, &ControlFile_new, do_sync);
+
+			/*
+			 * Make the timeline history file available to the target. Avoids
+			 * having to fetch it immediately at startup, and ensures that it
+			 * has the same idea of what the timeline is as we did.
+			 */
+			open_target_file(sourceHistfileName, true);
+			write_target_range(sourceHistfile, 0, strlen(sourceHistfile));
+			close_target_file();
+		}
 	}
 
 	if (!rewind_needed)
@@ -872,6 +902,12 @@ getTimelineHistory(TimeLineID tli, bool is_source, int *nentries)
 			histfile = slurpFile(datadir_target, path, NULL);
 
 		history = rewind_parseTimeLineHistory(histfile, tli, nentries);
+		if (is_source)
+		{
+			sourceHistfileName = pstrdup(path);
+			sourceHistfile = histfile;
+		}
+		else
 		pg_free(histfile);
 	}
 
diff --git a/src/bin/pg_rewind/t/010_target_is_ancestor.pl b/src/bin/pg_rewind/t/010_target_is_ancestor.pl
index 541581a4e8..15aaa60f2d 100644
--- a/src/bin/pg_rewind/t/010_target_is_ancestor.pl
+++ b/src/bin/pg_rewind/t/010_target_is_ancestor.pl
@@ -67,8 +67,8 @@ $orig_node->backup_fs_cold('backup_C');
 my $source_node = $node_2;
 my $source_pgdata = $source_node->data_dir;
 
-# Create a new node from a backup, and run pg_rewind to rewind it as a
-# follower of $source_node
+# Create a new node from a backup, and rewind it as a follower of
+# $source_node
 sub rewind_from_backup
 {
 	my ($backup, $expected_stderr) = @_;
@@ -90,22 +90,29 @@ sub rewind_from_backup
 		],
 		0,
 		[],
-		[$expected_stderr],
+		$expected_stderr,
 		"pg_rewind on node restored from $backup");
 
 	# Now move back postgresql.conf with old settings
 	move("$tmp_folder/postgresql.conf.tmp", "$target_pgdata/postgresql.conf");
 
+	# Make the more recent WAL from timeline 1 available to the
+	# restored server.  It doesn't need it, the point of this is to
+	# test that the server ignores this extra WAL.
+	copy($orig_node->data_dir . "/pg_wal/000000010000000000000001",
+	  $target_node->data_dir . "/pg_wal/000000010000000000000001")
+	  || die "copying 000000010000000000000001: $!";
+
 	# Configure it to connect to the new primary
 	my $node_2_connstr = $node_2->connstr();
 	$target_node->append_conf(
 		'postgresql.conf', qq(
 primary_conninfo = '$node_2_connstr'
 ));
+	$node_2->start;
 	$target_node->set_standby_mode();
 	$target_node->start;
 
-	$node_2->start;
 	$node_2->wait_for_catchup($target_node->name);
 	my $result =
 	  $target_node->safe_psql('postgres', "SELECT * FROM test_tab");
@@ -119,8 +126,8 @@ in node_2),
 }
 
 rewind_from_backup('backup_C',
-	qr/pg_rewind: rewinding from last common checkpoint at .* on timeline 1/);
-rewind_from_backup('backup_B', qr/pg_rewind: no rewind required/);
-rewind_from_backup('backup_A', qr/pg_rewind: no rewind required/);
+	[qr/pg_rewind: rewinding from last common checkpoint at .* on timeline 1/]);
+rewind_from_backup('backup_B', [qr/no rewind required, updating target to source's timeline/]);
+rewind_from_backup('backup_A', [qr/no rewind required, updating target to source's timeline/]);
 
 done_testing();
-- 
2.39.2

view thread (11+ messages)  latest in thread

Message-ID: <fe7af87d-b9bf-481a-902c-089c49aa911d@iki.fi>
Permalink:  ../fe7af87d-b9bf-481a-902c-089c49aa911d@iki.fi/
Also on:    postgresql.org/message-id/fe7af87d-b9bf-481a-902c-089c49aa911d@iki.fi

copy link · copy postgr.es · copy git am

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: pgsql-bugs@postgresql.org
  Cc: hlinnaka@iki.fi, g.shelkovy@arenadata.io, pgsql-bugs@lists.postgresql.org
  Subject: Re: BUG #18575: Sometimes pg_rewind mistakenly assumes that nothing needs to be done.
  In-Reply-To: <fe7af87d-b9bf-481a-902c-089c49aa911d@iki.fi>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox