From: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Michael Paquier <michael@paquier.xyz>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Paul Guo <pguo@pivotal.io>
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: Thu, 3 Oct 2019 12:43:37 +0300
Message-ID: <c62d6a22-2976-8491-8e66-bcbb33ac3eb0@postgrespro.ru> (raw)
In-Reply-To: <20191003030752.GE1586@paquier.xyz>
References: <20190930171349.GA21905@alvherre.pgsql>
<7ca88204-3e0b-2f4c-c8af-acadc4b266e5@postgrespro.ru>
<20191003030752.GE1586@paquier.xyz>
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
Attachments:v2-0002-Increase-pg_rewind-code-coverage.patch (text/x-patch, 6.2K) download
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.plindex 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.plindex 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.plindex 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.plindex 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.plindex 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.plnew file mode 100644index 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.pmindex 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
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, michael@paquier.xyz, alvherre@2ndquadrant.com, pguo@pivotal.io, 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: <c62d6a22-2976-8491-8e66-bcbb33ac3eb0@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