[Asterisk-code-review] res odbc: Implement a connection pool. (asterisk[master])

[Mark Michelson]

Mark Michelson has posted comments on this change.

Change subject: res_odbc: Implement a connection pool.
......................................................................


Patch Set 2: Code-Review-1

(5 comments)

https://gerrit.asterisk.org/#/c/2945/2/res/res_odbc.c
File res/res_odbc.c:

Line 564: 					if (sscanf(v->value, "%d", &maxconnections) != 1 || maxconnections < 1) {
*puts on Richard hat*

Should probably use "%30d" here


Line 788: 	return ((res != SQL_SUCCESS) && (res != SQL_SUCCESS_WITH_INFO)) ? 1 : 0;
unixODBC provides a macro called SQL_SUCCEEDED(res) that exapnds out to (res == SQL_SUCCESS || res == SQL_SUCCESS_WITH_INFO). So you can just do:

    return SQL_SUCCEEDED(res) ? 0 : 1

or if you prefer the negative version

    return !SQL_SUCCEEDED(res) ? 1: : 0


Line 826: 				ast_debug(2, "Created ODBC handle %p\n", obj);
Suggestion: Add the ODBC class name and the current connection count to this debug. It could be useful if we're trying to debug an issue where the connection pool is not operating as planned, and it could also be a good indicator if the pool is hovering at its maximum connection count all the time.


Line 838: 			ast_debug(2, "ODBC handle %p dead - removing\n", obj);
Like with the previous suggestion, I think the ODBC class name and connection count would be useful information here.


PS2, Line 834: 		} else if (connection_dead(obj, class)) {
             : 			/* If the connection is dead try to grab another functional one from the
             : 			 * pool instead of trying to resurrect this one.
             : 			 */
             : 			ast_debug(2, "ODBC handle %p dead - removing\n", obj);
             : 			ao2_ref(obj, -1);
             : 			obj = NULL;
             : 			class->connection_cnt--;
Two Questions:

1) Does this path leak a reference to obj->parent?
2) If the connection is dead, and we detect it, do we need to call SQLDisconnect()? I don't actually know, and Googling has not been helpful. However, my thought process is that SQLConnect() in addition to creating a TCP connection to the DB may also have allocated other bits of data. Just because the TCP connection is dead, I'm not sure if the other hypothetical allocations are cleared up automatically.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6774bf4bac49a0b30242c76a09c403d2e856ecff
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: Yes