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

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.
>

Added.

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

Regards
--
Alexey Kondratov

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