From: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Paul Guo <pguo@pivotal.io>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Thomas Munro <thomas.munro@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Cc: Jimmy Yih <jyih@pivotal.io>
Cc: Ashwin Agrawal <aagrawal@pivotal.io>
Subject: Re: Two pg_rewind patches (auto generate recovery conf and ensure clean shutdown)
Date: Wed, 2 Oct 2019 20:28:09 +0300
Message-ID: <7ca88204-3e0b-2f4c-c8af-acadc4b266e5@postgrespro.ru> (raw)
In-Reply-To: <20190930171349.GA21905@alvherre.pgsql>
References: <20190930171349.GA21905@alvherre.pgsql>
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.
Attachments:download all (2 files, .tar.gz)v1-0001-Fix-functionality-of-pg_rewind-dry-run-and-no-ens.patch (text/x-patch, 1.6K) download
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.cindex 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
v1-0002-Increase-pg_rewind-code-coverage.patch (text/x-patch, 4.8K) download
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.plindex 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.plindex 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.plindex 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.plindex 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.plindex 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.pmindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: a.kondratov@postgrespro.ru, alvherre@2ndquadrant.com, pguo@pivotal.io, michael@paquier.xyz, thomas.munro@gmail.com, jyih@pivotal.io, aagrawal@pivotal.io
Subject: Re: Two pg_rewind patches (auto generate recovery conf and ensure clean shutdown)
In-Reply-To: <7ca88204-3e0b-2f4c-c8af-acadc4b266e5@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox