Improve the granularity of PQsocketPoll's timeout parameter?

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

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

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

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

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.

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

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 #
<https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNECT-CONNECT-TIMEOUT&gt;

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