Supply restore_command to pg_rewind via CLI argument

Hi,

On Fri, Jun 18, 2021 at 5:42 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:

If we run 'pg_rewind --restore-target-wal' there must be restore_command in config of target installation. But if the config is not within $PGDATA\postgresql.conf pg_rewind cannot use it.
If we run postmaster with `-c config_file=/etc/postgresql/10/data/postgresql.conf`, we simply cannot use the feature. We solved the problem by putting config into PGDATA only during pg_rewind, but this does not seem like a very robust solution.

Yeah, Michael was against it, while we had no good arguments, so
Alexander removed it, IIRC. This example sounds reasonable to me. I
also recall some complaints from PostgresPro support folks, that it is
sad to not have a cli option to pass restore_command. However, I just
thought about another recent feature --- ensure clean shutdown, which
is turned on by default. So you usually run Postgres with one config,
but pg_rewind may start it with another, although in single-user mode.
Is it fine for you?

Maybe we could add "-C, --target-restore-command=COMMAND target WAL restore_command\n" as was proposed within earlier versions of the patch[0]? Or instruct pg_rewind to pass config to 'postgres -C restore_command' run?

Hm, adding --target-restore-command is the simplest way, sure, but
forwarding something like '-c config_file=...' to postgres sounds
interesting too. Could it have any use case beside providing a
restore_command? I cannot imagine anything right now, but if any
exist, then it could be a more universal approach.

From my POV adding --target-restore-command is simplest way, I can extract corresponding portions from original patch.

I will have a look, maybe I even already have this patch separately. I
remember that we were considering adding this option to PostgresPro,
when we did a backport of this feature.

--
Alexey Kondratov

On Fri, Jun 18, 2021 at 10:06 PM Alexey Kondratov
<kondratov.aleksey@gmail.com> wrote:

On Fri, Jun 18, 2021 at 5:42 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:

If we run 'pg_rewind --restore-target-wal' there must be restore_command in config of target installation. But if the config is not within $PGDATA\postgresql.conf pg_rewind cannot use it.
If we run postmaster with `-c config_file=/etc/postgresql/10/data/postgresql.conf`, we simply cannot use the feature. We solved the problem by putting config into PGDATA only during pg_rewind, but this does not seem like a very robust solution.

Yeah, Michael was against it, while we had no good arguments, so
Alexander removed it, IIRC. This example sounds reasonable to me. I
also recall some complaints from PostgresPro support folks, that it is
sad to not have a cli option to pass restore_command. However, I just
thought about another recent feature --- ensure clean shutdown, which
is turned on by default. So you usually run Postgres with one config,
but pg_rewind may start it with another, although in single-user mode.
Is it fine for you?

Maybe we could add "-C, --target-restore-command=COMMAND target WAL restore_command\n" as was proposed within earlier versions of the patch[0]? Or instruct pg_rewind to pass config to 'postgres -C restore_command' run?

Hm, adding --target-restore-command is the simplest way, sure, but
forwarding something like '-c config_file=...' to postgres sounds
interesting too. Could it have any use case beside providing a
restore_command? I cannot imagine anything right now, but if any
exist, then it could be a more universal approach.

From my POV adding --target-restore-command is simplest way, I can extract corresponding portions from original patch.

I will have a look, maybe I even already have this patch separately. I
remember that we were considering adding this option to PostgresPro,
when we did a backport of this feature.

Here it is. I have slightly adapted the previous patch to the recent
pg_rewind changes. In this version -C does not conflict with -c, it
just overrides it.

--
Alexey Kondratov

29 июня 2021 г., в 19:34, Alexey Kondratov <kondratov.aleksey@gmail.com> написал(а):

On Fri, Jun 18, 2021 at 10:06 PM Alexey Kondratov
<kondratov.aleksey@gmail.com> wrote:

On Fri, Jun 18, 2021 at 5:42 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:

If we run 'pg_rewind --restore-target-wal' there must be restore_command in config of target installation. But if the config is not within $PGDATA\postgresql.conf pg_rewind cannot use it.
If we run postmaster with `-c config_file=/etc/postgresql/10/data/postgresql.conf`, we simply cannot use the feature. We solved the problem by putting config into PGDATA only during pg_rewind, but this does not seem like a very robust solution.

Yeah, Michael was against it, while we had no good arguments, so
Alexander removed it, IIRC. This example sounds reasonable to me. I
also recall some complaints from PostgresPro support folks, that it is
sad to not have a cli option to pass restore_command. However, I just
thought about another recent feature --- ensure clean shutdown, which
is turned on by default. So you usually run Postgres with one config,
but pg_rewind may start it with another, although in single-user mode.
Is it fine for you?

We rewind failovered node, so clean shutdown was not performed. But I do not see how it could help anyway.
To pass restore command we had to setup new config in PGDATA configured as standby, because either way we cannot set restore_command there.

Maybe we could add "-C, --target-restore-command=COMMAND target WAL restore_command\n" as was proposed within earlier versions of the patch[0]? Or instruct pg_rewind to pass config to 'postgres -C restore_command' run?

Hm, adding --target-restore-command is the simplest way, sure, but
forwarding something like '-c config_file=...' to postgres sounds
interesting too. Could it have any use case beside providing a
restore_command? I cannot imagine anything right now, but if any
exist, then it could be a more universal approach.

I think --target-restore-command is the best solution right now.

From my POV adding --target-restore-command is simplest way, I can extract corresponding portions from original patch.

I will have a look, maybe I even already have this patch separately. I
remember that we were considering adding this option to PostgresPro,
when we did a backport of this feature.

Here it is. I have slightly adapted the previous patch to the recent
pg_rewind changes. In this version -C does not conflict with -c, it
just overrides it.

Great, thanks!

There is a small bug
+	/*
+	 * Take restore_command from the postgresql.conf only if it is not already
+	 * provided as a command line option.
+	 */
+	if (!restore_wal && restore_command == NULL)
 		return;

I think we should use condition (!restore_wal || restore_command != NULL).

Besides this patch looks good and is ready for committer IMV.

Thanks!

Best regards, Andrey Borodin.

On Fri, Aug 27, 2021 at 10:05 AM Andrey Borodin <x4mmm@yandex-team.ru> wrote:

There is a small bug
+       /*
+        * Take restore_command from the postgresql.conf only if it is not already
+        * provided as a command line option.
+        */
+       if (!restore_wal && restore_command == NULL)
return;

I think we should use condition (!restore_wal || restore_command != NULL).

Yes, you are right, thanks. Attached is a fixed version. Tests were
passing since PostgresNode->enable_restoring is adding restore_command
to the postgresql.conf anyway.

Besides this patch looks good and is ready for committer IMV.

--
Alexey Kondratov

Besides this patch looks good and is ready for committer IMV.

A variant of this patch was originally objected against by Michael, and as this
version is marked Ready for Committer I would like to hear his opinions on
whether the new evidence changes anything.

--
Daniel Gustafsson https://vmware.com/

14 сент. 2021 г., в 18:41, Daniel Gustafsson <daniel@yesql.se> написал(а):

Besides this patch looks good and is ready for committer IMV.

A variant of this patch was originally objected against by Michael, and as this
version is marked Ready for Committer I would like to hear his opinions on
whether the new evidence changes anything.

I skimmed the thread for reasoning. --target-restore-command was rejected on the following grounds

Do we actually need --target-restore-command at all? It seems to me
that we have all we need with --restore-target-wal, and that's not
really instinctive to pass down a command via another command..

Currently we know that --restore-target-wal is not enough if postgresql.conf does not reside within PGDATA.

Best regards, Andrey Borodin.

On 14 Sep 2021, at 16:05, Andrey Borodin <x4mmm@yandex-team.ru> wrote:

Do we actually need --target-restore-command at all? It seems to me
that we have all we need with --restore-target-wal, and that's not
really instinctive to pass down a command via another command..

Currently we know that --restore-target-wal is not enough if postgresql.conf does not reside within PGDATA.

That's a useful reason which wasn't brought up in the earlier thread, and may
tip the scales in favor.

The patch no longer applies, can you submit a rebased version please?

--
Daniel Gustafsson https://vmware.com/

4 нояб. 2021 г., в 17:55, Daniel Gustafsson <daniel@yesql.se> написал(а):

The patch no longer applies, can you submit a rebased version please?

Thanks, Daniel! PFA rebase.

Best regards, Andrey Borodin.

Hi,

On 2021-11-05 15:10:29 +0500, Andrey Borodin wrote:

4 нояб. 2021 г., в 17:55, Daniel Gustafsson <daniel@yesql.se> написал(а):

The patch no longer applies, can you submit a rebased version please?

Thanks, Daniel! PFA rebase.

Doesn't apply once more: http://cfbot.cputube.org/patch_37_3213.log

Greetings,

Andres Freund

Hi,

On Tue, Mar 22, 2022 at 3:32 AM Andres Freund <andres@anarazel.de> wrote:

Doesn't apply once more: http://cfbot.cputube.org/patch_37_3213.log

Thanks for the reminder, a rebased version is attached.

Regards
--
Alexey Kondratov

On Thu, Nov 04, 2021 at 01:55:43PM +0100, Daniel Gustafsson wrote:

On 14 Sep 2021, at 16:05, Andrey Borodin <x4mmm@yandex-team.ru> wrote:

Do we actually need --target-restore-command at all? It seems to me
that we have all we need with --restore-target-wal, and that's not
really instinctive to pass down a command via another command..

Currently we know that --restore-target-wal is not enough if postgresql.conf does not reside within PGDATA.

That's a useful reason which wasn't brought up in the earlier thread, and may
tip the scales in favor.

It does now, as of 0d5c3875. FWIW, I am not much a fan of the design
where we pass down a command line as an option value of a different
command line (more games with quoting comes into mind first), and
--config-file should give enough room for the case of this thread. I
have switched the status of the patch to reflect that.
--
Michael