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

On Tue, Jan 20, 2015 at 12:57 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Tue, Jan 20, 2015 at 1:56 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> On 2015-01-19 17:16:11 +0900, Michael Paquier wrote:
>>> On Mon, Jan 19, 2015 at 4:10 PM, Michael Paquier
>>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> > On Sat, Jan 17, 2015 at 2:44 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>> >> Not this patch's fault, but I'm getting a bit tired seeing the above open
>>> >> coded. How about adding a function that does the sleeping based on a
>>> >> timestamptz and a ms interval?
>>> > You mean in plugins, right? I don't recall seeing similar patterns in
>>> > other code paths of backend. But I think that we can use something
>>> > like that in timestamp.c then because we need to leverage that between
>>> > two timestamps, the last failure and now():
>>> > TimestampSleepDifference(start_time, stop_time, internal_ms);
>>> > Perhaps you have something else in mind?
>>> >
>>> > Attached is an updated patch.
>>
>>> Actually I came with better than last patch by using a boolean flag as
>>> return value of TimestampSleepDifference and use
>>> TimestampDifferenceExceeds directly inside it.
>>
>>> Subject: [PATCH] Add wal_availability_check_interval to control WAL fetching
>>> on failure
>>
>> I think that name isn't a very good. And its isn't very accurate
>> either.
>> How about wal_retrieve_retry_interval? Not very nice, but I think it's
>> still better than the above.
> Fine for me.
>
>>> + <varlistentry id="wal-availability-check-interval" xreflabel="wal_availability_check_interval">
>>> + <term><varname>wal_availability_check_interval</varname> (<type>integer</type>)
>>> + <indexterm>
>>> + <primary><varname>wal_availability_check_interval</> recovery parameter</primary>
>>> + </indexterm>
>>> + </term>
>>> + <listitem>
>>> + <para>
>>> + This parameter specifies the amount of time to wait when
>>> + WAL is not available for a node in recovery. Default value is
>>> + <literal>5s</>.
>>> + </para>
>>> + <para>
>>> + A node in recovery will wait for this amount of time if
>>> + <varname>restore_command</> returns nonzero exit status code when
>>> + fetching new WAL segment files from archive or when a WAL receiver
>>> + is not able to fetch a WAL record when using streaming replication.
>>> + </para>
>>> + </listitem>
>>> + </varlistentry>
>>> +
>>> </variablelist>
>>
>> Walreceiver doesn't wait that amount, but rather how long the connection
>> is intact. And restore_command may or may not retry.
> I changed this paragraph as follows:
> + This parameter specifies the amount of time to wait when a failure
> + occurred when reading WAL from a source (be it via streaming
> + replication, local <filename>pg_xlog</> or WAL archive) for a node
> + in standby mode, or when WAL is expected from a source. Default
> + value is <literal>5s</>.
> Pondering more about this patch, I am getting the feeling that it is
> not that much a win to explain in details in the doc each failure
> depending on the source from which WAL is taken. But it is essential
> to mention that the wait is done only for a node using standby mode.
>
> Btw, I noticed two extra things that I think should be done for consistency:
> - We should use as well wal_retrieve_retry_interval when waiting for
> WAL to arrive after checking for the failures:
> /*
> - * 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
> + * wal_retrieve_retry_interval
> ms, 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 * 1L);
> -
>
> Updated patch attached.

+ TimestampDifference(start_time, stop_time, &secs, &microsecs);
+ pg_usleep(interval_msec * 1000L - (1000000L * secs + 1L * microsecs));

What if the large interval is set and a signal arrives during the sleep?
I'm afraid that a signal cannot terminate the sleep on some platforms.
This problem exists even now because pg_usleep is used, but the sleep
time is just 5 seconds, so it's not so bad. But the patch allows a user to
set large sleep time. Shouldn't we use WaitLatch or split the pg_usleep
like recoveryPausesHere() does?

Regards,

--
Fujii Masao