Thread: Supply restore_command to pg_rewind via CLI argument
Hi hackers! Starting from v13 pg_rewind can use restore_command if it lacks necessary WAL segments. And this is awesome for HA clusterswith many nodes! Thanks to everyone who worked on the feature! Here's some feedback on how to make things even better. If we run 'pg_rewind --restore-target-wal' there must be restore_command in config of target installation. But if the configis 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. Wesolved the problem by putting config into PGDATA only during pg_rewind, but this does not seem like a very robust solution. Maybe we could add "-C, --target-restore-command=COMMAND target WAL restore_command\n" as was proposed within earlier versionsof the patch[0]? Or instruct pg_rewind to pass config to 'postgres -C restore_command' run? From my POV adding --target-restore-command is simplest way, I can extract corresponding portions from original patch. Thanks! Best regards, Andrey Borodin. [0] https://www.postgresql.org/message-id/flat/CAPpHfduUqKLr2CRpcpHcv1qjaz%2B-%2Bi9bOL2AOvdWSr954ti8Xw%40mail.gmail.com#1d4b372b5aa26f93af9ed1d5dd0693cd
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 configis 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. Wesolved 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 earlierversions 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 theconfig 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 robustsolution. > > > > 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 earlierversions 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
Attachment
> 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
theconfig 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
robustsolution.
>>>
>>
>> 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_commandthere.
>>> Maybe we could add "-C, --target-restore-command=COMMAND target WAL restore_command\n" as was proposed within
earlierversions 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
Attachment
>> 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.
Attachment
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
Attachment
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