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

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

It would be nice to add the --dry-run part, though I think that we
could just make that part of one of the existing tests, and stop the
target server first (got to think about that part, please see below).

> As for incompatible options and sanity checks testing, yes, I agree that it
> is a matter of different patch. I attached it as a separate WIP patch just
> for history. Maybe I will try to gather more cases there later.

Thanks.  I have applied the first patch for the various improvements
around --no-ensure-shutdown.

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?
- --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.  I tend to think that this would live better
as part of another existing test, only running for say the local mode.
It is also possible to group all your tests from patch 2 and
006_actions.pl in this area.
- 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.

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