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

From: Alexey Kondratov <a(dot)kondratov(at)postgrespro(dot)ru>
To: Michael Paquier <michael(at)paquier(dot)xyz>, Andrey Borodin <x4mmm(at)yandex-team(dot)ru>
Cc: David Steele <david(at)pgmasters(dot)net>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>, vladimirlesk(at)yandex-team(dot)ru, dsarafan(at)yandex-team(dot)ru
Subject: Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line
Date: 2019-03-27 17:48:56
Message-ID: 9269d05f-ce96-c8ef-2e76-b9e3345cb8c4@postgrespro.ru
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: 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

Attachment Content-Type Size
v6-0001-pg_rewind-options-to-use-restore_command.patch text/x-patch 20.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alvaro Herrera 2019-03-27 18:03:47 Re: PostgreSQL pollutes the file system
Previous Message Fred .Flintstone 2019-03-27 17:40:05 Re: PostgreSQL pollutes the file system