Subject: Re: Two pg_rewind patches (auto generate recovery conf and ensure
 clean shutdown)
To: Michael Paquier <michael@paquier.xyz>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>, Paul Guo <pguo@pivotal.io>,
 Thomas Munro <thomas.munro@gmail.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>, Jimmy Yih <jyih@pivotal.io>,
 Ashwin Agrawal <aagrawal@pivotal.io>
References: <20190930171349.GA21905@alvherre.pgsql>
 <7ca88204-3e0b-2f4c-c8af-acadc4b266e5@postgrespro.ru>
 <20191003030752.GE1586@paquier.xyz>
 <c62d6a22-2976-8491-8e66-bcbb33ac3eb0@postgrespro.ru>
 <20191004083721.GA1829@paquier.xyz>
 <ab5c15e0-773c-fbb4-f0a2-ca00a90035a6@postgrespro.ru>
 <20191007010651.GD14532@paquier.xyz>
From: Alexey Kondratov <a.kondratov@postgrespro.ru>
Message-ID: <88ef8c84-ffc7-f5d6-073b-83464e8bf2e7@postgrespro.ru>
Date: Mon, 7 Oct 2019 15:31:45 +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: <20191007010651.GD14532@paquier.xyz>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

On 07.10.2019 4:06, Michael Paquier wrote:
> On Fri, Oct 04, 2019 at 05:21:25PM +0300, Alexey Kondratov wrote:
>> I've checked your patch, but it seems that it cannot be applied as is, since
>> it e.g. adds a comment to 005_same_timeline.pl without actually changing the
>> test. So I've slightly modified your patch and tried to fit both dry-run and
>> ensureCleanShutdown testing together. It works just fine and fails
>> immediately if any of recent fixes is reverted. I still think that dry-run
>> testing is worth adding, since it helped to catch this v12 refactoring
>> issue, but feel free to throw it way if it isn't commitable right now, of
>> course.
> I can guarantee the last patch I sent can be applied on top of HEAD:
> https://www.postgresql.org/message-id/20191004083721.GA1829@paquier.xyz

Yes, it did, but my comment was about these lines:

diff --git a/src/bin/pg_rewind/t/005_same_timeline.pl 
b/src/bin/pg_rewind/t/005_same_timeline.pl
index 40dbc44caa..df469d3939 100644
--- a/src/bin/pg_rewind/t/005_same_timeline.pl
+++ b/src/bin/pg_rewind/t/005_same_timeline.pl
@@ -1,3 +1,7 @@
+#
+# Test that running pg_rewind with the source and target clusters
+# on the same timeline runs successfully.
+#

You have added this new comment section, but kept the old one, which was 
pretty much the same [1].

> Regarding the rest, I have hacked my way through as per the attached.
> The previous set of patches did the following, which looked either
> overkill or not necessary:
> - Why running test 005 with the remote mode?

OK, it was definitely an overkill, since remote control file fetch will 
be also tested in any other remote test case.

> - --dry-run coverage is basically the same with the local and remote
> modes, so it seems like a waste of resource to run it for all the
> tests and all the modes.

My point was to test --dry-run + --write-recover-conf in remote, since 
the last one may cause recovery configuration write without doing any 
actual work, due to some wrong refactoring for example.

> - There is no need for the script checking for options combinations to
> initialize a data folder.  It is important to design the tests to be
> cheap and meaningful.

Yes, I agree, moving some of those tests to just a 001_basic seems to be 
a proper optimization.

> Patch v3-0002 also had a test to make sure that the source server is
> shut down cleanly before using it.  I have included that part as
> well, as the flow feels right.
>
> So, Alexey, what do you think?

It looks good for me. Two minor remarks:

+    # option combinations.  As the code paths taken by those tests
+    # does not change for the "local" and "remote" modes, just run them

I am far from being fluent in English, but should it be 'do not change' 
instead?

+command_fails(
+    [
+        'pg_rewind',     '--target-pgdata',
+        $primary_pgdata, '--source-pgdata',
+        $standby_pgdata, 'extra_arg1'
+    ],

Here and below I would prefer traditional options ordering "'--key', 
'value'". It should be easier to recognizefrom the reader perspective:

+command_fails(
+    [
+        'pg_rewind',
+        '--target-pgdata', $primary_pgdata,
+        '--source-pgdata', $standby_pgdata,
+        'extra_arg1'
+    ],


[1] 
https://github.com/postgres/postgres/blob/caa078353ecd1f3b3681c0d4fa95ad4bb8c2308a/src/bin/pg_rewind/t/005_same_timeline.pl#L15


-- 
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company