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>
Cc: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>, Kuntal Ghosh <kuntalghosh(dot)2007(at)gmail(dot)com>, Liudmila Mantrova <l(dot)mantrova(at)postgrespro(dot)ru>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Andrey Borodin <x4mmm(at)yandex-team(dot)ru>, David Steele <david(at)pgmasters(dot)net>, 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: 2020-03-30 14:00:01
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 2020-03-30 04:56, Michael Paquier wrote:
> On Fri, Mar 27, 2020 at 07:47:29AM +0900, Michael Paquier wrote:
>> On Thu, Mar 26, 2020 at 06:56:36PM -0300, Alvaro Herrera wrote:
>>> I don't think any such cleanup should hamper the patch at hand
>>> anyway.
>> I don't think either, so I would do the split for the archive routines
>> at least. It still feels strange to me to have a different routine
>> name for the frontend-side of RestoreArchivedFile(). That's not
>> really consistent with the current practice we have palloc() & co, as
>> well as the sync routines.
> Okay. Hearing nothing, I have rebased the patch set this morning,
> improving some comments and the code format while on it. I would like
> to commit both 0001 (creation of xlogarchiver.h) and 0002 attached in
> the next couple of days. If you have an objection, please feel free.


What do think about adding following sanity check into xlogarchive.c?

+#ifdef FRONTEND
+#error "This file is not expected to be compiled for frontend code"

It would prevent someone from adding typedefs and any other common
definitions into xlogarchive.h in the future, leading to the accidental
inclusion of both xlogarchive.h and fe_archive.h in the same time.


+ */
+RestoreArchivedFile(const char *path, const char *xlogfname,
+ off_t expectedSize, const char *restoreCommand)

There is a couple of extra tabs IMO.

+extern int RestoreArchivedFile(const char *path,
+ const char *xlogfname,
+ off_t expectedSize,
+ const char *restoreCommand);

And the same here.

+ * This uses a logic based on "postgres -C" to get the value from
+ * from the cluster.

Repetitive 'from'.

extractPageMap(const char *datadir, XLogRecPtr startpoint, int
- XLogRecPtr endpoint)
+ XLogRecPtr endpoint, const char *restore_command)

Let us use camel case 'restoreCommand' here as in the header for

I have left 0001 intact, but fixed all these small remarks in the 0002.
Please, find it attached.

Alexey Kondratov

Postgres Professional
Russian Postgres Company

Attachment Content-Type Size
v6-0002-Add-c-restore-target-wal-to-pg_rewind.patch text/x-diff 23.6 KB
v6-0001-Move-routines-of-xlogarchive.c-into-their-own-hea.patch text/x-diff 5.2 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Alexander Korotkov 2020-03-30 14:04:00 Re: Improving connection scalability: GetSnapshotData()
Previous Message Rémi Zara 2020-03-30 13:42:59 Re: pgsql: Add kqueue(2) support to the WaitEventSet API.