Re: Patch: add recovery_timeout option to control timeout of restore_command nonzero status code

On Fri, Feb 20, 2015 at 9:12 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Tue, Feb 10, 2015 at 10:32 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Mon, Feb 9, 2015 at 8:29 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>> On Sun, Feb 8, 2015 at 2:54 PM, Michael Paquier
>>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>>> On Fri, Feb 6, 2015 at 4:58 PM, Fujii Masao wrote:
>>>>> - * Wait for more WAL to arrive. Time out after 5 seconds,
>>>>> - * like when polling the archive, to react to a trigger
>>>>> - * file promptly.
>>>>> + * Wait for more WAL to arrive. Time out after
>>>>> the amount of
>>>>> + * time specified by wal_retrieve_retry_interval, like
>>>>> + * when polling the archive, to react to a
>>>>> trigger file promptly.
>>>>> */
>>>>> WaitLatch(&XLogCtl->recoveryWakeupLatch,
>>>>> WL_LATCH_SET | WL_TIMEOUT,
>>>>> - 5000L);
>>>>> + wal_retrieve_retry_interval * 1000L);
>>>>>
>>>>> This change can prevent the startup process from reacting to
>>>>> a trigger file. Imagine the case where the large interval is set
>>>>> and the user want to promote the standby by using the trigger file
>>>>> instead of pg_ctl promote. I think that the sleep time should be 5s
>>>>> if the interval is set to more than 5s. Thought?
>>>>
>>>> I disagree here. It is interesting to accelerate the check of WAL
>>>> availability from a source in some cases for replication, but the
>>>> opposite is true as well as mentioned by Alexey at the beginning of
>>>> the thread to reduce the number of requests when requesting WAL
>>>> archives from an external storage type AWS. Hence a correct solution
>>>> would be to check periodically for the trigger file with a maximum
>>>> one-time wait of 5s to ensure backward-compatible behavior. We could
>>>> reduce it to 1s or something like that as well.
>>>
>>> You seem to have misunderstood the code in question. Or I'm missing something.
>>> The timeout of the WaitLatch is just the interval to check for the trigger file
>>> while waiting for more WAL to arrive from streaming replication. Not related to
>>> the retry time to restore WAL from the archive.
>>
>> [Re-reading the code...]
>> Aah.. Yes you are right. Sorry for the noise. Yes let's wait for a
>> maximum of 5s then. I also noticed in previous patch that the wait was
>> maximized to 5s. To begin with, a loop should have been used if it was
>> a sleep, but as now patch uses a latch this limit does not make much
>> sense... Patch updated is attached.
>
> On second thought, the interval to check the trigger file is very different
> from the wait time to retry to retrieve WAL. So it seems strange and even
> confusing to control them by one parameter. If we really want to make
> the interval for the trigger file configurable, we should invent new GUC for it.
> But I don't think that it's worth doing that. If someone wants to react the
> trigger file more promptly for "fast" promotion, he or she basically can use
> pg_ctl promote command, instead. Thought?

Hm, OK.

> Attached is the updated version of the patch. I changed the parameter so that
> it doesn't affect the interval of checking the trigger file.
>
> - static pg_time_t last_fail_time = 0;
> - pg_time_t now;
> + TimestampTz now = GetCurrentTimestamp();
> + TimestampTz last_fail_time = now;
>
> I reverted the code here as it was. I don't think GetCurrentTimestamp() needs
> to be called for each WaitForWALToBecomeAvailable().
>
> + WaitLatch(&XLogCtl->recoveryWakeupLatch,
> + WL_LATCH_SET | WL_TIMEOUT,
> + wait_time / 1000);
>
> Don't we need to specify WL_POSTMASTER_DEATH flag here? Added.

Yeah, I am wondering though why this has not been added after 89fd72cb though.

> + {"wal_retrieve_retry_interval", PGC_SIGHUP, WAL_SETTINGS,
>
> WAL_SETTINGS should be REPLICATION_STANDBY? Changed.

Sure.
--
Michael