On 07/25/2010 01:48 PM, Tom Donovan wrote:
On 7/19/2010 12:14 PM, Kappa wrote:
After doing some testing and then comparing the odbc driver to the other
drivers (pgsql, oracle, etc). I noticed
what appears to be a bug in the ODBC driver. For getting binary data (in
odbc_datum_get), it is using the
/void *data/ argument differently than the other drivers--it is copying
over the contents pointed to by the data pointer
instead of assigning the pointer address into the location of the data
pointer. The first patch below shows the change
that fixes it to be like the other drivers. (Oracle and the other
drivers all do something like: '/*(char**)data = (char*)entry;/".)
I tested this and it seems to fix it to work like the other drivers (at
least the pgsql driver--that is the one I have tested
for comparison).

The second patch below fixes it to (silently) return zero rows if that
is what is selected within the odbc_pbquery function.
It does not seem proper to me to report a (noisy) error if no rows are
selected--that could be the point of the query after all.

*** apr_dbd_odbc.c.orig 2010-07-19 10:52:17.000000000 -0500
--- apr_dbd_odbc.c 2010-07-19 11:01:54.000000000 -0500
***************
*** 1323,1329 ****
return APR_ENOENT; /* SQL NULL value */

if (len < 0)
! strcpy(data, p);
else
memcpy(data, p, len);

--- 1323,1329 ----
return APR_ENOENT; /* SQL NULL value */

if (len < 0)
! *(char**)data = (char *)p;
else
memcpy(data, p, len);

***************
*** 1557,1565 ****

if (SQL_SUCCEEDED(rc)) {
rc = SQLExecute(statement->stmt);
! CHECK_ERROR(handle, "SQLExecute", rc, SQL_HANDLE_STMT,
statement->stmt);
}
if (SQL_SUCCEEDED(rc)) {
SQLLEN rowcount;

--- 1557,1569 ----

if (SQL_SUCCEEDED(rc)) {
rc = SQLExecute(statement->stmt);
! if (rc == SQL_NO_DATA) {
! *nrows = (int) 0;
! } else {
! CHECK_ERROR(handle, "SQLExecute", rc, SQL_HANDLE_STMT,
statement->stmt);
}
+ }
if (SQL_SUCCEEDED(rc)) {
SQLLEN rowcount;


------------------------------------------end
patch--------------------------

Brian Dunford-Shore
--
ψ (λ _κ )
PsiLambda LLC
2430 Tesson Ferry Road #203
Saint Louis, Missouri 63128-2702

Hi Brian,

Thanks for your suggestions about the ODBC driver.

You are right that the ODBC driver delivers binary data differently from the other dbd drivers for some datatypes: TEXT, STRING, TIME, DATE, etc.   It isn't clear which driver is correct.

The User Manual at http://apr.apache.org/docs/apr/trunk/group___a_p_r___util___d_b_d.html#g67e57ef4eb7952df79ceaa6e92767d41
says: "data     - pointer to data, allocated by the caller"
which implies that the caller manages the memory for binary data retrieved with apr_dbd_datum_get.

Does this mean - for datatypes like STRING and TEXT -
A) the caller allocates memory for the data, and the data remains accessible until the caller releases it?  This is what the ODBC driver does.
-or-
B) the caller allocates only the pointer, not the data, and the data becomes inaccessible after the next call to apr_dbd_get_row?  This is what the other drivers do.

I think A) is probably the better choice, else the caller manages the data lifetime for some datatypes but not for others with apr_dbd_datum_get.  That just seems prone to error.

We do, however, need to make the dbd drivers consistent.  Does anyone else have an opinion on which way is correct here?

I would prefer A) myself--but I am a newcomer and am willing to go with the APR design--whichever way people decide that it is. 


re: "It does not seem proper to me to report a (noisy) error if no rows are selected"

The error shouldn't get reported on stderr unless the debugging environment variable 'apr_dbd_odbc_log' is set.  It is still necessary to create and save some string for the SQL_NO_DATA result, in case the apr_dbd_error function gets called next.

Are you finding that the calls to the check_error function are a performance problem?  or do you just not want to see any SQL_NO_DATA messages when you have 'apr_dbd_odbc_log' set?

I guess my bug report was not clear on this--I noticed the error not because of an error message being displayed but because an error code was being returned (and my code aborts the data flow operations on an error code).  Actually, I suspect a broader error in the check_error functionality of the odbc driver but can not claim to have a definite handle on it.  The broader error seems to be that check_error is returning either a SQL_* status code or otherwise returning with a nonsuccess APR status code when it should be returning an APR success status.  Someone who has some more experience with the odbc check_error function (macro) might be able to understand better than I have yet been able to.


Regards,
-tom-


Brian Dunford-Shore
--
ψ (λ κ)
PsiLambda LLC
2430 Tesson Ferry Road #203
Saint Louis, Missouri 63128-2702