Re: [HACKERS] Re: [doc fix] PG10: wroing description onconnect_timeout when multiple hosts are specified - Mailing list pgsql-hackers

On Tue, May 16, 2017 at 3:13 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> FWIW, I think the position most of us were taking is that this feature
> is meant to retry transport-level connection failures, not cases where
> we successfully make a connection to a server and then it rejects our
> login attempt.  I would classify a timeout as a transport-level failure
> as long as it occurs before we got any server response --- if it happens
> during the authentication protocol, that's less clear.  But it might not
> be very practical to distinguish those two cases.
>
> In short, +1 for retrying on timeout during connection, and I'm okay with
> retrying a timeout during authentication if it's not practical to treat
> that differently.

Sensible argument here. It could happen that a server is unresponsive,
for example in split-brains (?).

I have been playing a bit with the patch.

+ *
+ * Returns -1 on failure, 0 if the socket is readable/writable, 1 if
it timed out. */
pqWait is internal to libpq, so we are free to set up what we want
here. Still I think that we should be consistent with what
pqSocketCheck returns:
* >0 means that the socket is readable/writable, counting things.
* 0 is for timeout.
* -1 on failure.

+    int            ret = 0;
+    int            timeout = 0;
The declaration of "ret" should be internal in the for(;;) loop.

+           /* Attempt connection to the next host, starting the
connect_timeout timer */
+           pqDropConnection(conn, true);
+           conn->addr_cur = conn->connhost[conn->whichhost].addrlist;
+           conn->status = CONNECTION_NEEDED;
+           finish_time = time(NULL) + timeout;
+       }
I think that it would be safer to not set finish_time if
conn->connect_timeout is NULL. I agree that your code works because
pqWaitTimed() will never complain on timeout reached if finish_time is
-1. That's for robustness sake.

The docs say that for connect_timeout:     <para>      Maximum wait for connection, in seconds (write as a decimal
integer     string). Zero or not specified means wait indefinitely.  It is not      recommended to use a timeout of
lessthan 2 seconds.      This timeout applies separately to each connection attempt.      For example, if you specify
twohosts and both of them are unreachable,      and <literal>connect_timeout</> is 5, the total time spent waiting for
a     connection might be up to 10 seconds.     </para>
 

It seems to me that this implies that if a timeout occurs on the first
connection, the counter is reset, which is what this patch is doing.
So we are all set.
-- 
Michael