[collectd] [PATCH] mysql plugin: add support for multipledatabases

[Doug MacEachern]

Hi Sebastian,

You're right about the database being option, I realized this after  
sending my last email.  I'd added the db->database == NULL check  
because it was being used as the name param in  
plugin_register_complex_read.  Then, given that most of (all?) the  
options are optional, and the plugin now supports monitoring multiple  
dbs that might be remote, a more clever name for complex_read would be  
required.  I see the current code will set the name to "mysql" if db- 
 >database == NULL, but that doesn't handle the case of:
<Database>
Host "foo"
</Database>
<Database>
Host "bar"
</Database>

Your idea to use `Instance' could be used to solve this.  How about  
the attached patch as a start?  Not sure if it should be required to  
have (Database || Instance), or just have a default value for Instance?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-mysql-plugin-database-name-is-optional.patch
Type: application/octet-stream
Size: 2669 bytes
Desc: not available
Url : http://mailman.verplant.org/pipermail/collectd/attachments/20090324/de306d77/attachment.obj 
-------------- next part --------------



On Mar 20, 2009, at 12:34 PM, Sebastian Harl wrote:

> Hi Doug and Florian,
>
> On Wed, Mar 18, 2009 at 12:51:24PM +0100, Florian Forster wrote:
>> On Tue, Mar 17, 2009 at 04:51:53PM -0400, Doug MacEachern wrote:
>>> db->database was NULL, which mysql_real_connect doesn't seem to  
>>> mind?
>>> +		db->database = strdup (db->instance);
>>
>> I didn't test without a `Database' setting. I *thought* it was  
>> optional,
>> though..
>
> According to the MySQL (5.0) reference manual, specifying a database  
> is
> optional when using mysql_real_connect(). If the respective  
> parameter is
> NULL, MySQL does not set a default database after connecting (which is
> perfectly fine, since the "SHOW * STATUS" queries are database
> independent).
>
> Doug, what was your motivation to not make that optional? Later on,
> there's the following check (in mysql_config()):
>
>  if (db->database == NULL)
>  {
>      ERROR ("mysql plugin: No `Database' configured");
>      status = -1;
>  }
>
> ... which is executed independent of legacy mode. This is a backward-
> incompatible change, since, previously, the "Database" option has not
> been mandatory.
>
> I guess, your motivation was something like ensuring a consistent
> behavior. If the argument of a <Database> block is used as instance  
> only
> (instead of specifying the name of a database as well), the behavior
> would be somewhat strange (and inconsistent with the other database
> plugins). What do you think about the following:
>
>  * Let the argument of a <Database> block be optional.
>
>  * If there is an argument, use it as database name (to be consistent
>    with the other plugins).
>
>  * Introduce a new option "Instance" - if that's not specified, use  
> the
>    database name as plugin instance (unless in legacy mode). (This
>    option would make sense in the postgresql and dbi plugins as well.)
>
>  * Either a database name or "Instance" would have to be specified  
> when
>    not in legacy mode to ensure that we end up having a plugin
>    instance.
>
>  * Possibly mark the "Database" _option_ inside a <Database> _block_  
> as
>    deprecated (or not allowed at all).
>
> Did I miss anything? What do you think? Imho, this would be the most
> straight-forward and consistent solution.
>
> Cheers,
> Sebastian
>
> -- 
> Sebastian "tokkee" Harl +++ GnuPG-ID: 0x8501C7FC +++ http:// 
> tokkee.org/
>
> Those who would give up Essential Liberty to purchase a little  
> Temporary
> Safety, deserve neither Liberty nor Safety.         -- Benjamin  
> Franklin
>