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

On 30.10.2018 06:01, Michael Paquier wrote:

> On Mon, Oct 29, 2018 at 12:09:21PM +0300, Alexey Kondratov wrote:
>> Currently in the patch, with dry-run option (-n) pg_rewind only fetches
>> missing WALs to be able to build file map, while doesn't touch any data
>> files. So I guess it behaves exactly as you described and we do not need a
>> separate tool.
> Makes sense perhaps.  Fetching only WAL segments which are needed for
> the file map is critical, as you don't want to spend bandwidth for
> nothing.  Now, I look at your patch, and I can see things to complain
> about, at least three at short glance:
> - The TAP test added will fail on Windows.

Thank you for this. Build on Windows has been broken as well. I fixed it 
in the new version of patch, please find attached.

> - Simply copy-pasting RestoreArchivedWAL() from the backend code to
> pg_rewind is not an acceptable option.  You don't care about %r either
> in this case.

According to the docs [1] %r is a valid alias and may be used in 
restore_command too, so if we take restore_command from recovery.conf it 
might be there. If we just drop it, then restore_command may stop 
working. Though I do not know real life examples of restore_command with 
%r, we should treat it in expected way (as backend does), of course if 
we want an option to take it from recovery.conf.

> - Reusing the GUC parser is something I would avoid as well.  Not worth
> the complexity.

Yes, I don't like it either. I will try to make guc-file.l frontend safe.

[1] https://www.postgresql.org/docs/11/archive-recovery-settings.html

-- 
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company