Summary:	ASTERISK-01463: [patch] Clean up SQL queries
Reporter:	nix (nix)	Labels:
Date Opened:	2004-04-24 12:19:58	Date Closed:	2008-01-15 14:51:53.000-0600
Priority:	Trivial	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) sql_query_cleanup.txt
Description:	The current SQL queries in chan_cip and chan_iax(x) use "SELECT *" to retrieve records from the DB. This is very dangerous, and does not allow for external extension of the table schema without also modifying asterisk. This patch fixes this.
Comments:	By: Mark Spencer (markster) 2004-04-24 14:35:21 Don't we check the fields when we use the results of the select? By: Brian West (bkw918) 2004-04-24 16:33:11 maybe a hashref who knows but isn't the point of res_data to provide the abstraction layer for database integration?? By: khb (khb) 2004-04-24 16:41:37 Yes, from a dba point of view this is definitely a robuster solution, since it is independent from the actual layout of the table and the retrieval method of the data.  It also adds to the self-documentation aspect of the code. edited on: 04-24-04 15:37 edited on: 04-24-04 15:38 By: tpsailer (tpsailer) 2004-04-24 22:52:47 Any SQL/RDBMS programmer will know that this is the accepted way to handle DB fields. It's safer, and could be less memory intensive if you don't need all the fields, instead of having to buffer all fields for every record in the select. This should be the normal style of RBDMS programming. ALWAYS use the standardized methods... By: nix (nix) 2004-04-25 07:33:41 Yes, tpsailer is correct, there can also be speed and memory benefits to not returning all fields if they are not needed. (In a VoIP billing application I wrote, I doubled the speed of the application simply by being selective about which fields I returned for each query) Speed however was not the motivation behind this patch, as I have not changed at all what is being returned. Quite simply, the existing query code is wrong, and should not be like it is. With my patch, it is no longer a problem to add extra columns to the SQL tables for external processing information etc. (This also means that extra columns can be added for Asterisk's internal usage, without having to modify the C code wrapping all the other queries that use the same table.) By: Olle Johansson (oej) 2004-04-25 09:10:09 I can't claim to be an SQL expert at all, but from reading your report this seems to be a good patch. By: nix (nix) 2004-04-25 10:49:28 There is another SQL related patch/addition at http://bugs.digium.com/bug_view_page.php?bug_id=0001482 I am going through all the perl and SQL code in Asterisk and cleaning it up. On another note, what is the plan for the handling of database abstraction? I have been looking at the current mysql in chan_sip and chan_iax as I want to port it to postgres but there is currently no defined way to add additional SQL driver support. I suggest that we need something similar to the way FreeRADIUS handles DB backends with a SQL driver which is DB independent, then individual MySQL, Postgres, Oracle, ODBC etc drivers which plug into it. Alternatively I could just copy the existing MySQL code and make it work instead of (or along side) Postgresql By: Olle Johansson (oej) 2004-04-25 11:57:12 There is a PostgreSQL patch in bugs, but we've stopped it in favour of a new architecture that is being developed right now, called res_data. It seems to be what you describe. When res_data and res_conf is done, we'll remove all #ifdef MYSQL stuff from the channel and voicemail. By: Mark Spencer (markster) 2004-04-26 01:02:54 Fixed in CVS.  Thanks! By: Digium Subversion (svnbot) 2008-01-15 14:51:53.000-0600 Repository: asterisk Revision: 2766 U   trunk/channels/chan_iax.c U   trunk/channels/chan_iax2.c U   trunk/channels/chan_sip.c ------------------------------------------------------------------------ r2766 | markster | 2008-01-15 14:51:53 -0600 (Tue, 15 Jan 2008) | 2 lines Clean up SQL queries (bug ASTERISK-1463) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=2766