Horton

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

Michael Paquier <michael@paquier.xyz>

View thread
From: Michael Paquier <michael@paquier.xyz>
To: Alexander Korotkov <a.korotkov@postgrespro.ru>
Cc: Alexey Kondratov <a.kondratov@postgrespro.ru>, 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-27T01:52:38Z
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 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.

+ * 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

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

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

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

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