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, >> >> WAL_SETTINGS should be REPLICATION_STANDBY? Changed. > > Sure.
So I pushed the patch. Regards, -- Fujii Masao -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers