From: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Paul Guo <pguo@pivotal.io>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
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: Fri, 27 Sep 2019 15:18:59 +0300
Message-ID: <2f726102-3f1e-bf16-061e-501919473ace@postgrespro.ru> (raw)
In-Reply-To: <CAEET0ZEQZQ1TEDxbTLeu1B_L6Y7G5AYVOs6HH3kSrzGffSMd4Q@mail.gmail.com>
References: <CAEET0ZHdPgZmvcu18yNtVwqYqTo1kKs=9WVHyZXyU9g9pp1T9Q@mail.gmail.com>
<20190926145154.GA22406@alvherre.pgsql>
<CAEET0ZEQZQ1TEDxbTLeu1B_L6Y7G5AYVOs6HH3kSrzGffSMd4Q@mail.gmail.com>
On 27.09.2019 6:27, Paul Guo wrote:
>
>
> Secondarily, I see no reason to test connstr_source rather than just
> "conn" in the other patch; doing it the other way is more natural,
> since
> it's that thing that's tested as an argument.
>
> pg_rewind.c: Please put the new #include line keeping the alphabetical
> order.
>
>
> Agreed to the above suggestions. I attached the v9.
>
I went through the remaining two patches and they seem to be very clear
and concise. However, there are two points I could complain about:
1) Maybe I've missed it somewhere in the thread above, but currently
pg_rewind allows to run itself with -R and --source-pgdata. In that case
-R option is just swallowed and neither standby.signal, nor
postgresql.auto.conf is written, which is reasonable though. Should it
be stated somehow in the docs that -R option always has to go altogether
with --source-server? Or should pg_rewind notify user that options are
incompatible and no recovery configuration will be written?
2) Are you going to leave -R option completely without tap-tests?
Attached is a small patch, which tests -R option along with the existing
'remote' case. If needed it may be split into two separate cases. First,
it tests that pg_rewind is able to succeed with minimal permissions
according to the Michael's patch d9f543e [1]. Next, it checks presence
of standby.signal and adds REPLICATION permission to rewind_user to test
that new standby is able to start with generated recovery configuration.
[1]
https://github.com/postgres/postgres/commit/d9f543e9e9be15f92abdeaf870e57ef289020191
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
Attachments:v9-0003-Test-new-standby-start-with-generated-config-duri.patch (text/x-patch, 2.8K) download
From 8c607794f259cd4dec0fa6172b69d62e6468bee3 Mon Sep 17 00:00:00 2001
From: Alexey Kondratov <kondratov.aleksey@gmail.com>
Date: Fri, 27 Sep 2019 14:30:57 +0300
Subject: [PATCH v9 3/3] Test new standby start with generated config during
pg_rewind remote
---
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/RewindTest.pm | 11 ++++++++++-
5 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/src/bin/pg_rewind/t/001_basic.pl b/src/bin/pg_rewind/t/001_basic.plindex 115192170e..c3293e93df 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 => 10;+use Test::More tests => 11;
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 f1eb4fe1d2..1db534c0dc 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 => 6;+use Test::More tests => 7;
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 c4040bd562..f4710440fc 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 => 4;+use Test::More tests => 5;
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 ed1ddb6b60..639eeb9c91 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 => 4;+ plan tests => 5;
}
use FindBin;
diff --git a/src/bin/pg_rewind/t/RewindTest.pm b/src/bin/pg_rewind/t/RewindTest.pmindex 68b6004e94..fcc48cb1d9 100644--- a/src/bin/pg_rewind/t/RewindTest.pm+++ b/src/bin/pg_rewind/t/RewindTest.pm@@ -266,9 +266,18 @@ sub run_pg_rewind
[
'pg_rewind', "--debug",
"--source-server", $standby_connstr,
- "--target-pgdata=$master_pgdata", "--no-sync"+ "--target-pgdata=$master_pgdata", "-R", "--no-sync"
],
'pg_rewind remote');
++ # Check that standby.signal has been created.+ ok(-e "$master_pgdata/standby.signal");++ # Now, when pg_rewind apparently succeeded with minimal permissions,+ # add REPLICATION privilege. So we could test that new standby+ # is able to connect to the new master with generated config.+ $node_standby->psql(+ 'postgres', "ALTER ROLE rewind_user WITH REPLICATION;");
}
else
{
--
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, pguo@pivotal.io, alvherre@2ndquadrant.com, 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: <2f726102-3f1e-bf16-061e-501919473ace@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