Re: Patch: add recovery_timeout option to control timeout of restore_command nonzero status code - Mailing list pgsql-hackers

On Fri, Feb 6, 2015 at 1:23 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Thu, Feb 5, 2015 at 11:58 PM, Michael Paquier wrote:
>> An updated patch is attached.
> I just noticed that the patch I sent was incorrect:
> - Parameter name was still wal_availability_check_interval and not
> wal_retrieve_retry_interval
> - Documentation was incorrect.
> Please use the patch attached instead for further review.

Thanks for updating the patch!

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().
    * 5. Sleep 5 seconds, and loop back to 1.

In WaitForWALToBecomeAvailable(), the above comment should
be updated.

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

+# specifies an optional internal to wait for WAL to become available when

s/internal/interval?

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

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.

Regards,

-- 
Fujii Masao