[Home]

Summary:ASTERISK-04925: [patch] [post 1.2] Simple SQL queries from the dialplan
Reporter:Tilghman Lesher (tilghman)Labels:
Date Opened:2005-08-29 11:41:03Date Closed:2005-12-21 11:44:15.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Functions/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20050912__funcs_Makefile.diff.txt
( 1) func_odbc.c
( 2) func_odbc.conf.sample
Description:This function allows templated SQL queries to be made from the dialplan.
Comments:By: Tilghman Lesher (tilghman) 2005-09-03 09:43:47

Updated.  Now allows for writes, as well as reads.

By: Tilghman Lesher (tilghman) 2005-09-06 22:50:13

Updated again, to reduce code duplication.

By: Anthony Minessale (anthm) 2005-09-07 09:07:30

It's cool it lets you write custom functions in sql.  It could easily be added now and can't hurt anything since it's completely autonomous.

By: hristo (hristo) 2005-09-08 09:02:56

works great for me too. i'd like to see in 1.2 also...

By: jalsot (jalsot) 2005-09-12 20:10:44

A newbie question: is it thread safe?
What happens if the given connection is under execution and another request comes? Will the 2nd command wait for the database handler? Or will the odbc layer handle multiplexing of requests?
Sorry, I'm completly newbie in ODBC...

By: twisted (twisted) 2005-09-12 21:56:03

can we not combine this with the existing extconfig.conf so that we're not configuring ODBC in an insane number of places?

By: jalsot (jalsot) 2005-09-13 05:40:48

Could you make an example how to use the read and write mode? Read looks to be ok, example:
exten => 100,1,Set(something=${ODBC_SQL('SELECT COUNT(*) FROM cdr')})
exten => 100,n,NoOp(${something})

How to use write mode?

BTW, it looks very useful addition! I like it :)

By: Tilghman Lesher (tilghman) 2005-09-13 16:22:03

twisted:  I'm not sure what you mean.  It's using the connection defined in res_odbc.conf, but it's not a replacement for an existing configuration file, so extconfig.conf shouldn't be related.

By: Tilghman Lesher (tilghman) 2005-09-13 16:27:31

Example of reading and writing using the PRESENCE example:

GotoIf($[${ODBC_PRESENCE(101)} = here]?ring)
Set(ODBC_PRESENCE(101)=away)

By: twisted (twisted) 2005-09-13 17:03:16

I love the idea of this patch. I do not like the idea of having to configure the odbc connector yet again to make this work.  extconfig.conf could be used just fine.  regardless if it's a config or not, we could add a section for [functions] if need be, and keep the same syntax.   We already have too many conf files as is.

By: Tilghman Lesher (tilghman) 2005-09-13 17:15:42

Maybe it's because it's called "dsn" that you're confused.  Perhaps it should be called "connection".  It's not a reconfiguration of ODBC; it's just specifying which connection ought to be used for this particular function.  There's certainly a possibility of using more than a single ODBC connection with this code.

extconfig.conf is specifically for alternative configuration of /etc/asterisk files from an alternative source.  Suggesting that we use extconfig.conf is as inappropriate as suggesting that we configure this out of voicemail.conf; it's not related to the purpose of either file.

By: jalsot (jalsot) 2005-09-13 17:25:56

Corydon, thanks for the example!
Is there a way to process multiple rows returned by the query? How?

By: twisted (twisted) 2005-09-13 17:36:49

I completely disagree.  extconfig.conf is <i>currently</i> used for nothing but replacing config files with config from a database.  It <i>could</i> be used for more.  I did not suggest configuring it from voicemail.conf, as thas <b>would</b> be completely inappropriate.

extconfig.conf COULD be used for more than what is now.  Why have YACF when extconfig.conf already deals with ODBC relations.  ie, relating voicemail configuration to a db dsn, table, and type.  

why could we not put this config in the same place, give it a different contextual label, since it too, relates to ODBC?  I do not see the problem here.  Honestly, I think it would make more sense, and keep the clutter down.



By: Tilghman Lesher (tilghman) 2005-09-13 17:49:29

extconfig.conf has a well-defined purpose now; let's not muddle it with extra unrelated configuration (the same argument you made there could be applied to res_odbc.conf and cdr_odbc.conf -- and I don't think you'd agree that either should be in extconfig.conf).

In any case, let's have kpfleming or kram weigh in on the discussion.  No point in making a change that isn't going to be approved.

By: Kevin P. Fleming (kpfleming) 2005-09-13 18:26:58

I agree with Corydon here... extconfig is specifically for 'external configuration', which is pretty clearly intended to replace/supplement other forms of configuration data.

By: Tilghman Lesher (tilghman) 2005-09-13 18:41:10

MikeJ:  there was some supposition about whether or not this could go into 1.2 or not, given that this is a self-contained block of code.

kpfleming:  can it?

By: Kevin P. Fleming (kpfleming) 2005-09-13 18:55:56

Technically, no, since it is a new feature, regardless of whether it is modifying existing code or not. We have other "entirely independent" code features already marked as post-1.2, and I'm very hesitant to relax the feature freeze given how far behind we already are on getting 1.2 ready to release.

By: Tilghman Lesher (tilghman) 2005-09-16 15:10:52

So I was thinking a little more, we already had the ability to do expressions and functions in the definition of an ODBC_ func, but certain ones (like CALLERID()) wouldn't work, because I was using a fake channel.

So now, the fake channel saves the values of ARG1..ARGn and VAL1..VALn and we use the real channel to do our substitution.  So the templates will now be able to do any expression that you can do in the dialplan.

By: Anthony Minessale (anthm) 2005-09-16 15:15:33

maybe change the names to ODBC_ARG1..ARGn then you never need a fake channel.

By: Tilghman Lesher (tilghman) 2005-09-16 15:51:57

Not a good idea, since the result of that will be that you can do:

Set(foo=ODBC_FOO(1,2,3,4,5))
Set(bar=ODBC_BAR())

and ARG2..ARG5 will still be set for the second call.  Yes, we could clear all variables that we set, but I think that it's better to solve the problem, rather than to hack our way around it.

By: Tilghman Lesher (tilghman) 2005-09-16 16:02:06

jalsot:  No, this only deals with the first row of a resultset.  As said in the description, this is a _simple_ SQL interface.

By: hristo (hristo) 2005-09-21 05:26:37

It looks like there is some kind of problem when writing to the function (reading works fine). For example sometimes I get error when I try SQL query similar to the one below. The real query I execute is not exactly the same but I am only changing it here to make it more clear and to explain the problem:

[UPDATE_TEST]
dsn=ast_cnf
write=UPDATE table SET temp='${VAL1}' WHERE number='${ARG1}'

So now if I try in the dialplan something like:

exten => 1,1,Set(ODBC_UPDATE_TEST(123)=test1)

sometimes I get (note the strange characters at the end of the query!!):

Sep 20 20:26:53 WARNING[3547]: res_odbc.c:112 odbc_smart_execute: SQL Execute error! Attempting a reconnect...
Sep 20 20:26:53 WARNING[3547]: res_odbc.c:433 odbc_obj_disconnect: res_odbc: disconnected 0 from ast_cnf [MySQL-pa]
Sep 20 20:26:53 NOTICE[3547]: res_odbc.c:490 odbc_obj_connect: Connecting ast_cnf
Sep 20 20:26:53 NOTICE[3547]: res_odbc.c:505 odbc_obj_connect: res_odbc: Connected to ast_cnf [MySQL-pa]
Sep 20 20:26:53 WARNING[3547]: func_odbc.c:165 acf_odbc_write: SQL Execute error!
[UPDATE table SET temp='test1' WHERE number='123'`M&ASTERISK-1057;]

sometimes I will only get:

Sep 21 12:46:06 WARNING[6820]: res_odbc.c:112 odbc_smart_execute: SQL Execute error! Attempting a reconnect...
Segmentation fault

What I've noticed is that this problem seems to be related the the resulting string length of the query (UPDATE table and so on.....) and for example if I insert some spaces in order to make the "magic" length it will execute cleanly.

an example to make it more clear:
[UPDATE table SET temp='test1' WHERE number='123'`M&ASTERISK-1057;] - fails and gives the errors above

but if I add 3 spaces (indicated with <space> below) between the "UPDATE" and "table" in func_odbc.conf and reload asterisk (so the config gets reread) I will get:

[UPDATE <space><space><space>table SET temp='test1' WHERE number='123']

Note that the 3 extra characters that were present are now pushed away and the query will execute properly.

The <space> trick will not work for me because the values I try to update are different length, but I think this at lest demonstrates that there is a problem.

This was tested with the CVS HEAD from yesterday (20-Sep-2005) and also a couple of days before. Linux kernel is 2.6.9-11.ELsmp.



By: Tilghman Lesher (tilghman) 2005-09-21 11:58:45

Added an initialization to the SQL buffer in the write code.

By: hristo (hristo) 2005-09-22 17:27:44

this change solved my problem. thanks!

By: wlloyd (wlloyd) 2005-10-06 22:31:28

what are the differences between this and http://www.loligo.com/asterisk/misc/apps/odbc/app_odbcexec/ ?

The other runs as an app and this runs as a function...

Can this versions also do inline sql without building the queries in func_odbc.conf?

exten => 99,1,ODBCquery(temp=select credit from credits where custommer=${CALLERIDNUM} limit 1)
exten => 99,2,GotoIf($[${temp} = 0]?10:3)
exten => 99,3,Dial(channel)
exten => 99,10,ODBCexec(insert into unapproved(custommer,ext) values(${CALLERIDNUM},${EXTEN}))
exten => 99,11,Playback(no-credit,noanswer)
exten => 99,12,Hangup

-bill

By: Tilghman Lesher (tilghman) 2005-10-06 22:52:38

The main difference is that these are template functions:  you define the query, then call it with arguments to fill in the blanks.  While you certainly can specify full SQL statements from the dialplan, the real benefit is the fill-in-the-blank approach.

The other difference is that this is fully disclaimed and is going to become a part of Asterisk.  ;-)

By: hristo (hristo) 2005-10-18 18:08:02

is there a way to get the status of the sql request - if it was successful or not. right now if there is a problem with the sql server i will simply get empty result (which is also pretty normal case if there is no data matched by the query). it will be nice if i can tell these two cases apart thus having the option to take different actions later in the dialplan depending on sql server availability? maybe setting a status variable will do the trick?

By: Tilghman Lesher (tilghman) 2005-10-18 18:36:41

[PING]
dsn=sqlserver
read=SELECT 1


GotoIf(0${ODBC_PING()})?serverok:serverdown)

By: hristo (hristo) 2005-10-19 11:47:09

That will indeed solve the problem for SELECTs. In fact I think it will be even better to include this this into the actual select statement and checking for that dummy column in the result will give me a clue if the select itself was processed or not (in case the sql server fails for some reason between the PING and the actual SELECT you can detect it like that).

Still there will be no way to know if UPDATE/INSERT queries are successful. Something like the "1 row affected". Again if something fails between SQL ping and the real query there is not way to know this for INSERT/UPDATE.

The write function only allows writes and cannot be read that's why I've suggested status variable (I'm only looking at this from the user perspective, maybe you'll have a better idea?)

By: Tilghman Lesher (tilghman) 2005-10-19 12:25:24

Right, because you can do CUT(result,\,,1) for one field and CUT(result,\,,2) for the second.

As far as checking for success, I'll have to think about the "right" way to do that.  Seems like a status variable is an ugly hack that will get in the way sooner or later.

By: Tilghman Lesher (tilghman) 2005-10-26 11:46:02

Okay, given that a write to a function can only happen once per application, I'm comfortable with setting a status variable.  In this case, I'm setting ODBCROWS, which contains the output of SQLRowCount, which should be the number of affected rows by the query.  I'm additionally setting the value to -1 if there was a hard error (differentiates from 0, where a query might have succeeded, but the value did not change).

By: hristo (hristo) 2005-11-01 14:38:17.000-0600

I believe this will do the trick for me. Thanks! Will give it a try later during the week. Any chance of seeing this in 1.2? Even though it is marked post 1.2 I think it will be a great addition to the final release.

With the introduction of functions in the extensions logic in 1.2 and the move of so many applicatoins to functions it seems quite logical to look for as many new functions as possible. I've been using it for 5-6 weeks now - it works great for me and it sure doesn't break anything.

By: Tilghman Lesher (tilghman) 2005-11-01 15:48:27.000-0600

hristo:  that question has already been asked and answered earlier in the bugnotes.  It will not appear in 1.2.0.

By: hristo (hristo) 2005-11-09 09:43:43.000-0600

I've tested this a little bit more here is what i get:

1. when everything is ok (db is up) it seems to work as expected.

2. when i shutdown the database and try a write function, ODBCROWS is not set to -1 (not sure if it is normal, but this is what i thought would happen if db is down). i get the following warning, indicating that the db is indeed down:

Nov  9 17:27:59 WARNING[22815]: func_odbc.c:279 acf_odbc_read: SQL Alloc Handle failed!

3. i haven't tested a case when the write function will fail even if the db is up (for example if permissions on the table disallow upadte/insert), but i guess this is where i should get ODBCROWS set to -1.

4. for the sake of consistency between read/write functions maybe it is a good idea to have the ODBCROWS set for read functions too, although the select and CUT trick can be used instead.

By: Tilghman Lesher (tilghman) 2005-11-09 10:07:46.000-0600

Can't do that, because SQLRowCount is *ONLY* for INSERT/UPDATE statements.  See the documentation for ODBC.  The code in app_voicemail.c recently had to be revised to get rid of that misunderstanding.  The *ONLY* portable way to know how many rows a SELECT returned is to retrieve each row, incrementing each time, until SQL_NO_DATA is returned.

By: Tilghman Lesher (tilghman) 2005-11-09 10:09:58.000-0600

New func_odbc.c uploaded, that sets ODBCROWS to -1, in the case when a database is down.

By: Tilghman Lesher (tilghman) 2005-11-09 10:12:31.000-0600

The other reason I'm not willing to set ODBCROWS in the case of a read is that you can have multiple functions within the execution of a single application.  In that case, the meaning of ODBCROWS is ambiguous as to which function call return it refers to.

By: hristo (hristo) 2005-11-21 09:20:33.000-0600

Just wanted to confirm that ODBCROWS is now correctly set to -1 if DB is down.

I have noticed something else (not directly related to func_odbc) - asterisk fails to reconnect to the database when DB is up again. It reconnects after you issue "odbc show" in the CLI, but not if a function (or even ARA enabled SIP, dialplan, etc) tries to use the odbc connection. I guess this is for another bug but just wanted to mention it as it is somewhat related.

By: wlloyd (wlloyd) 2005-11-21 09:24:23.000-0600

I've also noticced the reconnect issue.  I was wondering if it was just something in my setup.  The ODBC server in my case is Postgres.  If I restart the postgres daemon asterisk is never able to handle a dialplan query, cdr update etc.  Maybe we should open a second bug.

By: Tilghman Lesher (tilghman) 2005-11-21 09:57:03.000-0600

You are correct in that it is a separate unrelated issue.

By: Tilghman Lesher (tilghman) 2005-12-21 11:43:36.000-0600

Committed to trunk