Fix libpq's implementation of per-host connection timeouts.

Commit 5f374fe7a attempted to turn the connect_timeout from an overall
maximum time limit into a per-host limit, but it didn't do a great job of
that.  The timer would only get restarted if we actually detected timeout
within connectDBComplete(), not if we changed our attention to a new host
for some other reason.  In that case the old timeout continued to run,
possibly causing a premature timeout failure for the new host.

Fix that, and also tweak the logic so that if we do get a timeout,
we advance to the next available IP address, not to the next host name.
There doesn't seem to be a good reason to assume that all the IP
addresses supplied for a given host name will necessarily fail the
same way as the current one.  Moreover, this conforms better to the
admittedly-vague documentation statement that the timeout is "per
connection attempt".  I changed that to "per host name or IP address"
to be clearer.  (Note that reconnections to the same server, such as for
switching protocol version or SSL status, don't get their own separate
timeout; that was true before and remains so.)

Also clarify documentation about the interpretation of connect_timeout
values less than 2.

This seems like a bug, so back-patch to v10 where this logic came in.

Tom Lane, reviewed by Fabien Coelho

Discussion: https://postgr.es/m/5735.1533828184@sss.pgh.pa.us

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index c24a69f00cce9c99fa1cbe0dd1a1e31723f1b170..80e55f5f382a08fcd5b2b5228681167888315930 100644 (file)
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1110,10 +1110,11 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
       <term><literal>connect_timeout</literal></term>
       <listitem>
       <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 less than 2 seconds.
-       This timeout applies separately to each connection attempt.
+       Maximum wait for connection, in seconds (write as a decimal integer,
+       e.g. <literal>10</literal>).  Zero, negative, or not specified means
+       wait indefinitely.  The minimum allowed timeout is 2 seconds, therefore
+       a value of <literal>1</literal> is interpreted as <literal>2</literal>.
+       This timeout applies separately to each host name or IP address.
        For example, if you specify two hosts and <literal>connect_timeout</literal>
        is 5, each host will time out if no connection is made within 5
        seconds, so the total time spent waiting for a connection might be

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 986f74ff006ea2c6d095f49a66af6290584ff3f0..9315a27561f0ba199ec06fdd03f7e66fd3da2084 100644 (file)
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1905,6 +1905,8 @@ connectDBComplete(PGconn *conn)
        PostgresPollingStatusType flag = PGRES_POLLING_WRITING;
        time_t          finish_time = ((time_t) -1);
        int                     timeout = 0;
+       int                     last_whichhost = -2;    /* certainly different from whichhost */
+       struct addrinfo *last_addr_cur = NULL;
 
        if (conn == NULL || conn->status == CONNECTION_BAD)
                return 0;
@@ -1918,12 +1920,12 @@ connectDBComplete(PGconn *conn)
                if (timeout > 0)
                {
                        /*
-                        * Rounding could cause connection to fail; need at least 2 secs
+                        * Rounding could cause connection to fail unexpectedly quickly;
+                        * to prevent possibly waiting hardly-at-all, insist on at least
+                        * two seconds.
                         */
                        if (timeout < 2)
                                timeout = 2;
-                       /* calculate the finish time based on start + timeout */
-                       finish_time = time(NULL) + timeout;
                }
        }
 
@@ -1931,6 +1933,21 @@ connectDBComplete(PGconn *conn)
        {
                int                     ret = 0;
 
+               /*
+                * (Re)start the connect_timeout timer if it's active and we are
+                * considering a different host than we were last time through.  If
+                * we've already succeeded, though, needn't recalculate.
+                */
+               if (flag != PGRES_POLLING_OK &&
+                       timeout > 0 &&
+                       (conn->whichhost != last_whichhost ||
+                        conn->addr_cur != last_addr_cur))
+               {
+                       finish_time = time(NULL) + timeout;
+                       last_whichhost = conn->whichhost;
+                       last_addr_cur = conn->addr_cur;
+               }
+
                /*
                 * Wait, if necessary.  Note that the initial state (just after
                 * PQconnectStart) is to wait for the socket to select for writing.
@@ -1975,18 +1992,10 @@ connectDBComplete(PGconn *conn)
                if (ret == 1)                   /* connect_timeout elapsed */
                {
                        /*
-                        * Attempt connection to the next host, ignoring any remaining
-                        * addresses for the current host.
+                        * Give up on current server/address, try the next one.
                         */
-                       conn->try_next_addr = false;
-                       conn->try_next_host = true;
+                       conn->try_next_addr = true;
                        conn->status = CONNECTION_NEEDED;
-
-                       /*
-                        * Restart the connect_timeout timer for the new host.
-                        */
-                       if (timeout > 0)
-                               finish_time = time(NULL) + timeout;
                }
 
                /*