[Asterisk-code-review] func odbc: single database connection should be optional (asterisk[master])
Joshua Colp
asteriskteam at digium.com
Tue May 24 09:28:04 CDT 2016
Joshua Colp has submitted this change and it was merged.
Change subject: func_odbc: single database connection should be optional
......................................................................
func_odbc: single database connection should be optional
func_odbc was changed in Asterisk 13.9.0
to make func_odbc use a single database connection per DSN
because of reported bug ASTERISK-25938
with MySQL/MariaDB LAST_INSERT_ID().
This is drawback in performance when func_odbc is used
very often in dialplan.
Single database connection should be optional.
ASTERISK-26010
Change-Id: I7091783a7150252de8eeb455115bd00514dfe843
---
M CHANGES
M configs/samples/func_odbc.conf.sample
M funcs/func_odbc.c
3 files changed, 177 insertions(+), 60 deletions(-)
Approvals:
Mark Michelson: Looks good to me, but someone else must approve
Joshua Colp: Looks good to me, approved; Verified
diff --git a/CHANGES b/CHANGES
index ec44e30..0e99ab9 100644
--- a/CHANGES
+++ b/CHANGES
@@ -183,6 +183,8 @@
Functions
------------------
+ * The func_odbc global option "single_db_connection" default value has been
+ changed to 'no'.
CHANNEL
------------------
@@ -275,6 +277,12 @@
--- Functionality changes from Asterisk 13.9.0 to Asterisk 13.10.0 -----------
------------------------------------------------------------------------------
+func_odbc
+------------------
+ * Added new global option "single_db_connection".
+ Enabling this option func_odbc will use a single database connection per DSN.
+ This option is enabled by default.
+
res_fax
------------------
* Added FAXMODE variable to let dialplan know what fax transport was used.
diff --git a/configs/samples/func_odbc.conf.sample b/configs/samples/func_odbc.conf.sample
index fd528d2..f4d90b7 100644
--- a/configs/samples/func_odbc.conf.sample
+++ b/configs/samples/func_odbc.conf.sample
@@ -1,6 +1,20 @@
;
; func_odbc.conf
;
+[general]
+;
+; Asterisk uses separate connections for every database operation.
+; If single_db_connection is enabled then func_odbc will use a single
+; database connection per DSN.
+; This option exists for those who expect that a second func_odbc call
+; works on the same connection. That allows you to do a LAST_INSERT_ID()
+; in a second func_odbc call.
+; Note that you'll need additional dialplan locks for this behaviour to work.
+; There are better ways: using stored procedures/functions instead.
+; This option is disabled by default.
+;single_db_connection=no
+;
+;
; Each context is a separately defined function. By convention, all
; functions are entirely uppercase, so the defined contexts should also
; be all-uppercase, but there is nothing that enforces this. All functions
diff --git a/funcs/func_odbc.c b/funcs/func_odbc.c
index d8ffd90..489f373 100644
--- a/funcs/func_odbc.c
+++ b/funcs/func_odbc.c
@@ -101,6 +101,12 @@
static char *config = "func_odbc.conf";
+#define DEFAULT_SINGLE_DB_CONNECTION 0
+
+static int single_db_connection;
+
+AST_RWLOCK_DEFINE_STATIC(single_db_connection_lock);
+
enum odbc_option_flags {
OPT_ESCAPECOMMAS = (1 << 0),
OPT_MULTIROW = (1 << 1),
@@ -221,6 +227,10 @@
{
struct dsn *dsn;
+ if (!dsns) {
+ return NULL;
+ }
+
dsn = ao2_alloc(sizeof(*dsn) + strlen(name) + 1, dsn_destructor);
if (!dsn) {
return NULL;
@@ -296,6 +306,10 @@
{
struct dsn *dsn;
+ if (!dsns) {
+ return NULL;
+ }
+
ao2_lock(dsns);
dsn = ao2_find(dsns, name, OBJ_SEARCH_KEY | OBJ_NOLOCK);
if (!dsn) {
@@ -332,21 +346,55 @@
}
/*!
- * \brief Unlock and unreference a DSN
+ * \brief Get a DB handle via a DSN or directly
*
- * \param dsn The dsn to unlock and unreference
- * \return NULL
+ * If single db connection then get the DB handle via DSN
+ * else by requesting a connection directly
+ *
+ * \param dsn_name Name of the DSN as found in res_odbc.conf
+ * \param dsn The pointer to the DSN
+ * \retval NULL Unable to retrieve the DB handle
+ * \retval non-NULL The retrieved DB handle
*/
-static void *release_dsn(struct dsn *dsn)
+static struct odbc_obj *get_odbc_obj(const char *dsn_name, struct dsn **dsn)
{
- if (!dsn) {
- return NULL;
+ struct odbc_obj *obj = NULL;
+
+ ast_rwlock_rdlock(&single_db_connection_lock);
+ if (single_db_connection) {
+ if (dsn) {
+ *dsn = get_dsn(dsn_name);
+ if (*dsn) {
+ obj = (*dsn)->connection;
+ }
+ }
+ } else {
+ obj = ast_odbc_request_obj(dsn_name, 0);
}
+ ast_rwlock_unlock(&single_db_connection_lock);
- ao2_unlock(dsn);
- ao2_ref(dsn, -1);
+ return obj;
+}
- return NULL;
+/*!
+ * \brief Release an ODBC obj or a DSN
+ *
+ * If single db connection then unlock and unreference the DSN
+ * else release the ODBC obj
+ *
+ * \param obj The pointer to the ODBC obj to release
+ * \param dsn The pointer to the dsn to unlock and unreference
+ */
+static inline void release_obj_or_dsn(struct odbc_obj **obj, struct dsn **dsn)
+{
+ if (dsn && *dsn) {
+ ao2_unlock(*dsn);
+ ao2_ref(*dsn, -1);
+ *dsn = NULL;
+ } else if (obj && *obj) {
+ ast_odbc_release_obj(*obj);
+ *obj = NULL;
+ }
}
static AST_RWLIST_HEAD_STATIC(queries, acf_odbc_query);
@@ -568,19 +616,16 @@
if ((obj = ast_odbc_retrieve_transaction_obj(chan, query->writehandle[dsn_num]))) {
transactional = 1;
} else {
- dsn = get_dsn(query->writehandle[dsn_num]);
- if (!dsn) {
- continue;
- }
- obj = dsn->connection;
+ obj = get_odbc_obj(query->writehandle[dsn_num], &dsn);
transactional = 0;
}
if (obj && (stmt = ast_odbc_direct_execute(obj, generic_execute, ast_str_buffer(buf)))) {
break;
}
-
- dsn = release_dsn(dsn);
+ if (!transactional) {
+ release_obj_or_dsn (&obj, &dsn);
+ }
}
}
@@ -593,25 +638,23 @@
status = "SUCCESS";
} else if (query->sql_insert) {
- dsn = release_dsn(dsn);
+ if (!transactional) {
+ release_obj_or_dsn (&obj, &dsn);
+ }
for (transactional = 0, dsn_num = 0; dsn_num < 5; dsn_num++) {
if (!ast_strlen_zero(query->writehandle[dsn_num])) {
if (transactional) {
/* This can only happen second time through or greater. */
ast_log(LOG_WARNING, "Transactions do not work well with multiple DSNs for 'writehandle'\n");
- } else if (obj) {
- dsn = release_dsn(dsn);
+ } else {
+ release_obj_or_dsn (&obj, &dsn);
}
if ((obj = ast_odbc_retrieve_transaction_obj(chan, query->writehandle[dsn_num]))) {
transactional = 1;
} else {
- dsn = get_dsn(query->writehandle[dsn_num]);
- if (!dsn) {
- continue;
- }
- obj = dsn->connection;
+ obj = get_odbc_obj(query->writehandle[dsn_num], &dsn);
transactional = 0;
}
if (obj) {
@@ -641,7 +684,9 @@
pbx_builtin_setvar_helper(chan, "ODBCSTATUS", status);
}
- dsn = release_dsn(dsn);
+ if (!transactional) {
+ release_obj_or_dsn (&obj, &dsn);
+ }
if (!bogus_chan) {
ast_autoservice_stop(chan);
@@ -652,6 +697,7 @@
static int acf_odbc_read(struct ast_channel *chan, const char *cmd, char *s, char *buf, size_t len)
{
+ struct odbc_obj *obj = NULL;
struct acf_odbc_query *query;
char varname[15], rowcount[12] = "-1";
struct ast_str *colnames = ast_str_thread_get(&colnames_buf, 16);
@@ -757,21 +803,21 @@
for (dsn_num = 0; dsn_num < 5; dsn_num++) {
if (!ast_strlen_zero(query->readhandle[dsn_num])) {
- dsn = get_dsn(query->readhandle[dsn_num]);
- if (!dsn) {
+ obj = get_odbc_obj(query->readhandle[dsn_num], &dsn);
+ if (!obj) {
continue;
}
- stmt = ast_odbc_direct_execute(dsn->connection, generic_execute, ast_str_buffer(sql));
+ stmt = ast_odbc_direct_execute(obj, generic_execute, ast_str_buffer(sql));
}
if (stmt) {
break;
}
- dsn = release_dsn(dsn);
+ release_obj_or_dsn (&obj, &dsn);
}
if (!stmt) {
ast_log(LOG_ERROR, "Unable to execute query [%s]\n", ast_str_buffer(sql));
- dsn = release_dsn(dsn);
+ release_obj_or_dsn (&obj, &dsn);
if (!bogus_chan) {
pbx_builtin_setvar_helper(chan, "ODBCROWS", rowcount);
ast_autoservice_stop(chan);
@@ -785,7 +831,7 @@
ast_log(LOG_WARNING, "SQL Column Count error!\n[%s]\n\n", ast_str_buffer(sql));
SQLCloseCursor(stmt);
SQLFreeHandle (SQL_HANDLE_STMT, stmt);
- dsn = release_dsn(dsn);
+ release_obj_or_dsn (&obj, &dsn);
if (!bogus_chan) {
pbx_builtin_setvar_helper(chan, "ODBCROWS", rowcount);
ast_autoservice_stop(chan);
@@ -809,7 +855,7 @@
}
SQLCloseCursor(stmt);
SQLFreeHandle(SQL_HANDLE_STMT, stmt);
- dsn = release_dsn(dsn);
+ release_obj_or_dsn (&obj, &dsn);
if (!bogus_chan) {
pbx_builtin_setvar_helper(chan, "ODBCROWS", rowcount);
pbx_builtin_setvar_helper(chan, "ODBCSTATUS", status);
@@ -832,7 +878,7 @@
odbc_datastore_free(resultset);
SQLCloseCursor(stmt);
SQLFreeHandle(SQL_HANDLE_STMT, stmt);
- dsn = release_dsn(dsn);
+ release_obj_or_dsn (&obj, &dsn);
if (!bogus_chan) {
pbx_builtin_setvar_helper(chan, "ODBCSTATUS", "MEMERROR");
ast_autoservice_stop(chan);
@@ -864,7 +910,7 @@
odbc_datastore_free(resultset);
SQLCloseCursor(stmt);
SQLFreeHandle(SQL_HANDLE_STMT, stmt);
- dsn = release_dsn(dsn);
+ release_obj_or_dsn (&obj, &dsn);
if (!bogus_chan) {
pbx_builtin_setvar_helper(chan, "ODBCROWS", rowcount);
pbx_builtin_setvar_helper(chan, "ODBCSTATUS", "MEMERROR");
@@ -973,7 +1019,7 @@
odbc_datastore_free(resultset);
SQLCloseCursor(stmt);
SQLFreeHandle(SQL_HANDLE_STMT, stmt);
- dsn = release_dsn(dsn);
+ release_obj_or_dsn (&obj, &dsn);
pbx_builtin_setvar_helper(chan, "ODBCSTATUS", "MEMERROR");
ast_autoservice_stop(chan);
return -1;
@@ -986,7 +1032,7 @@
}
SQLCloseCursor(stmt);
SQLFreeHandle(SQL_HANDLE_STMT, stmt);
- dsn = release_dsn(dsn);
+ release_obj_or_dsn (&obj, &dsn);
if (resultset && !multirow) {
/* Fetch the first resultset */
if (!acf_fetch(chan, "", buf, buf, len)) {
@@ -1413,6 +1459,7 @@
if (a->argc == 5 && !strcmp(a->argv[4], "exec")) {
/* Execute the query */
+ struct odbc_obj *obj = NULL;
struct dsn *dsn = NULL;
int dsn_num, executed = 0;
SQLHSTMT stmt;
@@ -1432,14 +1479,14 @@
if (ast_strlen_zero(query->readhandle[dsn_num])) {
continue;
}
- dsn = get_dsn(query->readhandle[dsn_num]);
- if (!dsn) {
+ obj = get_odbc_obj(query->readhandle[dsn_num], &dsn);
+ if (!obj) {
continue;
}
ast_debug(1, "Found handle %s\n", query->readhandle[dsn_num]);
- if (!(stmt = ast_odbc_direct_execute(dsn->connection, generic_execute, ast_str_buffer(sql)))) {
- dsn = release_dsn(dsn);
+ if (!(stmt = ast_odbc_direct_execute(obj, generic_execute, ast_str_buffer(sql)))) {
+ release_obj_or_dsn (&obj, &dsn);
continue;
}
@@ -1450,7 +1497,7 @@
ast_cli(a->fd, "SQL Column Count error!\n[%s]\n\n", ast_str_buffer(sql));
SQLCloseCursor(stmt);
SQLFreeHandle (SQL_HANDLE_STMT, stmt);
- dsn = release_dsn(dsn);
+ release_obj_or_dsn (&obj, &dsn);
AST_RWLIST_UNLOCK(&queries);
return CLI_SUCCESS;
}
@@ -1459,7 +1506,7 @@
if ((res != SQL_SUCCESS) && (res != SQL_SUCCESS_WITH_INFO)) {
SQLCloseCursor(stmt);
SQLFreeHandle(SQL_HANDLE_STMT, stmt);
- dsn = release_dsn(dsn);
+ release_obj_or_dsn (&obj, &dsn);
if (res == SQL_NO_DATA) {
ast_cli(a->fd, "Returned %d rows. Query executed on handle %d:%s [%s]\n", rows, dsn_num, query->readhandle[dsn_num], ast_str_buffer(sql));
break;
@@ -1488,7 +1535,7 @@
ast_cli(a->fd, "SQL Get Data error %d!\n[%s]\n\n", res, ast_str_buffer(sql));
SQLCloseCursor(stmt);
SQLFreeHandle(SQL_HANDLE_STMT, stmt);
- dsn = release_dsn(dsn);
+ release_obj_or_dsn (&obj, &dsn);
AST_RWLIST_UNLOCK(&queries);
return CLI_SUCCESS;
}
@@ -1506,11 +1553,11 @@
}
SQLCloseCursor(stmt);
SQLFreeHandle(SQL_HANDLE_STMT, stmt);
- dsn = release_dsn(dsn);
+ release_obj_or_dsn (&obj, &dsn);
ast_cli(a->fd, "Returned %d row%s. Query executed on handle %d [%s]\n", rows, rows == 1 ? "" : "s", dsn_num, query->readhandle[dsn_num]);
break;
}
- dsn = release_dsn(dsn);
+ release_obj_or_dsn (&obj, &dsn);
if (!executed) {
ast_cli(a->fd, "Failed to execute query. [%s]\n", ast_str_buffer(sql));
@@ -1633,7 +1680,8 @@
if (a->argc == 6 && !strcmp(a->argv[5], "exec")) {
/* Execute the query */
- struct dsn *dsn;
+ struct odbc_obj *obj = NULL;
+ struct dsn *dsn = NULL;
int dsn_num, executed = 0;
SQLHSTMT stmt;
SQLLEN rows = -1;
@@ -1642,19 +1690,19 @@
if (ast_strlen_zero(query->writehandle[dsn_num])) {
continue;
}
- dsn = get_dsn(query->writehandle[dsn_num]);
- if (!dsn) {
+ obj = get_odbc_obj(query->writehandle[dsn_num], &dsn);
+ if (!obj) {
continue;
}
- if (!(stmt = ast_odbc_direct_execute(dsn->connection, generic_execute, ast_str_buffer(sql)))) {
- dsn = release_dsn(dsn);
+ if (!(stmt = ast_odbc_direct_execute(obj, generic_execute, ast_str_buffer(sql)))) {
+ release_obj_or_dsn (&obj, &dsn);
continue;
}
SQLRowCount(stmt, &rows);
SQLCloseCursor(stmt);
SQLFreeHandle(SQL_HANDLE_STMT, stmt);
- dsn = release_dsn(dsn);
+ release_obj_or_dsn (&obj, &dsn);
ast_cli(a->fd, "Affected %d rows. Query executed on handle %d [%s]\n", (int)rows, dsn_num, query->writehandle[dsn_num]);
executed = 1;
break;
@@ -1680,30 +1728,47 @@
int res = 0;
struct ast_config *cfg;
char *catg;
+ const char *s;
struct ast_flags config_flags = { 0 };
-
- dsns = ao2_container_alloc(DSN_BUCKETS, dsn_hash, dsn_cmp);
- if (!dsns) {
- return AST_MODULE_LOAD_DECLINE;
- }
res |= ast_custom_function_register(&fetch_function);
res |= ast_register_application_xml(app_odbcfinish, exec_odbcfinish);
- AST_RWLIST_WRLOCK(&queries);
cfg = ast_config_load(config, config_flags);
if (!cfg || cfg == CONFIG_STATUS_FILEINVALID) {
ast_log(LOG_NOTICE, "Unable to load config for func_odbc: %s\n", config);
- AST_RWLIST_UNLOCK(&queries);
- ao2_ref(dsns, -1);
return AST_MODULE_LOAD_DECLINE;
}
+ ast_rwlock_wrlock(&single_db_connection_lock);
+ if ((s = ast_variable_retrieve(cfg, "general", "single_db_connection"))) {
+ single_db_connection = ast_true(s);
+ } else {
+ single_db_connection = DEFAULT_SINGLE_DB_CONNECTION;
+ }
+
+ dsns = NULL;
+
+ if (single_db_connection) {
+ dsns = ao2_container_alloc(DSN_BUCKETS, dsn_hash, dsn_cmp);
+ if (!dsns) {
+ ast_log(LOG_ERROR, "Could not initialize DSN container\n");
+ ast_rwlock_unlock(&single_db_connection_lock);
+ return AST_MODULE_LOAD_DECLINE;
+ }
+ }
+ ast_rwlock_unlock(&single_db_connection_lock);
+
+ AST_RWLIST_WRLOCK(&queries);
for (catg = ast_category_browse(cfg, NULL);
catg;
catg = ast_category_browse(cfg, catg)) {
struct acf_odbc_query *query = NULL;
int err;
+
+ if (!strcasecmp(catg, "general")) {
+ continue;
+ }
if ((err = init_acf_query(cfg, catg, &query))) {
if (err == ENOMEM)
@@ -1750,7 +1815,9 @@
AST_RWLIST_UNLOCK(&queries);
- ao2_ref(dsns, -1);
+ if (dsns) {
+ ao2_ref(dsns, -1);
+ }
return res;
}
@@ -1760,11 +1827,35 @@
struct ast_config *cfg;
struct acf_odbc_query *oldquery;
char *catg;
+ const char *s;
struct ast_flags config_flags = { CONFIG_FLAG_FILEUNCHANGED };
cfg = ast_config_load(config, config_flags);
if (cfg == CONFIG_STATUS_FILEUNCHANGED || cfg == CONFIG_STATUS_FILEINVALID)
return 0;
+
+ ast_rwlock_wrlock(&single_db_connection_lock);
+
+ if (dsns) {
+ ao2_ref(dsns, -1);
+ dsns = NULL;
+ }
+
+ if (cfg && (s = ast_variable_retrieve(cfg, "general", "single_db_connection"))) {
+ single_db_connection = ast_true(s);
+ } else {
+ single_db_connection = DEFAULT_SINGLE_DB_CONNECTION;
+ }
+
+ if (single_db_connection) {
+ dsns = ao2_container_alloc(DSN_BUCKETS, dsn_hash, dsn_cmp);
+ if (!dsns) {
+ ast_log(LOG_ERROR, "Could not initialize DSN container\n");
+ ast_rwlock_unlock(&single_db_connection_lock);
+ return 0;
+ }
+ }
+ ast_rwlock_unlock(&single_db_connection_lock);
AST_RWLIST_WRLOCK(&queries);
@@ -1784,6 +1875,10 @@
catg = ast_category_browse(cfg, catg)) {
struct acf_odbc_query *query = NULL;
+ if (!strcasecmp(catg, "general")) {
+ continue;
+ }
+
if (init_acf_query(cfg, catg, &query)) {
ast_log(LOG_ERROR, "Cannot initialize query %s\n", catg);
} else {
--
To view, visit https://gerrit.asterisk.org/2820
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I7091783a7150252de8eeb455115bd00514dfe843
Gerrit-PatchSet: 4
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
More information about the asterisk-code-review
mailing list