Horton

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

Michael Paquier <michael@paquier.xyz>

View thread
From: Michael Paquier <michael@paquier.xyz>
To: Andrey Borodin <x4mmm@yandex-team.ru>
Cc: Alexey Kondratov <a.kondratov@postgrespro.ru>, David Steele <david@pgmasters.net>, Alvaro Herrera <alvherre@2ndquadrant.com>, Andres Freund <andres@anarazel.de>, PostgreSQL-development <pgsql-hackers@postgresql.org>, Dmitry Dolgov <9erthalion6@gmail.com>, vladimirlesk@yandex-team.ru, dsarafan@yandex-team.ru
Date: 2019-03-26T08:19:32Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Move frontend-side archive APIs from src/common/ to src/fe_utils/

  2. Add -c/--restore-target-wal to pg_rewind

  3. Move routine definitions of xlogarchive.c to a new header file

  4. Move routine building restore_command to src/common/

  5. Integrate recovery.conf into postgresql.conf

On Wed, Mar 20, 2019 at 01:55:51PM +0800, Andrey Borodin wrote:
> I'm a bit confused by by console output routines. E.g. in
> pg_rewind's main() you call pg_fatal()s, and printf(), and pg_log()
> with various levels. Shouldn't we use all the pg_* functions?

pg_fatal() would exit immediately, and sometimes the error code paths
want to have multiple lines, which is why printf() gets used.

> But most of this printing usages were there before your patch.
> 
> I'm marking the patch as RFC, since I have no more notices and patch
> really looks good.

That does not look fully baked yet, at least in my opinion.

+ * 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. 

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.

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.
--
Michael