Am 28.02.22 um 12:56 schrieb Michael Paquier:
On Sun, Feb 27, 2022 at 02:30:33PM +0100, Gunnar "Nick" Bluth wrote:
That's universally true ;-)
-# 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) = @_;
@@ -1103,7 +1104,7 @@ restore_command = '$copy_command'
{
$self->set_recovery_mode();
}
- return;
+ return $copy_command;
I don't like this change. This makes an existing routine do some
extra work for something it is not designed for. You could get this
information with a postgres -C command, for example. Now, you cannot
use postgres -C because of.. Reasons (1a9d802). But you could use a
pg_ctl command for the same purpose. I guess that we'd better
refactor this into a new routine of Cluster.pm where we execute a
pg_ctl command and return both stdout and stderr back to the caller?
Turns out this change isn't needed anyway (it was copied from the other
patch). Afterall, the patch is about extracting the restore_command from
the config, so...
The TAP part could be refactored on its own.
Agreed. I've struggled quite a bit even understanding what's happening
in there ;-)
+ In case the <filename>postgresql.conf</filename> of your
target cluster is not in the
+ target pgdata and you want to use the <option>-c /
--restore-target-wal</option> option,
+ provide a (relative or absolute) path to the
<filename>postgresql.conf</filename> using this option.
This could do a better job to explain in which cases this option can
be used for. You could say something like that:
"This option specifies a path to a configuration file to be used when
starting the target cluster. This makes easier the use of
-c/--restore-target-wal by setting restore_command in the extra
configuration file specified via this option."
I reviewed the wording and think it is pretty universal now.
Hmm. What about the target cluster started in --single mode as of
ensureCleanShutdown()? Should we try to apply this option also in
this case?
I'd say so; as far as I can tell, "postgres" would deny starting a
(Debian/non-standard) cluster the way the code is right now.
Which led me to realize we have
/* find postgres executable */
rc = find_other_exec(argv0, "postgres",
PG_BACKEND_VERSIONSTR,
postgres_exec_path);
in getRestoreCommand _and_
/* locate postgres binary */
if ((ret = find_other_exec(argv0, "postgres",
PG_BACKEND_VERSIONSTR,
exec_path)) < 0)
in ensureCleanShutdown.
Both with the same error messages, AFAICS. D'oh... can of worms.
So, how should we call the global "find the ***** 'postgres' executable
and boil out if that fails" function?
char postgres_exec_path[MAXPGPATH] = findPostgresExec();
?
Anyway, v4 attached so we can settle on the tests and wording...
--
Gunnar "Nick" Bluth
Eimermacherweg 106
D-48159 Münster
Mobil +49 172 8853339
Email: gunnar.bl...@pro-open.de
__________________________________________________________________________
"Ceterum censeo SystemD esse delendam" - Catodiff --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),
+ use this option to provide a (relative or absolute) path to the <filename>postgresql.conf</filename> to 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 efb82a4034..b8c92c5590 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -23,6 +23,7 @@
#include "common/restricted_token.h"
#include "common/string.h"
#include "fe_utils/recovery_gen.h"
+#include "fe_utils/string_utils.h"
#include "file_ops.h"
#include "filemap.h"
#include "getopt_long.h"
@@ -60,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;
@@ -86,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"));
@@ -120,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},
{NULL, 0, NULL, 0}
};
int option_index;
@@ -204,6 +208,10 @@ main(int argc, char **argv)
case 4:
no_ensure_shutdown = true;
break;
+
+ case 5:
+ config_file = pg_strdup(optarg);
+ break;
}
}
@@ -1016,8 +1024,8 @@ getRestoreCommand(const char *argv0)
{
int rc;
char postgres_exec_path[MAXPGPATH],
- postgres_cmd[MAXPGPATH],
cmd_output[MAXPGPATH];
+ PQExpBuffer postgres_cmd;
if (!restore_wal)
return;
@@ -1051,11 +1059,21 @@ getRestoreCommand(const char *argv0)
* Build a command able to retrieve the value of GUC parameter
* restore_command, if set.
*/
- snprintf(postgres_cmd, sizeof(postgres_cmd),
- "\"%s\" -D \"%s\" -C restore_command",
- postgres_exec_path, datadir_target);
+ postgres_cmd = createPQExpBuffer();
- if (!pipe_read_line(postgres_cmd, cmd_output, sizeof(cmd_output)))
+ /* path to postgres, properly quoted */
+ appendShellString(postgres_cmd, postgres_exec_path);
+
+ appendPQExpBufferStr(postgres_cmd, " -D ");
+ appendShellString(postgres_cmd, datadir_target);
+ if (config_file != NULL)
+ {
+ appendPQExpBufferStr(postgres_cmd, " --config_file=");
+ appendShellString(postgres_cmd, config_file);
+ }
+ appendPQExpBufferStr(postgres_cmd, " -C restore_command");
+
+ if (!pipe_read_line(postgres_cmd->data, cmd_output, sizeof(cmd_output)))
exit(1);
(void) pg_strip_crlf(cmd_output);
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..f93e138b35 100644
--- a/src/bin/pg_rewind/t/RewindTest.pm
+++ b/src/bin/pg_rewind/t/RewindTest.pm
@@ -208,6 +208,68 @@ sub promote_standby
return;
}
+sub run_pg_rewind_archive
+{
+ my $test_mode = shift;
+ my $primary_pgdata = $node_primary->data_dir;
+
+ # 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.
+
+ # In archive_cli mode pass the config file as a command line option
+ if ($test_mode eq 'archive_cli') {
+ command_ok(
+ [
+ 'pg_rewind',
+ '--debug',
+ '--source-pgdata=' . $node_standby->data_dir,
+ '--target-pgdata=' . $node_primary->data_dir,
+ '--no-sync',
+ '--no-ensure-shutdown',
+ '--restore-target-wal',
+ "--config-file=$primary_pgdata/postgresql.conf"
+ ],
+ "pg_rewind $test_mode");
+ } else {
+ command_ok(
+ [
+ 'pg_rewind',
+ '--debug',
+ '--source-pgdata=' . $node_standby->data_dir,
+ '--target-pgdata=' . $node_primary->data_dir,
+ '--no-sync',
+ '--no-ensure-shutdown',
+ '--restore-target-wal'
+ ],
+ "pg_rewind $test_mode");
+ }
+
+}
+
sub run_pg_rewind
{
my $test_mode = shift;
@@ -219,7 +281,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 +354,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
{
