Re: PATCH: add "--config-file=" option to pg_rewind

Heya,

some minor comments, I didn't look at the added test and I did not test
the patch at all, but (as part of the Debian/Ubuntu packaging team) I
think this patch is really important:

On Tue, Mar 01, 2022 at 12:35:27PM +0100, Gunnar "Nick" Bluth wrote:
> diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml
> index 33e6bb64ad..d0278e9b46 100644
> --- a/doc/src/sgml/ref/pg_rewind.sgml
> +++ b/doc/src/sgml/ref/pg_rewind.sgml
> @@ -241,6 +241,18 @@ PostgreSQL documentation
> </listitem>
> </varlistentry>
>
> + <varlistentry>
> + <term><option>--config-file=<replaceable class="parameter">filepath</replaceable></option></term>
> + <listitem>
> + <para>
> + When using the <option>-c / --restore-target-wal</option> option, the <varname>restore_command</varname>
> + is extracted from the configuration of the target cluster. If the <filename>postgresql.conf</filename>
> + of that cluster is not in the target pgdata directory (or if you want to use an alternative config),

I think we usually just say "data directory"; pgdata was previously only
used as part of the option name, not like this in the text.

> diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
> index b39b5c1aac..294353a9d5 100644
> --- a/src/bin/pg_rewind/pg_rewind.c
> +++ b/src/bin/pg_rewind/pg_rewind.c
> @@ -61,6 +61,7 @@ char *datadir_target = NULL;
> char *datadir_source = NULL;
> char *connstr_source = NULL;
> char *restore_command = NULL;
> +char *config_file = NULL;
>
> static bool debug = false;
> bool showprogress = false;
> @@ -87,6 +88,7 @@ usage(const char *progname)
> printf(_("Options:\n"));
> printf(_(" -c, --restore-target-wal use restore_command in target configuration to\n"
> " retrieve WAL files from archives\n"));
> + printf(_(" --config-file=FILE path to postgresql.conf if outside target-pgdata (for -c)\n"));

> printf(_(" -D, --target-pgdata=DIRECTORY existing data directory to modify\n"));
> printf(_(" --source-pgdata=DIRECTORY source data directory to synchronize with\n"));
> printf(_(" --source-server=CONNSTR source server to synchronize with\n"));

target-pgdata is the name of the option, but maybe it is useful to spell
out "target data directory" here, even if that means we spill over to
two lines (which we might have to do anyway, that new line is pretty
long).

> @@ -121,6 +123,7 @@ main(int argc, char **argv)
> {"no-sync", no_argument, NULL, 'N'},
> {"progress", no_argument, NULL, 'P'},
> {"debug", no_argument, NULL, 3},
> + {"config-file", required_argument, NULL, 5},

Not sure what our policy is, but the way the numbered options are 1, 2,
4, 3, 5 after this is weird; this was already off before this patch
though.

> {NULL, 0, NULL, 0}
> };
> int option_index;
> @@ -205,6 +208,10 @@ main(int argc, char **argv)
> case 4:
> no_ensure_shutdown = true;
> break;
> +
> + case 5:
> + config_file = pg_strdup(optarg);
> + break;
> }
> }
>
> @@ -1060,7 +1067,11 @@ getRestoreCommand(const char *argv0)
> /* add -D switch, with properly quoted data directory */
> appendPQExpBufferStr(postgres_cmd, " -D ");
> appendShellString(postgres_cmd, datadir_target);
> -
> + if (config_file != NULL)
> + {
> + appendPQExpBufferStr(postgres_cmd, " --config_file=");
> + appendShellString(postgres_cmd, config_file);
> + }
> /* add -C switch, for restore_command */

You removed an empty line here. Maybe rather prepend this with a comment
on what's going on, and have en empty line before and afterwards.

> appendPQExpBufferStr(postgres_cmd, " -C restore_command");
>
> @@ -1138,6 +1149,11 @@ ensureCleanShutdown(const char *argv0)
> /* add set of options with properly quoted data directory */
> appendPQExpBufferStr(postgres_cmd, " --single -F -D ");
> appendShellString(postgres_cmd, datadir_target);
> + if (config_file != NULL)
> + {
> + appendPQExpBufferStr(postgres_cmd, " --config_file=");
> + appendShellString(postgres_cmd, config_file);
> + }
>
> /* finish with the database name, and a properly quoted redirection */

Kinde same here.

Cheers,

Michael

--
Michael Banck
Teamleiter PostgreSQL-Team
Projektleiter
Tel.: +49 2166 9901-171
Email: michael(dot)banck(at)credativ(dot)de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Geoff Richardson, Peter Lilley

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz