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

On Fri, Feb 6, 2015 at 4:58 PM, Fujii Masao wrote:
> timestamp.c:1708: warning: implicit declaration of function
> 'HandleStartupProcInterrupts'
>
> I got the above warning at the compilation.
>
> +        pg_usleep(wait_time);
> +        HandleStartupProcInterrupts();
> +        total_time -= wait_time;
>
> It seems strange to call the startup-process-specific function in
> the "generic" function like TimestampSleepDifference().

I changed the sleep to a WaitLatch and moved all the logic back to
xlog.c, removing at the same the stuff in timestamp.c because it seems
strange to add a dependency with even latch there.

>      * 5. Sleep 5 seconds, and loop back to 1.
>
> In WaitForWALToBecomeAvailable(), the above comment should
> be updated.

Done.

> -                     * 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.

> +# specifies an optional internal to wait for WAL to become available when
>
> s/internal/interval?

This part has been removed in the new part as parameter is set as a GUC.

>
> +        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.
>
> At least to me, it seems not easy to understand what the parameter is
> from this description. What about the following, for example?
>     This parameter specifies the amount of time to wait when WAL is not
>     available from any sources (streaming replication, local
>     <filename>pg_xlog</> or WAL archive) before retrying to retrieve WAL....

OK, done.

> Isn't it better to place this parameter in postgresql.conf rather than
> recovery.conf? I'd like to change the value of this parameter without
> restarting the server.

Yes, agreed. Done so with a SIGHUP guc.
Regards,
--
Michael