Обсуждение: Improve the granularity of PQsocketPoll's timeout parameter?
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);
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
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
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
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
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
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
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.
*/
* 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;
}
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
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).
+ * 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).
+ * 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;
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;
+ }
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
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
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
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
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.
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
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
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
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
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
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
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
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 of1
is interpreted as2
. This timeout applies separately to each host name or IP address. For example, if you specify two hosts andconnect_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
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
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
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
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