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

Hi Andrey,

Thank you for the review! I have updated the patch according to your
comments and remarks. Please, find new version attached.

On 2019-01-07 12:12, Andrey Borodin wrote:
> 
> Here are some my notes:
> 1. RestoreArchivedWAL() shares a lot of code with
> RestoreArchivedFile(). Is it possible\viable to refactor and extract
> common part?
> 

I am not sure, but I guess that differences in errors reporting and
points 2-3 are enough to leave new RestoreArchivedWAL() apart. There
are not many common parts left.

> 
> 2. IMV pg_rewind with %r restore_command should fail. %r is designed
> to clean archive from WALs, nothing should be deleted in case of
> fetching WALs for rewind. Last restartpoint has no meaning during
> rewind. Or does it? If so, let's comment about it.
> 

Yes, during rewind we need the last common checkpoint, not restart 
point.
Taking into account that Michael pointed me to this place too and I 
cannot
propose a real-life example of restore_command with %r to use in 
pg_rewind,
I decided to add an exception in such a case. So now pg_rewind will fail
with error.

> 
> 3. RestoreArchivedFile() checks for signals, is it done by pg_rewind 
> elsewhere?
> 

There is a comment part inside RestoreArchivedFile():

   * However, if the failure was due to any sort of signal, it's best to
   * punt and abort recovery.  (If we "return false" here, upper levels 
will
   * assume that recovery is complete and start up the database!)

In other words, there is a possibility to start up the database assuming
that recovery went well, while actually it was terminated by signal. It
happens since failure is expected during the recovery, so some kind of
ambiguity occurs, which we trying to solve checking for termination 
signals.

In contrast, we are looking in the archive for each missing WAL file 
during
pg_rewind and if we failed to restore it by any means rewind will fail
indifferent of was it due to the termination signal or file is actually
missing in the archive. Thus, there no ambiguity occurs and we do not 
need
to check for signals here.

That is how I understand it. Probably someone can explain why I am 
wrong.

> 
> 4. No documentation is updated
> 

I have updated docs in order to reflect the new functionality as well.

> 
> 5. -R takes precedence over -r without notes. Shouldn't we complain?
> Or may be we should take one from config, iif nothing found use -R?
> 

I do not think it is worth of additional complexity and we could expect,
that end-users know, what they want to use–either -r or -R–so I added
an error message due to the conflicting options.



Regards

-- 
Alexey Kondratov

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