On Tue, Nov 20, 2018 at 12:07:12PM +0100, Peter Eisentraut wrote:
> I went over the patch and did a bunch of small refinements. I have also
> updated the documentation more extensively. The attached patch is
> committable to me.
Thanks for putting energy into that.
> Recovery is now initiated by a file recovery.signal. Standby mode is
> initiated by a file standby.signal. The standby_mode setting is
> gone. If a recovery.conf file is found, an error is issued.
I am having second thoughts about this part of the patch actually.
What's bad in keeping standby_mode and just rely on recovery.signal to
enforce recovery to happen? When the startup process starts all the
parameters should be loaded. That would also need less work from users
to switch to the new APIs. I think that there could be a point to
*merge* hot_standby and standby_mode actually into an enum, so keeping
standby_mode would help with that (not this patch work of course). The
barrier between recovery.trigger standby.trigger is also rather thin.
> The trigger_file setting has been renamed to promote_trigger_file as
> part of the move.
This rename looks fine.
> pg_basebackup -R now appends settings to postgresql.auto.conf and
> creates a standby.signal file.
>
> Author: Simon Riggs <simon@2ndquadrant.com>
> Author: Abhijit Menon-Sen <ams@2ndquadrant.com>
> Author: Sergei Kornilov <sk@zsrv.org>
I think that Fujii Masao should also be listed as an author, or at least
get a mention. He was one of the first to work on this stuff.
Using separate GUCs for each target is fine by me. I would have thought
that 003_recovery_targets.pl needed some tweaks, so I am happy to see
that it is not the case.
So overall this stuff looks fine per its shape, just the part for
standby.signal may not justify breaking compatibility more than
necessary... The first mention of this part comes from
https://postgr.es/m/CANP8+jLoYSDjB5ip7wynVcckoE4OynHabUnB8pQMgBJgFKQpiw@mail.gmail.com
as far as I know.
--
Michael
