Horton

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

Dmitry Dolgov <9erthalion6@gmail.com>

View thread
From: Dmitry Dolgov <9erthalion6@gmail.com>
To: a.kondratov@postgrespro.ru
Cc: Michael Paquier <michael@paquier.xyz>, Alvaro Herrera <alvherre@2ndquadrant.com>, x4mmm@yandex-team.ru, PostgreSQL-development <pgsql-hackers@postgresql.org>, vladimirlesk@yandex-team.ru, dsarafan@yandex-team.ru
Date: 2018-11-30T16:04:01Z
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, Nov 7, 2018 at 10:58 AM Alexey Kondratov <a.kondratov@postgrespro.ru> wrote:
>
> On 30.10.2018 06:01, Michael Paquier wrote:
>
> > On Mon, Oct 29, 2018 at 12:09:21PM +0300, Alexey Kondratov wrote:
> >> Currently in the patch, with dry-run option (-n) pg_rewind only fetches
> >> missing WALs to be able to build file map, while doesn't touch any data
> >> files. So I guess it behaves exactly as you described and we do not need a
> >> separate tool.
> > Makes sense perhaps.  Fetching only WAL segments which are needed for
> > the file map is critical, as you don't want to spend bandwidth for
> > nothing.  Now, I look at your patch, and I can see things to complain
> > about, at least three at short glance:
> > - The TAP test added will fail on Windows.
>
> Thank you for this. Build on Windows has been broken as well. I fixed it
> in the new version of patch, please find attached.

Just to confirm, patch still can be applied without conflicts, and pass all the
tests. Also I like the original motivation for the feature, sounds pretty
useful. For now I'm moving it to the next CF.

> > - Simply copy-pasting RestoreArchivedWAL() from the backend code to
> > pg_rewind is not an acceptable option.  You don't care about %r either
> > in this case.
>
> According to the docs [1] %r is a valid alias and may be used in
> restore_command too, so if we take restore_command from recovery.conf it
> might be there. If we just drop it, then restore_command may stop
> working. Though I do not know real life examples of restore_command with
> %r, we should treat it in expected way (as backend does), of course if
> we want an option to take it from recovery.conf.
>
> > - Reusing the GUC parser is something I would avoid as well.  Not worth
> > the complexity.
>
> Yes, I don't like it either. I will try to make guc-file.l frontend safe.

Any success with that?