[Home]

Summary:ASTERISK-04846: [patch] res_config_mysql ignores database specified in extconfig.conf
Reporter:John Riordan (john)Labels:
Date Opened:2005-08-16 14:10:31Date Closed:2005-10-13 16:51:24
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/Configuration
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) res_config_mysql_2.0.patch.txt
( 1) res_config_mysql_2.0A.patch.txt
Description:The current implementation ignores the database specified in extconfig.conf. It makes requests to the database specified in res_mysql.conf regardless of the database specified of extconfig.conf.


****** ADDITIONAL INFORMATION ******

The patch simply adds the database to the tbl_name in the queries. This works as of MySQL release 3.22.0 (18 May 1998) see...
http://dev.mysql.com/doc/mysql/en/news-3-22-0.html

Comments:By: Michael Jerris (mikej) 2005-08-16 15:02:30

can we do this to be backwards compatible if no db is specified, to use the one in res_mysql.conf. It should be quite trivial, then it can work either way.
By: John Riordan (john) 2005-08-17 11:01:07

I believe the patch is backwards compatible.

If the database is specified as "", the queries will default to the db specified in res_mysql.conf (as far as MySQL is concerned, 'FROM .table' and 'FROM table' are the same thing).

There doesn't appear to be a case where the database can be validly passed to the registered functions as NULL (at least that's the way config.c looks to me).

A case where it is not "backwards compatible" is if someone has specified different databases in extconfig.conf and res_mysql.conf. The way it currently works, res_config_mysql ignores whatever is specified extconfig.conf. The patched version uses the database specified in extconfig.conf, thus it could break a configuration where someone has specified different databases in the two config files. But I was thinking this was a minor bug in the existing - not a "feature" that one can specify anything for database in extconfig.conf and have it ignored.

Regardless, we can't have it both ways - if a database is specified in extconfig.conf we can't use it and ignore it at the same time.


By: Michael Jerris (mikej) 2005-08-17 11:09:50

I didn't realize you can do .table, that satisfies my concern.. if somone puts a diff table in the 2 diff conf files, I think that is not an issue to worry about.
By: drmac (drmac) 2005-08-18 11:50:39

damn..nice catch. let me look at this and see what I can do.
By: drmac (drmac) 2005-08-18 11:57:08

using the dot method works but is inefficient cause my driver will open the database specified in res_mysql.conf by default and then open any other databases you use with dot while keeping open the original database.

mysql_select_db() is probably best to use before doing each SELECT.

still lookin...
By: drmac (drmac) 2005-08-19 11:12:34

Try res_config_mysql_1.9.patch.txt

The first connection established by the driver uses the database specified in res_mysql.conf

Whenever an ARA request is called, it will switch to the database specified in extconfig.conf for that particular family.
By: drmac (drmac) 2005-08-23 08:35:12

john, any news?
By: John Riordan (john) 2005-08-23 11:15:01

One bug in 1.9 patch - line 377 is removed - doesn't look correct to me.

The only other concern I have is with a potential race condition. It looks like you could have one thread switch databases and before said thread gets a chance to make a query another thread could come through and switch the database to something else - leading to random query failures. I think you might need to include the mysql_select_db within the same lock as the mysql_real_query (etc) to avoid the race condition.

Generally however, I like the mysql_select_db approach better than my approach - aside from your stated reasons, I think the code comes out clearer about what is going on.
By: drmac (drmac) 2005-08-23 11:30:29

I'm looking at both 1.8 and 1.9 versions at line 377 and I don't see what you are referring to.

Let me look at the locks again.
By: drmac (drmac) 2005-08-23 11:36:56

oh wow. i see the missing lock you are talking about. crap. that was missing from 1.8 as well.
By: John Riordan (john) 2005-08-23 11:55:44

In this block of the patch, there is a '-' which removes the ast_mutex_lock line.

@@ -370,11 +373,10 @@
ast_log(LOG_DEBUG, "MySQL RealTime: Static SQL: %s\n", sql);

/* We now have our complete statement; Lets connect to the server and execute it. */
- if(!mysql_reconnect()) {
+ if(!mysql_reconnect(database)) {
return NULL;
}

- ast_mutex_lock(&mysql_lock);
if(mysql_real_query(&mysql, sql, strlen(sql))) {
ast_log(LOG_WARNING, "MySQL RealTime: Failed to query database. Check debug for more info.\n");
ast_log(LOG_DEBUG, "MySQL RealTime: Query: %s\n", sql);
By: drmac (drmac) 2005-08-26 13:34:58

OK. The 1.9 patch will remove a bunch of mutex calls in favor of reducing duplicated code.

When mysql_reconnect is called, the lock will be created in that function. if that function (re)connects successfully and changes databases successfully then the lock remains when the function returns and allows the query to execute under lock.

If mysql_reconnect fails for any reason, the lock is released because the calling function simply returns -1 (or NULL).
By: Kevin P. Fleming (kpfleming) 2005-08-26 16:33:28

Committed to CVS HEAD, thanks!
By: Clod Patry (junky) 2005-10-06 08:18:28

Dio_ wants to add comment here.
By: Dmytro Mishchenko (arkadia) 2005-10-06 08:26:59

drmac says: "When mysql_reconnect is called, the lock will be created in that function. if that function (re)connects successfully and changes databases successfully then the lock remains when the function returns and allows the query to execute under lock."

According 1.9 version code, when mysql_reconnect returns with success the mutex is unlocked. And later on in config_mysql() you call mysql_real_query without lock. Then you release non existent lock.

So it seems the line "ast_mutex_lock(&mysql_lock);" in config_mysql() was removed incorrectly.
By: drmac (drmac) 2005-10-07 12:18:50

seems like i missed more than 1 or 2 mutex changes. thanks for catching this. try the 2.0 patch.

TO: kevin or whoever applies this to CVS (after arkadia verifies it):
  if 2.0 is not the next CVS auto-increment version #, can you please change the version history in the patch to reflect the next number? i'd like to keep them in sync. thanks.
By: Dmytro Mishchenko (arkadia) 2005-10-10 05:26:18

There are still problems at  load_module(). If mysql_reconnect() fails you
are trying to unlock mutex that was already unlocked.

Another problem is in reload(). On successful mysql_reconnect() you keep
mutex locked! So nobody will be able to obtain lock. The same problem is in
realtime_mysql_status().


I think much better idea will be to move work with the mutex out of
mysql_reconnect() up to one level. So realtime_multi_mysql() will look like:

ast_mutex_lock(&mysql_lock);
mysql_reconnect()
...
mysql_real_query(..)
..
mysql_store_result(..)
..
ast_mutex_unlock(&mysql_lock);
By: drmac (drmac) 2005-10-11 11:41:13

doing how you suggest was how i originally had it. but then i noticed a lot of code repetition so i figured i'd streamline it. turns out that is harder than it looks.
By: drmac (drmac) 2005-10-11 11:53:48

try res_config_mysql_2.0A.patch.txt.

Can someone delete res_config_mysql_2.0.patch.txt?
By: Kevin P. Fleming (kpfleming) 2005-10-13 16:50:34

Committed to CVS HEAD... please don't add comments to the file that don't add any value (and just repeat what is already in the CVS log and in the bug tracker). The next revision number was not 2.0, but I'm not going to go back and change it :-)