Re: Millisecond-precision connect_timeout for libpq

On 9 July 2013 17:59, Markus Wanner <markus@bluegap.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@sss.pgh.pa.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