[Asterisk-code-review] func odbc: single database connection should be optional (asterisk[13])

[Walter Doekes]

Walter Doekes has posted comments on this change.

Change subject: func_odbc: single database connection should be optional
......................................................................


Patch Set 2: Code-Review-1

(6 comments)

Thanks for this patch. I assume you mainly reverted the ASTERISK-25938 code and added conditionals around them, so that should be good.

However, I'm not fond of the large scale code copy-pasting. Adding one single function for the release should alleviate most of my qualms with regards to that.

Further, I've noted below that we cannot change the behaviour to multiple-dsn in 13, as behaviour shouldn't change mid-branch. But I certainly agree with changing the default in master.

https://gerrit.asterisk.org/#/c/2800/2/CHANGES
File CHANGES:

Line 19:    This option is disabled by default.
For Asterisk 13, I don't think we can disable it again. Behaviour should remain consistent.

While I do agree that disabled is the right default. This default should only go in master. Go with 'enabled' by default in 13, and clearly mark the change from enabled to disabled in UPGRADE.txt in the patch to the master branch.


https://gerrit.asterisk.org/#/c/2800/2/configs/samples/func_odbc.conf.sample
File configs/samples/func_odbc.conf.sample:

Line 8: ; This option is disabled by default.
As I said, enabled by default in 13.

Further, I'd amend this with a short reasoning behind the option, e.g.:
""" 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. """


https://gerrit.asterisk.org/#/c/2800/2/funcs/func_odbc.c
File funcs/func_odbc.c:

Line 104: #define DEFAULT_SINGLE_DB_CONNECTION 0
1 in 13.


Line 106: static int single_db_connection = DEFAULT_SINGLE_DB_CONNECTION;
I'd drop the initialization here and move it to load/reload instead, see below.


PS2, Line 590: 			if (single_db_connection) {
             : 				dsn = release_dsn(dsn);
             : 			} else {
             : 				if (obj && !transactional) {
             : 					ast_odbc_release_obj(obj);
             : 					obj = NULL;
             : 				}
             : 			}
This bit occurs quite often in the source, maybe add a static (inline) function for it?

    release_obj_or_dsn(&obj, !transactional, &dsn);


Line 1873: 	}
I'd do the same as in reload. It worked like you did, but this keeps the differences between load/reload down (perhaps parts of them could be merged at some point).


	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;
	}


-- 
To view, visit https://gerrit.asterisk.org/2800
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I57d990616c957dabf7597dea5d5c3148f459dfb6
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Alexei Gradinari <alex2grad at gmail.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Walter Doekes <walter+asterisk at wjd.nu>
Gerrit-HasComments: Yes