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: Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>
Cc: Michael Paquier <michael(at)paquier(dot)xyz>, Alvaro Herrera <alvherre(at)2ndquadrant(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-02-26 20:45:05
Message-ID: d5f4883dc6ecf5a762e2b8843200ad61@postgrespro.ru
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 2020-02-26 22:03, Alexander Korotkov wrote:
> On Tue, Feb 25, 2020 at 1:48 PM Alexander Korotkov
> <a(dot)korotkov(at)postgrespro(dot)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. 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.

--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
The Russian Postgres Company

Attachment Content-Type Size
v17-0001-pg_rewind-Add-options-to-restore-WAL-files.patch text/x-diff 26.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Dave Cramer 2020-02-26 20:51:39 Re: Error on failed COMMIT
Previous Message Pavel Stehule 2020-02-26 20:40:02 Re: proposal: schema variables