FreeTDS Development Group

[Frediano Ziglio <freddyz77 AT tin.it>]

Il mar, 2004-06-01 alle 07:36, James K. Lowden ha scritto:
> I thought I'd try to implement dbsettime().  It's not simple, for a few
> reasons.  One, I'm not all that familiar with network programming a la
> Unix.  Two, it doesn't take a dbproc as an argument: the documentation
> says 
> 
>       "dbsettime can be called at any time during the application--before or
> after a call to dbopen. It takes effect immediately upon being called."  
> 
> Because we maintain per-connection timeouts (in TDSSOCKET), that implies
> that dbsettime() has to:
>       
> 1.  set a new default for future connections.  
> 2.  iterate over the open connections, adjusting each one's timeout.  
> 
> (a bit tricky in a multithreaded setting).  
> 
> That brings me to the third reason it's not simple: the number of timeouts
> in TDSCONNECTION.  We have:
> 
> $ sed -ne'/tds_connection$/,/TDSCONNECTION/p' include/tds.h |grep timeout 
> 
>         TDS_INT connect_timeout;
>         TDS_INT query_timeout;
>         TDS_INT longquery_timeout;
>         int timeout;
> 
> I think longquery_timeout is an ODBC thing that we can ignore for now
> (although I haven't found anything in the ODBC spec about a "longquery"
> timeout, either).  
> 
> The real confusion is in how the others interact, not least because there
> are 3 variables to control 2 things.  
> 
> To be absolutely clear, let us define our terms:
> 
> 1.  Connection timeout.  How long we'll wait for the server to acknowledge
> the formation of a connection.  E.g, how long dbopen() will wait for an
> answer.   
> 2.  Query timeout.  How long we'll wait for a response to a query.  
> 
> The code
> --------
> 
> For starters, in config.c, the TDSCONNECTION members are set thus:
> 
>       } else if (!strcmp(option, TDS_STR_TIMEOUT)) {
>               if (atoi(value))
>                       connection->timeout = atoi(value);
>       } else if (!strcmp(option, TDS_STR_CONNTIMEOUT)) {
>               if (atoi(value))
>                       connection->connect_timeout = atoi(value);
> 
> So, 'query_timeout' is not initialized from freetds.conf, but 'timeout'
> is.  
> 
> In login.c, we have this weird line:
> 
>       tds->timeout = (connect_timeout) ? connection->query_timeout : 0;
> 
> Weird, first, because nowhere that I know of do we say "if you indicate a
> query timeout, it won't be honored unless you indicate a connection
> timeout, too.  But, if you set a timeout in freetds.conf and *don't* set a
> connection timeout, then the query timeout will will work."  
> 
> Weird, second, because afaict that's the only place TDSSOCKET::timeout is
> set, and that's the value read.c::goodread() relies on.   
> 
> Weird, third, because again afaict connection->query_timeout is used
> otherwise only in odbc.c::_SQLExecute to set tds->longquery_timeout.  I
> don't see it referenced anywhere else in libtds.  
> 
> Also in login.c, we have a function tds_set_timeouts().  No one calls it. 
> 
> 
> Proposal
> --------
> 
> I like the name 'query_timeout' better than 'timeout'.  It's pretty clear,
> though, that 'timeout' is older and is used more extensively (but not
> quite correctly).  I suggest we:
> 
> 1.  Delete 'query_timeout' from the structures, shifting all references to
> use 'timeout' instead.   It looks to me like that's the intent anyway.  
> 
> 2.  Fix login.c.  Use 'connect_timeout' for the initial connection, and
> initialize 'timeout' according to the default from freetds.conf or e.g.
> dbsettime().  
> 

That's not use connect_timeout to init timeout and query_timeout.
Agreed. connect_timeout it's only used to timeout connect() call. The
only reason to use connect_timeout to init timeout it's to implement a
login_timeout (required by ODBC and dblib...). Was this the original
intent ??

> 3.  Rename 'timeout' to 'query_timeout'.  Add "query timeout" as a
> freetds.conf entry, while retaining "timeout" as an undocumented (but
> working) alias.  
> 

?? why delete query_timeout than use timeout than rename timeout to
query_timeout ??

> 4.  Delete tds_set_timeouts().  Add dbsettime().  
> 

Agreed.

> For the record, Microsoft defines DBSETLTIME(), a per-login connect
> timeout.  We don't support it, and no one's asked for it.  To support it,
> tdsdbopen() would need to know to preserve the TDSLOGIN::timeout when
> reading freetds.conf.  
> 
> I'm especially interested in any corrections to my understanding.  All
> suggestions welcome, naturally.  
> 

Cfr also same subject thread ("timeout") on ML (January)

Using a grep

$ grep -rw query_timeout *
include/tds.h:  TDS_INT query_timeout;
include/tds.h:  TDS_INT query_timeout;
include/tdsodbc.h:      SQLUINTEGER query_timeout;
src/odbc/odbc.c:        stmt->attr.query_timeout = 0;
src/odbc/odbc.c:        tds->longquery_timeout =
stmt->attr.query_timeout;
src/odbc/odbc.c:                size = sizeof(stmt->attr.query_timeout);
src/odbc/odbc.c:                src = &stmt->attr.query_timeout;
src/odbc/odbc.c:                stmt->attr.query_timeout = ui;
Il file binario src/dblib/unittests/data.bin corrisponde
src/tds/config.c:       connection->query_timeout =
login->query_timeout;
src/tds/login.c:        tds->timeout = (connect_timeout) ?
connection->query_timeout : 0;

as you can see query_timeout is not used. Well.. it's used to init
tds->timeout in login.c... Perhaps should longquery_timeout and
query_timeout be the same thing ??

The key in code it's goodread (src/tds/read.c, cfr "goodread... not so
fine" on ML). 
connect_timeout behavior it's clear (if we are not able to connect to
server in X seconds return connection fails). I don't understand why in
login.c we set timeout and longquery_timeout based on connect_timeout...
timeout is used in read.c to do polling (using chkintr and hndlintr
fields of TDSSOCKET) and in write.c to check if server it's reading our
data. Perhaps a write_timeout would help making things more clear ??
query_timeout not used :(
longquery_timeout it's used in goodread to break loop. However in
tds_read_packet if we get some bytes from goodread we continue calling
goodread... this IMHO lead to strange behavior and timeouts. Assuming
server returns a row every 10 seconds, timeout = 12 and
longquery_timeout = 120 even if we get 1000000 rows libTDS continue to
get data... Is this expected ?? Also if goodread (in tds_read_packet)
return <1 we close connection... opinable things... perhaps client wants
simply to cancel is a timeout is reached... however how to signal to
client this condition ? Looking at goodread again if timeout it's
reached while loop exit and so longquery_timeout (witch IMHO should be
longer) do not occur; goodread (tds_read_packet) got called again using
timeout. So we exit from main (tds_read_packet) loop if no data arrived
in timeout (even if longquery_timeout it's not reached)... If I'm
confusing somebody let's me say that I'm confused too :)

Another issue concerns no-blocking socket. If we have connect_timeout
socket it's no-blocking otherwise it's blocking; in read.c code works
either in blocking or no-blocking (cause it always use select before
read) however to simplify things it would better to decide blocking
option...

I don't like queryStarttime field name... we never used capitalized word
to compute names... query_starttime would IMHO be better (at least one
easy things to solve :) )

My head is becoming quite hot... I suggests more timeouts:
- read_timeout
- write_timeout
- connect_timeout
- login_timeout
- query_timeout
(I'm kidding :) )

freddy77