droped out precise time calculations in src/interfaces/libpq/fe-connect.c

Denis A Ustimenko wrote:

Bruce, why have all precise time calculations been droped out in 1.206? If there is no
gettimeofday in win32?

gettimeofday was not portable to win32 (at least not that I could find) and
hence broke the win32 build of the clients.

Joe

On Sun, Oct 13, 2002 at 09:02:55PM -0700, Joe Conway wrote:

Denis A Ustimenko wrote:

Bruce, why have all precise time calculations been droped out in 1.206?
If there is no
gettimeofday in win32?

gettimeofday was not portable to win32 (at least not that I could find) and
hence broke the win32 build of the clients.

GetSystemTimeAsFileTime should help.

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/sysinfo/base/getsystemtimeasfiletime.asp

--
Regards
Denis

Denis A Ustimenko wrote:

On Sun, Oct 13, 2002 at 09:02:55PM -0700, Joe Conway wrote:

Denis A Ustimenko wrote:

Bruce, why have all precise time calculations been droped out in 1.206?
If there is no
gettimeofday in win32?

gettimeofday was not portable to win32 (at least not that I could find) and
hence broke the win32 build of the clients.

GetSystemTimeAsFileTime should help.

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/sysinfo/base/getsystemtimeasfiletime.asp

It's not clear to me how we could get this into something we can deal
with like gettimeofday.

I looked at the Apache APR project, and they have a routine that returns
the microseconds since 1970 for Unix:

/* NB NB NB NB This returns GMT!!!!!!!!!! */
APR_DECLARE(apr_time_t) apr_time_now(void)
{
struct timeval tv;
gettimeofday(&tv, NULL);
return tv.tv_sec * APR_USEC_PER_SEC + tv.tv_usec;
}

and for Win32:

APR_DECLARE(apr_time_t) apr_time_now(void)
{
LONGLONG aprtime = 0;
FILETIME time;
#ifndef _WIN32_WCE
GetSystemTimeAsFileTime(&time);
#else
SYSTEMTIME st;
GetSystemTime(&st);
SystemTimeToFileTime(&st, &time);
#endif
FileTimeToAprTime(&aprtime, &time);
return aprtime;
}

and FileTimeToAprTime() is:

/* Number of micro-seconds between the beginning of the Windows epoch
* (Jan. 1, 1601) and the Unix epoch (Jan. 1, 1970)
*/
#define APR_DELTA_EPOCH_IN_USEC APR_TIME_C(11644473600000000);

__inline void FileTimeToAprTime(apr_time_t *result, FILETIME *input)
{
/* Convert FILETIME one 64 bit number so we can work with it. */
*result = input->dwHighDateTime;
*result = (*result) << 32;
*result |= input->dwLowDateTime;
*result /= 10; /* Convert from 100 nano-sec periods to micro-seconds. */
*result -= APR_DELTA_EPOCH_IN_USEC; /* Convert from Windows epoch to Unix epoch */
return;
}

So, this is what needs to be dealt with to get it working.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:

So, this is what needs to be dealt with to get it working.

More to the point, why is sub-second precision needed in this function?
Connection timeout is given to us in whole seconds (1.205 code, i.e. prior to
the patch in question):

remains.tv_sec = atoi(conn->connect_timeout);
if (!remains.tv_sec)
{
conn->status = CONNECTION_BAD;
return 0;
}
remains.tv_usec = 0;
rp = &remains;

So there is no way to bail out prior to one second. Once you accept that the
timeout is >= 1 second, and in whole second increments, why does it need
sub-second resolution?

Joe

Joe Conway wrote:

Bruce Momjian wrote:

So, this is what needs to be dealt with to get it working.

More to the point, why is sub-second precision needed in this function?
Connection timeout is given to us in whole seconds (1.205 code, i.e. prior to
the patch in question):

remains.tv_sec = atoi(conn->connect_timeout);
if (!remains.tv_sec)
{
conn->status = CONNECTION_BAD;
return 0;
}
remains.tv_usec = 0;
rp = &remains;

So there is no way to bail out prior to one second. Once you accept that the
timeout is >= 1 second, and in whole second increments, why does it need
sub-second resolution?

It could be argued that our seconds are not as exact as they could be
with subsecond timing. Not sure it is worth it, but I can see the
point. I would like to remove the tv_usec test because it suggests we
are doing something with microseconds when we are not. Also, should we
switch to a simple time() call, rather than gettimeofday()?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:

It could be argued that our seconds are not as exact as they could be
with subsecond timing. Not sure it is worth it, but I can see the
point.

Well, if we were specifying the timeout in microseconds instead of seconds, it
would make sense to have better resolution. But when you can only specify the
timeout in seconds, the internal time comparison doesn't need to be any more
accurate than seconds (IMHO anyway).

are doing something with microseconds when we are not. Also, should we
switch to a simple time() call, rather than gettimeofday()?

Already done -- that's what Denis is unhappy about.

Joe

On Sun, Oct 13, 2002 at 10:59:40PM -0700, Joe Conway wrote:

Well, if we were specifying the timeout in microseconds instead of seconds,
it would make sense to have better resolution. But when you can only
specify the timeout in seconds, the internal time comparison doesn't need
to be any more accurate than seconds (IMHO anyway).

Actually we have the state machine in connectDBComplete() and the timeout is
set for machine as the whole. Therefore if 1 second timeout is seted for the
connectDBComplete() the timeout of particualr iteration of loop can be less
then 1 second.

--
Regards
Denis

Denis A Ustimenko <denis@oldham.ru> writes:

On Sun, Oct 13, 2002 at 10:59:40PM -0700, Joe Conway wrote:

Well, if we were specifying the timeout in microseconds instead of seconds,
it would make sense to have better resolution. But when you can only
specify the timeout in seconds, the internal time comparison doesn't need
to be any more accurate than seconds (IMHO anyway).

Actually we have the state machine in connectDBComplete() and the timeout is
set for machine as the whole. Therefore if 1 second timeout is seted for the
connectDBComplete() the timeout of particualr iteration of loop can be less
then 1 second.

However, the code's been restructured so that we don't need to keep
track of the exact time spent in any one iteration. The error is only
on the overall delay. I agree with Joe that it's not worth the effort
needed (in the Win32 case) to make the timeout accurate to < 1 sec.

regards, tom lane

Joe Conway wrote:

Bruce Momjian wrote:

It could be argued that our seconds are not as exact as they could be
with subsecond timing. Not sure it is worth it, but I can see the
point.

Well, if we were specifying the timeout in microseconds instead of seconds, it
would make sense to have better resolution. But when you can only specify the
timeout in seconds, the internal time comparison doesn't need to be any more
accurate than seconds (IMHO anyway).

are doing something with microseconds when we are not. Also, should we
switch to a simple time() call, rather than gettimeofday()?

Already done -- that's what Denis is unhappy about.

OK, I see that, but now, we are stuffing everything into a timeval
struct. Does that make sense? Shouldn't we just use time_t? I realize
we need the timeval struct for select() in pqWaitTimed, but we are
making a copy of the timeval we pass in anyway. Seems it would be easier
just making it a time_t.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Already done -- that's what Denis is unhappy about.

OK, I see that, but now, we are stuffing everything into a timeval
struct. Does that make sense? Shouldn't we just use time_t?

Yeah, the code could be simplified now. I'm also still not happy about
the question of whether it's safe to assume tv_sec is signed. I think
the running state should be just finish_time, and then inside the
loop when we are about to wait, we could do

current_time = time(NULL);
if (current_time >= finish_time)
{
// failure exit
}
remains.tv_sec = finish_time - current_time;
remains.tv_usec = 0;
// pass &remains to select...

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Already done -- that's what Denis is unhappy about.

OK, I see that, but now, we are stuffing everything into a timeval
struct. Does that make sense? Shouldn't we just use time_t?

Yeah, the code could be simplified now. I'm also still not happy about
the question of whether it's safe to assume tv_sec is signed. I think
the running state should be just finish_time, and then inside the
loop when we are about to wait, we could do

current_time = time(NULL);
if (current_time >= finish_time)
{
// failure exit
}
remains.tv_sec = finish_time - current_time;
remains.tv_usec = 0;
// pass &remains to select...

That whole remains structure should be a time_t variable, and then we
_know_ we can't assume it is signed. The use of timeval should
happen only in pqWaitTimed because it has to use select().

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

That whole remains structure should be a time_t variable, and then we
_know_ we can't assume it is signed. The use of timeval should
happen only in pqWaitTimed because it has to use select().

I think it's fine to use struct timeval as the parameter type for
pqWaitTimed. This particular caller of pqWaitTimed has no need for
sub-second wait precision, but that doesn't mean we might not want it
for other purposes later.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

That whole remains structure should be a time_t variable, and then we
_know_ we can't assume it is signed. The use of timeval should
happen only in pqWaitTimed because it has to use select().

I think it's fine to use struct timeval as the parameter type for
pqWaitTimed. This particular caller of pqWaitTimed has no need for
sub-second wait precision, but that doesn't mean we might not want it
for other purposes later.

That was a question: whether pqWaitTimed() was something exported by
libpq and therefore something that has an API that shouldn't change. I
see it in libpq-int.h, which I think means it isn't exported, but yes,
there could be later cases where we need subsecond stuff.

I have applied the following patch to get us a little closer to sanity.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Oops, overoptimized a little. ptmp_timeout is needed in case no time is
passed; ptmp_timeout restored.

---------------------------------------------------------------------------

pgman wrote:

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

That whole remains structure should be a time_t variable, and then we
_know_ we can't assume it is signed. The use of timeval should
happen only in pqWaitTimed because it has to use select().

I think it's fine to use struct timeval as the parameter type for
pqWaitTimed. This particular caller of pqWaitTimed has no need for
sub-second wait precision, but that doesn't mean we might not want it
for other purposes later.

That was a question: whether pqWaitTimed() was something exported by
libpq and therefore something that has an API that shouldn't change. I
see it in libpq-int.h, which I think means it isn't exported, but yes,
there could be later cases where we need subsecond stuff.

I have applied the following patch to get us a little closer to sanity.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

I have applied the following comment patch. The current code resets the
timer when select() is interruped. On OS's that modify timeout to show
the remaining time, we should be using that value instead of resetting
the timer to its original value on select retry.

---------------------------------------------------------------------------

pgman wrote:

Oops, overoptimized a little. ptmp_timeout is needed in case no time is
passed; ptmp_timeout restored.

---------------------------------------------------------------------------

pgman wrote:

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

That whole remains structure should be a time_t variable, and then we
_know_ we can't assume it is signed. The use of timeval should
happen only in pqWaitTimed because it has to use select().

I think it's fine to use struct timeval as the parameter type for
pqWaitTimed. This particular caller of pqWaitTimed has no need for
sub-second wait precision, but that doesn't mean we might not want it
for other purposes later.

That was a question: whether pqWaitTimed() was something exported by
libpq and therefore something that has an API that shouldn't change. I
see it in libpq-int.h, which I think means it isn't exported, but yes,
there could be later cases where we need subsecond stuff.

I have applied the following patch to get us a little closer to sanity.

-- 
Bruce Momjian                        |  http://candle.pha.pa.us
pgman@candle.pha.pa.us               |  (610) 359-1001
+  If your life is a hard drive,     |  13 Roberts Road
+  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

/*
* select() may modify timeout argument on some platforms so
! * use copy.
! * XXX Do we really want to do that? If select() returns
! * the number of seconds remaining, we are resetting
! * the timeout to its original value. This will yeild
! * incorrect timings when select() is interrupted.
! * bjm 2002-10-14
*/
tmp_timeout = *timeout;
ptmp_timeout = &tmp_timeout;

Actually, now that I look at this, the API for PQwaitTimed is wrong
after all. The right way to implement this is for the caller to pass in
finish_time (or some indication that no timeout is wanted). Evaluation
of the time left to wait should happen inside this retry loop. That
way, you get the right behavior (plus or minus one second, anyway)
independently of whether the platform's select() reduces its timeout
argument or not.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

/*
* select() may modify timeout argument on some platforms so
! * use copy.
! * XXX Do we really want to do that? If select() returns
! * the number of seconds remaining, we are resetting
! * the timeout to its original value. This will yeild
! * incorrect timings when select() is interrupted.
! * bjm 2002-10-14
*/
tmp_timeout = *timeout;
ptmp_timeout = &tmp_timeout;

Actually, now that I look at this, the API for PQwaitTimed is wrong
after all. The right way to implement this is for the caller to pass in
finish_time (or some indication that no timeout is wanted). Evaluation
of the time left to wait should happen inside this retry loop. That
way, you get the right behavior (plus or minus one second, anyway)
independently of whether the platform's select() reduces its timeout
argument or not.

Yes, you are saying do the time() inside PQwaitTimed(), so we can
properly get new time() values on select() retry. Yep.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Tom, excuse me, I forget to copy previous posting to pgsql-hackers@postgresql.org.

On Mon, Oct 14, 2002 at 09:53:27AM -0400, Tom Lane wrote:

Denis A Ustimenko <denis@oldham.ru> writes:

On Sun, Oct 13, 2002 at 10:59:40PM -0700, Joe Conway wrote:

Well, if we were specifying the timeout in microseconds instead of seconds,
it would make sense to have better resolution. But when you can only
specify the timeout in seconds, the internal time comparison doesn't need
to be any more accurate than seconds (IMHO anyway).

Actually we have the state machine in connectDBComplete() and the timeout is
set for machine as the whole. Therefore if 1 second timeout is seted for the
connectDBComplete() the timeout of particualr iteration of loop can be less
then 1 second.

However, the code's been restructured so that we don't need to keep
track of the exact time spent in any one iteration. The error is only
on the overall delay. I agree with Joe that it's not worth the effort
needed (in the Win32 case) to make the timeout accurate to < 1 sec.

Beware of almost 1 second posiible error. For example: connect_timeout == 1,
we start at 0.999999 then finish_time == 1. If CPU is quite busy we will
do only one iteration. I don't know is it enough to make connection?
True timeout in this case == 0.000001

--
Regards
Denis

Denis A Ustimenko wrote:

Tom, excuse me, I forget to copy previous posting to pgsql-hackers@postgresql.org.

On Mon, Oct 14, 2002 at 09:53:27AM -0400, Tom Lane wrote:

Denis A Ustimenko <denis@oldham.ru> writes:

On Sun, Oct 13, 2002 at 10:59:40PM -0700, Joe Conway wrote:

Well, if we were specifying the timeout in microseconds instead of seconds,
it would make sense to have better resolution. But when you can only
specify the timeout in seconds, the internal time comparison doesn't need
to be any more accurate than seconds (IMHO anyway).

Actually we have the state machine in connectDBComplete() and the timeout is
set for machine as the whole. Therefore if 1 second timeout is seted for the
connectDBComplete() the timeout of particualr iteration of loop can be less
then 1 second.

However, the code's been restructured so that we don't need to keep
track of the exact time spent in any one iteration. The error is only
on the overall delay. I agree with Joe that it's not worth the effort
needed (in the Win32 case) to make the timeout accurate to < 1 sec.

Beware of almost 1 second posiible error. For example: connect_timeout == 1,
we start at 0.999999 then finish_time == 1. If CPU is quite busy we will
do only one iteration. I don't know is it enough to make connection?
True timeout in this case == 0.000001

Good question. What is going to happen is that select() is going to be
passed tv_sec = 1, and it is going to sleep for one second. Now, if
select is interrupted, another time() call is going to be made. If the
second ticked just before we run time(), we are going to think one
second has elapsed and return, so in the non-interrupt case, we are
going to be fine, but with select() interrupted, we are going to have
this problem. Now, if we used gettimeofday(), we would be fine, but we
don't have that on Win32 and the ported version of that looks pretty
complicated. Maybe we will go with what we have and see if anyone
complains.

One idea I am looking at is to pass a time_t into pqWaitTimeout, and do
that clock checking in there, and maybe I can do a gettimeofday() for
non-Win32 and a time() on Win32.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073