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

On Wed, Oct 02, 2019 at 08:28:09PM +0300, Alexey Kondratov wrote:
> I've directly followed your guess and tried to elaborate pg_rewind test
> cases and... It seems I've caught a few bugs:
>
> 1) --dry-run actually wasn't completely 'dry'. It did update target
> controlfile, which could cause repetitive pg_rewind calls to fail after
> dry-run ones.

I have just paid attention to this thread, but this is a bug which
goes down to 12 actually so let's treat it independently of the rest.
The control file was not written thanks to the safeguards in
write_target_range() in past versions, but the recent refactoring
around control file handling broke that promise.  Another thing which
is not completely exact is the progress reporting which should be
reported even if the dry-run mode runs.  That's less critical, but
let's make things consistent.

Patch 0001 also forgot that recovery.conf should not be written either
when no rewind is needed.

I have reworked your first patch as per the attached.  What do you
think about it?  The part with the control file needs to go down to
v12, and I would likely split that into two commits on HEAD: one for
the control file and a second for the recovery.conf portion with the
fix for --no-ensure-shutdown to keep a cleaner history.

+               # Check that incompatible options error out.
+               command_fails(
+                       [
+                               'pg_rewind', "--debug",
+                               "--source-pgdata=$standby_pgdata",
+                               "--target-pgdata=$master_pgdata", "-R",
+                               "--no-ensure-shutdown"
+                       ],
+                       'pg_rewind local with -R');
Incompatible options had better be checked within a separate perl
script?  We generally do that for the other binaries.
--
Michael