git:master: modules_k/rls: Used db_begin() and db_commit() around blocks of related DB queries and updates
20 Apr
2012
20 Apr
'12
4:02 p.m.
Module: sip-router
Branch: master
Commit: f71bdc3cbdf5acdf243217778e4159f2e6ab341d
URL:
http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=f71bdc3…
Author: Peter Dunkley <peter.dunkley(a)crocodile-rcs.com>
Committer: Peter Dunkley <peter.dunkley(a)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;
}
20 Apr
20 Apr
5:24 p.m.
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=f71bdc3…
Author: Peter Dunkley<peter.dunkley(a)crocodile-rcs.com>
Committer: Peter Dunkley<peter.dunkley(a)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(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
5:56 p.m.
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=f71bdc3…
>
> Author: Peter Dunkley<peter.dunkley(a)crocodile-rcs.com>
> Committer: Peter Dunkley<peter.dunkley(a)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(a)lists.sip-router.org
> http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
--
Peter Dunkley
Technical Director
Crocodile RCS Ltd
6:09 p.m.
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:
--
Daniel-Constantin Mierla
Kamailio Advanced Training, April 23-26, 2012, Berlin, Germany
http://www.asipto.com/index.php/kamailio-advanced-training/
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=f71…
>
> Author: Peter Dunkley<peter.dunkley(a)crocodile-rcs.com
<mailto:peter.dunkley@crocodile-rcs.com>>
> Committer: Peter Dunkley<peter.dunkley(a)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(a)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(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev 
6:18 p.m.
Hi Daniel,
How would we do the non-pooling stuff inside the module?
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?
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:
--
Peter Dunkley
Technical Director
Crocodile RCS Ltd
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:
--
Daniel-Constantin Mierla
Kamailio Advanced Training, April 23-26, 2012, Berlin, Germany
http://www.asipto.com/index.php/kamailio-advanced-training/ 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=f71bdc3…
>
> Author: Peter Dunkley<peter.dunkley(a)crocodile-rcs.com>
> Committer: Peter Dunkley<peter.dunkley(a)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(a)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(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev 
6:44 p.m.
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:
--
Daniel-Constantin Mierla
Kamailio Advanced Training, April 23-26, 2012, Berlin, Germany
http://www.asipto.com/index.php/kamailio-advanced-training/
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:
--
Peter Dunkley
Technical Director
Crocodile RCS Ltd
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:
--
Daniel-Constantin Mierla
Kamailio Advanced Training, April 23-26, 2012, Berlin, Germany
http://www.asipto.com/index.php/kamailio-advanced-training/ 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=f71…
>
> Author: Peter Dunkley<peter.dunkley(a)crocodile-rcs.com
<mailto:peter.dunkley@crocodile-rcs.com>>
> Committer: Peter Dunkley<peter.dunkley(a)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(a)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(a)lists.sip-router.org <mailto:sr-dev@lists.sip-router.org>
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev 
6:52 p.m.
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:
--
Peter Dunkley
Technical Director
Crocodile RCS Ltd
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:
--
Daniel-Constantin Mierla
Kamailio Advanced Training, April 23-26, 2012, Berlin, Germany
http://www.asipto.com/index.php/kamailio-advanced-training/ 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:
--
Peter Dunkley
Technical Director
Crocodile RCS Ltd 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=f71bdc3…
> >
> > Author: Peter Dunkley<peter.dunkley(a)crocodile-rcs.com>
> > Committer: Peter Dunkley<peter.dunkley(a)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(a)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(a)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/ 
7:29 p.m.
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:
--
Peter Dunkley
Technical Director
Crocodile RCS Ltd
Hello,
On 4/20/12 5:18 PM, Peter Dunkley wrote:
_______________________________________________
sr-dev mailing list
sr-dev(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
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:
--
Daniel-Constantin Mierla
Kamailio Advanced Training, April 23-26, 2012, Berlin, Germany
http://www.asipto.com/index.php/kamailio-advanced-training/ 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=f71bdc3…
> > >
> > > Author: Peter Dunkley<peter.dunkley(a)crocodile-rcs.com>
> > > Committer: Peter Dunkley<peter.dunkley(a)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(a)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(a)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 
8:08 p.m.
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:
--
Peter Dunkley
Technical Director
Crocodile RCS Ltd
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:
--
Daniel-Constantin Mierla
Kamailio Advanced Training, April 23-26, 2012, Berlin, Germany
http://www.asipto.com/index.php/kamailio-advanced-training/ 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=f71…
>> >
>> > Author: Peter Dunkley<peter.dunkley(a)crocodile-rcs.com
<mailto:peter.dunkley@crocodile-rcs.com>>
>> > Committer: Peter Dunkley<peter.dunkley(a)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(a)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(a)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
8:12 p.m.
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:
--
Peter Dunkley
Technical Director
Crocodile RCS Ltd
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=f71…
>>>>>> >
>>>>>> > Author: Peter Dunkley<peter.dunkley(a)crocodile-rcs.com
<mailto:peter.dunkley@crocodile-rcs.com>>
>>>>>> > Committer: Peter
Dunkley<peter.dunkley(a)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(a)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(a)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
>
8:05 p.m.
On 20.04.2012 17:09, 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.
+1
klaus
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:
--
Daniel-Constantin Mierla
Kamailio Advanced Training, April 23-26, 2012, Berlin, Germany
http://www.asipto.com/index.php/kamailio-advanced-training/
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=f71…
>
> Author: Peter Dunkley<peter.dunkley(a)crocodile-rcs.com
<mailto:peter.dunkley@crocodile-rcs.com>>
> Committer: Peter Dunkley<peter.dunkley(a)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(a)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(a)lists.sip-router.org
http://lists.sip-router.org/cgi-bin/mailman/listinfo/sr-dev
4630
days inactive
4630
days old
10 comments
3 participants
participants (3)
-
Daniel-Constantin Mierla -
Klaus Darilion -
Peter Dunkley