Обсуждение: Improve the granularity of PQsocketPoll's timeout parameter?

Поиск
Список
Период
Сортировка

Improve the granularity of PQsocketPoll's timeout parameter?

От
Tom Lane
Дата:
In [1] Dominique Devienne complained that PQsocketPoll would be
far more useful to him if it had better-than-one-second timeout
resolution.  I initially pushed back on that on the grounds that
post-beta1 is a bit late to be redefining public APIs.  Which it is,
but if we don't fix it now then we'll be stuck supporting that API
indefinitely.  And it's not like one-second resolution is great
for our internal usage either --- for example, I see that psql
is now doing

        end_time = time(NULL) + 1;
        rc = PQsocketPoll(sock, forRead, !forRead, end_time);

which claims to be waiting one second, but actually it's waiting
somewhere between 0 and 1 second.  So I thought I'd look into
whether we can still change it without too much pain, and I think
we can.

The $64 question is how to represent the end_time if not as time_t.
The only alternative POSIX offers AFAIK is gettimeofday's "struct
timeval", which is painful to compute with and I don't think it's
native on Windows.  What I suggest is that we use int64 microseconds
since the epoch, which is the same idea as the backend's TimestampTz
except I think we'd better use the Unix epoch not 2000-01-01.
Then converting code is just a matter of changing variable types
and adding some zeroes to constants.

The next question is how to spell "int64" in libpq-fe.h.  As a
client-exposed header, the portability constraints on it are pretty
stringent, so even in 2024 I'm loath to make it depend on <stdint.h>;
and certainly depending on our internal int64 typedef won't do.
What I did in the attached is to write "long long int", which is
required to be at least 64 bits by C99.  Other opinions are possible
of course.

Lastly, we need a way to get current time in this form.  My first
draft of the attached patch had the callers calling gettimeofday
and doing arithmetic from that, but it seems a lot better to provide
a function that just parallels time(2).

BTW, I think this removes the need for libpq-fe.h to #include <time.h>,
but I didn't remove that because it seems likely that some callers are
indirectly relying on it to be present.  Removing it wouldn't gain
very much anyway.

Thoughts?

            regards, tom lane

[1] https://www.postgresql.org/message-id/CAFCRh-8hf%3D7V8UoF63aLxSkeFmXX8-1O5tRxHL61Pngb7V9rcw%40mail.gmail.com

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 80179f9978..8c0ea444ad 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -268,30 +268,50 @@ 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);
+int PQsocketPoll(int sock, int forRead, int forWrite,
+                 long long int end_time_us);
 </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_us</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_us</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 +8059,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>
+long long int PQgetCurrentTimeUSec(void);
+</synopsis>
+     </para>
+
+     <para>
+      This is especially 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..f717de7a4f 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;
+        long long int end_time_us;

         /*
          * On every iteration of the connection sequence, let's check if the
@@ -3795,8 +3795,8 @@ 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;
-        rc = PQsocketPoll(sock, forRead, !forRead, end_time);
+        end_time_us = PQgetCurrentTimeUSec() + 1000000;
+        rc = PQsocketPoll(sock, forRead, !forRead, end_time_us);
         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 548ad118fb..774a8074fb 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);
+    long long int end_time_us = -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_us = PQgetCurrentTimeUSec() + 1000000LL * 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_us);
                 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_us);
                 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..70a56bbb7c 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);
+                          long long int end_time_us);

 /*
  * 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_us.
  *
  * Returns -1 on failure, 0 if the socket is readable/writable, 1 if it timed out.
+ *
+ * The timeout is specified by end_time_us, 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, long long int end_time_us)
 {
     int            result;

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

     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, long long int end_time_us)
 {
     int            result;

@@ -1058,7 +1061,7 @@ pqSocketCheck(PGconn *conn, int forRead, int forWrite, time_t end_time)

     /* We will retry as long as we get EINTR */
     do
-        result = PQsocketPoll(conn->sock, forRead, forWrite, end_time);
+        result = PQsocketPoll(conn->sock, forRead, forWrite, end_time_us);
     while (result < 0 && SOCK_ERRNO == EINTR);

     if (result < 0)
@@ -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_us, 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, long long int end_time_us)
 {
     /* We use poll(2) if available, otherwise select(2) */
 #ifdef HAVE_POLL
@@ -1103,14 +1108,14 @@ 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_us == -1)
         timeout_ms = -1;
     else
     {
-        time_t        now = time(NULL);
+        long long int now = PQgetCurrentTimeUSec();

-        if (end_time > now)
-            timeout_ms = (end_time - now) * 1000;
+        if (end_time_us > now)
+            timeout_ms = (end_time_us - now) / 1000;
         else
             timeout_ms = 0;
     }
@@ -1138,17 +1143,22 @@ 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_us == -1)
         ptr_timeout = NULL;
     else
     {
-        time_t        now = time(NULL);
+        long long int now = PQgetCurrentTimeUSec();

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

@@ -1157,6 +1167,22 @@ 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.  We assume that "long long"
+ * is at least 64 bits wide on all platforms, as required by C99.
+ */
+long long int
+PQgetCurrentTimeUSec(void)
+{
+    struct timeval tval;
+
+    gettimeofday(&tval, NULL);
+    return (long long int) 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..0d0359175a 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -673,7 +673,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,
+                         long long int end_time_us);
+
+/* Get current time in the form PQsocketPoll wants */
+extern long long int 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..7062998387 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);
+                        long long int end_time_us);
 extern int    pqReadReady(PGconn *conn);
 extern int    pqWriteReady(PGconn *conn);


Re: Improve the granularity of PQsocketPoll's timeout parameter?

От
Jeff Davis
Дата:
On Mon, 2024-06-10 at 17:39 -0400, Tom Lane wrote:
> What I suggest is that we use int64 microseconds
> since the epoch, which is the same idea as the backend's TimestampTz
> except I think we'd better use the Unix epoch not 2000-01-01.
> Then converting code is just a matter of changing variable types
> and adding some zeroes to constants.

...

> Lastly, we need a way to get current time in this form.  My first
> draft of the attached patch had the callers calling gettimeofday
> and doing arithmetic from that, but it seems a lot better to provide
> a function that just parallels time(2).

I briefly skimmed the thread and didn't find the reason why the API
requires an absolute time.

My expectation would be for the last parameter to be a relative timeout
("wait up to X microseconds"). That avoids the annoyance of creating a
new definition of absolute time and exposing a new function to retrieve
it.

Regards,
    Jeff Davis




Re: Improve the granularity of PQsocketPoll's timeout parameter?

От
Tom Lane
Дата:
Jeff Davis <pgsql@j-davis.com> writes:
> I briefly skimmed the thread and didn't find the reason why the API
> requires an absolute time.

Because a common call pattern is to loop around PQsocketPoll calls.
In that scenario you generally want to nail down the timeout time
before starting the loop, not have it silently move forward after
any random event that breaks the current wait (EINTR for example).
pqSocketCheck and pqConnectDBComplete both rely on this.

            regards, tom lane



Re: Improve the granularity of PQsocketPoll's timeout parameter?

От
Jeff Davis
Дата:
On Mon, 2024-06-10 at 19:57 -0400, Tom Lane wrote:
> Because a common call pattern is to loop around PQsocketPoll calls.
> In that scenario you generally want to nail down the timeout time
> before starting the loop, not have it silently move forward after
> any random event that breaks the current wait (EINTR for example).
> pqSocketCheck and pqConnectDBComplete both rely on this.

I agree it makes things easier for a caller following that pattern,
because it doesn't need to recalculate the timeout each time through
the loop.

But:

1. If your clock goes backwards, you can end up waiting for an
arbitrarily long time. To prevent that you need to do some
recalculation each time through the loop anyway.

2. Inventing a new absolute time type just for this single purpose
seems strange to me. Would it be useful in other places? Are we going
to define what kinds of operations/transformations are supported?

3. I can't recall another API that uses absolute time for a timeout;
are you aware of a precedent?

Regards,
    Jeff Davis




Re: Improve the granularity of PQsocketPoll's timeout parameter?

От
Tom Lane
Дата:
Jeff Davis <pgsql@j-davis.com> writes:
> I agree it makes things easier for a caller following that pattern,
> because it doesn't need to recalculate the timeout each time through
> the loop.

> But:

> 1. If your clock goes backwards, you can end up waiting for an
> arbitrarily long time. To prevent that you need to do some
> recalculation each time through the loop anyway.

[ shrug... ] The only reason this has come up is that f5e4dedfa
exposed what was previously just libpq-private code.  Given that
that code has operated in this way for a couple of decades with
approximately zero trouble reports, I'm not very interested in
re-litigating its theory of operation.  The more so if you don't
have a concrete better alternative to propose.

> 2. Inventing a new absolute time type just for this single purpose
> seems strange to me. Would it be useful in other places? Are we going
> to define what kinds of operations/transformations are supported?

I'm not that thrilled with inventing a new time type just for this,
either.  However, time_t is not very fit for purpose, so do you
have a different suggestion?

We could make it a bit nicer-looking by wrapping "long long int"
in a typedef, but that's only cosmetic.

> 3. I can't recall another API that uses absolute time for a timeout;
> are you aware of a precedent?

The other thing that I've seen done is for select(2) to treat the
timeout as an in/out parameter, decrementing it by the amount of
time slept.  I hope you'll agree that that's a monstrous kluge.

            regards, tom lane



Re: Improve the granularity of PQsocketPoll's timeout parameter?

От
Jeff Davis
Дата:
On Tue, 2024-06-11 at 00:52 -0400, Tom Lane wrote:
>
> I'm not that thrilled with inventing a new time type just for this,
> either.  However, time_t is not very fit for purpose, so do you
> have a different suggestion?

No, I don't have a great alternative, so I don't object to your
solutions for f5e4dedfa8.

Regards,
    Jeff Davis




Re: Improve the granularity of PQsocketPoll's timeout parameter?

От
Dominique Devienne
Дата:
On Mon, Jun 10, 2024 at 11:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The next question is how to spell "int64" in libpq-fe.h.

Hi. Out-of-curiosity, I grep'd for it in my 16.1 libpq:

[ddevienne@marsu include]$ grep 'long long' *.h
ecpg_config.h:/* Define to 1 if the system has the type `long long int'. */
ecpg_config.h:/* Define to 1 if `long long int' works and is 64 bits. */
pg_config.h:/* The normal alignment of `long long int', in bytes. */
pg_config.h:/* Define to 1 if `long long int' works and is 64 bits. */
pgtypes_interval.h:typedef long long int int64;

And the relevant snippet of pgtypes_interval.h is:

#ifdef HAVE_LONG_INT_64
#ifndef HAVE_INT64
typedef long int int64;
#endif
#elif defined(HAVE_LONG_LONG_INT_64)
#ifndef HAVE_INT64
typedef long long int int64;
#endif
#else
/* neither HAVE_LONG_INT_64 nor HAVE_LONG_LONG_INT_64 */
#error must have a working 64-bit integer datatype
#endif

Given this precedent, can't the same be done?

And if a 64-bit integer is too troublesome, why not just two 32-bit
parameters instead?
Either a (time_t + int usec), microsecond offset, clamped to [0, 1M),
or (int sec + int usec)?

I'm fine with any portable solution that allows sub-second timeouts, TBH.
Just thinking aloud here. Thanks, --DD



Re: Improve the granularity of PQsocketPoll's timeout parameter?

От
Ranier Vilela
Дата:


Em seg., 10 de jun. de 2024 às 18:39, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
In [1] Dominique Devienne complained that PQsocketPoll would be
far more useful to him if it had better-than-one-second timeout
resolution.  I initially pushed back on that on the grounds that
post-beta1 is a bit late to be redefining public APIs.  Which it is,
but if we don't fix it now then we'll be stuck supporting that API
indefinitely.  And it's not like one-second resolution is great
for our internal usage either --- for example, I see that psql
is now doing

                end_time = time(NULL) + 1;
                rc = PQsocketPoll(sock, forRead, !forRead, end_time);

which claims to be waiting one second, but actually it's waiting
somewhere between 0 and 1 second.  So I thought I'd look into
whether we can still change it without too much pain, and I think
we can.

The $64 question is how to represent the end_time if not as time_t.
The only alternative POSIX offers AFAIK is gettimeofday's "struct
timeval", which is painful to compute with and I don't think it's
native on Windows.  What I suggest is that we use int64 microseconds
since the epoch, which is the same idea as the backend's TimestampTz
except I think we'd better use the Unix epoch not 2000-01-01.
Then converting code is just a matter of changing variable types
and adding some zeroes to constants.

The next question is how to spell "int64" in libpq-fe.h.  As a
client-exposed header, the portability constraints on it are pretty
stringent, so even in 2024 I'm loath to make it depend on <stdint.h>;
and certainly depending on our internal int64 typedef won't do.
What I did in the attached is to write "long long int", which is
required to be at least 64 bits by C99.  Other opinions are possible
of course.

Lastly, we need a way to get current time in this form.  My first
draft of the attached patch had the callers calling gettimeofday
and doing arithmetic from that, but it seems a lot better to provide
a function that just parallels time(2).

BTW, I think this removes the need for libpq-fe.h to #include <time.h>,
but I didn't remove that because it seems likely that some callers are
indirectly relying on it to be present.  Removing it wouldn't gain
very much anyway.

Thoughts?
Hi Tom.

Why not use uint64?
I think it's available in (fe-misc.c)

IMO, gettimeofday It also seems to me that it is deprecated.

Can I suggest a version using *clock_gettime*, 
which I made based on versions available on the web?

/*
 * PQgetCurrentTimeUSec: get current time with nanosecond precision
 *
 * This provides a platform-independent way of producing a reference
 * value for PQsocketPoll's timeout parameter.
 */

uint64
PQgetCurrentTimeUSec(void)
{
#ifdef __MACH__
struct timespec ts;
clock_serv_t cclock;
mach_timespec_t mts;

host_get_clock_service(mach_host_self(), SYSTEM_CLOCK, &cclock);
clock_get_time(cclock, &mts);
mach_port_deallocate(mach_task_self(), cclock);
ts.tv_sec = mts.tv_sec;
ts.tv_nsec = mts.tv_nsec;
#eldef _WIN32_
struct timespec ts { long tv_sec; long tv_nsec; };
__int64 wintime;

GetSystemTimeAsFileTime((FILETIME*) &wintime);
wintime   -= 116444736000000000i64;             // 1jan1601 to 1jan1970
ts.tv_sec  = wintime / 10000000i64;             // seconds
ts.tv_nsec = wintime % 10000000i64 * 100;      // nano-seconds
#else
struct timespec ts;

clock_gettime(CLOCK_MONOTONIC, &ts);
#endif

return (ts.tv_sec * 1000000000L) + ts.tv_nsec;
}

best regards,
Ranier Vilela

Re: Improve the granularity of PQsocketPoll's timeout parameter?

От
Ranier Vilela
Дата:
Em seg., 10 de jun. de 2024 às 18:39, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
In [1] Dominique Devienne complained that PQsocketPoll would be
far more useful to him if it had better-than-one-second timeout
resolution.  I initially pushed back on that on the grounds that
post-beta1 is a bit late to be redefining public APIs.  Which it is,
but if we don't fix it now then we'll be stuck supporting that API
indefinitely.  And it's not like one-second resolution is great
for our internal usage either --- for example, I see that psql
is now doing

                end_time = time(NULL) + 1;
                rc = PQsocketPoll(sock, forRead, !forRead, end_time);

which claims to be waiting one second, but actually it's waiting
somewhere between 0 and 1 second.  So I thought I'd look into
whether we can still change it without too much pain, and I think
we can.

The $64 question is how to represent the end_time if not as time_t.
The only alternative POSIX offers AFAIK is gettimeofday's "struct
timeval", which is painful to compute with and I don't think it's
native on Windows.  What I suggest is that we use int64 microseconds
since the epoch, which is the same idea as the backend's TimestampTz
except I think we'd better use the Unix epoch not 2000-01-01.
Then converting code is just a matter of changing variable types
and adding some zeroes to constants.

The next question is how to spell "int64" in libpq-fe.h.  As a
client-exposed header, the portability constraints on it are pretty
stringent, so even in 2024 I'm loath to make it depend on <stdint.h>;
and certainly depending on our internal int64 typedef won't do.
What I did in the attached is to write "long long int", which is
required to be at least 64 bits by C99.  Other opinions are possible
of course.

Lastly, we need a way to get current time in this form.  My first
draft of the attached patch had the callers calling gettimeofday
and doing arithmetic from that, but it seems a lot better to provide
a function that just parallels time(2).

BTW, I think this removes the need for libpq-fe.h to #include <time.h>,
but I didn't remove that because it seems likely that some callers are
indirectly relying on it to be present.  Removing it wouldn't gain
very much anyway.

Thoughts?
Regarding your patch:

1. I think can remove *int64* in comments:
+ * The timeout is specified by end_time_us, which is the 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).

+ * The timeout is specified by end_time_us, which is the number of
+ * microseconds since the Unix epoch (that is, time_t times 1 million).

2. I think it's worth testing whether end_time_ns equals zero,
which can avoid a call to PQgetCurrentTimeNSec()

@@ -1103,14 +1113,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_ns == -1)
  timeout_ms = -1;
+ else if (end_time_ns == 0)
+ timeout_ms = 0;

3. I think it's worth testing whether end_time_ns equals zero,
which can avoid a call to PQgetCurrentTimeNSec() 

@@ -1138,17 +1150,29 @@ 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_ns == -1)
  ptr_timeout = NULL;
+ else if (end_time_ns == 0)
+ {
+ timeout.tv_sec = 0;
+ timeout.tv_usec = 0;
+
+ ptr_timeout = &timeout;
+ }

best regards,
Ranier Vilela

Re: Improve the granularity of PQsocketPoll's timeout parameter?

От
Robert Haas
Дата:
On Mon, Jun 10, 2024 at 5:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> In [1] Dominique Devienne complained that PQsocketPoll would be
> far more useful to him if it had better-than-one-second timeout
> resolution.  I initially pushed back on that on the grounds that
> post-beta1 is a bit late to be redefining public APIs.  Which it is,
> but if we don't fix it now then we'll be stuck supporting that API
> indefinitely.  And it's not like one-second resolution is great
> for our internal usage either --- for example, I see that psql
> is now doing
>
>                 end_time = time(NULL) + 1;
>                 rc = PQsocketPoll(sock, forRead, !forRead, end_time);

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.

--
Robert Haas
EDB: http://www.enterprisedb.com



Re: Improve the granularity of PQsocketPoll's timeout parameter?

От
Tom Lane
Дата:
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

Re: Improve the granularity of PQsocketPoll's timeout parameter?

От
Robert Haas
Дата:
On Wed, Jun 12, 2024 at 1:53 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> * 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 don't think it's a bad idea to have a typedef, but that particular
one is pretty unreadable. Mmm, let's separate some things with
underscores and others by a change in the capitalization conventIon!

I assume you're following an existing convention and therefore this is
the Right Thing To Do, but if there's some other approach that is less
like line noise, that would be great.

--
Robert Haas
EDB: http://www.enterprisedb.com



Re: Improve the granularity of PQsocketPoll's timeout parameter?

От
Ranier Vilela
Дата:
Em qua., 12 de jun. de 2024 às 14:53, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
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;
Perhaps pg_timeusec?
I think it combines with PQgetCurrentTimeUSec
 

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.
end_time seems much better to me.
 

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

* 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.
I agree.
For v18, it would be a case of thinking about not using it anymore
gettimeofday, as it appears to be deprecated. 

best regards,
Ranier Vilela

Re: Improve the granularity of PQsocketPoll's timeout parameter?

От
Tom Lane
Дата:
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Jun 12, 2024 at 1:53 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> * I decided to invent a typedef
>> typedef pg_int64 PGusec_time_t;

> I don't think it's a bad idea to have a typedef, but that particular
> one is pretty unreadable. Mmm, let's separate some things with
> underscores and others by a change in the capitalization conventIon!

"PG" as a prefix for typedefs in libpq-fe.h is a pretty ancient
precedent.  I'm not wedded to any of the rest of it --- do you
have a better idea?

            regards, tom lane



Re: Improve the granularity of PQsocketPoll's timeout parameter?

От
Robert Haas
Дата:
On Wed, Jun 12, 2024 at 2:25 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Wed, Jun 12, 2024 at 1:53 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> * I decided to invent a typedef
> >> typedef pg_int64 PGusec_time_t;
>
> > I don't think it's a bad idea to have a typedef, but that particular
> > one is pretty unreadable. Mmm, let's separate some things with
> > underscores and others by a change in the capitalization conventIon!
>
> "PG" as a prefix for typedefs in libpq-fe.h is a pretty ancient
> precedent.  I'm not wedded to any of the rest of it --- do you
> have a better idea?

Hmm, well, one thing I notice is that most of the other typedefs in
src/interfaces/libpq seem to do PGWordsLikeThis or PGwordsLikeThis
rather than PGwords_like_this. There are a few that randomly do
pg_words_like_this, too. But I know of no specific precedent for how a
microsecond type should be named.

--
Robert Haas
EDB: http://www.enterprisedb.com



Re: Improve the granularity of PQsocketPoll's timeout parameter?

От
Tom Lane
Дата:
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Jun 12, 2024 at 2:25 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> "PG" as a prefix for typedefs in libpq-fe.h is a pretty ancient
>> precedent.  I'm not wedded to any of the rest of it --- do you
>> have a better idea?

> Hmm, well, one thing I notice is that most of the other typedefs in
> src/interfaces/libpq seem to do PGWordsLikeThis or PGwordsLikeThis
> rather than PGwords_like_this. There are a few that randomly do
> pg_words_like_this, too. But I know of no specific precedent for how a
> microsecond type should be named.

Hmm ... pg_int64 is the only such typedef I'm seeing in that file.
But okay, it's a precedent.  The thing I'm having difficulty with
is that I'd like the typedef name to allude to time_t, and I don't
think fooling with the casing of that will be helpful in making
the allusion stick.  So how about one of

    pg_usec_time_t
    pg_time_t_usec

?

            regards, tom lane



Re: Improve the granularity of PQsocketPoll's timeout parameter?

От
Robert Haas
Дата:
On Wed, Jun 12, 2024 at 3:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Hmm ... pg_int64 is the only such typedef I'm seeing in that file.

I grepped the whole directory for '^} '.

> But okay, it's a precedent.  The thing I'm having difficulty with
> is that I'd like the typedef name to allude to time_t, and I don't
> think fooling with the casing of that will be helpful in making
> the allusion stick.  So how about one of
>
>         pg_usec_time_t
>         pg_time_t_usec
>
> ?

The former seems better to me, since having _t not at the end does not
seem too intuitive.

--
Robert Haas
EDB: http://www.enterprisedb.com



Re: Improve the granularity of PQsocketPoll's timeout parameter?

От
Tom Lane
Дата:
Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Jun 12, 2024 at 3:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> So how about one of
>>     pg_usec_time_t
>>     pg_time_t_usec
>> ?

> The former seems better to me, since having _t not at the end does not
> seem too intuitive.

True.  We can guess about how POSIX might spell this if they ever
invent the concept, but one choice they certainly would not make
is time_t_usec.

v3 attached uses pg_usec_time_t, and fixes one brown-paper-bag
bug the cfbot noticed in v2.

            regards, tom lane

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 80179f9978..814c49bdd1 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 pg_usec_time_t;
+
+int PQsocketPoll(int sock, int forRead, int forWrite,
+                 pg_usec_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>
+pg_usec_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..180781ecd0 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;
+        pg_usec_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..19d067f4a2 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);
+    pg_usec_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() + (pg_usec_time_t) timeout * 1000000;
             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..f235bfbb41 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);
+                          pg_usec_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, pg_usec_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, pg_usec_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, pg_usec_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);
+        pg_usec_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,28 @@ 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;
+        ptr_timeout = &timeout;
+    }
     else
     {
-        time_t        now = time(NULL);
+        pg_usec_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 +1175,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.
+ */
+pg_usec_time_t
+PQgetCurrentTimeUSec(void)
+{
+    struct timeval tval;
+
+    gettimeofday(&tval, NULL);
+    return (pg_usec_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..67a0aad40d 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;

+/* pg_usec_time_t is like time_t, but with microsecond resolution */
+typedef pg_int64 pg_usec_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,
+                         pg_usec_time_t end_time);
+
+/* Get current time in the form PQsocketPoll wants */
+extern pg_usec_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..f36d76bf3f 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);
+                        pg_usec_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..61ad417cde 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -3717,6 +3717,7 @@ pg_unicode_normprops
 pg_unicode_properties
 pg_unicode_range
 pg_unicode_recompinfo
+pg_usec_time_t
 pg_utf_to_local_combined
 pg_uuid_t
 pg_wc_probefunc

Re: Improve the granularity of PQsocketPoll's timeout parameter?

От
Tom Lane
Дата:
I wrote:
> v3 attached uses pg_usec_time_t, and fixes one brown-paper-bag
> bug the cfbot noticed in v2.

Oh, I just remembered that there's a different bit of
pqConnectDBComplete that we could simplify now:

        if (timeout > 0)
        {
            /*
             * 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;
        }
        else                    /* negative means 0 */
            timeout = 0;

With this infrastructure, there's no longer any need to discriminate
against timeout == 1 second, so we might as well reduce this to

        if (timeout < 0)
            timeout = 0;

as it's done elsewhere.

            regards, tom lane



Re: Improve the granularity of PQsocketPoll's timeout parameter?

От
Ranier Vilela
Дата:
Em qua., 12 de jun. de 2024 às 16:45, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
I wrote:
> v3 attached uses pg_usec_time_t, and fixes one brown-paper-bag
> bug the cfbot noticed in v2.

Oh, I just remembered that there's a different bit of
pqConnectDBComplete that we could simplify now:

        if (timeout > 0)
        {
            /*
             * 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;
        }
        else                    /* negative means 0 */
            timeout = 0;

With this infrastructure, there's no longer any need to discriminate
against timeout == 1 second, so we might as well reduce this to

        if (timeout < 0)
            timeout = 0;

as it's done elsewhere.
I'm unsure if the documentation matches the code?
"
connect_timeout #

Maximum time to wait while connecting, in seconds (write as a decimal integer, e.g., 10). Zero, negative, or not specified means wait indefinitely. The minimum allowed timeout is 2 seconds, therefore a value of 1 is interpreted as 2. This timeout applies separately to each host name or IP address. For example, if you specify two hosts and connect_timeout 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 up to 10 seconds.

"
The comments says that timeout = 0, means *Timeout is immediate (no blocking)*

Does the word "indefinitely" mean infinite?
If yes, connect_timeout = -1, mean infinite?

best regards,
Ranier Vilela

Re: Improve the granularity of PQsocketPoll's timeout parameter?

От
Tom Lane
Дата:
Ranier Vilela <ranier.vf@gmail.com> writes:
> I'm unsure if the documentation matches the code?
> " connect_timeout #
> <https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNECT-CONNECT-TIMEOUT>

> Maximum time to wait while connecting, in seconds (write as a decimal
> integer, e.g., 10). Zero, negative, or not specified means wait
> indefinitely. The minimum allowed timeout is 2 seconds, therefore a value
> of 1 is interpreted as 2. This timeout applies separately to each host name
> or IP address. For example, if you specify two hosts and connect_timeout 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 up to 10 seconds.
> "
> The comments says that timeout = 0, means *Timeout is immediate (no
> blocking)*

> Does the word "indefinitely" mean infinite?
> If yes, connect_timeout = -1, mean infinite?

The sentence about "minimum allowed timeout is 2 seconds" has to go
away, but the rest of that seems fine.

But now that you mention it, we could drop the vestigial

>>         if (timeout < 0)
>>             timeout = 0;

as well, because the rest of the function only applies the timeout
when "timeout > 0".  Otherwise end_time (nee finish_time) stays at -1.

            regards, tom lane



Re: Improve the granularity of PQsocketPoll's timeout parameter?

От
Ranier Vilela
Дата:
Em qui., 13 de jun. de 2024 às 12:25, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
Ranier Vilela <ranier.vf@gmail.com> writes:
> I'm unsure if the documentation matches the code?
> " connect_timeout #
> <https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNECT-CONNECT-TIMEOUT>

> Maximum time to wait while connecting, in seconds (write as a decimal
> integer, e.g., 10). Zero, negative, or not specified means wait
> indefinitely. The minimum allowed timeout is 2 seconds, therefore a value
> of 1 is interpreted as 2. This timeout applies separately to each host name
> or IP address. For example, if you specify two hosts and connect_timeout 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 up to 10 seconds.
> "
> The comments says that timeout = 0, means *Timeout is immediate (no
> blocking)*

> Does the word "indefinitely" mean infinite?
> If yes, connect_timeout = -1, mean infinite?

The sentence about "minimum allowed timeout is 2 seconds" has to go
away, but the rest of that seems fine.

But now that you mention it, we could drop the vestigial

>>         if (timeout < 0)
>>             timeout = 0;

as well, because the rest of the function only applies the timeout
when "timeout > 0".  Otherwise end_time (nee finish_time) stays at -1.
I think that's OK Tom.

+1 for push.

best regards,
Ranier Vilela

Re: Improve the granularity of PQsocketPoll's timeout parameter?

От
Tom Lane
Дата:
Ranier Vilela <ranier.vf@gmail.com> writes:
> +1 for push.

Done.  I noticed in final review that libpq-fe.h's "#include <time.h>",
which I'd feared to remove because I thought we'd shipped that
already, actually was new in f5e4dedfa.  So there shouldn't be
anything depending on it, and I thought it best to take it out again.
Widely-used headers shouldn't have unnecessary inclusions.

            regards, tom lane



Re: Improve the granularity of PQsocketPoll's timeout parameter?

От
Dominique Devienne
Дата:
On Thu, Jun 13, 2024 at 9:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Ranier Vilela <ranier.vf@gmail.com> writes:
> > +1 for push.
>
> Done.  [...]

Thanks a lot Tom (and reviewers)! --DD