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

----- Forwarded message from Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> -----

I don't know. Would you ask hackers list, and perhaps CC the author of
that patch.

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

Denis A Ustimenko wrote:
> Bruce, why have all precise time calculations been droped out in 1.206? If there is no
> gettimeofday in win32?
>
----- End forwarded message -----

--
Regards
Denis

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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)oldham(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)postgresql(dot)org(dot)

On Mon, Oct 14, 2002 at 09:53:27AM -0400, Tom Lane wrote:
> Denis A Ustimenko <denis(at)oldham(dot)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(at)postgresql(dot)org(dot)
>
> On Mon, Oct 14, 2002 at 09:53:27AM -0400, Tom Lane wrote:
> > Denis A Ustimenko <denis(at)oldham(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)us> writes:
> Denis A Ustimenko wrote:
>> 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.

There is a very simple answer to this, which I think I suggested to Joe
originally, but it's not in the code now: the initial calculation of
finish_time = now() + timeout must add one. This ensures that any
roundoff error is in the conservative direction of timing out later,
rather than sooner.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Denis A Ustimenko wrote:
> >> 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.
>
> There is a very simple answer to this, which I think I suggested to Joe
> originally, but it's not in the code now: the initial calculation of
> finish_time = now() + timeout must add one. This ensures that any
> roundoff error is in the conservative direction of timing out later,
> rather than sooner.

Yes, I saw that and will try to add it back into the code.

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

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
>>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.
>
> There is a very simple answer to this, which I think I suggested to Joe
> originally, but it's not in the code now: the initial calculation of
> finish_time = now() + timeout must add one. This ensures that any
> roundoff error is in the conservative direction of timing out later,
> rather than sooner.

Yes, my bad, I guess.

The thing was that with the extra +1, I was repeatedly getting a wall-clock
time of 2 seconds with a timeout set to 1 second. It seemed odd to have my 1
second timeout automatically turned into 2 seconds every time. With the
current code, I tried a timeout of 1 second at least a 100 times and it always
took about 1 full wall-clock second. But I guess if there is some corner case
that needs it...

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> The thing was that with the extra +1, I was repeatedly getting a wall-clock
> time of 2 seconds with a timeout set to 1 second. It seemed odd to have my 1
> second timeout automatically turned into 2 seconds every time.

That is odd; seems like you should get between 1 and 2 seconds. How
were you measuring the delay, exactly?

regards, tom lane

Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)com> writes:
> > The thing was that with the extra +1, I was repeatedly getting a wall-clock
> > time of 2 seconds with a timeout set to 1 second. It seemed odd to have my 1
> > second timeout automatically turned into 2 seconds every time.
>
> That is odd; seems like you should get between 1 and 2 seconds. How
> were you measuring the delay, exactly?

Remember, that if you add 1, the select() is going to get tv_sec = 2, so
yes, it will be two seconds.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)us> writes:
> Tom Lane wrote:
>> That is odd; seems like you should get between 1 and 2 seconds. How
>> were you measuring the delay, exactly?

> Remember, that if you add 1, the select() is going to get tv_sec = 2, so
> yes, it will be two seconds.

Yeah, but only if the value isn't recalculated shortly later. Consider

caller computes finish_time = time() + timeout;

...

inside select-wait loop, compute max_delay = finish_time - time();

If the time() value has incremented by 1 second between these two lines
of code, you have a problem with a 1-second timeout...

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Tom Lane wrote:
> >> That is odd; seems like you should get between 1 and 2 seconds. How
> >> were you measuring the delay, exactly?
>
> > Remember, that if you add 1, the select() is going to get tv_sec = 2, so
> > yes, it will be two seconds.
>
> Yeah, but only if the value isn't recalculated shortly later. Consider
>
> caller computes finish_time = time() + timeout;
>
> ...
>
> inside select-wait loop, compute max_delay = finish_time - time();
>
> If the time() value has incremented by 1 second between these two lines
> of code, you have a problem with a 1-second timeout...

Yep. If you track finish time, you get that 1 second rounding problem,
and if you track just duration/timeout, you get into the problem of not
knowing when the timeout has ended. I don't think these can be fixed
except by overestimating (+1) or by tracking subseconds along with
seconds so you really know when one second has elapsed.

Perhaps we need to modify a timeout of 1 to be 2 and leave other values
alone.

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

Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)com> writes:
>
>> The thing was that with the extra +1, I was repeatedly getting a
>> wall-clock time of 2 seconds with a timeout set to 1 second. It seemed
>> odd to have my 1 second timeout automatically turned into 2 seconds every
>> time.
>
> That is odd; seems like you should get between 1 and 2 seconds. How were
> you measuring the delay, exactly?

OK. I got a little more scientific about my testing. I used a php script,
running on the same machine, to connect/disconnect in a tight loop and timed
successful and unsuccessful connection attempts using microtime().

Here are the results. First with current cvs code:

current cvs libpq code
-----------------------
good connect info, using unix socket, timeout = 1 second:
=========================================================
unsuccessful 69 times: sum 0.41736388206482: avg 0.0060487519139829
successful 9931 times: sum 68.798981308937: avg 0.0069276992557584

good connect info, using hostaddr, timeout = 1 second
=====================================================
unsuccessful 72 times: sum 0.37020063400269: avg 0.0051416754722595
successful 9928 times: sum 75.047878861427: avg 0.0075592142285886

current cvs libpq code - bad hostaadr, using hostaddr, timeout = 1 second
=========================================================================
unsuccessful 100 times: sum 99.975758910179: avg 0.99975758910179
successful 0 times: sum 0: avg n/a

Clearly not good. The timeout code is causing connection failures about 0.7%
of the time. Next are the results using the attached patch. Per Bruce's
suggestion, it only adds 1 if the timeout is set to 1.

with patch libpq code
---------------------
good connect info, using unix socket, timeout = 1 second
========================================================
unsuccessful 0 times: sum 0: avg n/a
successful 10000 times: sum 68.95981669426: avg 0.006895981669426

with patch libpq code - good connect info, using hostaddr, timeout = 1 second
=============================================================================
unsuccessful 0 times: sum 0: avg n/a
successful 10000 times: sum 73.500863552094: avg 0.0073500863552094

with patch libpq code - good connect info, using hostaddr, timeout = 2 seconds
==============================================================================
unsuccessful 0 times: sum 0: avg n/a
successful 10000 times: sum 73.354710936546: avg 0.0073354710936546

with patch libpq code - bad hostaadr, using hostaddr, timeout = 1 second
========================================================================
unsuccessful 100 times: sum 149.98181843758: avg 1.4998181843758
successful 0 times: sum 0: avg n/a

with patch libpq code - bad hostaadr, using hostaddr, timeout = 2 seconds
=========================================================================
unsuccessful 100 times: sum 149.98445630074: avg 1.4998445630074
successful 0 times: sum 0: avg n/a

with patch libpq code - bad hostaadr, using hostaddr, timeout = 3 seconds
=========================================================================
unsuccessful 20 times: sum 59.842629671097: avg 2.9921314835548
successful 0 times: sum 0: avg n/a

With the patch there were 0 failures on 30000 attempts using good connect
information.

If there are no objections, please apply the attached. Otherwise let me know
if you'd like different tests or would like to try other approaches.

Thanks,

Joe

Joe Conway <mail(at)joeconway(dot)com> writes:
> [ some convincing test cases that timeout=1 is not good ]

> remains.tv_sec = atoi(conn->connect_timeout);
> + if (remains.tv_sec == 1)
> + remains.tv_sec += 1;
> if (!remains.tv_sec)
> {
> conn->status = CONNECTION_BAD;

On pure-paranoia grounds, I'd suggest the logic

+ /* force a sane minimum delay */
+ if (remains.tv_sec < 2)
+ remains.tv_sec = 2;

whereupon you could remove the failure check just below.

regards, tom lane

Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)com> writes:
> > [ some convincing test cases that timeout=1 is not good ]
>
> > remains.tv_sec = atoi(conn->connect_timeout);
> > + if (remains.tv_sec == 1)
> > + remains.tv_sec += 1;
> > if (!remains.tv_sec)
> > {
> > conn->status = CONNECTION_BAD;
>
> On pure-paranoia grounds, I'd suggest the logic
>
> + /* force a sane minimum delay */
> + if (remains.tv_sec < 2)
> + remains.tv_sec = 2;
>
> whereupon you could remove the failure check just below.

I think we should fail if they set the timeout to zero, rather than
cover it up by setting the delay to two.

Attached is a patch that implements most of what we have discussed:

use time()
get rid of timeval where not needed
allow restart of select() to properly compute remaining time
add 1 to timeout == 1
pass finish time to pqWaitTimed

Patch applied. I am applying it so it is in CVS and everyone can see
it. I will keep modifying it until everyone likes it. It is just
easier to do it that way when multiple people are reviewing it. They
can jump in and make changes too.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)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:
> Patch applied. I am applying it so it is in CVS and everyone can see
> it. I will keep modifying it until everyone likes it. It is just
> easier to do it that way when multiple people are reviewing it. They
> can jump in and make changes too.

I ran the same test as before, with the following results:

current cvs
-----------
good connect info, using hostaddr, timeout = 1 second
=====================================================
unsuccessful 0 times: avg n/a
successful 50000 times: avg 0.0087

bad connect info, using hostaddr, timeout = 1 second
====================================================
unsuccessful 100 times: avg 1.4998
successful 0 times: sum 0: avg n/a

Seems to work well. But one slight concern:

with previous 2 line patch
--------------------------
good connect info, using hostaddr, timeout = 1 || 2 second(s)
=============================================================
unsuccessful 0 times: avg n/a
successful 20000 times: avg 0.0074

These tests were on the same, otherwise unloaded development box. Not sure if
it is an artifact or not, but the average connection time has gone from 0.0074
to 0.0087, an increase of about 17%. Also worth noting is that there was very
little deviation from connect-to-connect on both of the tests (eye-balled the
total range at about 0.0003). I did not bother calculating standard deviation
of the connect times, but I'm certain it would not be enough to account for
the difference. Could anything in Bruce's patch account for this, or do you
think it is normal variation due to something on my dev box?

Joe

Joe Conway wrote:
> Seems to work well. But one slight concern:
>
> with previous 2 line patch
> --------------------------
> good connect info, using hostaddr, timeout = 1 || 2 second(s)
> =============================================================
> unsuccessful 0 times: avg n/a
> successful 20000 times: avg 0.0074
>
> These tests were on the same, otherwise unloaded development box. Not sure if
> it is an artifact or not, but the average connection time has gone from 0.0074
> to 0.0087, an increase of about 17%. Also worth noting is that there was very
> little deviation from connect-to-connect on both of the tests (eye-balled the
> total range at about 0.0003). I did not bother calculating standard deviation
> of the connect times, but I'm certain it would not be enough to account for
> the difference. Could anything in Bruce's patch account for this, or do you
> think it is normal variation due to something on my dev box?

Yes, the new code has _three_ time() calls, rather than the old code
that I think only had two. I was going to mention it but I figured
time() was a pretty light system call, sort of like getpid().

I needed the additional time() calls so the computation of remaining
time was more accurate, i.e. we are not resetting the timer on a
select() EINTR anymore.

Should I try to rework it?

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)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:
> Yes, the new code has _three_ time() calls, rather than the old code
> that I think only had two. I was going to mention it but I figured
> time() was a pretty light system call, sort of like getpid().
>
> I needed the additional time() calls so the computation of remaining
> time was more accurate, i.e. we are not resetting the timer on a
> select() EINTR anymore.
>
> Should I try to rework it?
>

I tried two more runs of 10000, and the average is pretty steady at 0.0087.
However the total range is a fair bit wider than I originally reported.

I added a forth time() call to see what the effect would be. It increased the
average to 0.0089 (two runs of 10000 connects each), so I don't think the
time() call explains the entire difference.

Not sure this is worth worrying about or not. I'd guess anyone serious about
keeping connect time to a minimum uses some kind of connection pool or
persistent connection anyway.

Joe

Joe Conway wrote:
> Bruce Momjian wrote:
> > Yes, the new code has _three_ time() calls, rather than the old code
> > that I think only had two. I was going to mention it but I figured
> > time() was a pretty light system call, sort of like getpid().
> >
> > I needed the additional time() calls so the computation of remaining
> > time was more accurate, i.e. we are not resetting the timer on a
> > select() EINTR anymore.
> >
> > Should I try to rework it?
> >
>
> I tried two more runs of 10000, and the average is pretty steady at 0.0087.
> However the total range is a fair bit wider than I originally reported.
>
> I added a forth time() call to see what the effect would be. It increased the
> average to 0.0089 (two runs of 10000 connects each), so I don't think the
> time() call explains the entire difference.
>
> Not sure this is worth worrying about or not. I'd guess anyone serious about
> keeping connect time to a minimum uses some kind of connection pool or
> persistent connection anyway.

Well, the fact you see a change of 0.0002 is significant. Let me add
that in the old code there was only one time() call _in_ the loop, while
now, there are two, so I can easily see there are several additional
time() calls. Did you put your calls in the while loop?

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)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:
> Well, the fact you see a change of 0.0002 is significant. Let me add
> that in the old code there was only one time() call _in_ the loop, while
> now, there are two, so I can easily see there are several additional
> time() calls. Did you put your calls in the while loop?
>

Not the first time, but I moved it around (into the loop in connectDBComplete,
and just before select() in pqWaitTimed) and it didn't seem to make any
difference. Then I removed it entirely again, and still got 0.0089 seconds
average. So, at the least, 0.0002 worth of variation seems to be related to
the development machine itself.

Joe

p.s. The good news is that with tens of thousands more tests at 1 second
timeout, still zero connection failures!

On Mon, Oct 14, 2002 at 01:00:07AM -0400, Bruce Momjian wrote:
> 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:
>

Here is my version of gettimeofday for win32. It was tested with Watcom C 11.0c. I think it can be used. I still belive that fine time calculation is the right way.

#include<stdio.h>
#ifdef _WIN32
#include<winsock.h>
#else
#include<sys/time.h>
#endif

main()
{
struct timeval t;
if (gettimeofday(&t,NULL)) {
printf("error\n\r");
} else {
printf("the time is %ld.%ld\n\r", t.tv_sec, t.tv_usec);
}
fflush(stdout);
}

#ifdef _WIN32
int gettimeofday(struct timeval *tp, void *tzp)
{
FILETIME time;
__int64 tmp;

if ( NULL == tp) return -1;

GetSystemTimeAsFileTime(&time);

tmp = time.dwHighDateTime;
tmp <<= 32;
tmp |= time.dwLowDateTime;
tmp /= 10; // it was in 100 nanosecond periods
tp->tv_sec = tmp / 1000000 - 11644473600L; // Windows Epoch begins at 12:00 AM 01.01.1601
tp->tv_usec = tmp % 1000000;
return 0;
}
#endif

--
Regards
Denis

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Yes, the new code has _three_ time() calls, rather than the old code
> that I think only had two. I was going to mention it but I figured
> time() was a pretty light system call, sort of like getpid().
> I needed the additional time() calls so the computation of remaining
> time was more accurate, i.e. we are not resetting the timer on a
> select() EINTR anymore.

As long as the time() calls aren't invoked in the default no-timeout
case, I doubt that the small additional slowdown matters too much.
Still, one could ask why we are expending extra cycles to make the
timeout more accurate. Who the heck needs an accurate timeout on
connect? Can you really give a use-case where the user won't have
picked a number out of the air anyway?

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Yes, the new code has _three_ time() calls, rather than the old code
> > that I think only had two. I was going to mention it but I figured
> > time() was a pretty light system call, sort of like getpid().
> > I needed the additional time() calls so the computation of remaining
> > time was more accurate, i.e. we are not resetting the timer on a
> > select() EINTR anymore.
>
> As long as the time() calls aren't invoked in the default no-timeout
> case, I doubt that the small additional slowdown matters too much.
> Still, one could ask why we are expending extra cycles to make the
> timeout more accurate. Who the heck needs an accurate timeout on
> connect? Can you really give a use-case where the user won't have
> picked a number out of the air anyway?

I think we do need to properly compute the timeout on an EINTR of
select() because if we don't, a 30 second timeout could become 90
seconds if select() is interrupted. The other time() calls are needed,
one above the loop, and one inside the loop.

The only thing I can do is to pass in the end time _and_ the duration to
pqWaitTimeout and do the time() recomputation only on EINTR. This would
compute duration in the loop where I call time() and therefore elminate
a time() call in the normal, non-EINTR case. Of course, it makes the
code more complicated, and that is why I avoided it.

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

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Yes, the new code has _three_ time() calls, rather than the old code
> > that I think only had two. I was going to mention it but I figured
> > time() was a pretty light system call, sort of like getpid().
> > I needed the additional time() calls so the computation of remaining
> > time was more accurate, i.e. we are not resetting the timer on a
> > select() EINTR anymore.
>
> As long as the time() calls aren't invoked in the default no-timeout
> case, I doubt that the small additional slowdown matters too much.
> Still, one could ask why we are expending extra cycles to make the
> timeout more accurate. Who the heck needs an accurate timeout on
> connect? Can you really give a use-case where the user won't have
> picked a number out of the air anyway?

Yes, the default no-timeout case makes no time() calls.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)us> writes:
> Tom Lane wrote:
>> Still, one could ask why we are expending extra cycles to make the
>> timeout more accurate. Who the heck needs an accurate timeout on
>> connect? Can you really give a use-case where the user won't have
>> picked a number out of the air anyway?

> I think we do need to properly compute the timeout on an EINTR of
> select() because if we don't, a 30 second timeout could become 90
> seconds if select() is interrupted. The other time() calls are needed,
> one above the loop, and one inside the loop.

AFAICS we need one time() call at the start, and then one inside the
select loop. I haven't looked at your recent patches, but you said
something about putting two calls in the loop; that seems like overkill.

regards, tom lane

I will keep this in case we need it later. I think we worked around
this problem by having timeout == 1 set equal to 2 so we get at least
one second for the connection.

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

Denis A Ustimenko wrote:
> On Mon, Oct 14, 2002 at 01:00:07AM -0400, Bruce Momjian wrote:
> > 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:
> >
>
> Here is my version of gettimeofday for win32. It was tested with Watcom C 11.0c. I think it can be used. I still belive that fine time calculation is the right way.
>
> #include<stdio.h>
> #ifdef _WIN32
> #include<winsock.h>
> #else
> #include<sys/time.h>
> #endif
>
> main()
> {
> struct timeval t;
> if (gettimeofday(&t,NULL)) {
> printf("error\n\r");
> } else {
> printf("the time is %ld.%ld\n\r", t.tv_sec, t.tv_usec);
> }
> fflush(stdout);
> }
>
> #ifdef _WIN32
> int gettimeofday(struct timeval *tp, void *tzp)
> {
> FILETIME time;
> __int64 tmp;
>
> if ( NULL == tp) return -1;
>
> GetSystemTimeAsFileTime(&time);
>
> tmp = time.dwHighDateTime;
> tmp <<= 32;
> tmp |= time.dwLowDateTime;
> tmp /= 10; // it was in 100 nanosecond periods
> tp->tv_sec = tmp / 1000000 - 11644473600L; // Windows Epoch begins at 12:00 AM 01.01.1601
> tp->tv_usec = tmp % 1000000;
> return 0;
> }
> #endif
>
>
>
> --
> Regards
> Denis
>

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

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Tom Lane wrote:
> >> Still, one could ask why we are expending extra cycles to make the
> >> timeout more accurate. Who the heck needs an accurate timeout on
> >> connect? Can you really give a use-case where the user won't have
> >> picked a number out of the air anyway?
>
> > I think we do need to properly compute the timeout on an EINTR of
> > select() because if we don't, a 30 second timeout could become 90
> > seconds if select() is interrupted. The other time() calls are needed,
> > one above the loop, and one inside the loop.
>
> AFAICS we need one time() call at the start, and then one inside the
> select loop. I haven't looked at your recent patches, but you said
> something about putting two calls in the loop; that seems like overkill.

Yes, one call at the start, and one in the loop. We need another in
pqWaitTimeout, but only if we hit EINTR. As the code stands now we do
time() unconditionally in pqWaitTimeout too because we only have to pass
in the funish time. If we want to pass in both finish and duration (for
use as select() timeout param), then we can eliminate the time() call in
there for the non-EINTR case. Is it worth the added code complexity?
That is what I am not sure about.

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