PATCH: add "--config-file=" option to pg_rewind
Hi!
During a Patroni PR discussion
(https://github.com/zalando/patroni/pull/2225), we realised that if one
wants to use the "-c" option on a typical Debian/Ubuntu installation
(where the config resides below /etc/postgresql/), pg_rewind needs a way
to be told where the postgresql.conf actually is.
The attached patch implements this as an option.
I'm extremely unhappy that I called it "--config-file" here, while
"postgres" itself wants "--config_file". But as the other dashed options
of pg_rewind are, well, dashed and not underscored, it seemed to be
better that the other way...
As the "-c" feature appeared in 12 and existing Debian installations are
unable to use it right now, I suggest to even backport the patch.
Oh, and I'm still not subscribed to -hackers, so the usual "please CC
me" applies!
Best regards,
--
Gunnar "Nick" Bluth
Eimermacherweg 106
D-48159 Münster
Mobil +49 172 8853339
Email: gunnar.bluth@pro-open.de
__________________________________________________________________________
"Ceterum censeo SystemD esse delendam" - Cato
Attachments:
add_configfile_to_pg_rewind.patchtext/x-patch; charset=UTF-8; name=add_configfile_to_pg_rewind.patchDownload
diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml
index 33e6bb64ad..f80adbf5ae 100644
--- a/doc/src/sgml/ref/pg_rewind.sgml
+++ b/doc/src/sgml/ref/pg_rewind.sgml
@@ -241,6 +241,17 @@ PostgreSQL documentation
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>--config-file=<replaceable class="parameter">filepath</replaceable></option></term>
+ <listitem>
+ <para>
+ 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</option> option,
+ provide a (relative or absolute) path to the <filename>postgresql.conf</filename> using this option.
+ </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..d57a3a6852 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -60,6 +60,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 +87,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 +122,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 +207,10 @@ main(int argc, char **argv)
case 4:
no_ensure_shutdown = true;
break;
+
+ case 5:
+ config_file = pg_strdup(optarg);
+ break;
}
}
@@ -1051,9 +1058,16 @@ 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),
+ if (config_file == NULL)
+ {
+ snprintf(postgres_cmd, sizeof(postgres_cmd),
"\"%s\" -D \"%s\" -C restore_command",
postgres_exec_path, datadir_target);
+ } else {
+ snprintf(postgres_cmd, sizeof(postgres_cmd),
+ "\"%s\" -D \"%s\" --config_file=\"%s\" -C restore_command",
+ postgres_exec_path, datadir_target, config_file);
+ }
if (!pipe_read_line(postgres_cmd, cmd_output, sizeof(cmd_output)))
exit(1);
On Wed, 2022-02-23 at 16:28 +0100, Gunnar "Nick" Bluth wrote:
I'm extremely unhappy that I called it "--config-file" here, while
"postgres" itself wants "--config_file". But as the other dashed options
of pg_rewind are, well, dashed and not underscored, it seemed to be
better that the other way...
The "--"-style options to postgres take either underscores or dashes in
the names (just double-checked with PG14), so this shouldn't be a
problem.
--Jacob
Hi Gunnar,
During a Patroni PR discussion
(https://github.com/zalando/patroni/pull/2225), we realised that if one
wants to use the "-c" option on a typical Debian/Ubuntu installation
(where the config resides below /etc/postgresql/), pg_rewind needs a way
to be told where the postgresql.conf actually is.The attached patch implements this as an option.
[...]
Good catch!
Could you also implement a TAP test for the new code?
--
Best regards,
Aleksander Alekseev
Hello Gunnar,
On Thu, 24 Feb 2022 at 10:03, Aleksander Alekseev <aleksander@timescale.com>
wrote:
wants to use the "-c" option on a typical Debian/Ubuntu installation
(where the config resides below /etc/postgresql/), pg_rewind needs a way
to be told where the postgresql.conf actually is.The attached patch implements this as an option.
[...]
Good catch!
Yeah, it is a known problem, and there was already another patch trying to
address it [1]/messages/by-id/16982-f12294cccd221480@postgresql.org.
Honestly, I like your approach much better, because, the previous make was
assuming that the data_directory GUC is specified in the postgresql.conf,
which is not very flexible.
Could you also implement a TAP test for the new code?
+1
+ <para>
+ 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</option> option,
+ provide a (relative or absolute) path to the
<filename>postgresql.conf</filename> using this option.
+ </para>
It took me a while to understand the meaning of <option>-c</option>. Maybe
changing it to <option>--restore-target-wal</option> will make it easier to
understand.
[1]: /messages/by-id/16982-f12294cccd221480@postgresql.org
/messages/by-id/16982-f12294cccd221480@postgresql.org
Regards,
--
Alexander Kukushkin
Hi hackers,
It took me a while to understand the meaning of <option>-c</option>. Maybe changing it to <option>--restore-target-wal</option> will make it easier to understand.
+1, I "stumbled" while reading this too at first.
--
Best regards,
Aleksander Alekseev
On 24 Feb 2022, at 10:27, Alexander Kukushkin <cyberdemn@gmail.com> wrote:
Hello Gunnar,
On Thu, 24 Feb 2022 at 10:03, Aleksander Alekseev <aleksander@timescale.com> wrote:
wants to use the "-c" option on a typical Debian/Ubuntu installation
(where the config resides below /etc/postgresql/), pg_rewind needs a way
to be told where the postgresql.conf actually is.The attached patch implements this as an option.
[...]
Good catch!
Yeah, it is a known problem, and there was already another patch trying to address it [1].
There is yet another related patch which provides a parameter to pass in
restore_command in cases where postgresq.conf isn't reachable:
https://commitfest.postgresql.org/37/3213/
--
Daniel Gustafsson https://vmware.com/
Am 24.02.2022 um 14:08 schrieb Daniel Gustafsson:
On 24 Feb 2022, at 10:27, Alexander Kukushkin <cyberdemn@gmail.com> wrote:
Hello Gunnar,
On Thu, 24 Feb 2022 at 10:03, Aleksander Alekseev <aleksander@timescale.com> wrote:
wants to use the "-c" option on a typical Debian/Ubuntu installation
(where the config resides below /etc/postgresql/), pg_rewind needs a way
to be told where the postgresql.conf actually is.The attached patch implements this as an option.
[...]
Good catch!
Yeah, it is a known problem, and there was already another patch trying to address it [1].
There is yet another related patch which provides a parameter to pass in
restore_command in cases where postgresq.conf isn't reachable:
That looks just as good to me, and it already has tests, so I'll happily
step down!
(Note to myself: check the CF first next time ;-)
--
Gunnar "Nick" Bluth
Eimermacherweg 106
D-48159 Münster
Mobil +49 172 8853339
Email: gunnar.bluth@pro-open.de
__________________________________________________________________________
"Ceterum censeo SystemD esse delendam" - Cato
On 24 Feb 2022, at 14:43, Gunnar Nick Bluth <gunnar.bluth@pro-open.de> wrote:
That looks just as good to me, and it already has tests, so I'll happily step down!
Actually, I think this looks like a saner approach. Putting a config setting
in two place (postgresql.conf and on the commandline for pg_rewind) is a recipe
for them diverging.
--
Daniel Gustafsson https://vmware.com/
Am 24.02.22 um 14:46 schrieb Daniel Gustafsson:
On 24 Feb 2022, at 14:43, Gunnar Nick Bluth <gunnar.bluth@pro-open.de> wrote:
That looks just as good to me, and it already has tests, so I'll happily step down!
Actually, I think this looks like a saner approach. Putting a config setting
in two place (postgresql.conf and on the commandline for pg_rewind) is a recipe
for them diverging.
Well, well,
reverted https://commitfest.postgresql.org/37/3562/ back to "needs
review" and moved it to "Replication & Recovery".
Patch v2 includes:
* doc improvements as suggested
* tests shamelessly copied & adapted from
https://commitfest.postgresql.org/37/3213/
Since the need is pretty obvious (I *think* the Debian-ish userbase is
quite significant alone), I'd love to see one of the approaches making
it into 15!
Best regards,
--
Gunnar "Nick" Bluth
Eimermacherweg 106
D-48159 Münster
Mobil +49 172 8853339
Email: gunnar.bluth@pro-open.de
__________________________________________________________________________
"Ceterum censeo SystemD esse delendam" - Cato
Attachments:
add_configfile_to_pg_rewind_v2.patchtext/x-patch; charset=UTF-8; name=add_configfile_to_pg_rewind_v2.patchDownload
diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml
index 33e6bb64ad..31a1a1dedb 100644
--- a/doc/src/sgml/ref/pg_rewind.sgml
+++ b/doc/src/sgml/ref/pg_rewind.sgml
@@ -241,6 +241,17 @@ PostgreSQL documentation
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>--config-file=<replaceable class="parameter">filepath</replaceable></option></term>
+ <listitem>
+ <para>
+ 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.
+ </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..d57a3a6852 100644
--- a/src/bin/pg_rewind/pg_rewind.c
+++ b/src/bin/pg_rewind/pg_rewind.c
@@ -60,6 +60,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 +87,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 +122,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 +207,10 @@ main(int argc, char **argv)
case 4:
no_ensure_shutdown = true;
break;
+
+ case 5:
+ config_file = pg_strdup(optarg);
+ break;
}
}
@@ -1051,9 +1058,16 @@ 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),
+ if (config_file == NULL)
+ {
+ snprintf(postgres_cmd, sizeof(postgres_cmd),
"\"%s\" -D \"%s\" -C restore_command",
postgres_exec_path, datadir_target);
+ } else {
+ snprintf(postgres_cmd, sizeof(postgres_cmd),
+ "\"%s\" -D \"%s\" --config_file=\"%s\" -C restore_command",
+ postgres_exec_path, datadir_target, config_file);
+ }
if (!pipe_read_line(postgres_cmd, cmd_output, sizeof(cmd_output)))
exit(1);
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..94d413dcb6 100644
--- a/src/bin/pg_rewind/t/RewindTest.pm
+++ b/src/bin/pg_rewind/t/RewindTest.pm
@@ -208,6 +208,69 @@ sub promote_standby
return;
}
+sub run_pg_rewind_archive
+{
+ my $test_mode = shift;
+ my $restore_command;
+ 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
+ $restore_command = $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 +282,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 +355,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 be05845248..4778f6eaeb 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -1070,7 +1070,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) = @_;
@@ -1103,7 +1104,7 @@ restore_command = '$copy_command'
{
$self->set_recovery_mode();
}
- return;
+ return $copy_command;
}
=pod
On Fri, Feb 25, 2022 at 10:35:49AM +0100, Gunnar "Nick" Bluth wrote:
Am 24.02.22 um 14:46 schrieb Daniel Gustafsson:
Actually, I think this looks like a saner approach. Putting a config setting
in two place (postgresql.conf and on the commandline for pg_rewind) is a recipe
for them diverging.
FWIW, I have a bad feeling about passing down directly a command
through an option itself part of a command, so what's discussed on
this thread is refreshing.
+ } else {
+ snprintf(postgres_cmd, sizeof(postgres_cmd),
+ "\"%s\" -D \"%s\" --config_file=\"%s\" -C restore_command",
+ postgres_exec_path, datadir_target, config_file);
+ }
Shouldn't this one use appendShellString() on config_file?
--
Michael
Am 26.02.22 um 06:51 schrieb Michael Paquier:
On Fri, Feb 25, 2022 at 10:35:49AM +0100, Gunnar "Nick" Bluth wrote:
Am 24.02.22 um 14:46 schrieb Daniel Gustafsson:
Actually, I think this looks like a saner approach. Putting a config setting
in two place (postgresql.conf and on the commandline for pg_rewind) is a recipe
for them diverging.FWIW, I have a bad feeling about passing down directly a command
through an option itself part of a command, so what's discussed on
this thread is refreshing.
Ta.
+ } else { + snprintf(postgres_cmd, sizeof(postgres_cmd), + "\"%s\" -D \"%s\" --config_file=\"%s\" -C restore_command", + postgres_exec_path, datadir_target, config_file); + }Shouldn't this one use appendShellString() on config_file?
It probably should, yes. I don't fancy this repetitive code myself.
But then, pg_rewind as a whole could use an overhaul. I don't see any
use of PQExpBuffer in it, but a lot of char* ...
I myself am not a C hacker (as you can probably tell already) and don't
really feel fit (enough) to rewrite pg_rewind.c to use
fe_utils/string_utils.h :/
I might give it a try anyhow, but that may be a bit... heavy,,, just to
add one option?
While we're at it (I'm really good at opening cans of worms):
09:47 $ grep -r -l --include \*.c fe_utils/string_utils.h src/bin/ | wc -l
28
09:48 $ grep -r -L --include \*.c fe_utils/string_utils.h src/bin/ | wc -l
66
GSOC project? ;-)
Best,
--
Gunnar "Nick" Bluth
Eimermacherweg 106
D-48159 Münster
Mobil +49 172 8853339
Email: gunnar.bluth@pro-open.de
__________________________________________________________________________
"Ceterum censeo SystemD esse delendam" - Cato
On Sat, Feb 26, 2022 at 09:55:20AM +0100, Gunnar "Nick" Bluth wrote:
Am 26.02.22 um 06:51 schrieb Michael Paquier:
Shouldn't this one use appendShellString() on config_file?
It probably should, yes. I don't fancy this repetitive code myself.
But then, pg_rewind as a whole could use an overhaul. I don't see any use of
PQExpBuffer in it, but a lot of char* ...
Having a lot of char* does not necessarily mean that all of them have
to be changed to accomodate with PQExpBuffer. My guess that this is a
case-by-case, where we should apply that in places where it makes the
code cleaner to follow. In the case of your patch though, the two
areas changed would make the logic correct, instead, because we have
to apply correct quoting rules to any commands executed.
GSOC project? ;-)
It does not seem so, though there are surely more areas that could
gain in clarity.
--
Michael
Am 27.02.22 um 13:06 schrieb Michael Paquier:
On Sat, Feb 26, 2022 at 09:55:20AM +0100, Gunnar "Nick" Bluth wrote:
Am 26.02.22 um 06:51 schrieb Michael Paquier:
Shouldn't this one use appendShellString() on config_file?
It probably should, yes. I don't fancy this repetitive code myself.
But then, pg_rewind as a whole could use an overhaul. I don't see any use of
PQExpBuffer in it, but a lot of char* ...Having a lot of char* does not necessarily mean that all of them have
to be changed to accomodate with PQExpBuffer. My guess that this is a
case-by-case, where we should apply that in places where it makes the
code cleaner to follow. In the case of your patch though, the two
areas changed would make the logic correct, instead, because we have
to apply correct quoting rules to any commands executed.
Alas! v3 attached.
GSOC project? ;-)
It does not seem so, though there are surely more areas that could
gain in clarity.
That's universally true ;-)
Best regards,
--
Gunnar "Nick" Bluth
Eimermacherweg 106
D-48159 Münster
Mobil +49 172 8853339
Email: gunnar.bluth@pro-open.de
__________________________________________________________________________
"Ceterum censeo SystemD esse delendam" - Cato
Attachments:
add_configfile_to_pg_rewind_v3.patchtext/x-patch; charset=UTF-8; name=add_configfile_to_pg_rewind_v3.patchDownload
diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml
index 33e6bb64ad..31a1a1dedb 100644
--- a/doc/src/sgml/ref/pg_rewind.sgml
+++ b/doc/src/sgml/ref/pg_rewind.sgml
@@ -241,6 +241,17 @@ PostgreSQL documentation
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>--config-file=<replaceable class="parameter">filepath</replaceable></option></term>
+ <listitem>
+ <para>
+ 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.
+ </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..94d413dcb6 100644
--- a/src/bin/pg_rewind/t/RewindTest.pm
+++ b/src/bin/pg_rewind/t/RewindTest.pm
@@ -208,6 +208,69 @@ sub promote_standby
return;
}
+sub run_pg_rewind_archive
+{
+ my $test_mode = shift;
+ my $restore_command;
+ 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
+ $restore_command = $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 +282,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 +355,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 be05845248..4778f6eaeb 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -1070,7 +1070,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) = @_;
@@ -1103,7 +1104,7 @@ restore_command = '$copy_command'
{
$self->set_recovery_mode();
}
- return;
+ return $copy_command;
}
=pod
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?
The TAP part could be refactored on its own.
+ 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."
Hmm. What about the target cluster started in --single mode as of
ensureCleanShutdown()? Should we try to apply this option also in
this case?
--
Michael
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.bluth@pro-open.de
__________________________________________________________________________
"Ceterum censeo SystemD esse delendam" - Cato
Attachments:
add_configfile_to_pg_rewind_v4.patchtext/x-patch; charset=UTF-8; name=add_configfile_to_pg_rewind_v4.patchDownload
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),
+ 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
{
On Mon, Feb 28, 2022 at 08:48:23PM +0100, Gunnar "Nick" Bluth wrote:
So, how should we call the global "find the ***** 'postgres' executable and
boil out if that fails" function?
char postgres_exec_path[MAXPGPATH] = findPostgresExec();
?
That would mean only a couple of lines gained, and the readability
gained is minimal, so I'd be fine to keep the code as-is. I am not
sure about the full patch set yet, but the refactoring of the commands
to use PQExpBuffer is good by itself, so I have extracted this part of
the patch and applied that for now.
--
Michael
Am 01.03.22 um 05:18 schrieb Michael Paquier:
On Mon, Feb 28, 2022 at 08:48:23PM +0100, Gunnar "Nick" Bluth wrote:
So, how should we call the global "find the ***** 'postgres' executable and
boil out if that fails" function?
char postgres_exec_path[MAXPGPATH] = findPostgresExec();
?That would mean only a couple of lines gained, and the readability
gained is minimal, so I'd be fine to keep the code as-is. I am not
sure about the full patch set yet, but the refactoring of the commands
to use PQExpBuffer is good by itself, so I have extracted this part of
the patch and applied that for now.
Which made the remaining patch v5 (which also appends the --config-file
for ensureCleanShutdown) much cleaner :)
Cheers,
--
Gunnar "Nick" Bluth
Eimermacherweg 106
D-48159 Münster
Mobil +49 172 8853339
Email: gunnar.bluth@pro-open.de
__________________________________________________________________________
"Ceterum censeo SystemD esse delendam" - Cato
Attachments:
add_configfile_to_pg_rewind_v5.patchtext/x-patch; charset=UTF-8; name=add_configfile_to_pg_rewind_v5.patchDownload
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),
+ 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 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"));
@@ -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},
{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 */
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 */
appendPQExpBufferStr(postgres_cmd, " template1 < ");
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
{
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.banck@credativ.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
Am 10.03.22 um 14:43 schrieb Michael Banck:
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:
Much appreciated!
[...]
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.
Fixed.
[...]
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).
Fixed.
@@ -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.
That one has been triggering my inner Monk too ;-)
Fixed!
[...]
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.
Kinde same here.
It does smell a bit like "stating the obvious", but alas! Both fixed.
Cheers,
--
Gunnar "Nick" Bluth
Eimermacherweg 106
D-48159 Münster
Mobil +49 172 8853339
Email: gunnar.bluth@pro-open.de
__________________________________________________________________________
"Ceterum censeo SystemD esse delendam" - Cato
Attachments:
add_configdir_to_pg_rewind_v6.patchtext/x-patch; charset=UTF-8; name=add_configdir_to_pg_rewind_v6.patchDownload
doc/src/sgml/ref/pg_rewind.sgml | 12 +++++
src/bin/pg_rewind/pg_rewind.c | 24 ++++++++-
src/bin/pg_rewind/t/001_basic.pl | 1 +
src/bin/pg_rewind/t/RewindTest.pm | 105 +++++++++++++++++++++++---------------
4 files changed, 101 insertions(+), 41 deletions(-)
diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml
index 33e6bb64ad..62fcb71825 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 data 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 b39b5c1aac..2e83c7ee8e 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,8 @@ 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 it resides outside\n"));
+ printf(_(" the target data directory (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"));
@@ -114,13 +117,14 @@ main(int argc, char **argv)
{"write-recovery-conf", no_argument, NULL, 'R'},
{"source-pgdata", required_argument, NULL, 1},
{"source-server", required_argument, NULL, 2},
+ {"debug", no_argument, NULL, 3},
{"no-ensure-shutdown", no_argument, NULL, 4},
+ {"config-file", required_argument, NULL, 5},
{"version", no_argument, NULL, 'V'},
{"restore-target-wal", no_argument, NULL, 'c'},
{"dry-run", no_argument, NULL, 'n'},
{"no-sync", no_argument, NULL, 'N'},
{"progress", no_argument, NULL, 'P'},
- {"debug", no_argument, NULL, 3},
{NULL, 0, NULL, 0}
};
int option_index;
@@ -205,6 +209,10 @@ main(int argc, char **argv)
case 4:
no_ensure_shutdown = true;
break;
+
+ case 5:
+ config_file = pg_strdup(optarg);
+ break;
}
}
@@ -1061,6 +1069,13 @@ getRestoreCommand(const char *argv0)
appendPQExpBufferStr(postgres_cmd, " -D ");
appendShellString(postgres_cmd, datadir_target);
+ /* add --config_file switch only if requested */
+ if (config_file != NULL)
+ {
+ appendPQExpBufferStr(postgres_cmd, " --config_file=");
+ appendShellString(postgres_cmd, config_file);
+ }
+
/* add -C switch, for restore_command */
appendPQExpBufferStr(postgres_cmd, " -C restore_command");
@@ -1139,6 +1154,13 @@ ensureCleanShutdown(const char *argv0)
appendPQExpBufferStr(postgres_cmd, " --single -F -D ");
appendShellString(postgres_cmd, datadir_target);
+ /* add --config_file switch only if requested */
+ if (config_file != NULL)
+ {
+ appendPQExpBufferStr(postgres_cmd, " --config_file=");
+ appendShellString(postgres_cmd, config_file);
+ }
+
/* finish with the database name, and a properly quoted redirection */
appendPQExpBufferStr(postgres_cmd, " template1 < ");
appendShellString(postgres_cmd, DEVNULL);
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
{
Hi,
Am Donnerstag, dem 10.03.2022 um 15:28 +0100 schrieb Gunnar "Nick"
Bluth:
Am 10.03.22 um 14:43 schrieb Michael Banck:
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:Much appreciated!
[...]
Fixed.
[...]
Thanks for the updated patch.
The patch applies, make is ok, make check is ok, pg_rewind TAP tests
are ok.
I did some tests now with Patroni 2.1.3 and the attached patch applied.
The following test was made:
1. Deploy 3-node (pg1, pg2, pg3) patroni cluster on Debian unstable
running postgresql-15 (approx. master)
2. Switch on archive_mode, and set archive_command and restore_command
3. Switchover so that pg1 is the primary (if not already)
4. Kill pg1 hard
5. Wait till a new leader has taken over and the (now 2-node) cluster
is healthy again
6. Restart pg1
without this patch:
|Apr 03 19:09:01 pg1 patroni@15-main[99]: 2022-04-03 19:09:01,084 INFO: running pg_rewind from pg3
|Apr 03 19:09:01 pg1 patroni@15-main[99]: 2022-04-03 19:09:01,102 INFO: running pg_rewind from dbname=postgres user=postgres_rewind host=10.0.3.227 port=5432 target_session_attrs=read-write
|Apr 03 19:09:01 pg1 patroni@15-main[99]: 2022-04-03 19:09:01,135 INFO: pg_rewind exit code=1
|Apr 03 19:09:01 pg1 patroni@15-main[99]: 2022-04-03 19:09:01,136 INFO: stdout=
|Apr 03 19:09:01 pg1 patroni@15-main[99]: 2022-04-03 19:09:01,136 INFO: stderr=postgres: could not access the server configuration file "/var/lib/postgresql/15/main/postgresql.conf": No such file or directory
|Apr 03 19:09:01 pg1 patroni@15-main[99]: no data was returned by command "/usr/lib/postgresql/15/bin/postgres -D /var/lib/postgresql/15/main -C restore_command"
|Apr 03 19:09:01 pg1 patroni@15-main[99]: 2022-04-03 19:09:01,149 ERROR: Failed to rewind from healty master: pg3
|Apr 03 19:09:01 pg1 patroni@15-main[99]: 2022-04-03 19:09:01,149 WARNING: remove_data_directory_on_diverged_timelines is set. removing...
|Apr 03 19:09:01 pg1 patroni@15-main[99]: 2022-04-03 19:09:01,149 INFO: Removing data directory: /var/lib/postgresql/15/main
|Apr 03 19:09:01 pg1 patroni@15-main[99]: 2022-04-03 19:09:01,245 INFO: Lock owner: pg3; I am pg1
|Apr 03 19:09:01 pg1 patroni@15-main[99]: 2022-04-03 19:09:01,248 INFO: trying to bootstrap from leader 'pg3'
So pg_rewind fails and Patroni does a re-bootstrap.
with this patch:
|Apr 03 19:12:35 pg1 patroni@15-main[99]: 2022-04-03 19:12:35,576 INFO: running pg_rewind from pg2
|Apr 03 19:12:35 pg1 patroni@15-main[99]: 2022-04-03 19:12:35,590 INFO: running pg_rewind from dbname=postgres user=postgres_rewind host=10.0.3.145 port=5432 target_session_attrs=read-write
|Apr 03 19:12:37 pg1 patroni@15-main[99]: 2022-04-03 19:12:37,147 INFO: pg_rewind exit code=0
|Apr 03 19:12:37 pg1 patroni@15-main[99]: 2022-04-03 19:12:37,148 INFO: stdout=
|Apr 03 19:12:37 pg1 patroni@15-main[99]: 2022-04-03 19:12:37,148 INFO: stderr=pg_rewind: servers diverged at WAL location 0/1D000180 on timeline 38
|Apr 03 19:12:37 pg1 patroni@15-main[99]: pg_rewind: rewinding from last common checkpoint at 0/1D000108 on timeline 38
|Apr 03 19:12:37 pg1 patroni@15-main[99]: pg_rewind: Done!
|Apr 03 19:12:37 pg1 patroni@15-main[99]: 2022-04-03 19:12:37,151 WARNING: Postgresql is not running.
|Apr 03 19:12:37 pg1 patroni@15-main[99]: 2022-04-03 19:12:37,151 INFO: Lock owner: pg2; I am pg1
Here, pg_rewind is called and works fine.
So I think this works as intended, and I'm marking it Ready for
Committer.
Michael
--
Michael Banck
Teamleiter PostgreSQL-Team
Projektleiter
Tel.: +49 2166 9901-171
E-Mail: michael.banck@credativ.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
Attachments:
patroni.patchtext/x-patch; charset=UTF-8; name=patroni.patchDownload
--- /usr/lib/python3/dist-packages/patroni/postgresql/__init__.py 2022-02-18 13:16:15.000000000 +0000
+++ __init__.py 2022-04-03 19:17:29.952665383 +0000
@@ -798,7 +798,8 @@
return True
def get_guc_value(self, name):
- cmd = [self.pgcommand('postgres'), '-D', self._data_dir, '-C', name]
+ cmd = [self.pgcommand('postgres'), '-D', self._data_dir, '-C', name,
+ '--config-file={}'.format(self.config.postgresql_conf)]
try:
data = subprocess.check_output(cmd)
if data:
--- /usr/lib/python3/dist-packages/patroni/postgresql/rewind.py 2022-02-18 13:16:15.000000000 +0000
+++ rewind.py 2022-04-03 19:21:14.479726127 +0000
@@ -314,6 +314,7 @@
cmd = [self._postgresql.pgcommand('pg_rewind')]
if self._postgresql.major_version >= 130000 and restore_command:
cmd.append('--restore-target-wal')
+ cmd.append('--config-file={0}/postgresql.conf'.format(self._postgresql.config._config_dir))
cmd.extend(['-D', self._postgresql.data_dir, '--source-server', dsn])
while True:
Just reposting the previous patches to straighten out the cfbot.
Attachments:
patroni.patchtext/x-patch; charset=US-ASCII; name=patroni.patchDownload
--- /usr/lib/python3/dist-packages/patroni/postgresql/__init__.py 2022-02-18 13:16:15.000000000 +0000
+++ __init__.py 2022-04-03 19:17:29.952665383 +0000
@@ -798,7 +798,8 @@
return True
def get_guc_value(self, name):
- cmd = [self.pgcommand('postgres'), '-D', self._data_dir, '-C', name]
+ cmd = [self.pgcommand('postgres'), '-D', self._data_dir, '-C', name,
+ '--config-file={}'.format(self.config.postgresql_conf)]
try:
data = subprocess.check_output(cmd)
if data:
--- /usr/lib/python3/dist-packages/patroni/postgresql/rewind.py 2022-02-18 13:16:15.000000000 +0000
+++ rewind.py 2022-04-03 19:21:14.479726127 +0000
@@ -314,6 +314,7 @@
cmd = [self._postgresql.pgcommand('pg_rewind')]
if self._postgresql.major_version >= 130000 and restore_command:
cmd.append('--restore-target-wal')
+ cmd.append('--config-file={0}/postgresql.conf'.format(self._postgresql.config._config_dir))
cmd.extend(['-D', self._postgresql.data_dir, '--source-server', dsn])
while True:
add_configdir_to_pg_rewind_v6.patchtext/x-patch; charset=US-ASCII; name=add_configdir_to_pg_rewind_v6.patchDownload
doc/src/sgml/ref/pg_rewind.sgml | 12 +++++
src/bin/pg_rewind/pg_rewind.c | 24 ++++++++-
src/bin/pg_rewind/t/001_basic.pl | 1 +
src/bin/pg_rewind/t/RewindTest.pm | 105 +++++++++++++++++++++++---------------
4 files changed, 101 insertions(+), 41 deletions(-)
diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml
index 33e6bb64ad..62fcb71825 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 data 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 b39b5c1aac..2e83c7ee8e 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,8 @@ 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 it resides outside\n"));
+ printf(_(" the target data directory (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"));
@@ -114,13 +117,14 @@ main(int argc, char **argv)
{"write-recovery-conf", no_argument, NULL, 'R'},
{"source-pgdata", required_argument, NULL, 1},
{"source-server", required_argument, NULL, 2},
+ {"debug", no_argument, NULL, 3},
{"no-ensure-shutdown", no_argument, NULL, 4},
+ {"config-file", required_argument, NULL, 5},
{"version", no_argument, NULL, 'V'},
{"restore-target-wal", no_argument, NULL, 'c'},
{"dry-run", no_argument, NULL, 'n'},
{"no-sync", no_argument, NULL, 'N'},
{"progress", no_argument, NULL, 'P'},
- {"debug", no_argument, NULL, 3},
{NULL, 0, NULL, 0}
};
int option_index;
@@ -205,6 +209,10 @@ main(int argc, char **argv)
case 4:
no_ensure_shutdown = true;
break;
+
+ case 5:
+ config_file = pg_strdup(optarg);
+ break;
}
}
@@ -1061,6 +1069,13 @@ getRestoreCommand(const char *argv0)
appendPQExpBufferStr(postgres_cmd, " -D ");
appendShellString(postgres_cmd, datadir_target);
+ /* add --config_file switch only if requested */
+ if (config_file != NULL)
+ {
+ appendPQExpBufferStr(postgres_cmd, " --config_file=");
+ appendShellString(postgres_cmd, config_file);
+ }
+
/* add -C switch, for restore_command */
appendPQExpBufferStr(postgres_cmd, " -C restore_command");
@@ -1139,6 +1154,13 @@ ensureCleanShutdown(const char *argv0)
appendPQExpBufferStr(postgres_cmd, " --single -F -D ");
appendShellString(postgres_cmd, datadir_target);
+ /* add --config_file switch only if requested */
+ if (config_file != NULL)
+ {
+ appendPQExpBufferStr(postgres_cmd, " --config_file=");
+ appendShellString(postgres_cmd, config_file);
+ }
+
/* finish with the database name, and a properly quoted redirection */
appendPQExpBufferStr(postgres_cmd, " template1 < ");
appendShellString(postgres_cmd, DEVNULL);
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
{
On Sun, Apr 03, 2022 at 09:31:32PM +0200, Michael Banck wrote:
The patch applies, make is ok, make check is ok, pg_rewind TAP tests
are ok.
+ appendPQExpBufferStr(postgres_cmd, " --config_file=");
+ appendShellString(postgres_cmd, config_file);
Nit. This is a valid option for the postgres command, but I'd rather
use a -c switch instead, mostly as a matter of self-documentation.
In the tests, the only difference between the modes "archive_cli" and
"archive" is the extra option given to the pg_rewind command, and
that's a simple redirect to what pg_rewind would choose by default
anyway. A more elegant solution would be to have an extra option in
RewindTest::run_pg_rewind(), where any caller of the routine can feed
extra options to the pg_rewind command. Now, in this case, we are
not going to lose any coverage if the existing "archive" mode is
changed so as we move away postgresql.conf from the target data folder
and just use --config-file by default, no? I would make the choice of
simplicity, by giving up on "archive_cli" and using
primary-postgresql.conf.tmp as value for --config-file. There could
be an argument for using --config-file for all the modes, as well.
--
Michael
On Tue, Apr 05, 2022 at 11:10:30AM +0900, Michael Paquier wrote:
In the tests, the only difference between the modes "archive_cli" and
"archive" is the extra option given to the pg_rewind command, and
that's a simple redirect to what pg_rewind would choose by default
anyway. A more elegant solution would be to have an extra option in
RewindTest::run_pg_rewind(), where any caller of the routine can feed
extra options to the pg_rewind command. Now, in this case, we are
not going to lose any coverage if the existing "archive" mode is
changed so as we move away postgresql.conf from the target data folder
and just use --config-file by default, no? I would make the choice of
simplicity, by giving up on "archive_cli" and using
primary-postgresql.conf.tmp as value for --config-file. There could
be an argument for using --config-file for all the modes, as well.
The clock is ticking, so I have looked at this patch by myself. I
have finished by tweaking the docs, the code and the tests as of the
attached. One thing that I found annoying is the lack of description
about the fact that this option affects pg_rewind when it internally
starts the target cluster, as of when we retrieve restore_command and
when we enforce crash recovery to have a target cluster with a clean
shutdown state. The tests are heavily simplified, having no impact on
the run-time while improving the coverage for the code paths changed
here.
--
Michael
Attachments:
add_configdir_to_pg_rewind_v7.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c
index b39b5c1aac..59838d6a07 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,8 @@ 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 use specified main server configuration\n"));
+ printf(_(" file when running target cluster\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"));
@@ -115,6 +118,7 @@ main(int argc, char **argv)
{"source-pgdata", required_argument, NULL, 1},
{"source-server", required_argument, NULL, 2},
{"no-ensure-shutdown", no_argument, NULL, 4},
+ {"config-file", required_argument, NULL, 5},
{"version", no_argument, NULL, 'V'},
{"restore-target-wal", no_argument, NULL, 'c'},
{"dry-run", no_argument, NULL, 'n'},
@@ -205,6 +209,10 @@ main(int argc, char **argv)
case 4:
no_ensure_shutdown = true;
break;
+
+ case 5:
+ config_file = pg_strdup(optarg);
+ break;
}
}
@@ -1061,6 +1069,13 @@ getRestoreCommand(const char *argv0)
appendPQExpBufferStr(postgres_cmd, " -D ");
appendShellString(postgres_cmd, datadir_target);
+ /* add custom configuration file only if requested */
+ if (config_file != NULL)
+ {
+ appendPQExpBufferStr(postgres_cmd, " -c config_file=");
+ appendShellString(postgres_cmd, config_file);
+ }
+
/* add -C switch, for restore_command */
appendPQExpBufferStr(postgres_cmd, " -C restore_command");
@@ -1139,6 +1154,13 @@ ensureCleanShutdown(const char *argv0)
appendPQExpBufferStr(postgres_cmd, " --single -F -D ");
appendShellString(postgres_cmd, datadir_target);
+ /* add custom configuration file only if requested */
+ if (config_file != NULL)
+ {
+ appendPQExpBufferStr(postgres_cmd, " -c config_file=");
+ appendShellString(postgres_cmd, config_file);
+ }
+
/* finish with the database name, and a properly quoted redirection */
appendPQExpBufferStr(postgres_cmd, " template1 < ");
appendShellString(postgres_cmd, DEVNULL);
diff --git a/src/bin/pg_rewind/t/RewindTest.pm b/src/bin/pg_rewind/t/RewindTest.pm
index 1e34768e27..8fd1f4b9de 100644
--- a/src/bin/pg_rewind/t/RewindTest.pm
+++ b/src/bin/pg_rewind/t/RewindTest.pm
@@ -263,7 +263,9 @@ sub run_pg_rewind
"--debug",
"--source-pgdata=$standby_pgdata",
"--target-pgdata=$primary_pgdata",
- "--no-sync"
+ "--no-sync",
+ "--config-file",
+ "$tmp_folder/primary-postgresql.conf.tmp"
],
'pg_rewind local');
}
@@ -276,7 +278,8 @@ sub run_pg_rewind
'pg_rewind', "--debug",
"--source-server", $standby_connstr,
"--target-pgdata=$primary_pgdata", "--no-sync",
- "--write-recovery-conf"
+ "--write-recovery-conf", "--config-file",
+ "$tmp_folder/primary-postgresql.conf.tmp"
],
'pg_rewind remote');
@@ -323,7 +326,8 @@ sub run_pg_rewind
# Note the use of --no-ensure-shutdown here. WAL files are
# gone in this mode and the primary has been stopped
- # gracefully already.
+ # gracefully already. --config-file reuses the original
+ # postgresql.conf as restore_command has been enabled above.
command_ok(
[
'pg_rewind',
@@ -332,7 +336,9 @@ sub run_pg_rewind
"--target-pgdata=$primary_pgdata",
"--no-sync",
"--no-ensure-shutdown",
- "--restore-target-wal"
+ "--restore-target-wal",
+ "--config-file",
+ "$primary_pgdata/postgresql.conf"
],
'pg_rewind archive');
}
diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml
index 33e6bb64ad..e808239aa5 100644
--- a/doc/src/sgml/ref/pg_rewind.sgml
+++ b/doc/src/sgml/ref/pg_rewind.sgml
@@ -241,6 +241,21 @@ PostgreSQL documentation
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>--config-file=<replaceable class="parameter">filename</replaceable></option></term>
+ <listitem>
+ <para>
+ Use the specified main server configuration file for the target
+ cluster. This affects <application>pg_rewind</application> when
+ it uses internally the <application>postgres</application> command
+ for the rewind operation on this cluster (when retrieving
+ <varname>restore_command</varname> with the option
+ <option>-c/--restore-target-wal</option> and when forcing a
+ completion of crash recovery).
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>--debug</option></term>
<listitem>
Am 06.04.22 um 14:04 schrieb Michael Paquier:
On Tue, Apr 05, 2022 at 11:10:30AM +0900, Michael Paquier wrote:
In the tests, the only difference between the modes "archive_cli" and
"archive" is the extra option given to the pg_rewind command, and
that's a simple redirect to what pg_rewind would choose by default
anyway. A more elegant solution would be to have an extra option in
RewindTest::run_pg_rewind(), where any caller of the routine can feed
extra options to the pg_rewind command. Now, in this case, we are
not going to lose any coverage if the existing "archive" mode is
changed so as we move away postgresql.conf from the target data folder
and just use --config-file by default, no? I would make the choice of
simplicity, by giving up on "archive_cli" and using
primary-postgresql.conf.tmp as value for --config-file. There could
be an argument for using --config-file for all the modes, as well.The clock is ticking, so I have looked at this patch by myself. I
Ta! Sorry, been ridiculously busy these days, and I do confess that I've
been struggling a bit with those tests before ;-)
have finished by tweaking the docs, the code and the tests as of the
attached. One thing that I found annoying is the lack of description
about the fact that this option affects pg_rewind when it internally
starts the target cluster, as of when we retrieve restore_command and
when we enforce crash recovery to have a target cluster with a clean
shutdown state. The tests are heavily simplified, having no impact on
the run-time while improving the coverage for the code paths changed
here.
--
Michael
LGTM!
Thx again!
--
Gunnar "Nick" Bluth
Eimermacherweg 106
D-48159 Münster
Mobil +49 172 8853339
Email: gunnar.bluth@pro-open.de
__________________________________________________________________________
"Ceterum censeo SystemD esse delendam" - Cato
On Wed, Apr 06, 2022 at 02:32:44PM +0200, Gunnar "Nick" Bluth wrote:
Ta! Sorry, been ridiculously busy these days, and I do confess that I've
been struggling a bit with those tests before ;-)
No problem. Double-checked this morning and applied, then.
--
Michael
Am 07.04.22 um 01:54 schrieb Michael Paquier:
On Wed, Apr 06, 2022 at 02:32:44PM +0200, Gunnar "Nick" Bluth wrote:
Ta! Sorry, been ridiculously busy these days, and I do confess that I've
been struggling a bit with those tests before ;-)No problem. Double-checked this morning and applied, then.
--
Michael
Wonderful!
So, @cyberdemn, who would have thought we get this fixed so quickly?
Time to update the Patroni PR once more! ;-)
Thanks & best regards,
--
Gunnar "Nick" Bluth
Eimermacherweg 106
D-48159 Münster
Mobil +49 172 8853339
Email: gunnar.bluth@pro-open.de
__________________________________________________________________________
"Ceterum censeo SystemD esse delendam" - Cato