[Home]

Summary:ASTERISK-04950: ODBC SQLRowCount() handling in res/res_config_odbc.c v1.28
Reporter:dburdelski (dburdelski)Labels:
Date Opened:2005-08-31 16:55:55Date Closed:2008-01-15 15:52:56.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Resources/res_features
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) odbc_rowcount.patch.txt
( 1) odbc_rowcount2.patch.txt
( 2) odbc_rowcount3.patch.txt
Description:on lines 115 and 269 / 546 in v1.28 res/res_config_odbc.c there is a call to SQLRowCount().  According to the ODBC API (ref: http://msdn.microsoft.com/library/default.asp?url=/library/en-us/odbc/htm/odbcsqlrowcount.asp), SQLRowCount() can return SQL_SUCCESS but the rowcount is -1 (number of rows not available).  On FreeTDS 0.63 (latest) with unixodbc 2.2.11 (latest) with a MS SQL2000 database, this rowcount = -1 return is the case (in my tests) on the queue_member_table SELECTS:

SELECT * FROM queue_member_table WHERE ( interface LIKE ?) AND (queue_name = ?) ORDER BY interface

On line 293 / 546 in realtime_multi_odbc() rowcount is used without checking for this rowcount = -1 check.  Thus while(rowcount--){} never terminates and the agent are never retrieved from the database (in this test).

What is required to fix this code is that SQLRowCount() should not be used to determine rowcount as is advised in the ODBC reference cited above.  Rather, there should be a:

SELECT COUNT(*) FROM queue_member_table WHERE ( interface LIKE ?) AND (queue_name = ?) ORDER BY interface

to get the rowcount and then:
 
SELECT * FROM queue_member_table WHERE ( interface LIKE ?) AND (queue_name = ?) ORDER BY interface

followed by the SQLFetch based on the rowcount determined from the SELECT COUNT(*) .....



****** ADDITIONAL INFORMATION ******

Platform: Redhat Linux 7.3 / FreeTDS 0.63 (latest) / unixodbc 2.2.11 (latest) with a MS SQL2000 database.  I am testing with SIP and queues / agents configured via res_config_odbc.

Comments:By: Kevin P. Fleming (kpfleming) 2005-08-31 17:22:53

We have already discussed this at length previously, and I find it very weird that the SQLRowCount() function cannot be used for the most common type of query...

If this is going to get changed, someone with the proper environment is going to have make and test a patch for it. Can you do that?
By: Brian West (bkw918) 2005-08-31 17:38:53

This is a driver issue not an ODBC issue.  I think we talked abou this in great lengths before.

/b
By: dburdelski (dburdelski) 2005-09-01 06:18:59

I can test the patch most definitely.  I can take a stab at creating the patch, but based on the code that is already there.  (I am primarily a perl writer and C reader).

I apologize that I did not find a record on your previous discussions of issue.  Please send a URL so I can read up on your thoughts and know where these types of discussions are recorded (so I don't make *that* mistake again ;-)).
By: Michael Jerris (mikej) 2005-09-09 20:33:11

I know we have discussed this at length, but I re-read the ODBC spec and it does very clearly state that sqlrowcount is only valid after Delete, insert, and update calls.  Some drivers will work on this, and some will work in some situations, but it can never be depended upon to be accurate.  The reason for this is based upon the wireline protocol when performing queries returing very large recordsets.  We do not necessarily know the full count when a select is done on a very large table as the full recordset is not returned, just the first page (I beleive) and a marker that more is available.  The driver hides this from us but still does not know what the total recordcount will be until all the data is acually returned.  I think it is very minimal overhead to add a check for if rowcout = -1 then set rowcount to the select count(*) return value, and would make asterisk closer to be in spec, and actually work with freetds.
By: Kevin P. Fleming (kpfleming) 2005-09-13 22:12:18

OK, I'm willing to commit a tested patch for this if you guys can create one.
By: Michael Jerris (mikej) 2005-09-13 22:16:36

dburdelski-  Are you able to produce a disclaimed patch for this?
By: Donny Kavanagh (donnyk) 2005-09-14 09:53:12

per this function here

int odbc_smart_execute(odbc_obj *obj, SQLHSTMT stmt)
{
       int res = 0;  
       res = SQLExecute(stmt);
       if ((res != SQL_SUCCESS) && (res != SQL_SUCCESS_WITH_INFO)) {
               ast_log(LOG_WARNING, "SQL Execute error! Attempting a reconnect...\n");
               ast_mutex_lock(&obj->lock);
               obj->up = 0;
               ast_mutex_unlock(&obj->lock);
               odbc_obj_disconnect(obj);
               odbc_obj_connect(obj);
               res = SQLExecute(stmt);
       }
       
       return res;
}

which is contained within res_odbc.c, am i correct in saying that we should be able to detect the existance of rows so long as the return != 0?

The docs for SQLExecute (http://msdn.microsoft.com/library/en-us/odbc/htm/odbcsqlexecute.asp?frame=true) state that when there is no data, it should return 'SQL_NO_DATA'
By: Michael Jerris (mikej) 2005-09-14 11:11:14

Your patch is eagerly awaited!
By: Kevin P. Fleming (kpfleming) 2005-09-25 23:11:51

I believe that relying on SQL_NO_DATA, and changing the row-processing loops to just keep processing until the fetch operation doesn't return another row, is the proper fix. Is there someone with an ODBC database already setup that can create and test a patch?
By: Donny Kavanagh (donnyk) 2005-09-25 23:47:44

I have the setup at work i've just been uber busy... i'll try and look @ it monday.
By: paulohm2 (paulohm2) 2005-10-19 08:57:27

I'm having the same issues making voicemail work with an oracle database using unixODBC.

as stated on the Microsoft documentation, the SQLRowCount return after any query that is NOT an INSERT, REPLACE or UPDATE is driver-dependent.

Regarding res_odbc.c, most of the issues that I had were corrected by the following patch

Index: res/res_odbc.c
===================================================================
RCS file: /usr/cvsroot/webfone/asterisk-1.2/res/res_odbc.c,v
retrieving revision 1.2
diff -r1.2 res_odbc.c
94,95c94,96
<       if ((res != SQL_SUCCESS) && (res != SQL_SUCCESS_WITH_INFO)) {
<               ast_log(LOG_WARNING, "SQL Execute error! Attempting a reconnect...\n");
---
>       // [PHM 19/10/2005]
>       if ((res != SQL_SUCCESS) && (res != SQL_SUCCESS_WITH_INFO) && (res != SQL_NO_DATA)) {
>               ast_log(LOG_WARNING, "SQL Execute error (%d)! Attempting a reconnect...\n", res);
By: Donny Kavanagh (donnyk) 2005-10-19 09:35:03

I started working on the fix for this yesterday (slow i know)  I should have the patch posted sometime today.  I've essentially removed the dependancy on SQLRowCount after a select statement, as well as removed it from alot of other places where it simply is not needed.


By: paulohm2 (paulohm2) 2005-10-19 10:05:09

Yes I've done the same. Most of the calls to SQLRowCount, at least in apps/app_voicemail.c are not necessary.
By: Donny Kavanagh (donnyk) 2005-10-19 12:18:52

odbc_rowcount2.patch.txt uploaded.  In some light testing this works fine on my mysql + odbc config.  Voicemail works fine, sip peers load from odbc etc.  Only one call to SQLRowCount remains (out of about 15) and its actually the only correct one!  Mysql never originally had a problem however so i need some people to test this against oracle odbc, freetds etc, and let me know.  Kevin wants to get this in for 1.2 so this needs to be tested asap.
By: paulohm2 (paulohm2) 2005-10-19 12:32:31

can I piggyback on your patch and submit 2 important changes? ;-)

Oracle ODBC driver returns field names in upper case

there are 2 strcmp's in apps/app_voicemail.c that need to become strcasecmp

best,

PHM
By: Donny Kavanagh (donnyk) 2005-10-19 12:34:51

i dont have an oracle setup...

if you want, resubmit the patch with those changes and i'll test it against mysql to make sure it doesnt break anything there.
By: paulohm2 (paulohm2) 2005-10-19 12:38:53

I'm not a cvs specialist, so here it goes inline

line
                               if (!strcmp(coltitle, "recording")) {
should read
                               if (!strcasecmp(coltitle, "recording")) {

and line
                                       if (strcmp(coltitle, "msgnum") && strcmp(coltitle, "dir") && f)

should read
                                       if (strcasecmp(coltitle, "msgnum") && strcasecmp(coltitle, "dir") && f)
By: paulohm2 (paulohm2) 2005-10-19 12:39:06

I'm not a cvs specialist, so here it goes inline

line
                               if (!strcmp(coltitle, "recording")) {
should read
                               if (!strcasecmp(coltitle, "recording")) {

and line
                                       if (strcmp(coltitle, "msgnum") && strcmp(coltitle, "dir") && f)

should read
                                       if (strcasecmp(coltitle, "msgnum") && strcasecmp(coltitle, "dir") && f)
By: Donny Kavanagh (donnyk) 2005-10-19 12:41:16

and that solves both things you mentioned?
By: paulohm2 (paulohm2) 2005-10-19 12:58:05

Yes. I have a current setup with unixODBC and a oracle backend. I'll get your patch and test it against it.
By: Donny Kavanagh (donnyk) 2005-10-19 13:01:22

ok, try odbc_rowcount3.patch.txt against a clean cvs head.

Let me know.
By: paulohm2 (paulohm2) 2005-10-19 15:03:30

are you sure the patch is for cvs-head? it is working only on stable
By: Donny Kavanagh (donnyk) 2005-10-19 15:10:18

yes, this most definitally patches against cvshead.

asterisk2*CLI> show version
Asterisk CVS HEAD built by root@asterisk2.trn.ca on a i686 running Linux on 2005-10-19 17:55:04 UTC

try from fresh source, it wont merge if your changes are still there, and doing a cvs update wont remove your changes.
By: Donny Kavanagh (donnyk) 2005-10-19 16:53:53

i should add that yes i have a disclaimer on file.
By: Donny Kavanagh (donnyk) 2005-10-31 09:19:24.000-0600

ping.
By: Kevin P. Fleming (kpfleming) 2005-10-31 16:39:29.000-0600

Committed to CVS HEAD, thanks!
By: Digium Subversion (svnbot) 2008-01-15 15:52:56.000-0600

Repository: asterisk
Revision: 6904

U   trunk/apps/app_voicemail.c
U   trunk/res/res_config_odbc.c
U   trunk/res/res_odbc.c

------------------------------------------------------------------------
r6904 | kpfleming | 2008-01-15 15:52:56 -0600 (Tue, 15 Jan 2008) | 2 lines

don't use 'rowcount' after SELECT statements, since the ODBC API does not say it is allowed (issue ASTERISK-4950)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=6904