[sr-dev] git:master: modules_k/rls: Used db_begin() and db_commit() around blocks of related DB queries and updates
Daniel-Constantin Mierla
miconda at gmail.com
Fri Apr 20 17:44:27 CEST 2012
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/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sip-router.org/pipermail/sr-dev/attachments/20120420/5568240c/attachment-0001.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: image/png
Size: 1057 bytes
Desc: not available
URL: <http://lists.sip-router.org/pipermail/sr-dev/attachments/20120420/5568240c/attachment-0001.png>
More information about the sr-dev
mailing list