Horton

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

Michael Paquier <michael@paquier.xyz>

View thread View on postgr.es
From: Michael Paquier <michael@paquier.xyz>
To: Alexey Kondratov <a.kondratov@postgrespro.ru>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>, Paul Guo <pguo@pivotal.io>, Thomas Munro <thomas.munro@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>, Jimmy Yih <jyih@pivotal.io>, Ashwin Agrawal <aagrawal@pivotal.io>
Date: 2019-10-03T03:07:52Z
Lists: pgsql-hackers

Attachments

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

Commits

  1. Improve test coverage of pg_rewind

  2. Improve handling and coverage of --no-ensure-shutdown in pg_rewind

  3. Fix issues in pg_rewind with --no-ensure-shutdown/--write-recovery-conf

  4. Fix --dry-run mode of pg_rewind

  5. pg_rewind: test new --write-recovery-conf functionality

  6. pg_rewind: Allow writing recovery configuration

  7. Have pg_rewind run crash recovery before rewinding

  8. Split out recovery confing-writing code from pg_basebackup

  9. Switch TAP tests of pg_rewind to use non-superuser role, take two