FreeTDS Development Group

["James K. Lowden" <jklowden AT freetds.org>]

On Tue, 10 Sep 2013 14:59:23 +0000
Marc Abramowitz <marca AT surveymonkey.com> wrote:

> More info then you probably care about pymssql

My bread and water these days comes from Continuum Analytics, so I have
great interest in things db pythonic.  

> And then folks stopped maintaining the 1.x code (written in C) and
> started a whole new 2.X version, that is written with Cython. That
> code doesn't seem to have this problem either, and that code is more
> actively maintained now (mostly by me), but it's
> never been officially released, so it isn't as proven in production.

I'm afraid I don't follow.  There's a 2.x version not in "production"?
If I'm using garden-variety Python, which pymssql should I have?  

> https://github.com/pymssql/pymssql/blob/master/_mssql.pyx#L1036

            # Since python doesn't have a do/while loop do it this way
            while True:
                with nogil:
                    self.last_dbresults = dbresults(self.dbproc)
                self.num_columns = dbnumcols(self.dbproc)
                if self.last_dbresults != SUCCEED or self.num_columns
>0: 
                        break
            check_cancel_and_raise(self.last_dbresults, self)

You're supposed to call dbnumcols() only after dbresults returns
SUCCEED.  

Wouldn't this work better as

        self.clear_metadata()
        while (self.last_dbresults = dbresults(self.dbproc)) == SUCCEED:
            self._rows_affected = dbcount(self.dbproc)
            if (self.num_columns = dbnumcols(self.dbproc)) > 0:
                    column_names = list()
                    column_types = list()
                    for col in xrange(1, self.num_columns + 1):
                        column_names.append(dbcolname(self.dbproc, col))
                        coltype = dbcoltype(self.dbproc, col)
                        column_types.append(get_api_coltype(coltype))
                    self.column_names = tuple(column_names)
                    self.column_types = tuple(column_types)
                    return True    
            if (mumble = dbnumalts(self.dbproc)) > 0
                # do the needful and 
                return True

        if self.last_dbresults == NO_MORE_RESULTS or \
           self.last_dbresults == NO_MORE_RPC_RESULTS: 
                self.clear_metadata()
                if dbhasretstat(self.dbproc): 
                        self.retstatus = dbretstatus(self.dbproc)
                # handle output parameters 
                # (not currently supported)
                return False

        check_cancel_and_raise(self.last_dbresults, self)
        return False
        
That way, each db-lib function is called just once, and every return
value of dbresults is handled.  

Studying this turned up a little bug in FreeTDS.  Consider

        create table #t( t int )
        insert into #t values (1)
        insert into #t values (2)

TDS has a variety of DONE packets, and DONE handling is one of the more
difficult parts to get right.  The problem is partly that the protocol
has changed over the years, partly in anticipating when the server
has stopped sending data, and partly (as I alluded to in my last
message) because libtds supports different APIs with different notions
of "results".   

Be that as it may, DONE packets carry the "rows affected" information
that dbcount returns.  It's one of the murkier corners of db-lib,
because there's no correspondence between the application's
opportunities to call dbcount and the number of DONE packets that the
server sends.  

When the above SQL is submitted as a single batch, the server returns 3
DONE packets, one for each statement.  dbresults returns SUCCEED after
the *first* done packet, the one resulting from the CREATE TABLE
statement, for which no count is valid.  (A bit in the DONE packet
signifies whether the count value can be trusted, cf. dbiscount.)

So the poor user calls dbresults, gets SUCCEED, calls dbiscount, gets
FALSE.  WTF?  

When dbresults is called a second time, the library returns to the
TDS stream and parses out the next two DONE packets, sets the count to
1 (twice, once per packet), and then returns NO_MORE_RESULTS.  At that
point dbiscount might return TRUE, but the user isn't supposed to call
dbcount after unless dbresults returned SUCCEED.  :-(  

This is what happens when the folks writing the library are learning as
they go.  

The corrective is both simple and difficult.  Simply, every TDS packet
carries a "more results" bit, telling the client whether or not the
server is finished sending.  For DONE tokens, db-lib should continue
reading the stream until either that bit is 0 -- indicating SUCCESS,
but no rows returned -- or another kind of packet arrives, say a
row-format packet.  

The difficult part is that, as I said, this is one of the gnarlier
parts of the library.  For ODBC, each DONE packet constitutes a
result.  For db-lib, only the last one does.  So the client library
passes into libtds a "read until" parameter, meaning that the libraries
have shared state.  It's enough to make you want to take up knitting
instead.  

The complexity is entirely unnecessary; it's an accidental outgrowth
the organic way in which libtds evolved.  And I have a very nice design
for libtds2.  What it needs is someone with time.  

--jkl