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

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?

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.

+ {"wal_retrieve_retry_interval", PGC_SIGHUP, WAL_SETTINGS,

WAL_SETTINGS should be REPLICATION_STANDBY? Changed.

Regards,

--
Fujii Masao