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: Mon, 7 Oct 2019 15:31:45 +0300
Message-ID: <88ef8c84-ffc7-f5d6-073b-83464e8bf2e7@postgrespro.ru> (raw)
In-Reply-To: <20191007010651.GD14532@paquier.xyz>
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>
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 100644inline patch 1: src/bin/pg_rewind/t/005_same_timeline.pl (+4/-0 lines) download inline-1.patch--- 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...
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
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: <88ef8c84-ffc7-f5d6-073b-83464e8bf2e7@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