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

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Alexey Kondratov <a(dot)kondratov(at)postgrespro(dot)ru>
Cc: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Paul Guo <pguo(at)pivotal(dot)io>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Jimmy Yih <jyih(at)pivotal(dot)io>, Ashwin Agrawal <aagrawal(at)pivotal(dot)io>
Subject: Re: Two pg_rewind patches (auto generate recovery conf and ensure clean shutdown)
Date: 2019-10-08 02:51:40
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Mon, Oct 07, 2019 at 03:31:45PM +0300, Alexey Kondratov wrote:
> On 07.10.2019 4:06, Michael Paquier wrote:
>> - --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.

Yes, that's possible. I agree that it would be nice to have an extra
test for that, still I would avoid making that run in all the tests.

>> 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?

That was wrong, fixed.

> +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:

While I agree with you, the perl indentation we use has formatted the
code this way. There is also an argument for keeping it at the end
for clarity (I recall that Windows also requires extra args to be
last when parsing options). Anyway, I have used a trick by adding
--debug to reach command, which is still useful, so the order of the
options is better at the end.

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Noah Misch 2019-10-08 04:59:46 Re: expressive test macros (was: Report test_atomic_ops() failures consistently, via macros)
Previous Message Smith, Peter 2019-10-07 23:13:28 RE: Proposal: Make use of C99 designated initialisers for nulls/values arrays