Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

Hi!

On Tue, Dec 3, 2019 at 12:41 PM Alexey Kondratov
<a(dot)kondratov(at)postgrespro(dot)ru> wrote:
> On 01.12.2019 5:57, Michael Paquier wrote:
> > On Thu, Sep 26, 2019 at 03:08:22PM +0300, Alexey Kondratov wrote:
> >> As Alvaro correctly pointed in the nearby thread [1], we've got an
> >> interference regarding -R command line argument. I agree that it's a good
> >> idea to reserve -R for recovery configuration write to be consistent with
> >> pg_basebackup, so I've updated my patch to use another letters:
> > The patch has rotten and does not apply anymore. Could you please
> > send a rebased version? I have moved the patch to next CF, waiting on
> > author for now.
>
> Rebased and updated patch is attached.
>
> There was a problem with testing new restore_command options altogether
> with recent ensureCleanShutdown. My test simply moves all WAL from
> pg_wal and generates restore_command for a new options testing, but this
> prevents startup recovery required by ensureCleanShutdown. To test both
> options in the same we have to leave some recent WAL segments in the
> pg_wal and be sure that they are enough for startup recovery, but not
> enough for successful pg_rewind run. I have manually figured out that
> required amount of inserted records (and generated WAL) to achieve this.
> However, I think that this approach is not good for test, since tests
> may be modified in the future (amount of writes to DB changed) or even
> volume of WAL written by Postgres will change. It will lead to falsely
> always failed or passed tests.
>
> Moreover, testing both ensureCleanShutdown and new options in the same
> time doesn't hit new code paths, so I decided to test new options with
> --no-ensure-shutdown for simplicity and stability of tests.

I think this patch is now in a good shape and already got enough of review.

I made some minor cleanup. In particular, I've to fix usage of terms
"WAL" and "WALs". This patch sometimes use term "WAL" to specify
single WAL file and term "WALs" to specify multiple WAL files. But
WAL stands for Write Ahead Log. So, "WALs" literally stands to
multiple logs. And we don't use term "WALs" to describe multiple WAL
files anywhere else. Usage of term "WAL" to describe single file is
not clear as well.

The revision of patch is attached. I'm going to push it if no objections.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company