FreeTDS Development Group

["Thompson, Bill D (London)" <ThompBil AT exchange.uk.ml.com>]

Thanks for your comments Freddy,

As I said yesterday, I am not 100% happy with all of this patch, and I will
try and make it more elegant in the near future.
However I was under some pressure to make these fixes very quickly.
I am converting an existing dblibrary application from Sybase to SQL server,
using freetds to replace Sybase open client.
We have just entered system testing, and I was finding some fairly
fundamental problems arising from small behavioural differences between
Sybase dblib and our own.
Regardless of the rights and wrongs, I had to *quickly* get our dblib to
behave the same as Sybases. Most of this patch was written in a few hours...


Enough excuses already. Here's my take on some of your comments. And some
explanation of the issues:

> ?? skipping data instead to send a proper cancel is quite bad... if server
> is returning huge amount of data cancel take ages.... IMHO libtds
(token.c)
> should be encharge for processing cancel, not dblib.

I'm not skipping data *instead* of sending a proper cancel, but prior to
sending the cancel. 
Notice I still call tds_send_cancel() after skipping the data.
The following is an extract from the Sybase manual page for dbcancel:
--------
This routine cancels execution of the current command batch on the server
and flushes any pending results. 
The application can call it after calling dbsqlexec, dbsqlsend, dbsqlok,
dbresults, or dbnextrow. 
dbcancel sends an attention packet to the server which causes the server to
cease execution of the command batch. 
Any pending results are read and discarded. 
dbcancel cancels all the commands in the current command batch. 
To cancel only the results from the current command, call dbcanquery
instead. 
--------

As you can see, this suggests that any pending results (however many there
are) need to be read and discarded.
It seemed to me that dblibrary and ctlibrary have different behaviours with
regards to "cancel" processing (see the ctlib manual pages for ct_cancel).
So I think that it is not correct to try and implement this in libtds.

> I don't understand the state TDS_LASTROW... Why old code didn't work?

Well the old code did *work*, it just didn't behave *exactly* as Sybase
dblibrary.
It seems that in sybase dblibrary you can do the following:

dbcmd(dbproc,"select getdate()"  ( or any query that returns a known number
of rows )
dbsqlexec()
dbresults()

for (known number of rows)
    dbnextrow()

dbcmd("another command")
dbsqlexec()...

In other worrds, if you have fetched all the rows with dbnextrow, it is not
necessary to "complete" the results processing before submitting another
request.

Prior to my fix, we didn't like this, and returned a "...results pending.."
error message on the second dbsqlexec()

Although we now behave as Sybase, this is the part of this patch that I am
least happy with, and I will rework it in the near future.


> @ +   if (t->tm_year == 0 && t->tm_mon == 0 && t->tm_mday == 0) {
> @ +           dt_days = 0;
> @ +   } else {
> @     i = (t->tm_mon - 13) / 12;
> @     dt_days = 1461 * ( t->tm_year + 300 + i ) / 4 +
> @             (367 * ( t->tm_mon - 1 - 12*i ) ) / 12 -
> @             (3 * ( ( t->tm_year + 400 + i ) / 100 ) ) / 4 +
> @             t->tm_mday - 109544;
> @ +   }
> @  

> year == month == day == 0 ???
> This seem a NULL value. tds_convert should not handle null values (library
> dependent).

Alas, not true. Year  = 0 and month = 0 and day = 0 represents a valid date,
namely "Jan 01 1900", a default date widely used in my system
It seems the marvellous bit of maths herein works for all dates except this
one. rather than try and figure out why, I thought the quickest and easiest
solution was to create an exception for this one case.
(as it was, the equation was turning this date into "Dec 31 1899" which had
disastrous consequences for my system!)

> @ +/* I don't think this is working as tds_set_null is not being called
prior to this...
> @ +   I can't figure out how where I should call tds_set_null() anyway....
> @  
> @     is_null = tds_get_null(current_row, i);
> @ +*/
> @     colsize = curcol->column_cur_size;
> @ +   if (colsize == 0)
> @ +           is_null = 1;
> @ +   else
> @ +           is_null = 0;
> @ +

> Bad. Think at empty and not null string (supported in TDS7+).

As I said in the comments, I think the real solution is to call
tds_set_null() somewhere. 
tds_get_null() is pointless unless this has been called earlier.
Like I said though I couldn't figure out how/where to call it. If you have
any ideas on that, let me know.
I know you did the original work on the rpc stuff in dblibrary, so perhaps
you'll get there quicker than I.

Thanks again for integrating these patches and your input.

Bill