From 6e5667edcad6b037004288635a7ae0eda40d4262 Mon Sep 17 00:00:00 2001
From: Alexey Kondratov <kondratov.aleksey@gmail.com>
Date: Fri, 4 Oct 2019 17:14:12 +0300
Subject: [PATCH v3 1/2] Improve functionality, docs and tests of -R,
 --no-ensure-shutdown and --dry-run options

Branch: pg-rewind-fixes
---
 doc/src/sgml/ref/pg_rewind.sgml            | 10 +--
 src/bin/pg_rewind/pg_rewind.c              | 19 +++---
 src/bin/pg_rewind/t/001_basic.pl           |  2 +-
 src/bin/pg_rewind/t/002_databases.pl       |  2 +-
 src/bin/pg_rewind/t/003_extrafiles.pl      |  2 +-
 src/bin/pg_rewind/t/004_pg_xlog_symlink.pl |  2 +-
 src/bin/pg_rewind/t/005_same_timeline.pl   | 32 +++++++---
 src/bin/pg_rewind/t/RewindTest.pm          | 71 +++++++++++++++++-----
 8 files changed, 103 insertions(+), 37 deletions(-)

diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml
index fbf454803b..42d29edd4e 100644
--- a/doc/src/sgml/ref/pg_rewind.sgml
+++ b/doc/src/sgml/ref/pg_rewind.sgml
@@ -169,12 +169,14 @@ PostgreSQL documentation
       <term><option>--no-ensure-shutdown</option></term>
       <listitem>
        <para>
-        <application>pg_rewind</application> verifies that the target server
-        is cleanly shutdown before rewinding; by default, if it isn't, it
-        starts the server in single-user mode to complete crash recovery.
+        <application>pg_rewind</application> requires that the target server
+        is cleanly shut down before rewinding. By default, if the target server
+        is not shut down cleanly, <application>pg_rewind</application> starts
+        the target server in single-user mode to complete crash recovery first,
+        and stops it.
         By passing this option, <application>pg_rewind</application> skips
         this and errors out immediately if the server is not cleanly shut
-        down.  Users are expected to handle the situation themselves in that
+        down. Users are expected to handle the situation themselves in that
         case.
        </para>
       </listitem>
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index fe1468b771..875a43b219 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -270,11 +270,12 @@ main(int argc, char **argv)
 	pg_free(buffer);
 
 	/*
-	 * If the target instance was not cleanly shut down, run a single-user
-	 * postgres session really quickly and reload the control file to get the
-	 * new state. Note if no_ensure_shutdown is specified, pg_rewind won't do
-	 * that automatically. That means users need to do themselves in advance,
-	 * else pg_rewind will soon quit, see sanityChecks().
+	 * If the target instance was not cleanly shut down, start and stop the
+	 * target cluster once in single-user mode to enforce recovery to finish,
+	 * ensuring that the cluster can be used by pg_rewind.  Note that if
+	 * no_ensure_shutdown is specified, pg_rewind ignores this step, and users
+	 * need to make sure by themselves that the target cluster is in a clean
+	 * state.
 	 */
 	if (!no_ensure_shutdown &&
 		ControlFile_target.state != DB_SHUTDOWNED &&
@@ -847,8 +848,12 @@ ensureCleanShutdown(const char *argv0)
 	if (dry_run)
 		return;
 
-	/* finally run postgres in single-user mode */
-	snprintf(cmd, MAXCMDLEN, "\"%s\" --single -D \"%s\" template1 < \"%s\"",
+	/*
+	 * Finally run postgres in single-user mode.  There is no need to use
+	 * fsync here.  This makes the recovery faster, and the target data folder
+	 * is synced at the end anyway.
+	 */
+	snprintf(cmd, MAXCMDLEN, "\"%s\" --single -F -D \"%s\" template1 < \"%s\"",
 			 exec_path, datadir_target, DEVNULL);
 
 	if (system(cmd) != 0)
diff --git a/src/bin/pg_rewind/t/001_basic.pl b/src/bin/pg_rewind/t/001_basic.pl
index c3293e93df..a1659460ec 100644
--- a/src/bin/pg_rewind/t/001_basic.pl
+++ b/src/bin/pg_rewind/t/001_basic.pl
@@ -1,7 +1,7 @@
 use strict;
 use warnings;
 use TestLib;
-use Test::More tests => 11;
+use Test::More tests => 14;
 
 use FindBin;
 use lib $FindBin::RealBin;
diff --git a/src/bin/pg_rewind/t/002_databases.pl b/src/bin/pg_rewind/t/002_databases.pl
index 1db534c0dc..921c4434f5 100644
--- a/src/bin/pg_rewind/t/002_databases.pl
+++ b/src/bin/pg_rewind/t/002_databases.pl
@@ -1,7 +1,7 @@
 use strict;
 use warnings;
 use TestLib;
-use Test::More tests => 7;
+use Test::More tests => 10;
 
 use FindBin;
 use lib $FindBin::RealBin;
diff --git a/src/bin/pg_rewind/t/003_extrafiles.pl b/src/bin/pg_rewind/t/003_extrafiles.pl
index f4710440fc..bce5b47148 100644
--- a/src/bin/pg_rewind/t/003_extrafiles.pl
+++ b/src/bin/pg_rewind/t/003_extrafiles.pl
@@ -3,7 +3,7 @@
 use strict;
 use warnings;
 use TestLib;
-use Test::More tests => 5;
+use Test::More tests => 8;
 
 use File::Find;
 
diff --git a/src/bin/pg_rewind/t/004_pg_xlog_symlink.pl b/src/bin/pg_rewind/t/004_pg_xlog_symlink.pl
index 639eeb9c91..a501be8f78 100644
--- a/src/bin/pg_rewind/t/004_pg_xlog_symlink.pl
+++ b/src/bin/pg_rewind/t/004_pg_xlog_symlink.pl
@@ -14,7 +14,7 @@ if ($windows_os)
 }
 else
 {
-	plan tests => 5;
+	plan tests => 8;
 }
 
 use FindBin;
diff --git a/src/bin/pg_rewind/t/005_same_timeline.pl b/src/bin/pg_rewind/t/005_same_timeline.pl
index 40dbc44caa..1d96740a55 100644
--- a/src/bin/pg_rewind/t/005_same_timeline.pl
+++ b/src/bin/pg_rewind/t/005_same_timeline.pl
@@ -1,19 +1,35 @@
+#
+# Test that running pg_rewind with the source and target clusters
+# on the same timeline runs successfully.
+#
 use strict;
 use warnings;
 use TestLib;
-use Test::More tests => 1;
+use Test::More tests => 6;
 
 use FindBin;
 use lib $FindBin::RealBin;
 
 use RewindTest;
 
-# Test that running pg_rewind if the two clusters are on the same
-# timeline runs successfully.
+sub run_test
+{
+	my $test_mode = shift;
+
+	RewindTest::setup_cluster($test_mode, ['-g']);
+	RewindTest::start_master();
+	RewindTest::create_standby($test_mode);
+
+	RewindTest::promote_standby();
+
+	RewindTest::run_pg_rewind($test_mode);
+
+	RewindTest::clean_rewind_test();
+	return;
+}
+
+# Run the test in both modes.
+run_test('local');
+run_test('remote');
 
-RewindTest::setup_cluster();
-RewindTest::start_master();
-RewindTest::create_standby();
-RewindTest::run_pg_rewind('local');
-RewindTest::clean_rewind_test();
 exit(0);
diff --git a/src/bin/pg_rewind/t/RewindTest.pm b/src/bin/pg_rewind/t/RewindTest.pm
index c540722420..9292092b1a 100644
--- a/src/bin/pg_rewind/t/RewindTest.pm
+++ b/src/bin/pg_rewind/t/RewindTest.pm
@@ -227,9 +227,57 @@ sub run_pg_rewind
 	# Append the rewind-specific role to the connection string.
 	$standby_connstr = "$standby_connstr user=rewind_user";
 
-	# Stop the master and be ready to perform the rewind
+	# Stop the master cleanly to check dry-run mode.
 	$node_master->stop;
 
+	# First run pg_rewind in dry-run mode
+	if ($test_mode eq "local")
+	{
+
+		# Stop the new master and be ready to perform the rewind
+		# Do rewind in dry-run mode using a local pgdata as source
+		$node_standby->stop;
+		command_ok(
+			[
+				'pg_rewind',
+				"--debug", "--no-sync", "--dry-run",
+				"--source-pgdata=$standby_pgdata",
+				"--target-pgdata=$master_pgdata",
+				"--no-ensure-shutdown"
+			],
+			'pg_rewind local with --dry-run');
+	}
+	elsif ($test_mode eq "remote")
+	{
+
+		# Do rewind in dry-run mode using a remote connection as source
+		command_ok(
+			[
+				'pg_rewind',
+				"--debug", "--no-sync", "--dry-run",
+				"--source-server", $standby_connstr,
+				"--target-pgdata=$master_pgdata",
+				"--write-recovery-conf", "--progress"
+			],
+			'pg_rewind remote with --dry-run');
+
+		# Check that standby.signal hasn't been created.
+		ok(! -e "$master_pgdata/standby.signal",
+			'standby.signal is absent');
+	}
+	else
+	{
+
+		# Cannot come here normally
+		croak("Incorrect test mode specified");
+	}
+
+	# Start and stop the master and be ready to perform the rewind.
+	# The cluster needs recovery to finish once, and pg_rewind makes
+	# sure that it happens automatically.
+	$node_master->start;
+	$node_master->stop('immediate');
+
 	# At this point, the rewind processing is ready to run.
 	# We now have a very simple scenario with a few diverged WAL record.
 	# The real testing begins really now with a bifurcation of the possible
@@ -246,8 +294,6 @@ sub run_pg_rewind
 	{
 
 		# Do rewind using a local pgdata as source
-		# Stop the master and be ready to perform the rewind
-		$node_standby->stop;
 		command_ok(
 			[
 				'pg_rewind',
@@ -261,18 +307,21 @@ sub run_pg_rewind
 	elsif ($test_mode eq "remote")
 	{
 
-		# Do rewind using a remote connection as source
+		# Do rewind using a remote connection as source, generating
+		# recovery configuration automatically.
 		command_ok(
 			[
 				'pg_rewind',                      "--debug",
 				"--source-server",                $standby_connstr,
-				"--target-pgdata=$master_pgdata", "-R",
-				"--no-sync"
+				"--target-pgdata=$master_pgdata", "--no-sync",
+				"--write-recovery-conf"
 			],
 			'pg_rewind remote');
 
-		# Check that standby.signal has been created.
-		ok(-e "$master_pgdata/standby.signal");
+		# Check that standby.signal is here as recovery configuration
+		# was requested.
+		ok( -e "$master_pgdata/standby.signal",
+			'standby.signal created after pg_rewind');
 
 		# Now, when pg_rewind apparently succeeded with minimal permissions,
 		# add REPLICATION privilege.  So we could test that new standby
@@ -280,12 +329,6 @@ sub run_pg_rewind
 		$node_standby->safe_psql('postgres',
 			"ALTER ROLE rewind_user WITH REPLICATION;");
 	}
-	else
-	{
-
-		# Cannot come here normally
-		croak("Incorrect test mode specified");
-	}
 
 	# Now move back postgresql.conf with old settings
 	move(

base-commit: 6837632b758e0470a2692d9f8303e8aebd6fbd8f
-- 
2.17.1

