Supply restore_command to pg_rewind via CLI argument
Hi hackers!
Starting from v13 pg_rewind can use restore_command if it lacks necessary WAL segments. And this is awesome for HA clusters with many nodes! Thanks to everyone who worked on the feature!
Here's some feedback on how to make things even better.
If we run 'pg_rewind --restore-target-wal' there must be restore_command in config of target installation. But if the config is not within $PGDATA\postgresql.conf pg_rewind cannot use it.
If we run postmaster with `-c config_file=/etc/postgresql/10/data/postgresql.conf`, we simply cannot use the feature. We solved the problem by putting config into PGDATA only during pg_rewind, but this does not seem like a very robust solution.
Maybe we could add "-C, --target-restore-command=COMMAND target WAL restore_command\n" as was proposed within earlier versions of the patch[0]/messages/by-id/CAPpHfduUqKLr2CRpcpHcv1qjaz+-+i9bOL2AOvdWSr954ti8Xw@mail.gmail.com? Or instruct pg_rewind to pass config to 'postgres -C restore_command' run?
From my POV adding --target-restore-command is simplest way, I can extract corresponding portions from original patch.
Thanks!
Best regards, Andrey Borodin.
[0]: /messages/by-id/CAPpHfduUqKLr2CRpcpHcv1qjaz+-+i9bOL2AOvdWSr954ti8Xw@mail.gmail.com
Hi,
On Fri, Jun 18, 2021 at 5:42 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
If we run 'pg_rewind --restore-target-wal' there must be restore_command in config of target installation. But if the config is not within $PGDATA\postgresql.conf pg_rewind cannot use it.
If we run postmaster with `-c config_file=/etc/postgresql/10/data/postgresql.conf`, we simply cannot use the feature. We solved the problem by putting config into PGDATA only during pg_rewind, but this does not seem like a very robust solution.
Yeah, Michael was against it, while we had no good arguments, so
Alexander removed it, IIRC. This example sounds reasonable to me. I
also recall some complaints from PostgresPro support folks, that it is
sad to not have a cli option to pass restore_command. However, I just
thought about another recent feature --- ensure clean shutdown, which
is turned on by default. So you usually run Postgres with one config,
but pg_rewind may start it with another, although in single-user mode.
Is it fine for you?
Maybe we could add "-C, --target-restore-command=COMMAND target WAL restore_command\n" as was proposed within earlier versions of the patch[0]? Or instruct pg_rewind to pass config to 'postgres -C restore_command' run?
Hm, adding --target-restore-command is the simplest way, sure, but
forwarding something like '-c config_file=...' to postgres sounds
interesting too. Could it have any use case beside providing a
restore_command? I cannot imagine anything right now, but if any
exist, then it could be a more universal approach.
From my POV adding --target-restore-command is simplest way, I can extract corresponding portions from original patch.
I will have a look, maybe I even already have this patch separately. I
remember that we were considering adding this option to PostgresPro,
when we did a backport of this feature.
--
Alexey Kondratov
On Fri, Jun 18, 2021 at 10:06 PM Alexey Kondratov
<kondratov.aleksey@gmail.com> wrote:
On Fri, Jun 18, 2021 at 5:42 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
If we run 'pg_rewind --restore-target-wal' there must be restore_command in config of target installation. But if the config is not within $PGDATA\postgresql.conf pg_rewind cannot use it.
If we run postmaster with `-c config_file=/etc/postgresql/10/data/postgresql.conf`, we simply cannot use the feature. We solved the problem by putting config into PGDATA only during pg_rewind, but this does not seem like a very robust solution.Yeah, Michael was against it, while we had no good arguments, so
Alexander removed it, IIRC. This example sounds reasonable to me. I
also recall some complaints from PostgresPro support folks, that it is
sad to not have a cli option to pass restore_command. However, I just
thought about another recent feature --- ensure clean shutdown, which
is turned on by default. So you usually run Postgres with one config,
but pg_rewind may start it with another, although in single-user mode.
Is it fine for you?Maybe we could add "-C, --target-restore-command=COMMAND target WAL restore_command\n" as was proposed within earlier versions of the patch[0]? Or instruct pg_rewind to pass config to 'postgres -C restore_command' run?
Hm, adding --target-restore-command is the simplest way, sure, but
forwarding something like '-c config_file=...' to postgres sounds
interesting too. Could it have any use case beside providing a
restore_command? I cannot imagine anything right now, but if any
exist, then it could be a more universal approach.From my POV adding --target-restore-command is simplest way, I can extract corresponding portions from original patch.
I will have a look, maybe I even already have this patch separately. I
remember that we were considering adding this option to PostgresPro,
when we did a backport of this feature.
Here it is. I have slightly adapted the previous patch to the recent
pg_rewind changes. In this version -C does not conflict with -c, it
just overrides it.
--
Alexey Kondratov
Attachments:
v1-0001-Allow-providing-restore_command-as-a-command-line.patchapplication/octet-stream; name=v1-0001-Allow-providing-restore_command-as-a-command-line.patchDownload
From 3c6489d50605955d313b022458cda2184600b489 Mon Sep 17 00:00:00 2001
From: Alexey Kondratov <kondratov.aleksey@gmail.com>
Date: Tue, 29 Jun 2021 17:17:47 +0300
Subject: [PATCH v1] Allow providing restore_command as a command line option
to pg_rewind
This could be useful when postgres is usually run with
-c config_file=..., so the actual configuration and restore_command
is not inside $PGDATA/postgresql.conf.
---
doc/src/sgml/ref/pg_rewind.sgml | 19 +++++++
src/bin/pg_rewind/pg_rewind.c | 45 +++++++++------
src/bin/pg_rewind/t/001_basic.pl | 3 +-
src/bin/pg_rewind/t/RewindTest.pm | 95 ++++++++++++++++++-------------
src/test/perl/PostgresNode.pm | 5 +-
5 files changed, 107 insertions(+), 60 deletions(-)
diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml
index 33e6bb64ad..af75f35867 100644
--- a/doc/src/sgml/ref/pg_rewind.sgml
+++ b/doc/src/sgml/ref/pg_rewind.sgml
@@ -241,6 +241,25 @@ PostgreSQL documentation
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>-C <replaceable class="parameter">restore_command</replaceable></option></term>
+ <term><option>--target-restore-command=<replaceable class="parameter">restore_command</replaceable></option></term>
+ <listitem>
+ <para>
+ Specifies the <varname>restore_command</varname> to use for retrieving
+ WAL files from the WAL archive if these files are no longer available
+ in the <filename>pg_wal</filename> directory of the target cluster.
+ </para>
+ <para>
+ If <varname>restore_command</varname> is already set in
+ <filename>postgresql.conf</filename>, you can provide the
+ <option>--restore-target-wal</option> option instead. If both options
+ are provided, then <option>--target-restore-command</option>
+ will be used.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>--debug</option></term>
<listitem>
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 2ac4910778..f42077fbcb 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -84,21 +84,22 @@ usage(const char *progname)
printf(_("%s resynchronizes a PostgreSQL cluster with another copy of the cluster.\n\n"), progname);
printf(_("Usage:\n %s [OPTION]...\n\n"), progname);
printf(_("Options:\n"));
- printf(_(" -c, --restore-target-wal use restore_command in target configuration to\n"
- " retrieve WAL files from archives\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"));
- printf(_(" -n, --dry-run stop before modifying anything\n"));
- printf(_(" -N, --no-sync do not wait for changes to be written\n"
- " safely to disk\n"));
- printf(_(" -P, --progress write progress messages\n"));
- printf(_(" -R, --write-recovery-conf write configuration for replication\n"
- " (requires --source-server)\n"));
- printf(_(" --debug write a lot of debug messages\n"));
- printf(_(" --no-ensure-shutdown do not automatically fix unclean shutdown\n"));
- printf(_(" -V, --version output version information, then exit\n"));
- printf(_(" -?, --help show this help, then exit\n"));
+ printf(_(" -c, --restore-target-wal use restore_command in target configuration to\n"
+ " retrieve WAL files from archives\n"));
+ printf(_(" -C, --target-restore-command=COMMAND target WAL restore_command\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"));
+ printf(_(" -n, --dry-run stop before modifying anything\n"));
+ printf(_(" -N, --no-sync do not wait for changes to be written\n"
+ " safely to disk\n"));
+ printf(_(" -P, --progress write progress messages\n"));
+ printf(_(" -R, --write-recovery-conf write configuration for replication\n"
+ " (requires --source-server)\n"));
+ printf(_(" --debug write a lot of debug messages\n"));
+ printf(_(" --no-ensure-shutdown do not automatically fix unclean shutdown\n"));
+ printf(_(" -V, --version output version information, then exit\n"));
+ printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nReport bugs to <%s>.\n"), PACKAGE_BUGREPORT);
printf(_("%s home page: <%s>\n"), PACKAGE_NAME, PACKAGE_URL);
}
@@ -116,6 +117,7 @@ main(int argc, char **argv)
{"no-ensure-shutdown", no_argument, NULL, 4},
{"version", no_argument, NULL, 'V'},
{"restore-target-wal", no_argument, NULL, 'c'},
+ {"target-restore-command", required_argument, NULL, 'C'},
{"dry-run", no_argument, NULL, 'n'},
{"no-sync", no_argument, NULL, 'N'},
{"progress", no_argument, NULL, 'P'},
@@ -156,7 +158,7 @@ main(int argc, char **argv)
}
}
- while ((c = getopt_long(argc, argv, "cD:nNPR", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "cC:D:nNPR", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -168,6 +170,11 @@ main(int argc, char **argv)
restore_wal = true;
break;
+ case 'C':
+ restore_wal = true;
+ restore_command = pg_strdup(optarg);
+ break;
+
case 'P':
showprogress = true;
break;
@@ -1024,7 +1031,11 @@ getRestoreCommand(const char *argv0)
postgres_cmd[MAXPGPATH],
cmd_output[MAXPGPATH];
- if (!restore_wal)
+ /*
+ * Take restore_command from the postgresql.conf only if it is not already
+ * provided as a command line option.
+ */
+ if (!restore_wal && restore_command == NULL)
return;
/* find postgres executable */
diff --git a/src/bin/pg_rewind/t/001_basic.pl b/src/bin/pg_rewind/t/001_basic.pl
index d636f35f5e..f7de47d296 100644
--- a/src/bin/pg_rewind/t/001_basic.pl
+++ b/src/bin/pg_rewind/t/001_basic.pl
@@ -4,7 +4,7 @@
use strict;
use warnings;
use TestLib;
-use Test::More tests => 23;
+use Test::More tests => 29;
use FindBin;
use lib $FindBin::RealBin;
@@ -190,5 +190,6 @@ in primary, before promotion
run_test('local');
run_test('remote');
run_test('archive');
+run_test('archive_cli');
exit(0);
diff --git a/src/bin/pg_rewind/t/RewindTest.pm b/src/bin/pg_rewind/t/RewindTest.pm
index 938c661afc..9f88ebd324 100644
--- a/src/bin/pg_rewind/t/RewindTest.pm
+++ b/src/bin/pg_rewind/t/RewindTest.pm
@@ -220,6 +220,58 @@ sub promote_standby
return;
}
+sub run_pg_rewind_archive
+{
+ my $test_mode = shift;
+ my $restore_command;
+ my $cli_option = '--restore-target-wal';
+
+ # Remove the existing archive directory and move all WAL
+ # segments from the old primary to the archives. These
+ # will be used by pg_rewind.
+ rmtree($node_primary->archive_dir);
+ RecursiveCopy::copypath($node_primary->data_dir . '/pg_wal',
+ $node_primary->archive_dir);
+
+ # Fast way to remove entire directory content
+ rmtree($node_primary->data_dir . '/pg_wal');
+ mkdir($node_primary->data_dir . '/pg_wal');
+
+ # Make sure that directories have the right umask as this is
+ # required by a follow-up check on permissions, and better
+ # safe than sorry.
+ chmod(0700, $node_primary->archive_dir);
+ chmod(0700, $node_primary->data_dir . '/pg_wal');
+
+ # Add appropriate restore_command to the target cluster
+ $restore_command = $node_primary->enable_restoring($node_primary, 0);
+
+ # In archive_cli mode pass restore_command as a command line option
+ # instead of taking it from postgresql.conf
+ if ($test_mode eq 'archive_cli') {
+ $cli_option = "--target-restore-command=$restore_command";
+ }
+
+ # Stop the new primary and be ready to perform the rewind.
+ $node_standby->stop;
+
+ # Note the use of --no-ensure-shutdown here. WAL files are
+ # gone in this mode and the primary has been stopped
+ # gracefully already.
+ command_ok(
+ [
+ 'pg_rewind',
+ '--debug',
+ '--source-pgdata=' . $node_standby->data_dir,
+ '--target-pgdata=' . $node_primary->data_dir,
+ '--no-sync',
+ '--no-ensure-shutdown',
+ $cli_option
+ ],
+ "pg_rewind $test_mode");
+
+}
+
sub run_pg_rewind
{
my $test_mode = shift;
@@ -231,7 +283,7 @@ sub run_pg_rewind
# Append the rewind-specific role to the connection string.
$standby_connstr = "$standby_connstr user=rewind_user";
- if ($test_mode eq 'archive')
+ if ($test_mode eq 'archive' or $test_mode eq 'archive_cli')
{
# pg_rewind is tested with --restore-target-wal by moving all
# WAL files to a secondary location. Note that this leads to
@@ -304,50 +356,13 @@ sub run_pg_rewind
$node_standby->safe_psql('postgres',
"ALTER ROLE rewind_user WITH REPLICATION;");
}
- elsif ($test_mode eq "archive")
+ elsif ($test_mode eq "archive" or $test_mode eq "archive_cli")
{
-
# Do rewind using a local pgdata as source and specified
# directory with target WAL archive. The old primary has
# to be stopped at this point.
- # Remove the existing archive directory and move all WAL
- # segments from the old primary to the archives. These
- # will be used by pg_rewind.
- rmtree($node_primary->archive_dir);
- RecursiveCopy::copypath($node_primary->data_dir . "/pg_wal",
- $node_primary->archive_dir);
-
- # Fast way to remove entire directory content
- rmtree($node_primary->data_dir . "/pg_wal");
- mkdir($node_primary->data_dir . "/pg_wal");
-
- # Make sure that directories have the right umask as this is
- # required by a follow-up check on permissions, and better
- # safe than sorry.
- chmod(0700, $node_primary->archive_dir);
- chmod(0700, $node_primary->data_dir . "/pg_wal");
-
- # Add appropriate restore_command to the target cluster
- $node_primary->enable_restoring($node_primary, 0);
-
- # Stop the new primary and be ready to perform the rewind.
- $node_standby->stop;
-
- # Note the use of --no-ensure-shutdown here. WAL files are
- # gone in this mode and the primary has been stopped
- # gracefully already.
- command_ok(
- [
- 'pg_rewind',
- "--debug",
- "--source-pgdata=$standby_pgdata",
- "--target-pgdata=$primary_pgdata",
- "--no-sync",
- "--no-ensure-shutdown",
- "--restore-target-wal"
- ],
- 'pg_rewind archive');
+ run_pg_rewind_archive($test_mode);
}
else
{
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index ed5b4a1c4b..bd38e4b70d 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -990,7 +990,8 @@ primary_conninfo='$root_connstr'
return;
}
-# Internal routine to enable archive recovery command on a standby node
+# Internal routine to enable archive recovery command on a standby node.
+# Returns generated restore_command.
sub enable_restoring
{
my ($self, $root_node, $standby) = @_;
@@ -1023,7 +1024,7 @@ restore_command = '$copy_command'
{
$self->set_recovery_mode();
}
- return;
+ return $copy_command;
}
=pod
base-commit: 48cb244fb9aca1620e35a14617ca5869b3ea065a
--
2.30.1 (Apple Git-130)
29 июня 2021 г., в 19:34, Alexey Kondratov <kondratov.aleksey@gmail.com> написал(а):
On Fri, Jun 18, 2021 at 10:06 PM Alexey Kondratov
<kondratov.aleksey@gmail.com> wrote:On Fri, Jun 18, 2021 at 5:42 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
If we run 'pg_rewind --restore-target-wal' there must be restore_command in config of target installation. But if the config is not within $PGDATA\postgresql.conf pg_rewind cannot use it.
If we run postmaster with `-c config_file=/etc/postgresql/10/data/postgresql.conf`, we simply cannot use the feature. We solved the problem by putting config into PGDATA only during pg_rewind, but this does not seem like a very robust solution.Yeah, Michael was against it, while we had no good arguments, so
Alexander removed it, IIRC. This example sounds reasonable to me. I
also recall some complaints from PostgresPro support folks, that it is
sad to not have a cli option to pass restore_command. However, I just
thought about another recent feature --- ensure clean shutdown, which
is turned on by default. So you usually run Postgres with one config,
but pg_rewind may start it with another, although in single-user mode.
Is it fine for you?
We rewind failovered node, so clean shutdown was not performed. But I do not see how it could help anyway.
To pass restore command we had to setup new config in PGDATA configured as standby, because either way we cannot set restore_command there.
Maybe we could add "-C, --target-restore-command=COMMAND target WAL restore_command\n" as was proposed within earlier versions of the patch[0]? Or instruct pg_rewind to pass config to 'postgres -C restore_command' run?
Hm, adding --target-restore-command is the simplest way, sure, but
forwarding something like '-c config_file=...' to postgres sounds
interesting too. Could it have any use case beside providing a
restore_command? I cannot imagine anything right now, but if any
exist, then it could be a more universal approach.
I think --target-restore-command is the best solution right now.
From my POV adding --target-restore-command is simplest way, I can extract corresponding portions from original patch.
I will have a look, maybe I even already have this patch separately. I
remember that we were considering adding this option to PostgresPro,
when we did a backport of this feature.Here it is. I have slightly adapted the previous patch to the recent
pg_rewind changes. In this version -C does not conflict with -c, it
just overrides it.
Great, thanks!
There is a small bug
+ /*
+ * Take restore_command from the postgresql.conf only if it is not already
+ * provided as a command line option.
+ */
+ if (!restore_wal && restore_command == NULL)
return;I think we should use condition (!restore_wal || restore_command != NULL).
Besides this patch looks good and is ready for committer IMV.
Thanks!
Best regards, Andrey Borodin.
On Fri, Aug 27, 2021 at 10:05 AM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
There is a small bug + /* + * Take restore_command from the postgresql.conf only if it is not already + * provided as a command line option. + */ + if (!restore_wal && restore_command == NULL) return;I think we should use condition (!restore_wal || restore_command != NULL).
Yes, you are right, thanks. Attached is a fixed version. Tests were
passing since PostgresNode->enable_restoring is adding restore_command
to the postgresql.conf anyway.
Besides this patch looks good and is ready for committer IMV.
--
Alexey Kondratov
Attachments:
v2-0001-Allow-providing-restore_command-as-a-command-line.patchapplication/octet-stream; name=v2-0001-Allow-providing-restore_command-as-a-command-line.patchDownload
From 7c87248cc7af342faed4033853f7dd080e1be896 Mon Sep 17 00:00:00 2001
From: Alexey Kondratov <kondratov.aleksey@gmail.com>
Date: Tue, 29 Jun 2021 17:17:47 +0300
Subject: [PATCH v2] Allow providing restore_command as a command line option
to pg_rewind
This could be useful when postgres is usually run with
-c config_file=..., so the actual configuration and restore_command
is not inside $PGDATA/postgresql.conf.
---
doc/src/sgml/ref/pg_rewind.sgml | 19 +++++++
src/bin/pg_rewind/pg_rewind.c | 45 +++++++++------
src/bin/pg_rewind/t/001_basic.pl | 3 +-
src/bin/pg_rewind/t/RewindTest.pm | 95 ++++++++++++++++++-------------
src/test/perl/PostgresNode.pm | 5 +-
5 files changed, 107 insertions(+), 60 deletions(-)
diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml
index 33e6bb64ad..af75f35867 100644
--- a/doc/src/sgml/ref/pg_rewind.sgml
+++ b/doc/src/sgml/ref/pg_rewind.sgml
@@ -241,6 +241,25 @@ PostgreSQL documentation
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>-C <replaceable class="parameter">restore_command</replaceable></option></term>
+ <term><option>--target-restore-command=<replaceable class="parameter">restore_command</replaceable></option></term>
+ <listitem>
+ <para>
+ Specifies the <varname>restore_command</varname> to use for retrieving
+ WAL files from the WAL archive if these files are no longer available
+ in the <filename>pg_wal</filename> directory of the target cluster.
+ </para>
+ <para>
+ If <varname>restore_command</varname> is already set in
+ <filename>postgresql.conf</filename>, you can provide the
+ <option>--restore-target-wal</option> option instead. If both options
+ are provided, then <option>--target-restore-command</option>
+ will be used.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>--debug</option></term>
<listitem>
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 2ac4910778..da4a2c503f 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -84,21 +84,22 @@ usage(const char *progname)
printf(_("%s resynchronizes a PostgreSQL cluster with another copy of the cluster.\n\n"), progname);
printf(_("Usage:\n %s [OPTION]...\n\n"), progname);
printf(_("Options:\n"));
- printf(_(" -c, --restore-target-wal use restore_command in target configuration to\n"
- " retrieve WAL files from archives\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"));
- printf(_(" -n, --dry-run stop before modifying anything\n"));
- printf(_(" -N, --no-sync do not wait for changes to be written\n"
- " safely to disk\n"));
- printf(_(" -P, --progress write progress messages\n"));
- printf(_(" -R, --write-recovery-conf write configuration for replication\n"
- " (requires --source-server)\n"));
- printf(_(" --debug write a lot of debug messages\n"));
- printf(_(" --no-ensure-shutdown do not automatically fix unclean shutdown\n"));
- printf(_(" -V, --version output version information, then exit\n"));
- printf(_(" -?, --help show this help, then exit\n"));
+ printf(_(" -c, --restore-target-wal use restore_command in target configuration to\n"
+ " retrieve WAL files from archives\n"));
+ printf(_(" -C, --target-restore-command=COMMAND target WAL restore_command\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"));
+ printf(_(" -n, --dry-run stop before modifying anything\n"));
+ printf(_(" -N, --no-sync do not wait for changes to be written\n"
+ " safely to disk\n"));
+ printf(_(" -P, --progress write progress messages\n"));
+ printf(_(" -R, --write-recovery-conf write configuration for replication\n"
+ " (requires --source-server)\n"));
+ printf(_(" --debug write a lot of debug messages\n"));
+ printf(_(" --no-ensure-shutdown do not automatically fix unclean shutdown\n"));
+ printf(_(" -V, --version output version information, then exit\n"));
+ printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nReport bugs to <%s>.\n"), PACKAGE_BUGREPORT);
printf(_("%s home page: <%s>\n"), PACKAGE_NAME, PACKAGE_URL);
}
@@ -116,6 +117,7 @@ main(int argc, char **argv)
{"no-ensure-shutdown", no_argument, NULL, 4},
{"version", no_argument, NULL, 'V'},
{"restore-target-wal", no_argument, NULL, 'c'},
+ {"target-restore-command", required_argument, NULL, 'C'},
{"dry-run", no_argument, NULL, 'n'},
{"no-sync", no_argument, NULL, 'N'},
{"progress", no_argument, NULL, 'P'},
@@ -156,7 +158,7 @@ main(int argc, char **argv)
}
}
- while ((c = getopt_long(argc, argv, "cD:nNPR", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "cC:D:nNPR", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -168,6 +170,11 @@ main(int argc, char **argv)
restore_wal = true;
break;
+ case 'C':
+ restore_wal = true;
+ restore_command = pg_strdup(optarg);
+ break;
+
case 'P':
showprogress = true;
break;
@@ -1024,7 +1031,11 @@ getRestoreCommand(const char *argv0)
postgres_cmd[MAXPGPATH],
cmd_output[MAXPGPATH];
- if (!restore_wal)
+ /*
+ * Take restore_command from the postgresql.conf only if it is not already
+ * provided as a command line option.
+ */
+ if (!restore_wal || restore_command != NULL)
return;
/* find postgres executable */
diff --git a/src/bin/pg_rewind/t/001_basic.pl b/src/bin/pg_rewind/t/001_basic.pl
index d636f35f5e..f7de47d296 100644
--- a/src/bin/pg_rewind/t/001_basic.pl
+++ b/src/bin/pg_rewind/t/001_basic.pl
@@ -4,7 +4,7 @@
use strict;
use warnings;
use TestLib;
-use Test::More tests => 23;
+use Test::More tests => 29;
use FindBin;
use lib $FindBin::RealBin;
@@ -190,5 +190,6 @@ in primary, before promotion
run_test('local');
run_test('remote');
run_test('archive');
+run_test('archive_cli');
exit(0);
diff --git a/src/bin/pg_rewind/t/RewindTest.pm b/src/bin/pg_rewind/t/RewindTest.pm
index 367b99a438..43e727b3d2 100644
--- a/src/bin/pg_rewind/t/RewindTest.pm
+++ b/src/bin/pg_rewind/t/RewindTest.pm
@@ -220,6 +220,58 @@ sub promote_standby
return;
}
+sub run_pg_rewind_archive
+{
+ my $test_mode = shift;
+ my $restore_command;
+ my $cli_option = '--restore-target-wal';
+
+ # Remove the existing archive directory and move all WAL
+ # segments from the old primary to the archives. These
+ # will be used by pg_rewind.
+ rmtree($node_primary->archive_dir);
+ RecursiveCopy::copypath($node_primary->data_dir . '/pg_wal',
+ $node_primary->archive_dir);
+
+ # Fast way to remove entire directory content
+ rmtree($node_primary->data_dir . '/pg_wal');
+ mkdir($node_primary->data_dir . '/pg_wal');
+
+ # Make sure that directories have the right umask as this is
+ # required by a follow-up check on permissions, and better
+ # safe than sorry.
+ chmod(0700, $node_primary->archive_dir);
+ chmod(0700, $node_primary->data_dir . '/pg_wal');
+
+ # Add appropriate restore_command to the target cluster
+ $restore_command = $node_primary->enable_restoring($node_primary, 0);
+
+ # In archive_cli mode pass restore_command as a command line option
+ # instead of taking it from postgresql.conf
+ if ($test_mode eq 'archive_cli') {
+ $cli_option = "--target-restore-command=$restore_command";
+ }
+
+ # Stop the new primary and be ready to perform the rewind.
+ $node_standby->stop;
+
+ # Note the use of --no-ensure-shutdown here. WAL files are
+ # gone in this mode and the primary has been stopped
+ # gracefully already.
+ command_ok(
+ [
+ 'pg_rewind',
+ '--debug',
+ '--source-pgdata=' . $node_standby->data_dir,
+ '--target-pgdata=' . $node_primary->data_dir,
+ '--no-sync',
+ '--no-ensure-shutdown',
+ $cli_option
+ ],
+ "pg_rewind $test_mode");
+
+}
+
sub run_pg_rewind
{
my $test_mode = shift;
@@ -231,7 +283,7 @@ sub run_pg_rewind
# Append the rewind-specific role to the connection string.
$standby_connstr = "$standby_connstr user=rewind_user";
- if ($test_mode eq 'archive')
+ if ($test_mode eq 'archive' or $test_mode eq 'archive_cli')
{
# pg_rewind is tested with --restore-target-wal by moving all
# WAL files to a secondary location. Note that this leads to
@@ -304,50 +356,13 @@ sub run_pg_rewind
$node_standby->safe_psql('postgres',
"ALTER ROLE rewind_user WITH REPLICATION;");
}
- elsif ($test_mode eq "archive")
+ elsif ($test_mode eq "archive" or $test_mode eq "archive_cli")
{
-
# Do rewind using a local pgdata as source and specified
# directory with target WAL archive. The old primary has
# to be stopped at this point.
- # Remove the existing archive directory and move all WAL
- # segments from the old primary to the archives. These
- # will be used by pg_rewind.
- rmtree($node_primary->archive_dir);
- RecursiveCopy::copypath($node_primary->data_dir . "/pg_wal",
- $node_primary->archive_dir);
-
- # Fast way to remove entire directory content
- rmtree($node_primary->data_dir . "/pg_wal");
- mkdir($node_primary->data_dir . "/pg_wal");
-
- # Make sure that directories have the right umask as this is
- # required by a follow-up check on permissions, and better
- # safe than sorry.
- chmod(0700, $node_primary->archive_dir);
- chmod(0700, $node_primary->data_dir . "/pg_wal");
-
- # Add appropriate restore_command to the target cluster
- $node_primary->enable_restoring($node_primary, 0);
-
- # Stop the new primary and be ready to perform the rewind.
- $node_standby->stop;
-
- # Note the use of --no-ensure-shutdown here. WAL files are
- # gone in this mode and the primary has been stopped
- # gracefully already.
- command_ok(
- [
- 'pg_rewind',
- "--debug",
- "--source-pgdata=$standby_pgdata",
- "--target-pgdata=$primary_pgdata",
- "--no-sync",
- "--no-ensure-shutdown",
- "--restore-target-wal"
- ],
- 'pg_rewind archive');
+ run_pg_rewind_archive($test_mode);
}
else
{
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 8158ea5b2f..5992aa887e 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -1014,7 +1014,8 @@ primary_conninfo='$root_connstr'
return;
}
-# Internal routine to enable archive recovery command on a standby node
+# Internal routine to enable archive recovery command on a standby node.
+# Returns generated restore_command.
sub enable_restoring
{
my ($self, $root_node, $standby) = @_;
@@ -1047,7 +1048,7 @@ restore_command = '$copy_command'
{
$self->set_recovery_mode();
}
- return;
+ return $copy_command;
}
=pod
base-commit: abc0910e2e0adfc5a17e035465ee31242e32c4fc
--
2.32.0
Besides this patch looks good and is ready for committer IMV.
A variant of this patch was originally objected against by Michael, and as this
version is marked Ready for Committer I would like to hear his opinions on
whether the new evidence changes anything.
--
Daniel Gustafsson https://vmware.com/
14 сент. 2021 г., в 18:41, Daniel Gustafsson <daniel@yesql.se> написал(а):
Besides this patch looks good and is ready for committer IMV.
A variant of this patch was originally objected against by Michael, and as this
version is marked Ready for Committer I would like to hear his opinions on
whether the new evidence changes anything.
I skimmed the thread for reasoning. --target-restore-command was rejected on the following grounds
Do we actually need --target-restore-command at all? It seems to me
that we have all we need with --restore-target-wal, and that's not
really instinctive to pass down a command via another command..
Currently we know that --restore-target-wal is not enough if postgresql.conf does not reside within PGDATA.
Best regards, Andrey Borodin.
On 14 Sep 2021, at 16:05, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
Do we actually need --target-restore-command at all? It seems to me
that we have all we need with --restore-target-wal, and that's not
really instinctive to pass down a command via another command..Currently we know that --restore-target-wal is not enough if postgresql.conf does not reside within PGDATA.
That's a useful reason which wasn't brought up in the earlier thread, and may
tip the scales in favor.
The patch no longer applies, can you submit a rebased version please?
--
Daniel Gustafsson https://vmware.com/
4 нояб. 2021 г., в 17:55, Daniel Gustafsson <daniel@yesql.se> написал(а):
The patch no longer applies, can you submit a rebased version please?
Thanks, Daniel! PFA rebase.
Best regards, Andrey Borodin.
Attachments:
v3-0001-Allow-providing-restore_command-as-a-command-line.patchapplication/octet-stream; name=v3-0001-Allow-providing-restore_command-as-a-command-line.patch; x-unix-mode=0644Download
From dc2545092051eaae56d3a54936ba78ace751a0eb Mon Sep 17 00:00:00 2001
From: Alexey Kondratov <kondratov.aleksey@gmail.com>
Date: Tue, 29 Jun 2021 17:17:47 +0300
Subject: [PATCH v3] Allow providing restore_command as a command line option
to pg_rewind
This could be useful when postgres is usually run with
-c config_file=..., so the actual configuration and restore_command
is not inside $PGDATA/postgresql.conf.
---
doc/src/sgml/ref/pg_rewind.sgml | 19 +++++
src/bin/pg_rewind/pg_rewind.c | 45 ++++++-----
src/bin/pg_rewind/t/001_basic.pl | 3 +-
src/bin/pg_rewind/t/RewindTest.pm | 95 ++++++++++++++----------
src/test/perl/PostgreSQL/Test/Cluster.pm | 5 +-
5 files changed, 107 insertions(+), 60 deletions(-)
diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml
index 33e6bb64ad..af75f35867 100644
--- a/doc/src/sgml/ref/pg_rewind.sgml
+++ b/doc/src/sgml/ref/pg_rewind.sgml
@@ -241,6 +241,25 @@ PostgreSQL documentation
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>-C <replaceable class="parameter">restore_command</replaceable></option></term>
+ <term><option>--target-restore-command=<replaceable class="parameter">restore_command</replaceable></option></term>
+ <listitem>
+ <para>
+ Specifies the <varname>restore_command</varname> to use for retrieving
+ WAL files from the WAL archive if these files are no longer available
+ in the <filename>pg_wal</filename> directory of the target cluster.
+ </para>
+ <para>
+ If <varname>restore_command</varname> is already set in
+ <filename>postgresql.conf</filename>, you can provide the
+ <option>--restore-target-wal</option> option instead. If both options
+ are provided, then <option>--target-restore-command</option>
+ will be used.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>--debug</option></term>
<listitem>
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index 69ea214337..46408d6402 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -84,21 +84,22 @@ usage(const char *progname)
printf(_("%s resynchronizes a PostgreSQL cluster with another copy of the cluster.\n\n"), progname);
printf(_("Usage:\n %s [OPTION]...\n\n"), progname);
printf(_("Options:\n"));
- printf(_(" -c, --restore-target-wal use restore_command in target configuration to\n"
- " retrieve WAL files from archives\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"));
- printf(_(" -n, --dry-run stop before modifying anything\n"));
- printf(_(" -N, --no-sync do not wait for changes to be written\n"
- " safely to disk\n"));
- printf(_(" -P, --progress write progress messages\n"));
- printf(_(" -R, --write-recovery-conf write configuration for replication\n"
- " (requires --source-server)\n"));
- printf(_(" --debug write a lot of debug messages\n"));
- printf(_(" --no-ensure-shutdown do not automatically fix unclean shutdown\n"));
- printf(_(" -V, --version output version information, then exit\n"));
- printf(_(" -?, --help show this help, then exit\n"));
+ printf(_(" -c, --restore-target-wal use restore_command in target configuration to\n"
+ " retrieve WAL files from archives\n"));
+ printf(_(" -C, --target-restore-command=COMMAND target WAL restore_command\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"));
+ printf(_(" -n, --dry-run stop before modifying anything\n"));
+ printf(_(" -N, --no-sync do not wait for changes to be written\n"
+ " safely to disk\n"));
+ printf(_(" -P, --progress write progress messages\n"));
+ printf(_(" -R, --write-recovery-conf write configuration for replication\n"
+ " (requires --source-server)\n"));
+ printf(_(" --debug write a lot of debug messages\n"));
+ printf(_(" --no-ensure-shutdown do not automatically fix unclean shutdown\n"));
+ printf(_(" -V, --version output version information, then exit\n"));
+ printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nReport bugs to <%s>.\n"), PACKAGE_BUGREPORT);
printf(_("%s home page: <%s>\n"), PACKAGE_NAME, PACKAGE_URL);
}
@@ -116,6 +117,7 @@ main(int argc, char **argv)
{"no-ensure-shutdown", no_argument, NULL, 4},
{"version", no_argument, NULL, 'V'},
{"restore-target-wal", no_argument, NULL, 'c'},
+ {"target-restore-command", required_argument, NULL, 'C'},
{"dry-run", no_argument, NULL, 'n'},
{"no-sync", no_argument, NULL, 'N'},
{"progress", no_argument, NULL, 'P'},
@@ -156,7 +158,7 @@ main(int argc, char **argv)
}
}
- while ((c = getopt_long(argc, argv, "cD:nNPR", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "cC:D:nNPR", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -168,6 +170,11 @@ main(int argc, char **argv)
restore_wal = true;
break;
+ case 'C':
+ restore_wal = true;
+ restore_command = pg_strdup(optarg);
+ break;
+
case 'P':
showprogress = true;
break;
@@ -1019,7 +1026,11 @@ getRestoreCommand(const char *argv0)
postgres_cmd[MAXPGPATH],
cmd_output[MAXPGPATH];
- if (!restore_wal)
+ /*
+ * Take restore_command from the postgresql.conf only if it is not already
+ * provided as a command line option.
+ */
+ if (!restore_wal || restore_command != NULL)
return;
/* find postgres executable */
diff --git a/src/bin/pg_rewind/t/001_basic.pl b/src/bin/pg_rewind/t/001_basic.pl
index 7738c306d2..60c680dcaa 100644
--- a/src/bin/pg_rewind/t/001_basic.pl
+++ b/src/bin/pg_rewind/t/001_basic.pl
@@ -4,7 +4,7 @@
use strict;
use warnings;
use PostgreSQL::Test::Utils;
-use Test::More tests => 23;
+use Test::More tests => 29;
use FindBin;
use lib $FindBin::RealBin;
@@ -190,5 +190,6 @@ in primary, before promotion
run_test('local');
run_test('remote');
run_test('archive');
+run_test('archive_cli');
exit(0);
diff --git a/src/bin/pg_rewind/t/RewindTest.pm b/src/bin/pg_rewind/t/RewindTest.pm
index 5546ce456c..32fa45ccf8 100644
--- a/src/bin/pg_rewind/t/RewindTest.pm
+++ b/src/bin/pg_rewind/t/RewindTest.pm
@@ -220,6 +220,58 @@ sub promote_standby
return;
}
+sub run_pg_rewind_archive
+{
+ my $test_mode = shift;
+ my $restore_command;
+ my $cli_option = '--restore-target-wal';
+
+ # Remove the existing archive directory and move all WAL
+ # segments from the old primary to the archives. These
+ # will be used by pg_rewind.
+ rmtree($node_primary->archive_dir);
+ PostgreSQL::Test::RecursiveCopy::copypath($node_primary->data_dir . '/pg_wal',
+ $node_primary->archive_dir);
+
+ # Fast way to remove entire directory content
+ rmtree($node_primary->data_dir . '/pg_wal');
+ mkdir($node_primary->data_dir . '/pg_wal');
+
+ # Make sure that directories have the right umask as this is
+ # required by a follow-up check on permissions, and better
+ # safe than sorry.
+ chmod(0700, $node_primary->archive_dir);
+ chmod(0700, $node_primary->data_dir . '/pg_wal');
+
+ # Add appropriate restore_command to the target cluster
+ $restore_command = $node_primary->enable_restoring($node_primary, 0);
+
+ # In archive_cli mode pass restore_command as a command line option
+ # instead of taking it from postgresql.conf
+ if ($test_mode eq 'archive_cli') {
+ $cli_option = "--target-restore-command=$restore_command";
+ }
+
+ # Stop the new primary and be ready to perform the rewind.
+ $node_standby->stop;
+
+ # Note the use of --no-ensure-shutdown here. WAL files are
+ # gone in this mode and the primary has been stopped
+ # gracefully already.
+ command_ok(
+ [
+ 'pg_rewind',
+ '--debug',
+ '--source-pgdata=' . $node_standby->data_dir,
+ '--target-pgdata=' . $node_primary->data_dir,
+ '--no-sync',
+ '--no-ensure-shutdown',
+ $cli_option
+ ],
+ "pg_rewind $test_mode");
+
+}
+
sub run_pg_rewind
{
my $test_mode = shift;
@@ -231,7 +283,7 @@ sub run_pg_rewind
# Append the rewind-specific role to the connection string.
$standby_connstr = "$standby_connstr user=rewind_user";
- if ($test_mode eq 'archive')
+ if ($test_mode eq 'archive' or $test_mode eq 'archive_cli')
{
# pg_rewind is tested with --restore-target-wal by moving all
# WAL files to a secondary location. Note that this leads to
@@ -304,50 +356,13 @@ sub run_pg_rewind
$node_standby->safe_psql('postgres',
"ALTER ROLE rewind_user WITH REPLICATION;");
}
- elsif ($test_mode eq "archive")
+ elsif ($test_mode eq "archive" or $test_mode eq "archive_cli")
{
-
# Do rewind using a local pgdata as source and specified
# directory with target WAL archive. The old primary has
# to be stopped at this point.
- # Remove the existing archive directory and move all WAL
- # segments from the old primary to the archives. These
- # will be used by pg_rewind.
- rmtree($node_primary->archive_dir);
- PostgreSQL::Test::RecursiveCopy::copypath($node_primary->data_dir . "/pg_wal",
- $node_primary->archive_dir);
-
- # Fast way to remove entire directory content
- rmtree($node_primary->data_dir . "/pg_wal");
- mkdir($node_primary->data_dir . "/pg_wal");
-
- # Make sure that directories have the right umask as this is
- # required by a follow-up check on permissions, and better
- # safe than sorry.
- chmod(0700, $node_primary->archive_dir);
- chmod(0700, $node_primary->data_dir . "/pg_wal");
-
- # Add appropriate restore_command to the target cluster
- $node_primary->enable_restoring($node_primary, 0);
-
- # Stop the new primary and be ready to perform the rewind.
- $node_standby->stop;
-
- # Note the use of --no-ensure-shutdown here. WAL files are
- # gone in this mode and the primary has been stopped
- # gracefully already.
- command_ok(
- [
- 'pg_rewind',
- "--debug",
- "--source-pgdata=$standby_pgdata",
- "--target-pgdata=$primary_pgdata",
- "--no-sync",
- "--no-ensure-shutdown",
- "--restore-target-wal"
- ],
- 'pg_rewind archive');
+ run_pg_rewind_archive($test_mode);
}
else
{
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 9467a199c8..55fd02beb4 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -1014,7 +1014,8 @@ primary_conninfo='$root_connstr'
return;
}
-# Internal routine to enable archive recovery command on a standby node
+# Internal routine to enable archive recovery command on a standby node.
+# Returns generated restore_command.
sub enable_restoring
{
my ($self, $root_node, $standby) = @_;
@@ -1047,7 +1048,7 @@ restore_command = '$copy_command'
{
$self->set_recovery_mode();
}
- return;
+ return $copy_command;
}
=pod
--
2.24.3 (Apple Git-128)
Hi,
On 2021-11-05 15:10:29 +0500, Andrey Borodin wrote:
4 нояб. 2021 г., в 17:55, Daniel Gustafsson <daniel@yesql.se> написал(а):
The patch no longer applies, can you submit a rebased version please?
Thanks, Daniel! PFA rebase.
Doesn't apply once more: http://cfbot.cputube.org/patch_37_3213.log
Greetings,
Andres Freund
Hi,
On Tue, Mar 22, 2022 at 3:32 AM Andres Freund <andres@anarazel.de> wrote:
Doesn't apply once more: http://cfbot.cputube.org/patch_37_3213.log
Thanks for the reminder, a rebased version is attached.
Regards
--
Alexey Kondratov
Attachments:
v4-0001-Allow-providing-restore_command-as-a-command-line.patchtext/x-patch; charset=US-ASCII; name=v4-0001-Allow-providing-restore_command-as-a-command-line.patchDownload
From df56b5c7b882e781fdc0b92e7a83331f0baab094 Mon Sep 17 00:00:00 2001
From: Alexey Kondratov <kondratov.aleksey@gmail.com>
Date: Tue, 29 Jun 2021 17:17:47 +0300
Subject: [PATCH v4] Allow providing restore_command as a command line option
to pg_rewind
This could be useful when postgres is usually run with
-c config_file=..., so the actual configuration and restore_command
is not inside $PGDATA/postgresql.conf.
---
doc/src/sgml/ref/pg_rewind.sgml | 19 +++++
src/bin/pg_rewind/pg_rewind.c | 45 ++++++-----
src/bin/pg_rewind/t/001_basic.pl | 1 +
src/bin/pg_rewind/t/RewindTest.pm | 95 ++++++++++++++----------
src/test/perl/PostgreSQL/Test/Cluster.pm | 5 +-
5 files changed, 106 insertions(+), 59 deletions(-)
diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml
index 33e6bb64ad..af75f35867 100644
--- a/doc/src/sgml/ref/pg_rewind.sgml
+++ b/doc/src/sgml/ref/pg_rewind.sgml
@@ -241,6 +241,25 @@ PostgreSQL documentation
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>-C <replaceable class="parameter">restore_command</replaceable></option></term>
+ <term><option>--target-restore-command=<replaceable class="parameter">restore_command</replaceable></option></term>
+ <listitem>
+ <para>
+ Specifies the <varname>restore_command</varname> to use for retrieving
+ WAL files from the WAL archive if these files are no longer available
+ in the <filename>pg_wal</filename> directory of the target cluster.
+ </para>
+ <para>
+ If <varname>restore_command</varname> is already set in
+ <filename>postgresql.conf</filename>, you can provide the
+ <option>--restore-target-wal</option> option instead. If both options
+ are provided, then <option>--target-restore-command</option>
+ will be used.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>--debug</option></term>
<listitem>
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index b39b5c1aac..9aca041425 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -85,21 +85,22 @@ usage(const char *progname)
printf(_("%s resynchronizes a PostgreSQL cluster with another copy of the cluster.\n\n"), progname);
printf(_("Usage:\n %s [OPTION]...\n\n"), progname);
printf(_("Options:\n"));
- printf(_(" -c, --restore-target-wal use restore_command in target configuration to\n"
- " retrieve WAL files from archives\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"));
- printf(_(" -n, --dry-run stop before modifying anything\n"));
- printf(_(" -N, --no-sync do not wait for changes to be written\n"
- " safely to disk\n"));
- printf(_(" -P, --progress write progress messages\n"));
- printf(_(" -R, --write-recovery-conf write configuration for replication\n"
- " (requires --source-server)\n"));
- printf(_(" --debug write a lot of debug messages\n"));
- printf(_(" --no-ensure-shutdown do not automatically fix unclean shutdown\n"));
- printf(_(" -V, --version output version information, then exit\n"));
- printf(_(" -?, --help show this help, then exit\n"));
+ printf(_(" -c, --restore-target-wal use restore_command in target configuration to\n"
+ " retrieve WAL files from archives\n"));
+ printf(_(" -C, --target-restore-command=COMMAND target WAL restore_command\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"));
+ printf(_(" -n, --dry-run stop before modifying anything\n"));
+ printf(_(" -N, --no-sync do not wait for changes to be written\n"
+ " safely to disk\n"));
+ printf(_(" -P, --progress write progress messages\n"));
+ printf(_(" -R, --write-recovery-conf write configuration for replication\n"
+ " (requires --source-server)\n"));
+ printf(_(" --debug write a lot of debug messages\n"));
+ printf(_(" --no-ensure-shutdown do not automatically fix unclean shutdown\n"));
+ printf(_(" -V, --version output version information, then exit\n"));
+ printf(_(" -?, --help show this help, then exit\n"));
printf(_("\nReport bugs to <%s>.\n"), PACKAGE_BUGREPORT);
printf(_("%s home page: <%s>\n"), PACKAGE_NAME, PACKAGE_URL);
}
@@ -117,6 +118,7 @@ main(int argc, char **argv)
{"no-ensure-shutdown", no_argument, NULL, 4},
{"version", no_argument, NULL, 'V'},
{"restore-target-wal", no_argument, NULL, 'c'},
+ {"target-restore-command", required_argument, NULL, 'C'},
{"dry-run", no_argument, NULL, 'n'},
{"no-sync", no_argument, NULL, 'N'},
{"progress", no_argument, NULL, 'P'},
@@ -157,7 +159,7 @@ main(int argc, char **argv)
}
}
- while ((c = getopt_long(argc, argv, "cD:nNPR", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "cC:D:nNPR", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -169,6 +171,11 @@ main(int argc, char **argv)
restore_wal = true;
break;
+ case 'C':
+ restore_wal = true;
+ restore_command = pg_strdup(optarg);
+ break;
+
case 'P':
showprogress = true;
break;
@@ -1020,7 +1027,11 @@ getRestoreCommand(const char *argv0)
cmd_output[MAXPGPATH];
PQExpBuffer postgres_cmd;
- if (!restore_wal)
+ /*
+ * Take restore_command from the postgresql.conf only if it is not already
+ * provided as a command line option.
+ */
+ if (!restore_wal || restore_command != NULL)
return;
/* find postgres executable */
diff --git a/src/bin/pg_rewind/t/001_basic.pl b/src/bin/pg_rewind/t/001_basic.pl
index db9201f38e..3c395ece12 100644
--- a/src/bin/pg_rewind/t/001_basic.pl
+++ b/src/bin/pg_rewind/t/001_basic.pl
@@ -190,5 +190,6 @@ in primary, before promotion
run_test('local');
run_test('remote');
run_test('archive');
+run_test('archive_cli');
done_testing();
diff --git a/src/bin/pg_rewind/t/RewindTest.pm b/src/bin/pg_rewind/t/RewindTest.pm
index 5651602858..335f1a72ad 100644
--- a/src/bin/pg_rewind/t/RewindTest.pm
+++ b/src/bin/pg_rewind/t/RewindTest.pm
@@ -208,6 +208,58 @@ sub promote_standby
return;
}
+sub run_pg_rewind_archive
+{
+ my $test_mode = shift;
+ my $restore_command;
+ my $cli_option = '--restore-target-wal';
+
+ # Remove the existing archive directory and move all WAL
+ # segments from the old primary to the archives. These
+ # will be used by pg_rewind.
+ rmtree($node_primary->archive_dir);
+ PostgreSQL::Test::RecursiveCopy::copypath($node_primary->data_dir . '/pg_wal',
+ $node_primary->archive_dir);
+
+ # Fast way to remove entire directory content
+ rmtree($node_primary->data_dir . '/pg_wal');
+ mkdir($node_primary->data_dir . '/pg_wal');
+
+ # Make sure that directories have the right umask as this is
+ # required by a follow-up check on permissions, and better
+ # safe than sorry.
+ chmod(0700, $node_primary->archive_dir);
+ chmod(0700, $node_primary->data_dir . '/pg_wal');
+
+ # Add appropriate restore_command to the target cluster
+ $restore_command = $node_primary->enable_restoring($node_primary, 0);
+
+ # In archive_cli mode pass restore_command as a command line option
+ # instead of taking it from postgresql.conf
+ if ($test_mode eq 'archive_cli') {
+ $cli_option = "--target-restore-command=$restore_command";
+ }
+
+ # Stop the new primary and be ready to perform the rewind.
+ $node_standby->stop;
+
+ # Note the use of --no-ensure-shutdown here. WAL files are
+ # gone in this mode and the primary has been stopped
+ # gracefully already.
+ command_ok(
+ [
+ 'pg_rewind',
+ '--debug',
+ '--source-pgdata=' . $node_standby->data_dir,
+ '--target-pgdata=' . $node_primary->data_dir,
+ '--no-sync',
+ '--no-ensure-shutdown',
+ $cli_option
+ ],
+ "pg_rewind $test_mode");
+
+}
+
sub run_pg_rewind
{
my $test_mode = shift;
@@ -219,7 +271,7 @@ sub run_pg_rewind
# Append the rewind-specific role to the connection string.
$standby_connstr = "$standby_connstr user=rewind_user";
- if ($test_mode eq 'archive')
+ if ($test_mode eq 'archive' or $test_mode eq 'archive_cli')
{
# pg_rewind is tested with --restore-target-wal by moving all
# WAL files to a secondary location. Note that this leads to
@@ -292,50 +344,13 @@ sub run_pg_rewind
$node_standby->safe_psql('postgres',
"ALTER ROLE rewind_user WITH REPLICATION;");
}
- elsif ($test_mode eq "archive")
+ elsif ($test_mode eq "archive" or $test_mode eq "archive_cli")
{
-
# Do rewind using a local pgdata as source and specified
# directory with target WAL archive. The old primary has
# to be stopped at this point.
- # Remove the existing archive directory and move all WAL
- # segments from the old primary to the archives. These
- # will be used by pg_rewind.
- rmtree($node_primary->archive_dir);
- PostgreSQL::Test::RecursiveCopy::copypath($node_primary->data_dir . "/pg_wal",
- $node_primary->archive_dir);
-
- # Fast way to remove entire directory content
- rmtree($node_primary->data_dir . "/pg_wal");
- mkdir($node_primary->data_dir . "/pg_wal");
-
- # Make sure that directories have the right umask as this is
- # required by a follow-up check on permissions, and better
- # safe than sorry.
- chmod(0700, $node_primary->archive_dir);
- chmod(0700, $node_primary->data_dir . "/pg_wal");
-
- # Add appropriate restore_command to the target cluster
- $node_primary->enable_restoring($node_primary, 0);
-
- # Stop the new primary and be ready to perform the rewind.
- $node_standby->stop;
-
- # Note the use of --no-ensure-shutdown here. WAL files are
- # gone in this mode and the primary has been stopped
- # gracefully already.
- command_ok(
- [
- 'pg_rewind',
- "--debug",
- "--source-pgdata=$standby_pgdata",
- "--target-pgdata=$primary_pgdata",
- "--no-sync",
- "--no-ensure-shutdown",
- "--restore-target-wal"
- ],
- 'pg_rewind archive');
+ run_pg_rewind_archive($test_mode);
}
else
{
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index e7b9161137..51e289aaa0 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -1071,7 +1071,8 @@ primary_conninfo='$root_connstr'
return;
}
-# Internal routine to enable archive recovery command on a standby node
+# Internal routine to enable archive recovery command on a standby node.
+# Returns generated restore_command.
sub enable_restoring
{
my ($self, $root_node, $standby) = @_;
@@ -1104,7 +1105,7 @@ restore_command = '$copy_command'
{
$self->set_recovery_mode();
}
- return;
+ return $copy_command;
}
=pod
base-commit: f5576a21b0778f275d7418f6f7a44d9400ee90aa
--
2.32.0
On Thu, Nov 04, 2021 at 01:55:43PM +0100, Daniel Gustafsson wrote:
On 14 Sep 2021, at 16:05, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
Do we actually need --target-restore-command at all? It seems to me
that we have all we need with --restore-target-wal, and that's not
really instinctive to pass down a command via another command..Currently we know that --restore-target-wal is not enough if postgresql.conf does not reside within PGDATA.
That's a useful reason which wasn't brought up in the earlier thread, and may
tip the scales in favor.
It does now, as of 0d5c3875. FWIW, I am not much a fan of the design
where we pass down a command line as an option value of a different
command line (more games with quoting comes into mind first), and
--config-file should give enough room for the case of this thread. I
have switched the status of the patch to reflect that.
--
Michael