Re: Millisecond-precision connect_timeout for libpq

On 9 July 2013 17:59, Markus Wanner <markus(at)bluegap(dot)ch> wrote:
> Ian,
>
> On 07/05/2013 07:28 PM, ivan babrou wrote:
>> - /*
>> - * Rounding could cause connection to fail; need at least 2 secs
>> - */
>
> You removed this above comment... please check why it's there. The
> relevant revision seems to be:
>
> ###
> commit 2908a838ac2cf8cdccaa115249f8399eef8a731e
> Author: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
> Date: Thu Oct 24 23:35:55 2002 +0000

That's not correct, facb72007 is the relevant revision. It seems that
it's only applicable for small timeouts in seconds, but it you request
connect timeout in 1 ms you should be ready to fail. I may be wrong
about this, Bruce Momjian introduced that change in 2002.

> Code review for connection timeout patch. Avoid unportable assumption
> that tv_sec is signed; return a useful error message on timeout failure;
> honor PGCONNECT_TIMEOUT environment variable in PQsetdbLogin; make code
> obey documentation statement that timeout=0 means no timeout.
> ###
>
>> - if (timeout < 2)
>> - timeout = 2;
>> - /* calculate the finish time based on start + timeout */
>> - finish_time = time(NULL) + timeout;
>> + gettimeofday(&finish_time, NULL);
>> + finish_time.tv_usec += (int) timeout_usec;
>
> I vaguely recall tv_usec only being required to hold values up to
> 1000000 by some standard. A signed 32 bit value would qualify, but only
> hold up to a good half hour worth of microseconds. That doesn't quite
> seem enough to calculate finish_time the way you are proposing to do it.

Agree, this should be fixed.

>> + finish_time.tv_sec += finish_time.tv_usec / 1000000;
>> + finish_time.tv_usec = finish_time.tv_usec % 1000000;
>> }
>> }
>>
>> @@ -1073,15 +1074,15 @@ 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 == NULL)
>> timeout_ms = -1;
>> else
>> {
>> - time_t now = time(NULL);
>> + struct timeval now;
>> + gettimeofday(&now, NULL);
>>
>> - if (end_time > now)
>> - timeout_ms = (end_time - now) * 1000;
>> - else
>> + timeout_ms = (end_time->tv_sec - now.tv_sec) * 1000 + (end_time->tv_usec - now.tv_usec) / 1000;
>
> I think that's incorrect on a platform where tv_sec and/or tv_usec is
> unsigned. (And the cited commit above indicates there are such platforms.)

I don't get it. timeout_ms is signed, and can hold unsigned -
unsigned. Is it about anything else?

> On 07/09/2013 02:25 PM, ivan babrou wrote:
>> There's no complexity here :)
>
> Not so fast, cowboy... :-)
>
> Regards
>
> Markus Wanner

Is there anything else I should fix?

--
Regards, Ian Babrou
http://bobrik.name http://twitter.com/ibobrik skype:i.babrou