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

On 04.03.2020 10:45, Michael Paquier wrote:
> On Mon, Mar 02, 2020 at 08:59:49PM +0300, Alexey Kondratov wrote:
>> All other remarks look clear for me, so I fix them in the next patch
>> version, thanks.
> Already done as per the attached, with a new routine named
> getRestoreCommand() and more done.

Many thanks for doing that. I went through the diff between v21 and v20. 
Most of the changes look good to me.

- *        Functions for finding and validating executable files
+ *        Functions for finding and validating from executables files

There is probably something missing here. Finding and validating what? 
And 'executables files' does not seem to be correct as well.

+        # First, remove all the content in the archive directory,
+        # as RecursiveCopy::copypath does not support copying to
+        # existing directories.

I think that 'remove all the content' is not completely correct in this 
case. We are simply removing archive directory. There is no content 
there yet, so 'First, remove archive directory...' should be fine.

> - I did not actually get why you don't check for a missing command
> when using wait_result_is_any_signal.  In this case I'd think that it
> is better to exit immediately as follow-up calls would just fail.

Believe me or not, but I put 'false' there intentionally. The idea was 
that if the reason is a signal, then maybe user tired of waiting and 
killed that restore_command process theirself or something like that, so 
it is better to exit immediately. If it was a missing command, then 
there is no hurry, so we can go further and complain that attempt of 
recovering WAL segment has failed.

Actually, I guess that there is no big difference if we include missing 
command here or not. There is no complicated logic further compared to 
real recovery process in Postgres, where we cannot simply return false 
in that case.

> - The code was rather careless about error handling and
> RestoreArchivedWALFile(), and it seemed to me that it is rather
> pointless to report an extra message "could not restore file \"%s\"
> from archive" on top of the other error.

Probably you mean several pg_log_error calls not followed by 'return 
-1;'. Yes, I did it to fall down to the function end and show this extra 
message, but I agree that there is no much sense in doing so.


Regards

-- 
Alexey Kondratov

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