Horton

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

Alexander Korotkov <a.korotkov@postgrespro.ru>

View thread
From: Alexander Korotkov <a.korotkov@postgrespro.ru>
To: Alexey Kondratov <a.kondratov@postgrespro.ru>
Cc: Michael Paquier <michael@paquier.xyz>, Alvaro Herrera <alvherre@2ndquadrant.com>, 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-02-26T21:43:55Z
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, Feb 26, 2020 at 11:45 PM Alexey Kondratov
<a.kondratov@postgrespro.ru> wrote:
> On 2020-02-26 22:03, Alexander Korotkov wrote:
> > On Tue, Feb 25, 2020 at 1:48 PM Alexander Korotkov
> > <a.korotkov@postgrespro.ru> wrote:
> >>
> >> I think usage of chmod() deserves comment.  As I get default
> >> permissions are sufficient for work, but we need to set them to
> >> satisfy 'check PGDATA permissions' test.
> >
> >
> > I've added this comment myself.
> >
>
> Thanks for doing it yourself, I was going to answer tonight, but it
> would be obviously too late.
>
> >
> > I've also fixes some indentation.
> > Patch now looks good to me.  I'm going to push it if no objections.
> >
>
> I think that docs should be corrected. Previously Michael was against
> the phrase 'restore_command defined in the postgresql.conf', since it
> also could be defined in any config file included there. We corrected it
> in the pg_rewind --help output, but now docs say:
>
> +        Use the <varname>restore_command</varname> defined in
> +        <filename>postgresql.conf</filename> to retrieve WAL files from
> +        the WAL archive if these files are no longer available in the
> +        <filename>pg_wal</filename> directory of the target cluster.
>
> Probably it should be something like:
>
> +        Use the <varname>restore_command</varname> defined in
> +        the target cluster configuration to retrieve WAL files from
> +        the WAL archive if these files are no longer available in the
> +        <filename>pg_wal</filename> directory.
>
> Here the only text split changed:
>
> -        * Ignore restore_command when not in archive recovery (meaning
> -        * we are in crash recovery).
> +        * Ignore restore_command when not in archive recovery (meaning we are
> in
> +        * crash recovery).
>
> Should we do so in this patch?
>
> I think that this extra dot at the end is not necessary here:
>
> +               pg_log_debug("using config variable restore_command=\'%s\'.",
> restore_command);
>
> If you agree then attached is a patch with all the corrections above.

Thank you!  I'll push the v17 version of patch.

Regarding text split change, it was made by pgindent.  I didn't notice
it belongs to unchanged part of code.  Sure, we shouldn't include this
into the patch.

> It is made with default git format-patch format, but yours were in a
> slightly different format, so I only was able to apply them with git am
> --patch-format=stgit.

I made this patch using 'git show'. Yes, it would be better if I use
'git format-patch' instead.  Thank you for noticing.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company