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

On Fri, Feb 20, 2015 at 9:33 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> 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.

So I pushed the patch.

Regards,

--
Fujii Masao