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.
>From 7286e31ab0ebf50bb4ab460dd81b82f1c5989272 Mon Sep 17 00:00:00 2001
From: Alexey Kondratov <kondratov.alek...@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
>From 28fdd2fa58af718d8a894cb3c3d8f9b2cdf6759e Mon Sep 17 00:00:00 2001
From: Alexey Kondratov <kondratov.alek...@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
