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.ga1...@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
