Re: [fw-db] Comments on Zend_Db_Adapter_Sqlsrv

Hi,

initially I used Oracle adapter/statement code as a framework to create sql functionality, so 1.) might be true. I/Ralph will fix it.

Checking options in 2.) might be possible, but not necessary needed :) I mean current approach will work with future sqlsrv_* improvements also, because we relay on that driver to check if options exist.

Transactions functions throws exceptions because it seemed for me that errors in sql transactions are more dangerous than in other drivers (maybe). Returning false/true is different from abstract adapter so probably not good idea to use and user code shouldn't handle return in sqlsrv-specific way. That was my idea, but I might be wrong :)

Limit stuff is wid in sql server it self :) I have seen ways to do it in 2008-only (which are more elegant), but since this driver focuses on general case I chose to use good-old approach :) For a possible bug maybe you can create full test-case? That would be really helpful.

Lastinsertid was made with intention (again) to not to break abstract adapter. It works in all general cases and will only break in some "crazy" cases :) However, adapter can't decide tha and developer should know, that in complex queries it might break. As I said - it works in all normal cases and is tested with unit-tests :)

Server version is a little bit tricky. We use http://msdn.microsoft.com/en-us/library/cc296165%28SQL.90%29.aspx to get version and we don't know actual server version - only client info is available. Are there sql queries to get actual server version and do we want to use it? It seemed for me that getServerVersion is used to check for driver/server version specific cases so driver version in this case is not a full bug :)

Statement first point - can't understand :)

fetchArray is again work-around for abstract cases. Adapter wants to have ability to make columns lowercase,uppercase,default hence we need to loop through all keys and do it. Although keys are generated once per statement and this operation is not expensive so I don't think that is a problem. Correct me if I'm wrong :)

Very good point about error messages ;) I'm definitely going to look at it and will probably change exception to store all errors and give access to first one/all/etc. It won't break anything just give addition options.

This driver was created with an intention to have (as much as possible) transparent adapter - only a few unit-tests from abstract suite are marked as skipped (bind by name etc.) and all others work just fine.

Thanks a lot for these comments!

Juozas

On Wed, Jul 29, 2009 at 8:09 PM, Andrew Ballard <aballard@gmail.com> wrote:

I'm glad to see an adapter for the SQL Server Driver for PHP (Sqlsrv) being
added to Zend_Framework with the upcoming 1.9 release. Overall they look
similar to what I have been using, so it will be good to see them become
part of the core. After looking at RC1, I have a few observations:


Zend_Db_Adapter_Sqlsrv
1) I see some references to Oracle and things like tnsnames.ora still in the
comments.

2) In _connect(), I think it would be more efficient for the adapter to
throw an exception on any driver options that are not supported by the DLL
rather than passing them off to sqlsrv_connect() and letting it return an
error.

3) I see that the transaction handlers (_beginTransaction, _commit  and
_rollback) now return void and raise exceptions on failure. I simply had
them pass back the return value of the corresponding sqlsrv_ function, but
it appears that Zend_Db_Adapter_Abstract now returns a self reference to
provide a fluent interface. I guess this implementation is consistent with
the change in the abstract base class.

4) The implementation of limit() looks like it uses a pretty common
technique I have seen in other algorithms, but I think it contains a slight
flaw regarding the last "page" in the result set.

For example if the total rows matching the WHERE clause is 55, the value of
rows-per-page is 10, and selected page number is 6, the inner query would
request the top 60 (offset + rows = 50 + 10) rows. Since there are only 55
total rows, the inner query will return all 55 rows. The outer query then
takes the bottom 10 of the set returned by the inner query. In this case,
that would be rows 46-55 rather than just 51-55.

In my class, I started with this approach, but I modified it to add an
additional WHERE condition so that it would only grab the rows in the bottom
10 that were not also part of the top 50. Due to its complexity I left room
for the adapter to select alternate algorithms [yet to be added] for queries
against newer versions since there are now better ways to implement this in
2005 and 2008. (The trouble is, depending on the tables you may get better
or worse performance by using either row_number() or by various approaches
with CTEs, etc. On large tables, it can be the difference between queries
that finish in seconds and queries that finish in hours.)

5) lastInsertId can be tricky because of scope, but it looks like what
you've done here is better than what I used.  :-)


6) getServerVersion is returning the driver version, not the database server
version.



Zend_Db_Statement_Sqlsrv
1) I like the addition of the class constants for the parameter directions.
Perhaps the other constants should be added as well? A couple of them
probably have to be implemented as static functions, rather than members of
that enumeration array though.  :-\

2) I'm curious why internally fetch() always fetches a row as two separate
arrays (keys and values) and then uses the fetch-mode to determine whether
to discard one and return the other, or merge them together (and optionally
cast the combined array to an object). Is that faster than calling either
sqlsrv_fetch_array with the appropriate 2nd parameter or
sqlsrv_fetch_object?


Error handling:
I am glad to see the full array returned by sqlsrv_errors() being passed to
the exception constructors. A single query could return multiple error
messages, especially from stored procedures, and often the last error
message returned is simply "Statement failed." I wish there was a good way
to be able to access either the full array of messages and codes or specific
items in the errors array rather than just getting the first/last code
returned.

I'm not sure either of these are good ideas, but what about either having
getMessage() return each() on the message array or else providing
getMessages() and getCodes() as additional methods on those classes?

Thoughts?

Andrew
--
View this message in context: http://www.nabble.com/Comments-on-Zend_Db_Adapter_Sqlsrv-tp24725644p24725644.html
Sent from the Zend DB mailing list archive at Nabble.com.