| Summary: | ASTERISK-12399: [patch] several fixes in res_config_sqlite | ||
| Reporter: | Guillaume Knispel (gknispel_proformatique) | Labels: | |
| Date Opened: | 2008-07-17 08:06:24 | Date Closed: | 2008-07-30 12:24:14 |
| Priority: | Minor | Regression? | No |
| Status: | Closed/Complete | Components: | Resources/res_config_sqlite |
| Versions: | Frequency of Occurrence | ||
| Related Issues: | |||
| Environment: | Attachments: | ( 0) 20080729__bug13097.diff.txt ( 1) 20080730__bug13097.diff.txt ( 2) res_config_sqlite_const_calldate_disposition.patch | |
| Description: | The attached patch fixes three things: 1) Some const qualifiers are added on static constant strings. 2) The schema of the CDR table is changed to something more similar to other CDR backends. (start is replaced by calldate, which is at the beginning) 3) ast_cdr_disp2str(cdr->disposition) is stored in the disposition column (as in other CDR backends) instead of the numeric code cdr->disposition | ||
| Comments: | By: Tilghman Lesher (tilghman) 2008-07-17 08:14:06 It is perfectly valid to use 'start' as the CDR field. Additionally, this change is not backwards-compatible, so I have to reject the patch. By: Guillaume Knispel (gknispel_proformatique) 2008-07-17 09:19:34 About the backward compatibility, this module was not present in Asterisk 1.4. By: Tilghman Lesher (tilghman) 2008-07-17 10:41:36 Yes, but there are people already using 1.6.0, where this functionality is present. There is a change I would accept, however. If you want to add adaptive capabilities to the CDR driver, which adapts the data that we insert into the table as it exists, then that would be a welcome change. By: Guillaume Knispel (gknispel_proformatique) 2008-07-17 10:55:10 Ok, I'll see what I can do. By: Donny Kavanagh (donnyk) 2008-07-17 21:29:16 A better question would be why the cdr code is in res_config_sqllite to begin with rather then cdr_sqllite like it should be. By: Guillaume Knispel (gknispel_proformatique) 2008-07-18 10:16:04 Some reasons were given in commentaries. The "training" reason is not a valid reason anymore to keep the code here. The second one, to have the same database open at only one place for all kinds of operations, could be, but I don't know what really happen in the real life. The rational behind the claim of reliability and performance improvement is that SQLite does not support simultaneous write operations (the whole base is locked) so under high pressure there is a theoritical risk that RES_CONFIG_SQLITE_BEGIN / RES_CONFIG_SQLITE_END loop is used more that once, and under very high pressure that it loops until RES_CONFIG_SQLITE_MAX_LOOPS then fails. This is completely impossible if the only access to the same base are serialized with the same mutex. One possible solution could be that res_config_sqlite exports its mutex that is then used by cdr_sqlite, but this introduce a dependency that is IMHO not better that having all the code in res_config_sqlite. Another solution could be to introduce a new sqlite "middleware" module that would be used by both cdr_sqlite and res_config_sqlite but this complicate things and maybe this is even technically impossible now because i don't think there is a way to describe and enforce dependencies between modules. The volume of code in res_config_sqlite is probably too small to complicate things so much. By: Tilghman Lesher (tilghman) 2008-07-29 17:23:17 Patch uploaded to make res_config_sqlite adaptive for CDRs. Note that this change also makes disposition and amaflags string values. By: Tilghman Lesher (tilghman) 2008-07-30 12:07:48 Now also added require_func and unload_func support. By: Digium Subversion (svnbot) 2008-07-30 12:24:11 Repository: asterisk Revision: 134442 U trunk/res/res_config_sqlite.c ------------------------------------------------------------------------ r134442 | tilghman | 2008-07-30 12:24:10 -0500 (Wed, 30 Jul 2008) | 7 lines Add adaptive capabilities to the sqlite realtime driver (closes issue ASTERISK-12399) Reported by: gknispel_proformatique Patches: 20080730__bug13097.diff.txt uploaded by Corydon76 (license 14) Tested by: Corydon76 ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=134442 | ||