Subject: Re: Two pg_rewind patches (auto generate recovery conf and ensure
 clean shutdown)
To: Paul Guo <pguo@pivotal.io>, Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Michael Paquier <michael@paquier.xyz>,
 Thomas Munro <thomas.munro@gmail.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>, Jimmy Yih <jyih@pivotal.io>,
 Ashwin Agrawal <aagrawal@pivotal.io>
References: 
 <CAEET0ZHdPgZmvcu18yNtVwqYqTo1kKs=9WVHyZXyU9g9pp1T9Q@mail.gmail.com>
 <20190926145154.GA22406@alvherre.pgsql>
 <CAEET0ZEQZQ1TEDxbTLeu1B_L6Y7G5AYVOs6HH3kSrzGffSMd4Q@mail.gmail.com>
From: Alexey Kondratov <a.kondratov@postgrespro.ru>
Message-ID: <2f726102-3f1e-bf16-061e-501919473ace@postgrespro.ru>
Date: Fri, 27 Sep 2019 15:18:59 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: 
 <CAEET0ZEQZQ1TEDxbTLeu1B_L6Y7G5AYVOs6HH3kSrzGffSMd4Q@mail.gmail.com>
Content-Type: multipart/mixed;
 boundary="------------721E7657E4E28157BC3BE1AC"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------721E7657E4E28157BC3BE1AC
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit

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


--------------721E7657E4E28157BC3BE1AC
Content-Type: text/x-patch;
 name="v9-0003-Test-new-standby-start-with-generated-config-duri.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v9-0003-Test-new-standby-start-with-generated-config-duri.pa";
 filename*1="tch"

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.pl
index 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.pl
index 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.pl
index 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.pl
index 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.pm
index 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


--------------721E7657E4E28157BC3BE1AC--