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: Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>, 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-27 13:41:42
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 2020-02-27 04:52, Michael Paquier wrote:
> On Thu, Feb 27, 2020 at 12:43:55AM +0300, Alexander Korotkov wrote:
>> 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.
> I have read through v17 (not tested, sorry), and spotted a couple of
> issues that need to be addressed.
> + "--source-pgdata=$standby_pgdata",
> + "--target-pgdata=$master_pgdata",
> + "--no-sync", "--no-ensure-shutdown",
> FWIW, I think that perl indenting would reshape this part. I would
> recommend to run src/tools/pgindent/pgperltidy and
> ./src/tools/perlcheck/pgperlcritic before commit.

Thanks, formatted this part with perltidy. It also has modified
RecursiveCopy's indents. Pgperlcritic has no complains about this file.
BTW, being executed on the whole project pgperltidy modifies dozens of
perl files an even pgindent itself.

> + * Copyright (c) 2020, PostgreSQL Global Development Group
> Wouldn't it be better to just use the full copyright here? I mean the
> following:
> Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
> Portions Copyright (c) 1994, The Regents of the University of
> California

I think so, it contains some older code parts, so it is better to use
unified copyrights.

> +++ b/src/common/archive.c
> [...]
> +#include "postgres.h"
> +
> +#include "common/archive.h"
> This is incorrect. All files shared between the backend and the
> frontend in src/common/ have to include the following set of headers:
> #ifndef FRONTEND
> #include "postgres.h"
> #else
> #include "postgres_fe.h"
> #endif
> +++ b/src/common/fe_archive.c
> [...]
> +#include "postgres_fe.h"
> This is incomplete. The following piece should be added:
> #ifndef FRONTEND
> #error "This file is not expected to be compiled for backend code"
> #endif

Fixed both.

> + snprintf(postgres_cmd, sizeof(postgres_cmd), "%s -D %s
> -C restore_command",
> + postgres_exec_path, datadir_target);
> +
> I think that this is missing proper quoting.

Yep, added the same quoting as in pg_upgrade/options.

> I would rename ConstructRestoreCommand() to BuildRestoreCommand()
> while on it..

OK, shorter is better.

> I think that it would be saner to check the return status of
> ConstructRestoreCommand() in xlogarchive.c as a sanity check, with an
> elog(ERROR) if not 0, as that should never happen.


New version of the patch is attached. Thanks again for your review.

Alexey Kondratov

Postgres Professional
The Russian Postgres Company

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

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Alvaro Herrera 2020-02-27 14:23:07 Re: Commit fest manager for 2020-03
Previous Message Peter Eisentraut 2020-02-27 13:37:24 Re: Improve handling of parameter differences in physical replication