apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kappa <ka...@psilambda.com>
Subject Re: apr_dbd_odbc patches
Date Mon, 26 Jul 2010 02:58:28 GMT
  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


Mime
View raw message