Re: Two pg_rewind patches (auto generate recovery conf and ensure clean shutdown)

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