Prepared queries MUC-light #3011
Conversation
Prepare query for muc_light_insert_room function
Codecov Report
@@ Coverage Diff @@
## mu-prepared-queries #3011 +/- ##
========================================================
- Coverage 79.46% 21.68% -57.78%
========================================================
Files 382 381 -1
Lines 32717 32771 +54
========================================================
- Hits 26000 7108 -18892
- Misses 6717 25663 +18946
Continue to review full report at Codecov.
|
src/rdbms/mongoose_rdbms_odbc.erl
Outdated
| @@ -156,7 +165,7 @@ field_name_to_mapper(_ServerType, _TableDesc, <<"limit">>) -> | |||
| field_name_to_mapper(_ServerType, _TableDesc, <<"offset">>) -> | |||
| fun(P) -> {sql_integer, [P]} end; | |||
| field_name_to_mapper(_ServerType, TableDesc, FieldName) -> | |||
| {_, ODBCType} = lists:keyfind(unicode:characters_to_list(FieldName), 1, TableDesc), | |||
| {_, ODBCType} = find_key(unicode:characters_to_list(FieldName), TableDesc), | |||
There was a problem hiding this comment.
What is the advantage of knowing that find_key failed over knowing that lists:keyfind failed? For me the code before the change was more intuitive as everyone knows what lists:keyfind is while nobody knows what find_key is, so to interpret the error one would need to look at the source code. This is against the principle of least surprise as well because we already have many matches like Pattern = lists:keyfind(...) in the code and everyone knows what they do. Here we have suddenly something different.
I know that {badmatch, false} does not give the information about the key, but we have debugging tools that show it easily and this problem shouldn't occur in production env.
There was a problem hiding this comment.
About these two changes.
We have this function:
mongoose_rdbms:prepare(StatementName, IgnoredByEveryoneExceptMSSQL, IgnoredByEveryoneExceptMSSQL, SQL),
If someone makes a typo in one of two arguments (list of fields or a table name), then:
- MySQL would not complain
- PgSQL would not complain
- MSSQL would not complain, when you run
mongoose_rdbms:prepare - MSSQL would crash in
mongoose_rdbms:execute. Crash with that:
when=2021-01-18T15:12:18.010961+00:00 level=error reason="{{badmatch,false},[{mongoose_rdbms_odbc,field_name_to_mapper,3,[{file,\"/home/circleci/app/src/rdbms/mongoose_rdbms_odbc.erl\"},{line,159}]},{mongoose_rdbms_odbc,'-prepare/5-lc$^0/1-0-',3,[{file,\"/home/circleci/app/src/rdbms/mongoose_rdbms_odbc.erl\"},{line,81}]},{mongoose_rdbms_odbc,prepare,5,[{file,\"/home/circleci/app/src/rdbms/mongoose_rdbms_odbc.erl\"},{line,81}]},{mongoose_rdbms,prepare_statement,2,[{file,\"/home/circleci/app/src/rdbms/mongoose_rdbms.erl\"},{line,706}]},{mongoose_rdbms,sql_execute,3,[{file,\"/home/circleci/app/src/rdbms/mongoose_rdbms.erl\"},{line,691}]},{mongoose_rdbms,run_sql_cmd,4,[{file,\"/home/circleci/app/src/rdbms/mongoose_rdbms.erl\"},{line,568}]},{wpool_process,handle_call,3,[{file,\"/home/circleci/app/_build/default/lib/worker_pool/src/wpool_process.erl\"},{line,246}]},{gen_server,try_handle_call,4,[{file,\"gen_server.erl\"},{line,706}]}]}" pid=<0.23529.2> at=gen_server:error_info/7:934 state="{state,<0.23530.2>,#{muc_light_blocking_delete_all => {<<\"DELETE FROM muc_light_blocking\">>,[]},muc_light_config_delete_all => {<<\"DELETE FROM muc_light_config\">>,[]},muc_light_delete_blocking => {<<\"DELETE FROM muc_light_blocking WHERE luser = ? AND lserver = ?\">>,[#Fun<mongoose_rdbms_odbc.2.126473252>,#Fun<mongoose_rdbms_odbc.2.126473252>]},muc_light_insert_room => {<<\"INSERT INTO muc_light_rooms (luser, lserver, version) VALUES (?, ?, ?)\">>,[#Fun<mongoose_rdbms_odbc.2.126473252>,#Fun<mongoose_rdbms_odbc.2.126473252>,#Fun<mongoose_rdbms_odbc.2.126473252>]},muc_light_occupants_delete_all => {<<\"DELETE FROM muc_light_occupants\">>,[]},muc_light_rooms_delete_all => {<<\"DELETE FROM muc_light_rooms\">>,[]},muc_light_select_room_id => {<<\"SELECT id FROM muc_light_rooms WHERE luser = ? AND lserver = ?\">>,[#Fun<mongoose_rdbms_odbc.2.126473252>,#Fun<mongoose_rdbms_odbc.2.126473252>]},muc_light_select_user_rooms => {<<\"SELECT r.luser, r.lserver FROM muc_light_occupants AS o INNER JOIN muc_light_rooms AS r ON o.room_id = r.id WHERE o.luser = ? AND o.lserver = ?\">>,[#Fun<mongoose_rdbms_odbc.2.126473252>,#Fun<mongoose_rdbms_odbc.2.126473252>]},privacy_data_delete_user => {<<\"DELETE FROM privacy_list_data WHERE id IN (SELECT id FROM privacy_list WHERE username=?)\">>,[#Fun<mongoose_rdbms_odbc.2.126473252>]},privacy_default_delete => {<<\"DELETE from privacy_default_list WHERE username=?\">>,[#Fun<mongoose_rdbms_odbc.2.126473252>]},privacy_default_get_name => {<<\"SELECT name FROM privacy_default_list WHERE username=?\">>,[#Fun<mongoose_rdbms_odbc.2.126473252>]},privacy_list_delete_multiple => {<<\"DELETE FROM privacy_list WHERE username=?\">>,[#Fun<mongoose_rdbms_odbc.2.126473252>]},private_remove_user => {<<\"DELETE FROM private_storage WHERE username=?\">>,[#Fun<mongoose_rdbms_odbc.2.126473252>]},roster_delete => {<<\"DELETE FROM rosterusers WHERE username=?\">>,[#Fun<mongoose_rdbms_odbc.2.126473252>]},roster_get => {<<\"SELECT username, jid, nick, subscription, ask, askmessage FROM rosterusers WHERE username=?\">>,[#Fun<mongoose_rdbms_odbc.2.126473252>]},roster_group_delete => {<<\"DELETE FROM rostergroups WHERE username=?\">>,[#Fun<mongoose_rdbms_odbc.2.126473252>]},roster_group_get => {<<\"SELECT jid, grp FROM rostergroups WHERE username=?\">>,[#Fun<mongoose_rdbms_odbc.2.126473252>]},smart_markers_upsert => {<<\"MERGE INTO smart_markers WITH (SERIALIZABLE) as target USING (SELECT ? AS from_jid, ? AS to_jid, ? AS thread, ? AS type) AS source (from_jid, to_jid, thread, type) ON (targ\"...>>,[#Fun<mongoose_rdbms_odbc.2.126473252>,#Fun<mongoose_rdbms_odbc.2.126473252>,#Fun<mongoose_rdbms_odbc.2.126473252>,#Fun<mongoose_rdbms_odbc.2.126473252>,#Fun<mongoose_rdbms_odbc.2.126473252>,#Fun<mongoose_rdbms_odbc.2.126473252>,#Fun<mongoose_rdbms_odbc.2.126473252>,#Fun<mongoose_rdbms_odbc.2.126473252>,#Fun<mongoose_rdbms_odbc.2.126473252>,#Fun<mongoose_rdbms_odbc.4.126473252>,#Fun<mongoose_rdbms_odbc.2.126473252>,#Fun<mongoose_rdbms_odbc.4.126473252>]},vcard_remove => {<<\"DELETE FROM vcard WHERE username=? AND server=?\">>,[#Fun<mongoose_rdbms_odbc.2.126473252>,#Fun<mongoose_rdbms_odbc.2.126473252>]},vcard_search_remove => {<<\"DELETE FROM vcard_search WHERE lusername=? AND server=?\">>,[#Fun<mongoose_rdbms_odbc.2.126473252>,#Fun<mongoose_rdbms_odbc.2.126473252>]}},undefined}" name=wpool_pool-mongoose_wpool$rdbms$global$default-1 log= last_message="{sql_cmd,{sql_execute,muc_light_select_blocking_cnt,[<<\"bob_room_chatmarker_is_overriden_and_only_unique_markers_are_delivered_37.948585\">>,<<\"localhost\">>,\"2\",<<\"alice_room_chatmarker_is_overriden_and_only_unique_markers_are_delivered_37.948585@localhost\">>]},-576459807706}" label={gen_server,terminate} client_info="{<0.24039.2>,{<0.24039.2>,[{gen,do_call,4,[{file,\"gen.erl\"},{line,208}]},{gen_server,call,3,[{file,\"gen_server.erl\"},{line,242}]},{mongoose_rdbms,execute_successfully,3,[{file,\"/home/circleci/app/src/rdbms/mongoose_rdbms.erl\"},{line,203}]},{mod_muc_light_db_rdbms,get_blocking,3,[{file,\"/home/circleci/app/src/muc_light/mod_muc_light_db_rdbms.erl\"},{line,484}]},{timer,tc,3,[{file,\"timer.erl\"},{line,197}]},{mod_muc_light_db_backend,get_blocking,3,[{file,[]},{line,78}]},{mod_muc_light_utils,filter_out_loop,5,[{file,\"/home/circleci/app/src/muc_light/mod_muc_light_utils.erl\"},{line,132}]},{mod_muc_light,try_to_create_room,3,[{file,\"/home/circleci/app/src/muc_light/mod_muc_light.erl\"},{line,88}]}]}}"
when=2021-01-18T15:12:18.016294+00:00 level=error pid=<0.23529.2> at=proc_lib:crash_report/4:525 report="[[{initial_call,{wpool_process,init,['Argument__1']}},{pid,<0.23529.2>},{registered_name,'wpool_pool-mongoose_wpool$rdbms$global$default-1'},{error_info,{error,{badmatch,false},[{mongoose_rdbms_odbc,field_name_to_mapper,3,[{file,\"/home/circleci/app/src/rdbms/mongoose_rdbms_odbc.erl\"},{line,159}]},{mongoose_rdbms_odbc,'-prepare/5-lc$^0/1-0-',3,[{file,\"/home/circleci/app/src/rdbms/mongoose_rdbms_odbc.erl\"},{line,81}]},{mongoose_rdbms_odbc,prepare,5,[{file,\"/home/circleci/app/src/rdbms/mongoose_rdbms_odbc.erl\"},{line,81}]},{mongoose_rdbms,prepare_statement,2,[{file,\"/home/circleci/app/src/rdbms/mongoose_rdbms.erl\"},{line,706}]},{mongoose_rdbms,sql_execute,3,[{file,\"/home/circleci/app/src/rdbms/mongoose_rdbms.erl\"},{line,691}]},{mongoose_rdbms,run_sql_cmd,4,[{file,\"/home/circleci/app/src/rdbms/mongoose_rdbms.erl\"},{line,568}]},{wpool_process,handle_call,3,[{file,\"/home/circleci/app/_build/default/lib/worker_pool/src/wpool_process.erl\"},{line,246}]},{gen_server,try_handle_call,4,[{file,\"gen_server.erl\"},{line,706}]}]}},{ancestors,['wpool_pool-mongoose_wpool$rdbms$global$default-process-sup','mongoose_wpool$rdbms$global$default',mongoose_wpool_rdbms_sup,mongoose_wpool_sup,ejabberd_sup,<0.5764.1>]},{message_queue_len,1},{messages,[{'EXIT',<0.23530.2>,normal}]},{links,[<0.23468.2>]},{dictionary,[{wpool_task,{undefined,63778201938,{call,{sql_cmd,{sql_execute,muc_light_select_blocking_cnt,[<<\"bob_room_chatmarker_is_overriden_and_only_unique_markers_are_delivered_37.948585\">>,<<\"localhost\">>,\"2\",<<\"alice_room_chatmarker_is_overriden_and_only_unique_markers_are_delivered_37.9485\"...>>]},-576459807706}}}}]},{trap_exit,true},{status,running},{heap_size,28690},{stack_size,28},{reductions,2263881}],[]]" label={proc_lib,crash}
If someone develops a new query in MySQL, once he runs a test with MSSQL, he would get an error about a crash in rdbms server.
He would not know which query was it, which field is missing.
So if the query is INSERT_15_FIELDS, it could be a bit confusing.
Basically, it's like two hours debugging without proper error logging...
I could remove it though ;)
b3f3bb2 Debugging this bug was not optimal even for me (i.e 2 hours easily).
There was a problem hiding this comment.
Thanks for the explanation. This justifies the find_key/2 for me, but why catch C:R:S there if we are throwing a very specific error of type error:#{what => find_key_failed}? This catch-all is an anti-pattern to me, it can disguise random errors and crashes as query preparation failures. I would just log the issue directly from find_key and avoid the try..catch altogether, but I am open to any solution, just please do not catch everything.
Edit: I agreed to catching C:R:S, the risks are smaller than I thought. It's good as it is now.
| @@ -75,7 +75,16 @@ query(Connection, Query, Timeout) -> | |||
| -spec prepare(Connection :: term(), Name :: atom(), Table :: binary(), | |||
| Fields :: [binary()], Statement :: iodata()) -> | |||
| {ok, {binary(), [fun((term()) -> tuple())]}}. | |||
| prepare(Connection, _Name, Table, Fields, Statement) -> | |||
| prepare(Connection, Name, Table, Fields, Statement) -> | |||
| try prepare2(Connection, Table, Fields, Statement) | |||
There was a problem hiding this comment.
I am not convinced to add the extra code only to display the keys if someone makes a mistake in the prepared query...
arcusfelis
force-pushed
the
mu-prepared-queries-muclight
branch
from
16a97e3
to
dbf14c2
Compare
src/rdbms/mongoose_rdbms_odbc.erl
Outdated
| @@ -156,7 +165,7 @@ field_name_to_mapper(_ServerType, _TableDesc, <<"limit">>) -> | |||
| field_name_to_mapper(_ServerType, _TableDesc, <<"offset">>) -> | |||
| fun(P) -> {sql_integer, [P]} end; | |||
| field_name_to_mapper(_ServerType, TableDesc, FieldName) -> | |||
| {_, ODBCType} = lists:keyfind(unicode:characters_to_list(FieldName), 1, TableDesc), | |||
| {_, ODBCType} = find_key(unicode:characters_to_list(FieldName), TableDesc), | |||
There was a problem hiding this comment.
Thanks for the explanation. This justifies the find_key/2 for me, but why catch C:R:S there if we are throwing a very specific error of type error:#{what => find_key_failed}? This catch-all is an anti-pattern to me, it can disguise random errors and crashes as query preparation failures. I would just log the issue directly from find_key and avoid the try..catch altogether, but I am open to any solution, just please do not catch everything.
Edit: I agreed to catching C:R:S, the risks are smaller than I thought. It's good as it is now.
Make select_room_id_and_version prepared too
This PR converts SQL queries to prepared SQL queries.
Proposed changes include: