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: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Alexander Korotkov <a.korotkov@postgrespro.ru>, Kuntal Ghosh <kuntalghosh.2007@gmail.com>, Alexey Kondratov <a.kondratov@postgrespro.ru>, Liudmila Mantrova <l.mantrova@postgrespro.ru>, Thomas Munro <thomas.munro@gmail.com>, Andrey Borodin <x4mmm@yandex-team.ru>, David Steele <david@pgmasters.net>, 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: 2020-03-26T22:47:29Z
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 Thu, Mar 26, 2020 at 06:56:36PM -0300, Alvaro Herrera wrote:
> Uh, is XLOGDIR the only reason to include xlog_internal.h?  Maybe it
> would be easier to have a routine in xlog.c that returns the path?
> There's a few functions in xlog.c that could use it, as well.

Yep, that's all.  We could also just hardcode the path as we did when
we worked on the exclusion filter lists for pg_rewind and basebackup.c, 
though I'd prefer avoid that if possible.

> The patch downthread looks decent cleanup, but I'm not sure how it helps
> further the objective.

I actually think it does, because you get out of the way the conflicts
caused by RestoreArchivedFile() in the backend, as we are targetting
for fe_archive.c to be a frontend-only file, though I agree that it is
not the full of it.

> (A really good cleanup could be a situation where frontend files don't
> need xlog_internal.h -- for example, maybe a new file xlogpage.h could
> contain struct defs that relate to page and segment headers and the
> like, as well as useful macros.  I don't know if this can be made to
> work -- but xlog_internal.h contains stuff like xl_parameter_change etc
> as well as RmgrData which surely are of no interest to readers of wal
> files ... or, say, RequestXLogSwitch.)

A second header that could be created is xlogpaths.h (or xlogfiles.h?)
that includes all the routines and variables we use to build WAL
segment names and such, with XLogFileNameById, IsTLHistoryFileName,
etc.  I agree that splitting the record-related parts may make sense,
say xlogrecovery.h?

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