FreeTDS Development Group

["ZIGLIO, Frediano, VF-IT" <Frediano.Ziglio AT vodafone.com>]

> > 
> > Test to send query is 
> > 
> > if (tds_set_state(tds, TDS_QUERYING) != TDS_QUERYING)
> >     return TDS_FAIL;
> > 
> > this is bad. If state was already TDS_QUERYING it try to 
> set state, in
> > tds_set_state
> 
> if (tds->state != TDS_QUERYING) {
>       if (tds_set_state(tds, TDS_QUERYING) != TDS_QUERYING)
>               return TDS_FAIL;
> }
> 

Yes and not... let me explain changes in state and the importance of
tds_set_state (drums rumbling :) .. )

There are currently

TDS_IDLE
  no activity on network
TDS_QUERYING
  libTDS (and IMHO only libTDS would do so) is currently writing
query/rpc/whatever
TDS_PENDING
  expected data from server (but you should not read data from it!!)
TDS_READING
  we are reading data from server
TDS_DEAD
  where is the server ? :)

The key is that there is only a place (tds_set_state) where the state
change. One of my idea is to make tds_set_state atomic. This at two
thread that calls tds_submit_query at the same time. If tds_set_state
was atomic only a single tds_submit_query could set state to
TDS_QUERYING and so send a query while other return failure... Currently
is not atomic so it can happen that two thread write data on same
connection at the same time (strange results expected...). 

Normally state change in this way

IDLE -> QUERYING -> PENDING <-> READING -> IDLE

Oh.. I see changes in API documentation... very good!

All seems easy but is not so easy. For example in DONE processing state
change from READING to IDLE while in some occasions (bulk copy) it
should change to PENDING or QUERYING (this is why tds_set_state is
called outside libTDS...). Note that the distinguish from PENDING to
READING was required for cancel. If cancel is called inside a signal we
have to support cancel this we are reading data.

> > case TDS_QUERYING:
> > 
> >     if (tds->state == TDS_DEAD) {
> >       tds_client_msg(tds->tds_ctx, tds, 20006, 9, 0, 0, 
> "Write to SQL
> > Server failed.");
> >       return tds->state;
> >     } else if (tds->state != TDS_IDLE) {
> >       tdsdump_log(TDS_DBG_ERROR, "tds_submit_query(): state 
> is PENDING
> > \n");
> >      tds_client_msg(tds->tds_ctx, tds, 20019, 7, 0, 1,
> >                     "Attempt to initiate a new SQL Server 
> operation with
> > results pending.");
> 
> With my error handler work, the client will be able to cancel the
> operation, and tds->state will become TDS_IDLE.  
> 
> >      return tds->state;
> >     }
> > 
> > so it give error but continue... quite bad (mainly for 
> thread reasons).
> > Perhaps tds_set_state should return TDS_SUCCEED/TDS_FAIL ??
> 
> I think it's better as it is, because we can write code like:
> 
>       switch (tds_set_state(tds, TDS_QUERYING)) {
>       case TDS_QUERYING: /*...*/
>       case TDS_IDLE: /*...*/
>       default: /*...*/
>       }
> 
> I'm not a big fan of boolean return values.  Not getting what 
> you expected
> isn't always the same as failing.   
> 
> Regards, 
> 

Actually we just tests if returned state if what we requested. In case
of going to query TDS_QUERYING is an error or a success depending on
original state. Considering thread problems and 

if (tds->state != TDS_QUERYING) {
  if (tds_set_state(tds, TDS_QUERYING) != TDS_QUERYING)
    return TDS_FAIL;
}

if this is not atomic thread1 can check tds->state != TDS_QUERYING while
thread2 change same thing so each tds_set_state return TDS_QUERYING even
if tds_set_state is atomic.

freddy77