Horton

Thread

Commits

  1. Improve the granularity of PQsocketPoll's timeout parameter.

  1. Improve the granularity of PQsocketPoll's timeout parameter?

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-06-10T21:39:35Z

    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

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

    Jeff Davis <pgsql@j-davis.com> — 2024-06-10T23:47:28Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-06-10T23:57:24Z

    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

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

    Jeff Davis <pgsql@j-davis.com> — 2024-06-11T04:34:40Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-06-11T04:52:28Z

    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

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

    Jeff Davis <pgsql@j-davis.com> — 2024-06-11T06:27:30Z

    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

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

    Dominique Devienne <ddevienne@gmail.com> — 2024-06-11T06:55:00Z

    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

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

    Ranier Vilela <ranier.vf@gmail.com> — 2024-06-11T13:01:25Z

    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

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

    Ranier Vilela <ranier.vf@gmail.com> — 2024-06-11T16:34:21Z

    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

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

    Robert Haas <robertmhaas@gmail.com> — 2024-06-11T17:02:55Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-06-12T17:53:13Z

    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

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

    Robert Haas <robertmhaas@gmail.com> — 2024-06-12T17:56:23Z

    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

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

    Ranier Vilela <ranier.vf@gmail.com> — 2024-06-12T18:14:33Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-06-12T18:25:11Z

    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

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

    Robert Haas <robertmhaas@gmail.com> — 2024-06-12T18:45:35Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-06-12T19:00:03Z

    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

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

    Robert Haas <robertmhaas@gmail.com> — 2024-06-12T19:08:14Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-06-12T19:29:40Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-06-12T19:45:19Z

    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

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

    Ranier Vilela <ranier.vf@gmail.com> — 2024-06-13T11:43:50Z

    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>

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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-06-13T15:25:37Z

    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

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

    Ranier Vilela <ranier.vf@gmail.com> — 2024-06-13T16:30:06Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-06-13T19:18:48Z

    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

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

    Dominique Devienne <ddevienne@gmail.com> — 2024-06-14T07:16:25Z

    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