On Fri, Feb 20, 2015 at 9:33 PM, Michael Paquier
<michael.paqu...@gmail.com> wrote:
> On Fri, Feb 20, 2015 at 9:12 PM, Fujii Masao <masao.fu...@gmail.com> wrote:
>> On Tue, Feb 10, 2015 at 10:32 PM, Michael Paquier
>> <michael.paqu...@gmail.com> wrote:
>>> On Mon, Feb 9, 2015 at 8:29 PM, Fujii Masao <masao.fu...@gmail.com> wrote:
>>>> On Sun, Feb 8, 2015 at 2:54 PM, Michael Paquier
>>>> <michael.paqu...@gmail.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,
> Sure.

So I pushed the patch.


Fujii Masao

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to