Re: Improve the granularity of PQsocketPoll's timeout parameter? - Mailing list pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:
> I agree this is not great. I guess I didn't think about it very hard
> because, after all, we were just exposing an API that we'd already
> been using internally. But I think it's reasonable to adjust the API
> to allow for better resolution, as you propose. A second is a very
> long amount of time, and it's entirely reasonable for someone to want
> better granularity.

Here's a v2 responding to some of the comments.

* People pushed back against not using "int64", but the difficulty
with that is that we'd have to #include c.h or at least pg_config.h
in libpq-fe.h, and that would be a totally disastrous invasion of
application namespace.  However, I'd forgotten that libpq-fe.h
does include postgres_ext.h, and there's just enough configure
infrastructure behind that to allow defining pg_int64, which indeed
libpq-fe.h is already relying on.  So we can use that.

* I decided to invent a typedef 

    typedef pg_int64 PGusec_time_t;

instead of writing "pg_int64" explicitly everywhere.  This is perhaps
not as useful as it was when I was thinking the definition would be
"long long int", but it still seems to add some readability.  In my
eyes anyway ... anyone think differently?

* I also undid changes like s/end_time/end_time_us/.  I'd done that
mostly to ensure I looked at/fixed every reference to those variables,
but on reflection I don't think it's doing anything for readability.

* I took Ranier's suggestion to make fast paths for end_time == 0.
I'm not sure this will make any visible performance difference, but
it's simple and shouldn't hurt.  We do have some code paths that use
that behavior.

* Ranier also suggested using clock_gettime instead of gettimeofday,
but I see no reason to do that.  libpq already relies on gettimeofday,
but not on clock_gettime, and anyway post-beta1 isn't a great time to
start experimenting with portability-relevant changes.

            regards, tom lane

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 80179f9978..990c43ec36 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -268,30 +268,52 @@ PGconn *PQsetdb(char *pghost,
       <para>
        <indexterm><primary>nonblocking connection</primary></indexterm>
        Poll a connection's underlying socket descriptor retrieved with <xref linkend="libpq-PQsocket"/>.
+       The primary use of this function is iterating through the connection
+       sequence described in the documentation of
+       <xref linkend="libpq-PQconnectStartParams"/>.
 <synopsis>
-int PQsocketPoll(int sock, int forRead, int forWrite, time_t end_time);
+typedef pg_int64 PGusec_time_t;
+
+int PQsocketPoll(int sock, int forRead, int forWrite,
+                 PGusec_time_t end_time);
 </synopsis>
       </para>

       <para>
-       This function sets up polling of a file descriptor. The underlying function is either
+       This function performs polling of a file descriptor, optionally with
+       a timeout. The underlying function is either
        <function>poll(2)</function> or <function>select(2)</function>, depending on platform
-       support. The primary use of this function is iterating through the connection sequence
-       described in the documentation of <xref linkend="libpq-PQconnectStartParams"/>. If
-       <parameter>forRead</parameter> is specified, the function waits for the socket to be ready
-       for reading. If <parameter>forWrite</parameter> is specified, the function waits for the
-       socket to be ready for write. See <literal>POLLIN</literal> and <literal>POLLOUT</literal>
+       support.
+       If <parameter>forRead</parameter> is nonzero, the
+       function will terminate when the socket is ready for
+       reading. If <parameter>forWrite</parameter> is nonzero,
+       the function will terminate when the
+       socket is ready for writing.
+       See <literal>POLLIN</literal> and <literal>POLLOUT</literal>
        from <function>poll(2)</function>, or <parameter>readfds</parameter> and
-       <parameter>writefds</parameter> from <function>select(2)</function> for more information. If
-       <parameter>end_time</parameter> is not <literal>-1</literal>, it specifies the time at which
-       this function should stop waiting for the condition to be met.
+       <parameter>writefds</parameter> from <function>select(2)</function> for more information.
+      </para>
+
+      <para>
+       The timeout is specified by <parameter>end_time</parameter>, which
+       is the time to stop waiting expressed as a number of microseconds since
+       the Unix epoch (that is, <type>time_t</type> times 1 million).
+       Timeout is infinite if <parameter>end_time</parameter>
+       is <literal>-1</literal>.  Timeout is immediate (no blocking) if
+       end_time is <literal>0</literal> (or indeed, any time before now).
+       Timeout values can be calculated conveniently by adding the desired
+       number of microseconds to the result of
+       <xref linkend="libpq-PQgetCurrentTimeUSec"/>.
+       Note that the underlying system calls may have less than microsecond
+       precision, so that the actual delay may be imprecise.
       </para>

       <para>
        The function returns a value greater than <literal>0</literal> if the specified condition
        is met, <literal>0</literal> if a timeout occurred, or <literal>-1</literal> if an error
        occurred. The error can be retrieved by checking the <literal>errno(3)</literal> value. In
-       the event <literal>forRead</literal> and <literal>forWrite</literal> are not set, the
+       the event both <parameter>forRead</parameter>
+       and <parameter>forWrite</parameter> are zero, the
        function immediately returns a timeout condition.
       </para>
      </listitem>
@@ -8039,6 +8061,25 @@ int PQlibVersion(void);
     </listitem>
    </varlistentry>

+   <varlistentry id="libpq-PQgetCurrentTimeUSec">
+
<term><function>PQgetCurrentTimeUSec</function><indexterm><primary>PQgetCurrentTimeUSec</primary></indexterm></term>
+
+    <listitem>
+     <para>
+      Retrieves the current time, expressed as the number of microseconds
+      since the Unix epoch (that is, <type>time_t</type> times 1 million).
+<synopsis>
+PGusec_time_t PQgetCurrentTimeUSec(void);
+</synopsis>
+     </para>
+
+     <para>
+      This is primarily useful for calculating timeout values to use with
+      <xref linkend="libpq-PQsocketPoll"/>.
+     </para>
+    </listitem>
+   </varlistentry>
+
   </variablelist>

  </sect1>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index fae5940b54..91550a878b 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3764,7 +3764,7 @@ wait_until_connected(PGconn *conn)
     {
         int            rc;
         int            sock;
-        time_t        end_time;
+        PGusec_time_t end_time;

         /*
          * On every iteration of the connection sequence, let's check if the
@@ -3795,7 +3795,7 @@ wait_until_connected(PGconn *conn)
          * solution happens to just be adding a timeout, so let's wait for 1
          * second and check cancel_pressed again.
          */
-        end_time = time(NULL) + 1;
+        end_time = PQgetCurrentTimeUSec() + 1000000;
         rc = PQsocketPoll(sock, forRead, !forRead, end_time);
         if (rc == -1)
             return;
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index 8ee0811510..5d8213e0b5 100644
--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -204,3 +204,4 @@ PQcancelReset             201
 PQcancelFinish            202
 PQsocketPoll              203
 PQsetChunkedRowsMode      204
+PQgetCurrentTimeUSec      205
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 995621b626..cd125fa557 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2470,7 +2470,7 @@ int
 pqConnectDBComplete(PGconn *conn)
 {
     PostgresPollingStatusType flag = PGRES_POLLING_WRITING;
-    time_t        finish_time = ((time_t) -1);
+    PGusec_time_t end_time = -1;
     int            timeout = 0;
     int            last_whichhost = -2;    /* certainly different from whichhost */
     int            last_whichaddr = -2;    /* certainly different from whichaddr */
@@ -2519,7 +2519,7 @@ pqConnectDBComplete(PGconn *conn)
             (conn->whichhost != last_whichhost ||
              conn->whichaddr != last_whichaddr))
         {
-            finish_time = time(NULL) + timeout;
+            end_time = PQgetCurrentTimeUSec() + 1000000 * (PGusec_time_t) timeout;
             last_whichhost = conn->whichhost;
             last_whichaddr = conn->whichaddr;
         }
@@ -2534,7 +2534,7 @@ pqConnectDBComplete(PGconn *conn)
                 return 1;        /* success! */

             case PGRES_POLLING_READING:
-                ret = pqWaitTimed(1, 0, conn, finish_time);
+                ret = pqWaitTimed(1, 0, conn, end_time);
                 if (ret == -1)
                 {
                     /* hard failure, eg select() problem, aborts everything */
@@ -2544,7 +2544,7 @@ pqConnectDBComplete(PGconn *conn)
                 break;

             case PGRES_POLLING_WRITING:
-                ret = pqWaitTimed(0, 1, conn, finish_time);
+                ret = pqWaitTimed(0, 1, conn, end_time);
                 if (ret == -1)
                 {
                     /* hard failure, eg select() problem, aborts everything */
diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index f562cd8d34..99f706e3a9 100644
--- a/src/interfaces/libpq/fe-misc.c
+++ b/src/interfaces/libpq/fe-misc.c
@@ -54,7 +54,7 @@
 static int    pqPutMsgBytes(const void *buf, size_t len, PGconn *conn);
 static int    pqSendSome(PGconn *conn, int len);
 static int    pqSocketCheck(PGconn *conn, int forRead, int forWrite,
-                          time_t end_time);
+                          PGusec_time_t end_time);

 /*
  * PQlibVersion: return the libpq version number
@@ -977,22 +977,25 @@ pqFlush(PGconn *conn)
 int
 pqWait(int forRead, int forWrite, PGconn *conn)
 {
-    return pqWaitTimed(forRead, forWrite, conn, (time_t) -1);
+    return pqWaitTimed(forRead, forWrite, conn, -1);
 }

 /*
- * pqWaitTimed: wait, but not past finish_time.
- *
- * finish_time = ((time_t) -1) disables the wait limit.
+ * pqWaitTimed: wait, but not past end_time.
  *
  * Returns -1 on failure, 0 if the socket is readable/writable, 1 if it timed out.
+ *
+ * The timeout is specified by end_time, which is the int64 number of
+ * microseconds since the Unix epoch (that is, time_t times 1 million).
+ * Timeout is infinite if end_time is -1.  Timeout is immediate (no blocking)
+ * if end_time is 0 (or indeed, any time before now).
  */
 int
-pqWaitTimed(int forRead, int forWrite, PGconn *conn, time_t finish_time)
+pqWaitTimed(int forRead, int forWrite, PGconn *conn, PGusec_time_t end_time)
 {
     int            result;

-    result = pqSocketCheck(conn, forRead, forWrite, finish_time);
+    result = pqSocketCheck(conn, forRead, forWrite, end_time);

     if (result < 0)
         return -1;                /* errorMessage is already set */
@@ -1013,7 +1016,7 @@ pqWaitTimed(int forRead, int forWrite, PGconn *conn, time_t finish_time)
 int
 pqReadReady(PGconn *conn)
 {
-    return pqSocketCheck(conn, 1, 0, (time_t) 0);
+    return pqSocketCheck(conn, 1, 0, 0);
 }

 /*
@@ -1023,7 +1026,7 @@ pqReadReady(PGconn *conn)
 int
 pqWriteReady(PGconn *conn)
 {
-    return pqSocketCheck(conn, 0, 1, (time_t) 0);
+    return pqSocketCheck(conn, 0, 1, 0);
 }

 /*
@@ -1035,7 +1038,7 @@ pqWriteReady(PGconn *conn)
  * for read data directly.
  */
 static int
-pqSocketCheck(PGconn *conn, int forRead, int forWrite, time_t end_time)
+pqSocketCheck(PGconn *conn, int forRead, int forWrite, PGusec_time_t end_time)
 {
     int            result;

@@ -1079,11 +1082,13 @@ pqSocketCheck(PGconn *conn, int forRead, int forWrite, time_t end_time)
  * condition (without waiting).  Return >0 if condition is met, 0
  * if a timeout occurred, -1 if an error or interrupt occurred.
  *
+ * The timeout is specified by end_time, which is the int64 number of
+ * microseconds since the Unix epoch (that is, time_t times 1 million).
  * Timeout is infinite if end_time is -1.  Timeout is immediate (no blocking)
  * if end_time is 0 (or indeed, any time before now).
  */
 int
-PQsocketPoll(int sock, int forRead, int forWrite, time_t end_time)
+PQsocketPoll(int sock, int forRead, int forWrite, PGusec_time_t end_time)
 {
     /* We use poll(2) if available, otherwise select(2) */
 #ifdef HAVE_POLL
@@ -1103,14 +1108,16 @@ PQsocketPoll(int sock, int forRead, int forWrite, time_t end_time)
         input_fd.events |= POLLOUT;

     /* Compute appropriate timeout interval */
-    if (end_time == ((time_t) -1))
+    if (end_time == -1)
         timeout_ms = -1;
+    else if (end_time == 0)
+        timeout_ms = 0;
     else
     {
-        time_t        now = time(NULL);
+        PGusec_time_t now = PQgetCurrentTimeUSec();

         if (end_time > now)
-            timeout_ms = (end_time - now) * 1000;
+            timeout_ms = (end_time - now) / 1000;
         else
             timeout_ms = 0;
     }
@@ -1138,17 +1145,27 @@ PQsocketPoll(int sock, int forRead, int forWrite, time_t end_time)
     FD_SET(sock, &except_mask);

     /* Compute appropriate timeout interval */
-    if (end_time == ((time_t) -1))
+    if (end_time == -1)
         ptr_timeout = NULL;
+    else if (end_time == 0)
+    {
+        timeout.tv_sec = 0;
+        timeout.tv_usec = 0;
+    }
     else
     {
-        time_t        now = time(NULL);
+        PGusec_time_t now = PQgetCurrentTimeUSec();

         if (end_time > now)
-            timeout.tv_sec = end_time - now;
+        {
+            timeout.tv_sec = (end_time - now) / 1000000;
+            timeout.tv_usec = (end_time - now) % 1000000;
+        }
         else
+        {
             timeout.tv_sec = 0;
-        timeout.tv_usec = 0;
+            timeout.tv_usec = 0;
+        }
         ptr_timeout = &timeout;
     }

@@ -1157,6 +1174,21 @@ PQsocketPoll(int sock, int forRead, int forWrite, time_t end_time)
 #endif                            /* HAVE_POLL */
 }

+/*
+ * PQgetCurrentTimeUSec: get current time with microsecond precision
+ *
+ * This provides a platform-independent way of producing a reference
+ * value for PQsocketPoll's timeout parameter.
+ */
+PGusec_time_t
+PQgetCurrentTimeUSec(void)
+{
+    struct timeval tval;
+
+    gettimeofday(&tval, NULL);
+    return (PGusec_time_t) tval.tv_sec * 1000000 + tval.tv_usec;
+}
+

 /*
  * A couple of "miscellaneous" multibyte related functions. They used
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 73f6e65ae5..8620f7d454 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -202,6 +202,9 @@ typedef struct pgNotify
     struct pgNotify *next;        /* list link */
 } PGnotify;

+/* PGusec_time_t is like time_t, but with microsecond resolution */
+typedef pg_int64 PGusec_time_t;
+
 /* Function types for notice-handling callbacks */
 typedef void (*PQnoticeReceiver) (void *arg, const PGresult *res);
 typedef void (*PQnoticeProcessor) (void *arg, const char *message);
@@ -673,7 +676,11 @@ extern int    lo_export(PGconn *conn, Oid lobjId, const char *filename);
 extern int    PQlibVersion(void);

 /* Poll a socket for reading and/or writing with an optional timeout */
-extern int    PQsocketPoll(int sock, int forRead, int forWrite, time_t end_time);
+extern int    PQsocketPoll(int sock, int forRead, int forWrite,
+                         PGusec_time_t end_time);
+
+/* Get current time in the form PQsocketPoll wants */
+extern PGusec_time_t PQgetCurrentTimeUSec(void);

 /* Determine length of multibyte encoded char at *s */
 extern int    PQmblen(const char *s, int encoding);
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 3886204c70..0662c43176 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -755,7 +755,7 @@ extern int    pqReadData(PGconn *conn);
 extern int    pqFlush(PGconn *conn);
 extern int    pqWait(int forRead, int forWrite, PGconn *conn);
 extern int    pqWaitTimed(int forRead, int forWrite, PGconn *conn,
-                        time_t finish_time);
+                        PGusec_time_t end_time);
 extern int    pqReadReady(PGconn *conn);
 extern int    pqWriteReady(PGconn *conn);

diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 4f57078d13..fd2a7661f0 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1831,6 +1831,7 @@ PGresAttValue
 PGresParamDesc
 PGresult
 PGresult_data
+PGusec_time_t
 PIO_STATUS_BLOCK
 PLAINTREE
 PLAssignStmt