Re: [Patch] pg_rewind: options to use restore_command fromrecovery.conf or command line - Mailing list pgsql-hackers

On 26.03.2019 11:19, Michael Paquier wrote:
> + * This is a simplified and adapted to frontend version of
> + * RestoreArchivedFile function from transam/xlogarchive.c
> + */
> +static int
> +RestoreArchivedWAL(const char *path, const char *xlogfname,
> I don't think that we should have duplicates for that, so I would
> recommend refactoring the code so as a unique code path is taken by
> both, especially since the user can fetch the command from
> postgresql.conf.

This comment is here since the beginning of my work on this patch and 
now it is rather misleading.

Even if we does not take into account obvious differences like error 
reporting, different log levels based on many conditions, cleanup 
options, check for standby mode; restore_command execution at backend 
recovery and during pg_rewind has a very important difference. If it 
fails at backend, then as stated in the comment 'Remember, we 
rollforward UNTIL the restore fails so failure here is just part of the 
process' -- it is OK. In opposite, in pg_rewind if we failed to recover 
some required WAL segment, then it definitely means the end of the 
entire process, since we will fail at finding last common checkpoint or 
extracting page map.

The only part we can share is constructing restore_command with aliases 
replacement. However, even in this place the logic is slightly 
different, since we do not need %r alias for pg_rewind. The only use 
case of %r in restore_command I know is pg_standby, which seems to be as 
not a case for pg_rewind. I have tried to move this part to the common, 
but it becomes full of conditions and less concise.

Please, correct me if I am wrong, but it seems that there are enough 
differences to keep this function separated, isn't it?

> Why two options?  Wouldn't actually be enough use-postgresql-conf to
> do the job?  Note that "postgres" should always be installed if
> pg_rewind is present because it is a backend-side utility, so while I
> don't like adding a dependency to other binaries in one binary, having
> an option to pass out a command directly via the command line of
> pg_rewind stresses me more.

I am not familiar enough with DBA scenarios, where -R option may be 
useful, but I was asked a few times for that. I can only speculate that 
for example someone may want to run freshly rewinded cluster as master, 
not replica, so its config may differ from replica's one, where 
restore_command is surely intended to be. Thus, it is easier to leave 
master's config at the place and just specify restore_command as command 
line argument.

> Don't we need to worry about signals interrupting the restore command?
> It seems to me that some refactoring from the stuff in xlogarchive.c
> would be in order.

Thank you for pointing me to this place again. Previously, I thought 
that we should not care about it, since if restore_command was not 
successful due to any reason, then rewind failed, so we will stop and 
exit at upper levels. However, if it was due to a signal, then some of 
next messages may be misleading, if e.g. user manually interrupted it 
for some reason. So that, I added a similar check here as well.

Updated version of patch is attached.


-- 
Alexey Kondratov

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