Module: sip-router Branch: master Commit: 6fc84c2cf610791939ba73e38b8b5b3c0b5cd047 URL: http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=6fc84c2c...
Author: Juha Heinanen jh@tutpro.com Committer: Juha Heinanen jh@tutpro.com Date: Wed Oct 30 17:31:04 2013 +0200
modules/mtree: when loading data from db, load each tree separately
---
modules/mtree/mtree_mod.c | 13 ++++++++++++- 1 files changed, 12 insertions(+), 1 deletions(-)
diff --git a/modules/mtree/mtree_mod.c b/modules/mtree/mtree_mod.c index 0f4348c..eeec89c 100644 --- a/modules/mtree/mtree_mod.c +++ b/modules/mtree/mtree_mod.c @@ -291,6 +291,9 @@ static int mod_init(void)
while(pt!=NULL) { + LM_DBG("loading from tree <%.*s>\n", + pt->tname.len, pt->tname.s); + /* loading all information from database */ if(mt_load_db(&pt->tname)!=0) { @@ -491,6 +494,9 @@ error: static int mt_load_db(str *tname) { db_key_t db_cols[3] = {&tprefix_column, &tvalue_column}; + db_key_t key_cols[1]; + db_op_t op[1] = {OP_EQ}; + db_val_t vals[1]; str tprefix, tvalue; db1_res_t* db_res = NULL; int i, ret; @@ -498,6 +504,11 @@ static int mt_load_db(str *tname) m_tree_t *old_tree = NULL; mt_node_t *bk_head = NULL;
+ key_cols[0] = &tname_column; + VAL_TYPE(vals) = DB1_STRING; + VAL_NULL(vals) = 0; + VAL_STRING(vals) = tname->s; + if(db_con==NULL) { LM_ERR("no db connection\n"); @@ -521,7 +532,7 @@ static int mt_load_db(str *tname) }
if (DB_CAPABILITY(mt_dbf, DB_CAP_FETCH)) { - if(mt_dbf.query(db_con, 0, 0, 0, db_cols, 0, 2, 0, 0) < 0) + if(mt_dbf.query(db_con, key_cols, op, vals, db_cols, 1, 2, 0, 0) < 0) { LM_ERR("Error while querying db\n"); return -1;
Hello,
this doesn't look correct.
mtree module works with two types of database tables: - one having only (tprefix, tvalue) - one having also tree name (tname, tprefix, tvalue)
Your change seems to affect the first case, which is supposed not to have tname column, thus not working.
If you keep all the trees in the same database table, you should set db table parameter for the module: - http://kamailio.org/docs/modules/stable/modules/mtree.html#idp2546944
Check also the utils/kamctl/mysql/mtree-create.sql to see the structure of those database tables.
Cheers, Daniel
On 10/30/13 6:37 PM, Juha Heinanen wrote:
Module: sip-router Branch: master Commit: 6fc84c2cf610791939ba73e38b8b5b3c0b5cd047 URL: http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=6fc84c2c...
Author: Juha Heinanen jh@tutpro.com Committer: Juha Heinanen jh@tutpro.com Date: Wed Oct 30 17:31:04 2013 +0200
modules/mtree: when loading data from db, load each tree separately
modules/mtree/mtree_mod.c | 13 ++++++++++++- 1 files changed, 12 insertions(+), 1 deletions(-)
diff --git a/modules/mtree/mtree_mod.c b/modules/mtree/mtree_mod.c index 0f4348c..eeec89c 100644 --- a/modules/mtree/mtree_mod.c +++ b/modules/mtree/mtree_mod.c @@ -291,6 +291,9 @@ static int mod_init(void)
while(pt!=NULL) {
LM_DBG("loading from tree <%.*s>\n",pt->tname.len, pt->tname.s);/* loading all information from database */ if(mt_load_db(&pt->tname)!=0) {@@ -491,6 +494,9 @@ error: static int mt_load_db(str *tname) { db_key_t db_cols[3] = {&tprefix_column, &tvalue_column};
- db_key_t key_cols[1];
- db_op_t op[1] = {OP_EQ};
- db_val_t vals[1]; str tprefix, tvalue; db1_res_t* db_res = NULL; int i, ret;
@@ -498,6 +504,11 @@ static int mt_load_db(str *tname) m_tree_t *old_tree = NULL; mt_node_t *bk_head = NULL;
- key_cols[0] = &tname_column;
- VAL_TYPE(vals) = DB1_STRING;
- VAL_NULL(vals) = 0;
- VAL_STRING(vals) = tname->s;
- if(db_con==NULL) { LM_ERR("no db connection\n");
@@ -521,7 +532,7 @@ static int mt_load_db(str *tname) }
if (DB_CAPABILITY(mt_dbf, DB_CAP_FETCH)) {
if(mt_dbf.query(db_con, 0, 0, 0, db_cols, 0, 2, 0, 0) < 0)
if(mt_dbf.query(db_con, key_cols, op, vals, db_cols, 1, 2, 0, 0) < 0)
sr-dev mailing list sr-dev@lists.sip-router.org http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
Daniel-Constantin Mierla writes:
mtree module works with two types of database tables:
- one having only (tprefix, tvalue)
- one having also tree name (tname, tprefix, tvalue)
Your change seems to affect the first case, which is supposed not to have tname column, thus not working.
If you keep all the trees in the same database table, you should set db table parameter for the module:
it reads:
3.2. db_table (string)
Name of database table where data for trees is stored. It is ignored if a 'mtree' parameter is defined.
since i have defined two 'mtree' parameters, db_table param has no meaning.
in my tests, the change that i made has worked fine. in which case would it fail?
-- juha
On 10/30/13 4:45 PM, Juha Heinanen wrote:
Daniel-Constantin Mierla writes:
mtree module works with two types of database tables:
- one having only (tprefix, tvalue)
- one having also tree name (tname, tprefix, tvalue)
Your change seems to affect the first case, which is supposed not to have tname column, thus not working.
If you keep all the trees in the same database table, you should set db table parameter for the module:
it reads:
3.2. db_table (string)
Name of database table where data for trees is stored. It is ignored if a 'mtree' parameter is defined.
since i have defined two 'mtree' parameters, db_table param has no meaning.
in my tests, the change that i made has worked fine. in which case would it fail?
if you define the trees with module parameters, then you have to use different tables in database, one for each. Those tables don't need tname column.
If you just store all the trees in one table (which has tname column), then don't define them in the config, just set the table where they are stored. A reload command will reload all the trees in that table
Cheers, Daniel
Daniel-Constantin Mierla writes:
in my tests, the change that i made has worked fine. in which case would it fail?
if you define the trees with module parameters, then you have to use different tables in database, one for each. Those tables don't need tname column.
i have not found such a constraint in the readme. is my patch somehow conflicting the current readme?
If you just store all the trees in one table (which has tname column), then don't define them in the config, just set the table where they are stored. A reload command will reload all the trees in that table
the trees may have different types. how can you tell that without mtree parameter?
after the patch, one db table can have many trees each defined separately by mtree parameter.
does that break something in the readme?
-- juha
On 10/30/13 6:47 PM, Juha Heinanen wrote:
Daniel-Constantin Mierla writes:
in my tests, the change that i made has worked fine. in which case would it fail?
if you define the trees with module parameters, then you have to use different tables in database, one for each. Those tables don't need tname column.
i have not found such a constraint in the readme. is my patch somehow conflicting the current readme?
If you just store all the trees in one table (which has tname column), then don't define them in the config, just set the table where they are stored. A reload command will reload all the trees in that table
the trees may have different types. how can you tell that without mtree parameter?
after the patch, one db table can have many trees each defined separately by mtree parameter.
does that break something in the readme?
I haven't checked for now, but it doesn't really matters - readme is written after the code, not the other way around. If there is a missing description in the readme, it will be fixed.
Your patch breaks existing functionality, so practically you cannot have mtree table, only mtrees. That's not good because for large number of records (e.g., millions), I don't want to store tree name for each record when I know is only one (and do WHERE comparison for each load).
Again, see utils/kamctl/mysql/mtree-create.sql - both tables are there from the beginning of the module.
It is no a problem to add a patch that will enhance for defining trees that load from a table that has tname, But should not break the other one.
Cheers, Daniel
Daniel-Constantin Mierla writes:
It is no a problem to add a patch that will enhance for defining trees that load from a table that has tname, But should not break the other one.
which case it is that the patch breaks? perhaps it is possible to enhance the patch avoid breaking existing behavior.
-- juha
On 10/30/13 7:08 PM, Juha Heinanen wrote:
Daniel-Constantin Mierla writes:
It is no a problem to add a patch that will enhance for defining trees that load from a table that has tname, But should not break the other one.
which case it is that the patch breaks? perhaps it is possible to enhance the patch avoid breaking existing behavior.
# cat utils/kamctl/mysql/mtree-create.sql
INSERT INTO version (table_name, table_version) values ('mtree','1'); CREATE TABLE mtree ( id INT(10) UNSIGNED AUTO_INCREMENT PRIMARY KEY NOT NULL, tprefix VARCHAR(32) DEFAULT '' NOT NULL, tvalue VARCHAR(128) DEFAULT '' NOT NULL, CONSTRAINT tprefix_idx UNIQUE (tprefix) );
INSERT INTO version (table_name, table_version) values ('mtrees','2'); CREATE TABLE mtrees ( id INT(10) UNSIGNED AUTO_INCREMENT PRIMARY KEY NOT NULL, tname VARCHAR(128) DEFAULT '' NOT NULL, tprefix VARCHAR(32) DEFAULT '' NOT NULL, tvalue VARCHAR(128) DEFAULT '' NOT NULL, CONSTRAINT tname_tprefix_tvalue_idx UNIQUE (tname, tprefix, tvalue) );
As you can see from above sal table definitions, 'mtree' doesn't have tname column and you introduced a constraint that a table is loaded only when 'tname' column is present. So practically, the module doesn't work anymore with such table structure. I could suggest adding a new attribute to table definition, such as 'multiple=1' (derived from the fact that a db table has records for multiple trees), which when present, the load operation will do the query with WHERE condition on tname. Otherwise, will do it like it was so far.
Daniel
Daniel-Constantin Mierla writes:
mtree module works with two types of database tables:
- one having only (tprefix, tvalue)
- one having also tree name (tname, tprefix, tvalue)
Your change seems to affect the first case, which is supposed not to have tname column, thus not working.
there is another function mt_load_db_trees() that load all the trees at one shot. my change didn't affect that branch of the code.
-- juha
if(mt_defined_trees()) { LM_DBG("static trees defined\n");
pt = mt_get_first_tree();
while(pt!=NULL) { LM_DBG("loading from tree <%.*s>\n", pt->tname.len, pt->tname.s);
/* loading all information from database */ if(mt_load_db(&pt->tname)!=0) { LM_ERR("cannot load info from database\n"); goto error1; } pt = pt->next; } } else { if(db_table.len<=0) { LM_ERR("no trees table defined\n"); goto error1; } if(mt_init_list_head()<0) { LM_ERR("unable to init trees list head\n"); goto error1; } /* loading all information from database */ if(mt_load_db_trees()!=0) { LM_ERR("cannot load trees from database\n"); goto error1; } }
On 10/30/13 4:49 PM, Juha Heinanen wrote:
Daniel-Constantin Mierla writes:
mtree module works with two types of database tables:
- one having only (tprefix, tvalue)
- one having also tree name (tname, tprefix, tvalue)
Your change seems to affect the first case, which is supposed not to have tname column, thus not working.
there is another function mt_load_db_trees() that load all the trees at one shot. my change didn't affect that branch of the code.
This is the one you should use when you store many trees in one database table.
The code you changed is for dedicated table per tree, where tname column is not present (and not needed as there are only the records for one tree, thus the value will be the same).
Therefore, you should not define the trees in config file, just set the db table name in config.
Cheers, Daniel
-- juha
if(mt_defined_trees()) { LM_DBG("static trees defined\n");
pt = mt_get_first_tree(); while(pt!=NULL) { LM_DBG("loading from tree <%.*s>\n", pt->tname.len, pt->tname.s); /* loading all information from database */ if(mt_load_db(&pt->tname)!=0) { LM_ERR("cannot load info from database\n"); goto error1; } pt = pt->next; }} else { if(db_table.len<=0) { LM_ERR("no trees table defined\n"); goto error1; } if(mt_init_list_head()<0) { LM_ERR("unable to init trees list head\n"); goto error1; } /* loading all information from database */ if(mt_load_db_trees()!=0) { LM_ERR("cannot load trees from database\n"); goto error1; } }
On 10/30/13 6:48 PM, Juha Heinanen wrote:
Daniel-Constantin Mierla writes:
Therefore, you should not define the trees in config file, just set the db table name in config.
as i already replied, i don't see how that can be done when some trees hold int values and some hold string values.
Then use different tables (or views) per table.
Cheers, Daniel
Daniel-Constantin Mierla writes:
as i already replied, i don't see how that can be done when some trees hold int values and some hold string values.
Then use different tables (or views) per table.
i don't want so complicated solution. mtrees table is designed to hold many trees because it has tname column. with mtree parameter it is possible to define the type of each table. what is wrong with that?
-- juha
On 10/30/13 7:02 PM, Juha Heinanen wrote:
Daniel-Constantin Mierla writes:
as i already replied, i don't see how that can be done when some trees hold int values and some hold string values.
Then use different tables (or views) per table.
i don't want so complicated solution. mtrees table is designed to hold many trees because it has tname column. with mtree parameter it is possible to define the type of each table. what is wrong with that?
The wrong part here is that your patch breaks loading mtree-like tables. If you want to combine mtrees-like tables with definition in config, that's fine, do proper patch for it. Don't break what was designed from the start of the module. Daniel
Daniel-Constantin Mierla writes:
The wrong part here is that your patch breaks loading mtree-like tables. If you want to combine mtrees-like tables with definition in config, that's fine, do proper patch for it. Don't break what was designed from the start of the module.
i still don't understand the case that breaks. can you give an example of mtree module parameter config involding mtree table that has stopped working?
-- juha
On 10/30/13 7:12 PM, Juha Heinanen wrote:
Daniel-Constantin Mierla writes:
The wrong part here is that your patch breaks loading mtree-like tables. If you want to combine mtrees-like tables with definition in config, that's fine, do proper patch for it. Don't break what was designed from the start of the module.
i still don't understand the case that breaks. can you give an example of mtree module parameter config involding mtree table that has stopped working?
create a table that has the same structure as 'tree' table from the utils/kamctl/mysql/mtree-create.sql and provide it to mtree definition.
Daniel
Daniel-Constantin Mierla writes:
create a table that has the same structure as 'tree' table from the utils/kamctl/mysql/mtree-create.sql and provide it to mtree definition.
ok, now i got it. does k db api allow querying table description to find out if table has tname_column?
did you mean something like this:
modparam("mtree", "mtree", "name=mytable;dbtable=routes;multiple=1;type=0;")
-- juha
On 10/30/13 7:33 PM, Juha Heinanen wrote:
Daniel-Constantin Mierla writes:
create a table that has the same structure as 'tree' table from the utils/kamctl/mysql/mtree-create.sql and provide it to mtree definition.
ok, now i got it. does k db api allow querying table description to find out if table has tname_column?
Not a dedicated member for that and I would avoid adding one for now, it requires changes in all db modules.
did you mean something like this:
modparam("mtree", "mtree", "name=mytable;dbtable=routes;multiple=1;type=0;")
Yes. That seemed the simplest approach from my point of view, not to touch db structure, so it can be pushed as a fix, of course. Other ideas are welcome, of course.
Daniel
Daniel-Constantin Mierla writes:
did you mean something like this:
modparam("mtree", "mtree", "name=mytable;dbtable=routes;multiple=1;type=0;")
Yes. That seemed the simplest approach from my point of view, not to touch db structure, so it can be pushed as a fix, of course. Other ideas are welcome, of course.
i reverted the change, since i don't have time right now to work more on it. i'll see what i can do during the next couple of days.
-- juha
Daniel-Constantin Mierla writes:
modparam("mtree", "mtree", "name=mytable;dbtable=routes;multiple=1;type=0;")
Yes. That seemed the simplest approach from my point of view, not to touch db structure, so it can be pushed as a fix, of course. Other ideas are welcome, of course.
i just made a commit that adds 'multi' param to mtree definition and allows one db table to contain trees of different node types.
-- juha
-
Daniel-Constantin Mierla -
Juha Heinanen