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
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>
signature.asc
Description: PGP signature