Subject: Re: Two pg_rewind patches (auto generate recovery conf and ensure
 clean shutdown)
To: Michael Paquier <michael@paquier.xyz>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>, Paul Guo <pguo@pivotal.io>,
 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>
 <7ca88204-3e0b-2f4c-c8af-acadc4b266e5@postgrespro.ru>
 <20191003030752.GE1586@paquier.xyz>
From: Alexey Kondratov <a.kondratov@postgrespro.ru>
Message-ID: <c62d6a22-2976-8491-8e66-bcbb33ac3eb0@postgrespro.ru>
Date: Thu, 3 Oct 2019 12:43:37 +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: <20191003030752.GE1586@paquier.xyz>
Content-Type: multipart/mixed;
 boundary="------------9152C0CA87F324E25AC888EC"
Content-Language: en-US
Precedence: bulk

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

On 03.10.2019 6:07, Michael Paquier wrote:
> On Wed, Oct 02, 2019 at 08:28:09PM +0300, Alexey Kondratov wrote:
>> 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.
> I have just paid attention to this thread, but this is a bug which
> goes down to 12 actually so let's treat it independently of the rest.
> The control file was not written thanks to the safeguards in
> write_target_range() in past versions, but the recent refactoring
> around control file handling broke that promise.  Another thing which
> is not completely exact is the progress reporting which should be
> reported even if the dry-run mode runs.  That's less critical, but
> let's make things consistent.

I also thought about v12, though didn't check whether it's affected.

> Patch 0001 also forgot that recovery.conf should not be written either
> when no rewind is needed.

Yes, definitely, I forgot this code path, thanks.

> I have reworked your first patch as per the attached.  What do you
> think about it?  The part with the control file needs to go down to
> v12, and I would likely split that into two commits on HEAD: one for
> the control file and a second for the recovery.conf portion with the
> fix for --no-ensure-shutdown to keep a cleaner history.

It looks fine for me excepting the progress reporting part. It now adds 
PG_CONTROL_FILE_SIZE to fetch_done. However, I cannot find that control 
file is either included into filemap and fetch_size or counted during 
calculate_totals(). Maybe I've missed something, but now it looks like 
we report something that wasn't planned for progress reporting, doesn't it?

> +               # 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');
> Incompatible options had better be checked within a separate perl
> script?  We generally do that for the other binaries.

Yes, it makes sense. I've reworked the patch with tests and added a 
couple of extra cases.


-- 
Alexey Kondratov

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


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

From 9e828e311dc7c216e5bfb1936022be4f7fd3805f Mon Sep 17 00:00:00 2001
From: Alexey Kondratov <kondratov.aleksey@gmail.com>
Date: Thu, 3 Oct 2019 12:37:26 +0300
Subject: [PATCH v2 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   | 32 +++++++++---
 src/bin/pg_rewind/t/006_actions.pl         | 61 ++++++++++++++++++++++
 src/bin/pg_rewind/t/RewindTest.pm          | 20 ++++++-
 7 files changed, 107 insertions(+), 14 deletions(-)
 create mode 100644 src/bin/pg_rewind/t/006_actions.pl

diff --git a/src/bin/pg_rewind/t/001_basic.pl b/src/bin/pg_rewind/t/001_basic.pl
index c3293e93df..1ba1648af6 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 => 13;
 
 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..57674ff4b3 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 => 9;
 
 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..16c92cb2d6 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 => 7;
 
 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..6dabd11db6 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 => 7;
 }
 
 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..089466a06b 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 if the two clusters are on the same
+# timeline runs successfully.
+#
 use strict;
 use warnings;
 use TestLib;
-use Test::More tests => 1;
+use Test::More tests => 5;
 
 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/006_actions.pl b/src/bin/pg_rewind/t/006_actions.pl
new file mode 100644
index 0000000000..0667bffeae
--- /dev/null
+++ b/src/bin/pg_rewind/t/006_actions.pl
@@ -0,0 +1,61 @@
+#
+# Test incompatible options and pg_rewind internal sanity checks.
+#
+use strict;
+use warnings;
+use TestLib;
+use Test::More tests => 3;
+
+use FindBin;
+use lib $FindBin::RealBin;
+
+use RewindTest;
+
+RewindTest::setup_cluster('local', ['-g']);
+RewindTest::start_master();
+RewindTest::create_standby('local');
+
+my $master_pgdata   = $node_master->data_dir;
+my $standby_pgdata  = $node_standby->data_dir;
+
+RewindTest::promote_standby();
+
+# Check that pg_rewind errors out if target server
+# wasn't shutdown.
+command_fails(
+    [
+        'pg_rewind', "--debug",
+        "--source-pgdata=$standby_pgdata",
+        "--target-pgdata=$master_pgdata",
+        "--no-ensure-shutdown"
+    ],
+    'pg_rewind local without target shutdown');
+
+$node_master->stop;
+
+# Check that pg_rewind errors out if source server
+# wasn't shutdown.
+command_fails(
+    [
+        'pg_rewind', "--debug",
+        "--source-pgdata=$standby_pgdata",
+        "--target-pgdata=$master_pgdata",
+        "--no-ensure-shutdown"
+    ],
+    'pg_rewind local without source shutdown');
+
+$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');
+
+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..1cc88f8551 100644
--- a/src/bin/pg_rewind/t/RewindTest.pm
+++ b/src/bin/pg_rewind/t/RewindTest.pm
@@ -246,7 +246,7 @@ sub run_pg_rewind
 	{
 
 		# 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;
 		command_ok(
 			[
@@ -261,6 +261,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 +287,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


--------------9152C0CA87F324E25AC888EC--