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
>From 9e828e311dc7c216e5bfb1936022be4f7fd3805f Mon Sep 17 00:00:00 2001
From: Alexey Kondratov <kondratov.alek...@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
