[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 18:29:19 CEST 2012


Hi,

I have changed the non-pooling stuff to be C function based instead of
DB URL based.  I have only implemented and tested this for PostgreSQL as
that is the only DB module I use - but it should be trivial to add to
other DB modules in the future if anyone needs that.

If I have time next week I will change BEGIN/COMMIT/ROLLBACK to be API
based (I will only implement and test the APIs for PostgreSQL) and also
disable retries automatically (again just within PostgreSQL) when they
are used.  If I do this I will make sure I change the RLS code that
calls the APIs to check the functions are non-null before using them -
so they will continue to work with other DBs (but of course better with
PostgreSQL).  Once the function pointers exist and the PostgreSQL APIs
have been implemented it should be straight-forward enough for others to
port to MySQL/Oracle/SQLite and so on.

Regards,

Peter

On Fri, 2012-04-20 at 16:52 +0100, 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.
> 
> 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>
> > > > > > > Committer: Peter Dunkley<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
> > > > > > > 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
> > > > > 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/
> 
> 
> _______________________________________________
> sr-dev mailing list
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sip-router.org/pipermail/sr-dev/attachments/20120420/7420f335/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/7420f335/attachment-0001.png>


More information about the sr-dev mailing list