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

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(dot)bluth(at)pro-open(dot)de
__________________________________________________________________________
"Ceterum censeo SystemD esse delendam" - Cato