Why is insert_update doing nothing in Kamailio db insert_update ?
As explained in the following article the design of "UPSERT" in PostgreSQL requires to be explicit about the constraint on which we accept to do update modification to Kamailio database framework/API would be required to expose which specific key constraint should be handled as an update
http://pgeoghegan.blogspot.com/2015/10/avoid-naming-constraint-directly-when...
In this first step, DO NOTHING is providing the simple feature to ignore error on insert when facing a constraint violation You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/1039
-- Commit Summary --
* db_postgress: insert_update() with DO NOTHING
-- File Changes --
M src/modules/db_postgres/db_postgres.c (1) M src/modules/db_postgres/db_postgres.h (2) M src/modules/db_postgres/km_dbase.c (83) M src/modules/db_postgres/km_dbase.h (5) M src/modules/db_postgres/km_pg_con.c (4) M src/modules/db_postgres/pg_mod.c (4)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/1039.patch https://github.com/kamailio/kamailio/pull/1039.diff
Is this patch useful in existing code base? If the function doesn't exist in the db connector module, it should be not used by the modules, but do an alternative solution. Can you give some example where this is useful now?
Anyhow, as I could understand from the patch, it doesn't do an update on an insert conflict. This can mislead, by seeing the function exported by postgres module, but not fully implemented.
Regarding the commit message, it must be strictly related to what the patch adds/changes to Kamailio sources, not the story of doing the patch. Such details can be in the pull requests description or comments of PR.
The immediate value is the have a way to distinguish and ignore insert failures when they are caused by by a constraint violation. The first use case would be in usrloc when we write to db and use dmq replication. I would add a param to usrloc module "db_insert_update" defaulting to 0 but that could be set to 1, shit would already work completely with mysql
However with postgres, it would remove any unwanted errors and retries when syncing from dmq for example, I saw other posts about such errors. This way usrloc would only retry it there is a real failure not a constraint violation.
Then we could plan for a second step to implement the "DO UPDATE" instead of just "DO NOTHING"
- add an optional param to insert_update (modify the db framework slightly) to specify the name of the constraint where we want to update on failure - add a db_postgres module variables that could contain tablename_constraint and check for them when doing insert update
The most strati forward solution would probably be changing the function prototype :
```/** * \brief Insert a row into specified table, update on duplicate key. * * The function implements the INSERT ON DUPLICATE KEY UPDATE SQL directive. * It is possible to insert a row and update if one already exists. * The old row will not deleted before the insertion of the new data. * \param _h structure representing database connection * \param _k key names * \param _v values of the keys * \param _n number of key=value pairs * \return returns 0 if everything is OK, otherwise returns value < 0 */ typedef int (*db_insert_update_f) (const db1_con_t* _h, const db_key_t* _k, const db_val_t* _v, const int _n); ``` To
```/** * \brief Insert a row into specified table, update on duplicate key. * * The function implements the INSERT ON DUPLICATE KEY UPDATE SQL directive. * It is possible to insert a row and update if one already exists. * The old row will not deleted before the insertion of the new data. * \param _h structure representing database connection * \param _k key names * \param _v values of the keys * \param _n number of key=value pairs * \param _v values of the keys_constrains * \return returns 0 if everything is OK, otherwise returns value < 0 */ typedef int (*db_insert_update_f) (const db1_con_t* _h, const db_key_t* _k, const db_val_t* _v, const int _n, const db_key_t* _kc); ```
Then a db driver that does not need them would simply ignore them, I was not sure bout this option because postgres may be the only one requiring explicit constraints specification ?
This would require modifications to ``` ~/git/kamailio/src$ grep -R --include="*.c" insert_update *
the DB base framework and 4 implementations : db_mysql, db_cassandra, db_mysql, db_cluster
lib/srdb1/db.c: if (dbf->insert_update) { lib/srdb1/db.c: dbf.insert_update = (db_insert_update_f)find_mod_export(tmp, lib/srdb1/db.c: "db_insert_update", 2, 0); modules/db_postgres/km_dbase.c: * Why is insert_update doing nothing in Kamailio db insert_update ? modules/db_postgres/km_dbase.c:int db_postgres_insert_update(const db1_con_t* _h, const db_key_t* _k, const db_val_t* _v, modules/db_postgres/km_dbase.c: LM_ERR("error while preparing insert_update operation\n"); modules/db_postgres/db_postgres.c: dbb->insert_update = db_postgres_insert_update; modules/db_postgres/km_pg_con.c: LM_WARN("server version < 9.5 does not support insert_update\n"); modules/db_cassandra/db_cassandra.c: dbb->insert_update = db_cassa_insert; modules/db_mysql/km_dbase.c: int db_mysql_insert_update(const db1_con_t* _h, const db_key_t* _k, const db_val_t* _v, modules/db_mysql/km_dbase.c: LM_ERR("error while preparing insert_update operation\n"); modules/db_mysql/km_db_mysql.c: dbb->insert_update = db_mysql_insert_update; modules/db_cluster/db_cluster_mod.c: dbb->insert_update = db_cluster_insert_update; modules/db_cluster/dbcl_api.c:int db_cluster_insert_update(const db1_con_t* _h, const db_key_t* _k, const db_val_t* _v, modules/db_cluster/dbcl_api.c: DBCL_WRITE(insert_update, insert_update(dbh, _k, _v, _n));
as well as the clients :
modules/p_usrloc/ul_db_ins_upd.c:int db_insert_update(ul_db_handle_t * handle, modules/p_usrloc/ul_db.c:int ul_db_insert_update(str * table, str * first, str * second, modules/p_usrloc/ul_db.c: return db_insert_update(handle, table, _k, _v, _n); modules/p_usrloc/ul_db_form_query.c: if(dbf->insert_update(dbh, _k, _v, _n ) < 0) { modules/ims_usrloc_scscf/usrloc_db.c: if (ul_dbf.insert_update(ul_dbh, key, val, i) != 0) { modules/ims_usrloc_scscf/usrloc_db.c: if (ul_dbf.insert_update(ul_dbh, key, val, 7) != 0) { modules/ims_usrloc_scscf/usrloc_db.c: if (ul_dbf.insert_update(ul_dbh, key, val, col_num) != 0) { ```
There is another option to do it directly in db_postgres constraints "auto detection"
https://www.postgresql.org/docs/9.1/static/catalog-pg-constraint.html
We would search the catalog : ``` select conntype from pg_constraint where conrelid = (select oid from pg_class where relname like 'location_test') and contype = u; select conntype from pg_constraint where conrelid = (select oid from pg_class where relname like 'location_test') and contype = p; ``` wich would return : `location_ruid_idx_test` or `location_test_pkey`
Then we can save this in the module memory : ```struct constraint { char *db_name; char *db_table; char *unique_contraint; char *primary_key_constraint; }; ``` And use it automatically in the next queries ... ```insert into location_test (ruid, username, domain) values ('1234','jo', 'my dom') on conflict on constraint location_ruid_idx_test do update set ruid='1234', username='jo', domain='my dom'; ```
I am working on OPTION2, where not modification is required outside of db_postgres, if you think this is not an option or not valuable enough let me know ?
If you do not want to merge DO NOTHING because it can be confusing / misleading then we can clause this PR.
Traveling during the past week, didn't have time to follow up on this. You can close it if you plan another patch.
Closed #1039.
-
Daniel-Constantin Mierla -
Julien Chavanton