Re: Supply restore_command to pg_rewind via CLI argument

> 29 июня 2021 г., в 19:34, Alexey Kondratov <kondratov(dot)aleksey(at)gmail(dot)com> написал(а):
>
> On Fri, Jun 18, 2021 at 10:06 PM Alexey Kondratov
> <kondratov(dot)aleksey(at)gmail(dot)com> wrote:
>> On Fri, Jun 18, 2021 at 5:42 PM Andrey Borodin <x4mmm(at)yandex-team(dot)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.