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: Paul Guo <pguo@pivotal.io>
Cc: pgsql-hackers@postgresql.org, Jimmy Yih <jyih@pivotal.io>, Ashwin Agrawal <aagrawal@pivotal.io>
Date: 2019-03-19T06:18:51Z
Lists: pgsql-hackers 
On Tue, Mar 19, 2019 at 02:09:03PM +0800, Paul Guo wrote:
> The first patch adds an option to automatically generate recovery conf
> contents in related files, following pg_basebackup. In the patch,
> GenerateRecoveryConf(), WriteRecoveryConf() and escape_quotes() are almost
> same as them on pg_basebackup. The main difference is due to replication
> slot support and code (variables) limit. It seems that we could slightly
> refactor later to put some common code into another file after aligning
> pg_rewind with pg_basebackup. This was tested manually and was done by
> Jimmy (cc-ed), Ashiwin (cc-ed) and me.


Interesting.  The two routines have really the same logic, I would
recommend to have a first patch which does the refactoring and have
pg_rewind use it, and then a second patch which writes recovery.conf
and uses the first patch to get the contents.  Please note that the
common routine needs to be version-aware as pg_basebackup requires
compatibility with past versions, but you could just pass the version
number from the connection, and have pg_rewind pass the compiled-in
version value.

> Another patch does automatic clean shutdown by running a single mode
> postgres instance if the target was not clean shut down since that is
> required by pg_rewind. This was manually tested and was done by Jimmy
> (cc-ed) and me. I'm not sure if we want a test case for that though.

I am not sure that I see the value in that.  I'd rather let the
required service start and stop out of pg_rewind and not introduce
dependencies with other binaries.  This step can also take quite some
time depending on the amount of WAL to replay post-crash at recovery
and the shutdown checkpoint which is required to reach a consistent
on-disk state.
--
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