Subject: Re: Two pg_rewind patches (auto generate recovery conf and ensure
 clean shutdown)
To: Alvaro Herrera <alvherre@2ndquadrant.com>, Paul Guo <pguo@pivotal.io>
Cc: Michael Paquier <michael@paquier.xyz>,
 Thomas Munro <thomas.munro@gmail.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>, Jimmy Yih <jyih@pivotal.io>,
 Ashwin Agrawal <aagrawal@pivotal.io>
References: <20190930171349.GA21905@alvherre.pgsql>
From: Alexey Kondratov <a.kondratov@postgrespro.ru>
Message-ID: <7ca88204-3e0b-2f4c-c8af-acadc4b266e5@postgrespro.ru>
Date: Wed, 2 Oct 2019 20:28:09 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <20190930171349.GA21905@alvherre.pgsql>
Content-Type: multipart/mixed;
 boundary="------------3AC3D8C20F95EE3D3A49FF23"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------3AC3D8C20F95EE3D3A49FF23
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit

Hi Alvaro,

On 30.09.2019 20:13, Alvaro Herrera wrote:
> OK, I pushed this patch as well as Alexey's test patch.  It all works
> for me, and the coverage report shows that we're doing the new thing ...
> though only in the case that rewind *is* required.  There is no test to
> verify the case where rewind is *not* required.  I guess it'd also be
> good to test the case when we throw the new error, if only for
> completeness ...

I've directly followed your guess and tried to elaborate pg_rewind test 
cases and... It seems I've caught a few bugs:

1) --dry-run actually wasn't completely 'dry'. It did update target 
controlfile, which could cause repetitive pg_rewind calls to fail after 
dry-run ones.

2) --no-ensure-shutdown flag was broken, it simply didn't turn off this 
new feature.

3) --write-recovery-conf didn't obey the --dry-run flag.

Thus, it was definitely a good idea to add new tests. Two patches are 
attached:

1) First one fixes all the issues above;

2) Second one slightly increases pg_rewind overall code coverage from 
74% to 78.6%.

Should I put this fix on the next commitfest?


Regards

-- 
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company


P.S. My apologies that I've missed two of these bugs during review.


--------------3AC3D8C20F95EE3D3A49FF23
Content-Type: text/x-patch;
 name="v1-0001-Fix-functionality-of-pg_rewind-dry-run-and-no-ens.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v1-0001-Fix-functionality-of-pg_rewind-dry-run-and-no-ens.pa";
 filename*1="tch"

From 7286e31ab0ebf50bb4ab460dd81b82f1c5989272 Mon Sep 17 00:00:00 2001
From: Alexey Kondratov <kondratov.aleksey@gmail.com>
Date: Wed, 2 Oct 2019 19:24:46 +0300
Subject: [PATCH v1 1/2] Fix functionality of pg_rewind --dry-run and
 --no-ensure-shutdown options

Branch: pg-rewind-fixes
---
 src/bin/pg_rewind/pg_rewind.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index a7fd9e0cab..1a7fb5242b 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -101,7 +101,7 @@ main(int argc, char **argv)
 		{"write-recovery-conf", no_argument, NULL, 'R'},
 		{"source-pgdata", required_argument, NULL, 1},
 		{"source-server", required_argument, NULL, 2},
-		{"no-ensure-shutdown", no_argument, NULL, 44},
+		{"no-ensure-shutdown", no_argument, NULL, 4},
 		{"version", no_argument, NULL, 'V'},
 		{"dry-run", no_argument, NULL, 'n'},
 		{"no-sync", no_argument, NULL, 'N'},
@@ -435,13 +435,15 @@ main(int argc, char **argv)
 	ControlFile_new.minRecoveryPoint = endrec;
 	ControlFile_new.minRecoveryPointTLI = endtli;
 	ControlFile_new.state = DB_IN_ARCHIVE_RECOVERY;
-	update_controlfile(datadir_target, &ControlFile_new, do_sync);
+
+	if (!dry_run)
+		update_controlfile(datadir_target, &ControlFile_new, do_sync);
 
 	if (showprogress)
 		pg_log_info("syncing target data directory");
 	syncTargetDirectory();
 
-	if (writerecoveryconf)
+	if (!dry_run && writerecoveryconf)
 		WriteRecoveryConfig(conn, datadir_target,
 							GenerateRecoveryConfig(conn, NULL));
 

base-commit: df86e52cace2c4134db51de6665682fb985f3195
-- 
2.17.1


--------------3AC3D8C20F95EE3D3A49FF23
Content-Type: text/x-patch;
 name="v1-0002-Increase-pg_rewind-code-coverage.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="v1-0002-Increase-pg_rewind-code-coverage.patch"

From 28fdd2fa58af718d8a894cb3c3d8f9b2cdf6759e Mon Sep 17 00:00:00 2001
From: Alexey Kondratov <kondratov.aleksey@gmail.com>
Date: Wed, 2 Oct 2019 19:25:27 +0300
Subject: [PATCH v1 2/2] Increase pg_rewind code coverage

Branch: pg-rewind-fixes
---
 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   | 27 ++++++++++++++----
 src/bin/pg_rewind/t/RewindTest.pm          | 33 ++++++++++++++++++++--
 6 files changed, 55 insertions(+), 13 deletions(-)

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..142e675a23 100644
--- a/src/bin/pg_rewind/t/005_same_timeline.pl
+++ b/src/bin/pg_rewind/t/005_same_timeline.pl
@@ -1,7 +1,7 @@
 use strict;
 use warnings;
 use TestLib;
-use Test::More tests => 1;
+use Test::More tests => 6;
 
 use FindBin;
 use lib $FindBin::RealBin;
@@ -11,9 +11,24 @@ use RewindTest;
 # Test that running pg_rewind if the two clusters are on the same
 # timeline runs successfully.
 
-RewindTest::setup_cluster();
-RewindTest::start_master();
-RewindTest::create_standby();
-RewindTest::run_pg_rewind('local');
-RewindTest::clean_rewind_test();
+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');
+
 exit(0);
diff --git a/src/bin/pg_rewind/t/RewindTest.pm b/src/bin/pg_rewind/t/RewindTest.pm
index c540722420..497807718c 100644
--- a/src/bin/pg_rewind/t/RewindTest.pm
+++ b/src/bin/pg_rewind/t/RewindTest.pm
@@ -245,9 +245,20 @@ sub run_pg_rewind
 	if ($test_mode eq "local")
 	{
 
-		# Do rewind using a local pgdata as source
-		# Stop the master and be ready to perform the rewind
+		# Stop the new master and be ready to perform the rewind
 		$node_standby->stop;
+
+		# Check that incompatible options error out.
+		command_fails(
+			[
+				'pg_rewind', "--debug",
+				"--source-pgdata=$standby_pgdata",
+				"--target-pgdata=$master_pgdata", "-R",
+				"--no-ensure-shutdown"
+			],
+			'pg_rewind local with -R');
+
+		# Do rewind using a local pgdata as source
 		command_ok(
 			[
 				'pg_rewind',
@@ -261,6 +272,21 @@ sub run_pg_rewind
 	elsif ($test_mode eq "remote")
 	{
 
+		# Do rewind in dry-run mode using a remote connection as source
+		command_ok(
+			[
+				'pg_rewind',
+				"--source-server", $standby_connstr,
+				"--target-pgdata=$master_pgdata", "-R",
+				"--debug", "--no-ensure-shutdown", "-P",
+				"--no-sync", "--dry-run"
+			],
+			'pg_rewind remote with --dry-run');
+
+		# Check that standby.signal hasn't been created.
+		ok(! -e "$master_pgdata/standby.signal",
+		   'standby.signal is absent');
+
 		# Do rewind using a remote connection as source
 		command_ok(
 			[
@@ -272,7 +298,8 @@ sub run_pg_rewind
 			'pg_rewind remote');
 
 		# Check that standby.signal has been created.
-		ok(-e "$master_pgdata/standby.signal");
+		ok(-e "$master_pgdata/standby.signal",
+		   'standby.signal exists');
 
 		# Now, when pg_rewind apparently succeeded with minimal permissions,
 		# add REPLICATION privilege.  So we could test that new standby
-- 
2.17.1


--------------3AC3D8C20F95EE3D3A49FF23--