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
