From 430d4c8bd01ace7fdea614d500531af9e3ff2016 Mon Sep 17 00:00:00 2001 From: Walter Doekes Date: Tue, 11 May 2021 11:59:34 +0200 Subject: [PATCH 1/2] db: Add IS NULL and IS NOT NULL unary operators If you try to (mysql) prepare a statement with "... column IS ?" you'd get an error. For mysql, you could fix this by using the "... column <=> ?" special operator ('IS NOT DISTINCT FROM'), but that's not standard SQL. Instead, you can now do: update_cols[0] = &some_column; update_ops[0] = OP_IS_NULL; update_vals[0].nul = 1; You still need to set .nul=1 because prepared statements have to consume a single value. --- db/db_op.c | 25 +++++++++++++++++++++++++ db/db_op.h | 11 +++++++++++ db/db_ut.c | 27 ++++++++++++++++++++++----- 3 files changed, 58 insertions(+), 5 deletions(-) create mode 100644 db/db_op.c diff --git a/db/db_op.c b/db/db_op.c new file mode 100644 index 00000000000..a94cc7fafdc --- /dev/null +++ b/db/db_op.c @@ -0,0 +1,25 @@ +/* + * Copyright (C) 2021 OpenSIPS + * + * This file is part of opensips, a free SIP server. + * + * opensips is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version + * + * opensips is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include "db_op.h" + + +const char OP_IS_NULL[] = " IS NULL"; +const char OP_IS_NOT_NULL[] = " IS NOT NULL"; diff --git a/db/db_op.h b/db/db_op.h index 8938dbda17b..2f93dc2fc85 100644 --- a/db/db_op.h +++ b/db/db_op.h @@ -40,6 +40,17 @@ /** operator negation */ #define OP_NEQ "!=" +/* Special unary operators here, because mysql prepared statements do + * not cope with 'column IS ?' (they would cope with 'column <=> ?' but + * that's not standard SQL). + * (Declared as char array instead of define, so they can be pointer + * compared in code.) */ + +/** unary operator: IS NULL */ +extern const char OP_IS_NULL[]; /* " IS NULL" */ +/** unary operator: IS NOT NULL */ +extern const char OP_IS_NOT_NULL[]; /* " IS NOT NULL" */ + /** * This type represents an expression operator uses for SQL queries. diff --git a/db/db_ut.c b/db/db_ut.c index 674808295a6..4b3e6cf34c2 100644 --- a/db/db_ut.c +++ b/db/db_ut.c @@ -336,16 +336,33 @@ int db_print_where(const db_con_t* _c, char* _b, const int _l, const db_key_t* _ for(i = 0; i < _n; i++) { if (_o) { - ret = snprintf(_b + len, _l - len, "%.*s%s", - _k[i]->len, _k[i]->s, _o[i]); - if (ret < 0 || ret >= (_l - len)) goto error; - len += ret; + /* Special case: if there is unary operator and + * we use a prepared statement, we must still + * consume one value, even though we're not + * using it. Otherwise the number of values will + * be incorrect. */ + if ((_o[i] == OP_IS_NULL || _o[i] == OP_IS_NOT_NULL) && + CON_HAS_PS(_c)) { + /* ?=NULL will never be true; so we're + * safely consuming a single argument */ + ret = snprintf(_b + len, _l - len, "(%.*s%s OR ?=NULL)", + _k[i]->len, _k[i]->s, _o[i]); + if (ret < 0 || ret >= (_l - len)) goto error; + len += ret; + } else { + ret = snprintf(_b + len, _l - len, "%.*s%s", + _k[i]->len, _k[i]->s, _o[i]); + if (ret < 0 || ret >= (_l - len)) goto error; + len += ret; + } } else { ret = snprintf(_b + len, _l - len, "%.*s=", _k[i]->len, _k[i]->s); if (ret < 0 || ret >= (_l - len)) goto error; len += ret; } - if (CON_HAS_PS(_c)) { + if (_o && (_o[i] == OP_IS_NULL || _o[i] == OP_IS_NOT_NULL)) { + ; + } else if (CON_HAS_PS(_c)) { *(_b+len++) = '?'; } else { l = _l - len; From a6a22c9bec789b8c56f3b98ebc6b23c3a7abca3b Mon Sep 17 00:00:00 2001 From: Walter Doekes Date: Tue, 11 May 2021 12:10:27 +0200 Subject: [PATCH 2/2] presence: Fix leaking of activewatchers in database (with clustering) If you're using clustering, without tags (or with tags, but without fallback2db), you would end up with a lot of active watchers in the database that never got cleaned up. Before 929ab4d43c0ad1878d60d3ef27f2a71f905b80f0: - all stale activewatcher records in the db got purged. After: - if you're not clustering, all stale active watcher got purged; - if you are, only those with the sharing_tag set. However, the sharing tag is not necessarily set: - it is an optional argument to handle_subscribe; - and setting it in handle_subscribe does not write to the database because of missing code in the fallback2db==0 bit; - and even it it were set, then adding the argument to handle_subscribe does not "activate" the sharing tag. (Also interesting to know: a 408 or 481 after the this-subscription-is-expired NOTIFY _would_ cause the individual record to get deleted. But any other response, including 200, would leave the record to get sorted by the periodic purge.) This changeset reverts parts of the aforementioned commit by always purging stale records if the sharing_tag is NULL. Thus restoring behaviour to pre-3.1.0 and pre-2.4.8. --- modules/presence/subscribe.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/modules/presence/subscribe.c b/modules/presence/subscribe.c index 76ecbe14240..d0c6107c3c8 100644 --- a/modules/presence/subscribe.c +++ b/modules/presence/subscribe.c @@ -1356,7 +1356,7 @@ static inline int is_shtag_active( str *my_tag, str **active_tags) void update_db_subs(db_con_t *db,db_func_t *dbf, shtable_t hash_table, int htable_size, int no_lock, handle_expired_func_t handle_expired_func) { - static db_ps_t my_ps_delete = NULL; + static db_ps_t my_ps_delete = NULL, my_ps_delete_null = NULL; static db_ps_t my_ps_update = NULL, my_ps_insert = NULL; db_key_t query_cols[22], update_cols[8]; db_val_t query_vals[22], update_vals[8]; @@ -1664,32 +1664,32 @@ void update_db_subs(db_con_t *db,db_func_t *dbf, shtable_t hash_table, lock_release(&hash_table[i].lock); } - /* now that all records were updated, delete whatever + /* now that all records were updated, delete whatever was still left as expired */ - update_cols[0]= &str_expires_col; + update_cols[0] = &str_expires_col; update_vals[0].type = DB_INT; update_vals[0].nul = 0; update_vals[0].val.int_val = (int)time(NULL); update_ops[0] = OP_LT; + update_cols[1] = &str_sharing_tag_col; + update_vals[1].nul = 1; + update_ops[1] = OP_IS_NULL; + if (dbf->use_table(db, &active_watchers_table) < 0) { LM_ERR("deleting expired information from database\n"); return; } - if (sh_tags==NULL) { - - /* no clustering, simply delete all expired subs */ - LM_DBG("delete all expired subscriptions\n"); + /* no clustering, simply delete all expired subs with NULL sh tags */ + LM_DBG("delete all expired subscriptions\n"); - CON_SET_CURR_PS(db, &my_ps_delete); - if (dbf->delete(db, update_cols, update_ops, update_vals, 1) < 0) - LM_ERR("deleting expired information from database\n"); - - } else { + CON_SET_CURR_PS(db, &my_ps_delete_null); + if (dbf->delete(db, update_cols, update_ops, update_vals, 2) < 0) + LM_ERR("deleting expired information from database\n"); + if (sh_tags != NULL) { /* clustering, delete only expired subs with active sh tags */ - update_cols[1]= &str_sharing_tag_col; update_vals[1].type = DB_STR; update_vals[1].nul = 0; update_ops[1] = OP_EQ; @@ -1698,8 +1698,8 @@ void update_db_subs(db_con_t *db,db_func_t *dbf, shtable_t hash_table, while(sh_tags[i]) { LM_DBG("delete expired subscriptions for tag <%.*s>\n", sh_tags[i]->len, sh_tags[i]->s); - update_vals[1].val.str_val = *sh_tags[i]; + CON_SET_CURR_PS(db, &my_ps_delete); if (dbf->delete(db, update_cols, update_ops, update_vals, 2) < 0) LM_ERR("deleting expired information from database\n");