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:
Daniel-Constantin Mierla writes:
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:
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:
i have not found such a constraint in the readme. is my patch somehow conflicting the current readme?
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:
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:
# 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:
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:
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
On 10/30/13 6:48 PM, Juha Heinanen wrote:
Then use different tables (or views) per table.
Cheers, Daniel
On 10/30/13 7:02 PM, Juha Heinanen wrote:
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
On 10/30/13 7:12 PM, Juha Heinanen wrote:
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
On 10/30/13 7:33 PM, Juha Heinanen wrote:
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 -
Juha Heinanen