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@crocodile-rcs.com  <mailto:peter.dunkley@crocodile-rcs.com>>
>>>>>> >  Committer: Peter Dunkley<peter.dunkley@crocodile-rcs.com  <mailto:peter.dunkley@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@lists.sip-router.org  <mailto:sr-dev@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@lists.sip-router.org  <mailto:sr-dev@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