[sr-dev] git:master: modules_k/rls: Used db_begin() and db_commit() around blocks of related DB queries and updates
Peter Dunkley
peter.dunkley at crocodile-rcs.com
Fri Apr 20 19:12:52 CEST 2012
Hi,
The code for pooling/non-pooling is in the DB API. But the function in
the DB API that does this is called from db_do_init(), which in turn is
called from the various DB module init() functions.
So the change I have made is mostly in the DB API. However, a new
init() function to create a non-pooled connection is needed for each DB
module you want to do this from. I only need PostgreSQL, and only have
PostgreSQL to test with, so I have only added the new init() function to
modules/db_postgresql. It would be trivial to add to the other DB
modules if required, but should probably be done by someone who can test
it.
Peter
On Fri, 2012-04-20 at 19:08 +0200, Klaus Darilion wrote:
>
> On 20.04.2012 17:52, Peter Dunkley wrote:
> > Hi,
> >
> > I am just adding a new db init function (init_nopool) to the generic
> > _dbf data-structure now. I will implement the actual init_nopool
> > function just for PostgreSQL. We only need this for a small proprietary
> > module we have, so I don't expect anyone else to use this right away.
>
> AFAIK the pool handling is in the DB API. Thus, shouldn't pooling vs.
> non-pooling a feature of the DB API?
>
> klaus
>
> > If I have time I will make the BEGIN/COMMIT/ROLLBACK functions API
> > functions next week, but I want to get the presence notifier stuff done
> > first.
> >
> > Thanks,
> >
> > Peter
> >
> > On Fri, 2012-04-20 at 17:44 +0200, Daniel-Constantin Mierla wrote:
> >> Hello,
> >>
> >> On 4/20/12 5:18 PM, Peter Dunkley wrote:
> >>> Hi Daniel,
> >>>
> >>> How would we do the non-pooling stuff inside the module?
> >>
> >> it can be via new API field, like 'newinit' -- it is anyhow needed
> >> only in one or few places, so the other modules using the DB API will
> >> not change, just the db conenctors.
> >>
> >> My concern is actually about the fact that the user will have to know
> >> that a module requires internally new connections for same db url.
> >> From my point of view, the user knows that the module has to interact
> >> with the database and set a DB URL parameter for that.
> >>
> >> Jan Janak had also a point about the URL format, the '*' not being
> >> really right and safe used as part of schema.
> >>
> >> Therefore two things here:
> >> - do we need to specify from config whether it has to be a new
> >> connection or not? If it is mandatory to work properly, then it is not
> >> a configuration option
> >> - how do distinguish from asking a pooled and non-pooled connection.
> >> If we need non-pooled is needed to be specified via a config option,
> >> then we should do it according tot he URL format, otherwise, should be
> >> something internal, with no change in the existing DB url syntax.
> >>
> >>>
> >>> I believe BEGIN/COMMIT/ROLLBACK are standard SQL. I did a quick
> >>> search online before making the change and found that they seem to be
> >>> documented for PostgreSQL, MySQL SQLlite, and SQL Server. The wrapper
> >>> functions first check to see if DB_CAP_RAW_QUERY is set for the
> >>> database connection it the functions just return without error if raw
> >>> queries are not available. I would have thought it safe to assume
> >>> that if raw queries are supported the DB module supports SQL?
> >>
> >> It might be true right now, but even supporting raw_query, it may not
> >> have support for transactions. Raw query support can be implemented
> >> also for non-SQL. Relying on another generic API member to assume
> >> globally an existing specific feature, looks a bit odd for me.
> >>
> >> I don't know, right now it just feels that such extensions are more
> >> appropriate as API members and implemented inside connectors. It has
> >> also the benefit of being able to control better the behavior, if what
> >> Klaus pointed is needed, like disabling auto-reconnect.
> >>
> >> The feature is there, reworking a bit during the testing does not mean
> >> new features, so it is not a presure to change everything right now.
> >> But we should do the right decision so we don 't need to change in
> >> short term.
> >>
> >> Cheers,
> >> Daniel
> >>
> >>
> >>
> >>>
> >>> Peter
> >>>
> >>> On Fri, 2012-04-20 at 17:09 +0200, Daniel-Constantin Mierla wrote:
> >>>> Hello,
> >>>>
> >>>> since it came back in the spotlight, I think the non-pooling flag
> >>>> should not be exposed to the config. The admin has usually no
> >>>> knowledge of modules' internals to know it has to configure with a
> >>>> new connection -- it should be only inside the module.
> >>>>
> >>>> Then, is BEGIN/COMMIT/ROLLBACK a standard everywhere across DB/SQL
> >>>> servers? Even if in SQL, the DB api is not only for them. I seems to
> >>>> be that these new functions should be better part of DB api and
> >>>> implemented inside the db modules. That allows also for temporarily
> >>>> disabling of reconnect when needed, Eventually, other DB (non-SQL)
> >>>> connectors can have custom implementation.
> >>>>
> >>>> Cheers,
> >>>> Daniel
> >>>>
> >>>> On 4/20/12 4:56 PM, Peter Dunkley wrote:
> >>>>> I'll update the RLS documentation to make that clear.
> >>>>>
> >>>>> As for the second item... The non-pooling stuff is still done with
> >>>>> a '*' at the start of the DB URL. The DB stuff doesn't use the
> >>>>> standard Kamailio URI parser so making DB pooling a proper
> >>>>> parameter is not as simple as I'd hoped it'd be. Also, the
> >>>>> parameter stuff would be in lib/srdb1 whereas the retries parameter
> >>>>> is in the DB modules themselves (modules/db_mysql and
> >>>>> modules/db_postgres), so I am not sure disabling automatic
> >>>>> reconnect on a per connection basis will be that simple to add.
> >>>>>
> >>>>> I think I'll leave those DB changes to someone a bit more familiar
> >>>>> with the modules :-)
> >>>>>
> >>>>> Thanks,
> >>>>>
> >>>>> Peter
> >>>>>
> >>>>> On Fri, 2012-04-20 at 16:24 +0200, Klaus Darilion wrote:
> >>>>>> Now you should mention that auto reconnect must be disabled otherwise
> >>>>>> reconnect during transactions will cause inconsistencies.
> >>>>>>
> >>>>>> Probably it would be cool if automatically reconnect would be configured
> >>>>>> with the db_url (just as the non-pooling parameter)
> >>>>>>
> >>>>>> regards
> >>>>>> Klaus
> >>>>>>
> >>>>>> On 20.04.2012 15:02, Peter Dunkley wrote:
> >>>>>> > Module: sip-router
> >>>>>> > Branch: master
> >>>>>> > Commit: f71bdc3cbdf5acdf243217778e4159f2e6ab341d
> >>>>>> > URL:http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=f71bdc3cbdf5acdf243217778e4159f2e6ab341d
> >>>>>> >
> >>>>>> > Author: Peter Dunkley<peter.dunkley at crocodile-rcs.com <mailto:peter.dunkley at crocodile-rcs.com>>
> >>>>>> > Committer: Peter Dunkley<peter.dunkley at crocodile-rcs.com <mailto:peter.dunkley at crocodile-rcs.com>>
> >>>>>> > Date: Fri Apr 20 14:00:22 2012 +0100
> >>>>>> >
> >>>>>> > modules_k/rls: Used db_begin() and db_commit() around blocks of related DB queries and updates
> >>>>>> >
> >>>>>> > - This makes these related sets of DB queries a single transaction. As Klaus
> >>>>>> > pointed out this if you don't do this you can get inconsistencies when using
> >>>>>> > multiple presence servers.
> >>>>>> >
> >>>>>> > ---
> >>>>>> >
> >>>>>> > modules_k/rls/notify.c | 12 ++++++++++++
> >>>>>> > modules_k/rls/resource_notify.c | 24 ++++++++++++++++++++++++
> >>>>>> > modules_k/rls/rls_db.c | 21 +++++++++++++++++++++
> >>>>>> > 3 files changed, 57 insertions(+), 0 deletions(-)
> >>>>>> >
> >>>>>> > diff --git a/modules_k/rls/notify.c b/modules_k/rls/notify.c
> >>>>>> > index 082fbfa..1836808 100644
> >>>>>> > --- a/modules_k/rls/notify.c
> >>>>>> > +++ b/modules_k/rls/notify.c
> >>>>>> > @@ -128,6 +128,12 @@ int send_full_notify(subs_t* subs, xmlNodePtr rl_node, str* rl_uri,
> >>>>>> > goto error;
> >>>>>> > }
> >>>>>> >
> >>>>>> > + if (db_begin(&rlpres_dbf, rlpres_db)< 0)
> >>>>>> > + {
> >>>>>> > + LM_ERR("in BEGIN\n");
> >>>>>> > + goto error;
> >>>>>> > + }
> >>>>>> > +
> >>>>>> > if(rlpres_dbf.query(rlpres_db, query_cols, 0, query_vals, result_cols,
> >>>>>> > 1, n_result_cols,&str_resource_uri_col,&result )< 0)
> >>>>>> > {
> >>>>>> > @@ -257,6 +263,12 @@ int send_full_notify(subs_t* subs, xmlNodePtr rl_node, str* rl_uri,
> >>>>>> > goto error;
> >>>>>> > }
> >>>>>> >
> >>>>>> > + if (db_commit(&rlpres_dbf, rlpres_db)< 0)
> >>>>>> > + {
> >>>>>> > + LM_ERR("in COMMIT\n");
> >>>>>> > + goto error;
> >>>>>> > + }
> >>>>>> > +
> >>>>>> > xmlFree(rlmi_cont->s);
> >>>>>> > pkg_free(rlmi_cont);
> >>>>>> >
> >>>>>> > diff --git a/modules_k/rls/resource_notify.c b/modules_k/rls/resource_notify.c
> >>>>>> > index 29e0b11..87ce806 100644
> >>>>>> > --- a/modules_k/rls/resource_notify.c
> >>>>>> > +++ b/modules_k/rls/resource_notify.c
> >>>>>> > @@ -893,6 +893,12 @@ static void timer_send_full_state_notifies(int round)
> >>>>>> > goto done;
> >>>>>> > }
> >>>>>> >
> >>>>>> > + if (db_begin(&rls_dbf, rls_db)< 0)
> >>>>>> > + {
> >>>>>> > + LM_ERR("in BEGIN\n");
> >>>>>> > + goto done;
> >>>>>> > + }
> >>>>>> > +
> >>>>>> > /* Step 1: Find rls_watchers that require full-state notification */
> >>>>>> > if (rls_dbf.query(rls_db, query_cols, 0, query_vals, result_cols,
> >>>>>> > 1, n_result_cols, 0,&result)< 0)
> >>>>>> > @@ -912,6 +918,12 @@ static void timer_send_full_state_notifies(int round)
> >>>>>> > goto done;
> >>>>>> > }
> >>>>>> >
> >>>>>> > + if (db_commit(&rls_dbf, rls_db)< 0)
> >>>>>> > + {
> >>>>>> > + LM_ERR("in COMMIT\n");
> >>>>>> > + goto done;
> >>>>>> > + }
> >>>>>> > +
> >>>>>> > /* Step 3: Full-state notify each watcher we found */
> >>>>>> > rows = RES_ROWS(result);
> >>>>>> > for (i = 0; i< RES_ROW_N(result); i++)
> >>>>>> > @@ -1025,6 +1037,12 @@ static void timer_send_update_notifies(int round)
> >>>>>> > goto done;
> >>>>>> > }
> >>>>>> >
> >>>>>> > + if (db_begin(&rlpres_dbf, rlpres_db)< 0)
> >>>>>> > + {
> >>>>>> > + LM_ERR("in BEGIN\n");
> >>>>>> > + goto error;
> >>>>>> > + }
> >>>>>> > +
> >>>>>> > if(rlpres_dbf.query(rlpres_db, query_cols, 0, query_vals, result_cols,
> >>>>>> > 1, n_result_cols,&str_rlsubs_did_col,&result)< 0)
> >>>>>> > {
> >>>>>> > @@ -1042,6 +1060,12 @@ static void timer_send_update_notifies(int round)
> >>>>>> > goto error;
> >>>>>> > }
> >>>>>> >
> >>>>>> > + if (db_commit(&rlpres_dbf, rlpres_db)< 0)
> >>>>>> > + {
> >>>>>> > + LM_ERR("in COMMIT\n");
> >>>>>> > + goto error;
> >>>>>> > + }
> >>>>>> > +
> >>>>>> > send_notifies(result, did_col, resource_uri_col, auth_state_col, reason_col,
> >>>>>> > pres_state_col, content_type_col);
> >>>>>> > error:
> >>>>>> > diff --git a/modules_k/rls/rls_db.c b/modules_k/rls/rls_db.c
> >>>>>> > index 6a00c21..46bda91 100644
> >>>>>> > --- a/modules_k/rls/rls_db.c
> >>>>>> > +++ b/modules_k/rls/rls_db.c
> >>>>>> > @@ -124,6 +124,7 @@ int delete_expired_subs_rlsdb( void )
> >>>>>> > int i;
> >>>>>> > subs_t subs;
> >>>>>> > str rlsubs_did = {0, 0};
> >>>>>> > + int transaction_started = 0;
> >>>>>> >
> >>>>>> > if(rls_db == NULL)
> >>>>>> > {
> >>>>>> > @@ -148,6 +149,13 @@ int delete_expired_subs_rlsdb( void )
> >>>>>> > result_cols[r_to_tag_col=n_result_cols++] =&str_to_tag_col;
> >>>>>> > result_cols[r_from_tag_col=n_result_cols++] =&str_from_tag_col;
> >>>>>> >
> >>>>>> > + if (db_begin(&rls_dbf, rls_db)< 0)
> >>>>>> > + {
> >>>>>> > + LM_ERR("in BEGIN\n");
> >>>>>> > + goto error;
> >>>>>> > + }
> >>>>>> > + transaction_started = 1;
> >>>>>> > +
> >>>>>> > if(rls_dbf.query(rls_db, query_cols, query_ops, query_vals, result_cols,
> >>>>>> > n_query_cols, n_result_cols, 0,&result )< 0)
> >>>>>> > {
> >>>>>> > @@ -213,12 +221,25 @@ int delete_expired_subs_rlsdb( void )
> >>>>>> > pkg_free(rlsubs_did.s);
> >>>>>> > }
> >>>>>> >
> >>>>>> > + if (db_commit(&rls_dbf, rls_db)< 0)
> >>>>>> > + {
> >>>>>> > + LM_ERR("in COMMIT\n");
> >>>>>> > + goto error;
> >>>>>> > + }
> >>>>>> > +
> >>>>>> > if(result) rls_dbf.free_result(rls_db, result);
> >>>>>> > return 1;
> >>>>>> >
> >>>>>> > error:
> >>>>>> > if (result) rls_dbf.free_result(rls_db, result);
> >>>>>> > if (rlsubs_did.s) pkg_free(rlsubs_did.s);
> >>>>>> > +
> >>>>>> > + if (transaction_started)
> >>>>>> > + {
> >>>>>> > + if (db_commit(&rls_dbf, rls_db)< 0)
> >>>>>> > + LM_ERR("in COMMIT\n");
> >>>>>> > + }
> >>>>>> > +
> >>>>>> > return -1;
> >>>>>> > }
> >>>>>> >
> >>>>>> >
> >>>>>> >
> >>>>>> > _______________________________________________
> >>>>>> > sr-dev mailing list
> >>>>>> > sr-dev at lists.sip-router.org <mailto:sr-dev at lists.sip-router.org>
> >>>>>> > http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
> >>>>>
> >>>>> --
> >>>>> Peter Dunkley
> >>>>> Technical Director
> >>>>> Crocodile RCS Ltd
> >>>>>
> >>>>>
> >>>>>
> >>>>> _______________________________________________
> >>>>> sr-dev mailing list
> >>>>> sr-dev at lists.sip-router.org <mailto:sr-dev at lists.sip-router.org>
> >>>>> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
> >>>>
> >>>> --
> >>>> Daniel-Constantin Mierla
> >>>> Kamailio Advanced Training, April 23-26, 2012, Berlin, Germany
> >>>> http://www.asipto.com/index.php/kamailio-advanced-training/
> >>>
> >>> --
> >>> Peter Dunkley
> >>> Technical Director
> >>> Crocodile RCS Ltd
> >>>
> >>
> >> --
> >> Daniel-Constantin Mierla
> >> Kamailio Advanced Training, April 23-26, 2012, Berlin, Germany
> >> http://www.asipto.com/index.php/kamailio-advanced-training/
> >
> > --
> > Peter Dunkley
> > Technical Director
> > Crocodile RCS Ltd
> >
--
Peter Dunkley
Technical Director
Crocodile RCS Ltd
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sip-router.org/pipermail/sr-dev/attachments/20120420/6b034f1c/attachment-0001.htm>
More information about the sr-dev
mailing list