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

On Tue, Mar 19, 2019 at 2:18 PM Michael Paquier <michael@paquier.xyz> wrote:

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

This is a good suggestion also. Will do it.


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

This makes recovery more automatically. Yes, it will add the dependency on
the postgres
binary, but it seems that most time pg_rewind should be shipped as postgres
in the same install directory. From my experience of manually testing
pg_rewind,
I feel that this besides auto-recovery-conf writing really alleviate my
burden. I'm not sure how
other users usually do before running pg_rewind when the target is not
cleanly shut down,
but probably we can add an argument to pg_rewind to give those people who
want to
handle target separately another chance? default on or off whatever.


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

The time is still required for people who want to make the target ready for
pg_rewind in another way.

Thanks.