|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|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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
> Portions Copyright (c) 1996-2020, PostgreSQL Global Development Group
> Portions Copyright (c) 1994, The Regents of the University of
I think so, it contains some older code parts, so it is better to use
> +++ 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"
> #include "postgres_fe.h"
> +++ 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"
> + 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.
Postgres Professional https://www.postgrespro.com
The Russian Postgres Company
|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|