Prepared queries for MAM #2961

arcusfelis · 2020-12-01T09:44:42Z

This PR is rebased version of #2958

Proposed changes include:

We use prepared queries in mod_mam_rdbms_arch

codecov · 2020-12-01T10:04:40Z

Codecov Report

Merging #2961 (031e841) into mu-prepared-queries (0b736f6) will increase coverage by 0.28%.
The diff coverage is 84.92%.

@@                   Coverage Diff                   @@
##           mu-prepared-queries    #2961      +/-   ##
=======================================================
+ Coverage                79.10%   79.39%   +0.28%     
=======================================================
  Files                      377      382       +5     
  Lines                    32846    32810      -36     
=======================================================
+ Hits                     25982    26048      +66     
+ Misses                    6864     6762     -102

Impacted Files	Coverage Δ
src/mam/mod_mam.erl	`88.88% <ø> (ø)`
src/rdbms/mongoose_rdbms.erl	`63.55% <40.00%> (-1.85%)`	⬇️
src/mam/mod_mam_rdbms_arch.erl	`48.45% <47.14%> (+9.18%)`	⬆️
src/mam/mod_mam_utils.erl	`88.92% <85.71%> (-0.08%)`	⬇️
src/mam/mam_filter.erl	`91.66% <91.66%> (ø)`
src/mam/mod_mam_muc_rdbms_arch.erl	`94.73% <92.75%> (+6.22%)`	⬆️
src/mam/mam_lookup.erl	`96.15% <96.15%> (ø)`
src/mam/mam_lookup_sql.erl	`98.43% <98.43%> (ø)`
src/mam/mam_decoder.erl	`100.00% <100.00%> (ø)`
src/mam/mam_encoder.erl	`100.00% <100.00%> (ø)`
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b736f6...031e841. Read the comment docs.

chrzaszcz

Thanks for the improvements.
I added some comments and I think a few of them should be addressed to make the code even better.

src/mam/mod_mam_rdbms_arch.erl

chrzaszcz · 2020-12-01T16:23:18Z

src/rdbms/rdbms_queries.erl

@@ -28,8 +28,13 @@

 -export([get_db_type/0,
         begin_trans/0,
+         get_db_specific_limits/0,


Seems unused, right?

get_db_specific_limits/0 is used now...

src/mam/mod_mam_rdbms_arch.erl

chrzaszcz

I added some comments. There is a lot of good changes, thanks for that.

However, I think the PR itself became a bit too big, I hoped we could divide the work as agreed before.

Anyway, the only serious concerns are about the untested lines of code.

chrzaszcz · 2020-12-07T08:16:22Z

include/mongoose_mam.hrl

@@ -0,0 +1,2 @@
+-record(db_mapping, {column :: atom(), param :: atom(), format :: atom()}).
+-record(lookup_field, {op :: atom(), column :: atom(), param :: atom(), required :: atom(), value_maker :: atom()}).


Line too long, just put each field in a separate line.

There are more lines longer than 100 characters in this PR. I am not adding any comments to the rest them, but I think it's good to change them stay within this limit - inaka/elvis recommends that and I think it's a reasonable value.

Btw, is required boolean()?

required :: true | undefined.

Why not true | false?

chrzaszcz · 2020-12-07T08:18:18Z

src/mam/mam_decoder.erl

+decode_row({ExtMessID, ExtSrcJID, ExtData}, Env) ->
+    MessID = mongoose_rdbms:result_to_integer(ExtMessID),
+    SrcJID = decode_jid(ExtSrcJID, Env),
+    Packet = decode_packet(ExtData, Env),
+    {MessID, SrcJID, Packet}.
+
+decode_muc_row({ExtMessID, Nick, ExtData}, Env = #{archive_jid := RoomJID}) ->
+    MessID = mongoose_rdbms:result_to_integer(ExtMessID),
+    SrcJID = jid:replace_resource(RoomJID, Nick),
+    Packet = decode_packet(ExtData, Env),
+    {MessID, SrcJID, Packet}.
+
+decode_muc_gdpr_row({ExtMessID, ExtData}, Env) ->
+    Packet = decode_packet(ExtData, Env),
+    {ExtMessID, Packet}.
+
+decode_retraction_info(_Env, []) -> skip;
+decode_retraction_info(Env, [{ResMessID, Data}]) ->


Could you add type specs for exported functions? Or are you planning to do that later?

src/mam/mam_filter.erl

chrzaszcz · 2020-12-07T08:24:53Z

src/mam/mam_lookup.erl

+
+%% Private logic below
+
+%% There is no optimizations for these queries yet:


Suggested change

%% There is no optimizations for these queries yet:

%% There are no optimizations for these queries yet:

oops, skipped this. Would add that after the final review ^^

src/mam/mam_lookup.erl

src/mam/mod_mam_rdbms_arch.erl

src/rdbms/rdbms_queries.erl

@@ -28,8 +28,13 @@

 -export([get_db_type/0,
         begin_trans/0,
+         get_db_specific_limits/0,


src/mam/mod_mam_utils.erl

src/mam/mod_mam_muc_rdbms_arch.erl

src/mam/mam_lookup_sql.erl

chrzaszcz · 2020-12-10T09:24:20Z

@arcusfelis I think we need to rebase this branch - it shows many more commits than it should which makes it hard to review.

chrzaszcz

Could you remove the minio fix from the PR? The rest look really good, I only have a few concerns, once these few are addressed it can be merged.

chrzaszcz · 2020-12-14T09:01:09Z

src/mam/mam_decoder.erl

+
+-spec unescape_binary(binary(), env_vars()) -> binary().
+unescape_binary(Bin, #{host := Host}) ->
+    %% Funny, rdbms ignores this Host variable


Minor: It's good to have this remark, but maybe remove the word 'funny'? It just does not seem right for me.

removed the whole comment.

src/mam/mam_lookup.erl

chrzaszcz · 2020-12-14T09:21:42Z

src/mam/mam_lookup_sql.erl

+            What = #{what => mam_lookup_failed, statement => StmtName,
+                     sql_query => lookup_sql_binary(QueryType, Table, Env, Filters, Order, OffsetLimit),
+                     reason => Error, host => Host},
+            ?LOG_ERROR(What),
+            error(What)
+    catch error:Error:Stacktrace ->
+            What = #{what => mam_lookup_failed, statement => StmtName,
+                     sql_query => lookup_sql_binary(QueryType, Table, Env, Filters, Order, OffsetLimit),
+                     stacktrace => Stacktrace, reason => Error, host => Host},
+            ?LOG_ERROR(What),
+            erlang:raise(error, Error, Stacktrace)


It worries me a bit that these lines are not tested, but I understand that we only check the happy path in tests. Did you check both error cases manually? I think it's necessary here, especially given the recent formatter crashes - we often have errors in error handling (sic!).

The only reason why it's here is that it's good to see an SQL query string when something fails.
But it's pretty generic code. And we also have mod_mam_utils:success_sql_query/?.

So, I've created mongoose_rdbms:execute_successfully/3 for all these cases (and we don't have error handling in MAM anymore, that's good)

chrzaszcz · 2020-12-14T09:23:36Z

src/mam/mam_lookup_sql.erl

+
+filters_to_sql(Filters) ->
+    SQLs = [filter_to_sql(Filter) || Filter <- Filters],
+    case skip_undefined(SQLs) of


Do we still need to do this? It looks like we do not have undefined in the list. Btw, I prefer code without temporary undefined.

Address review comments

So, we can more easily print it Also, ets-lookup logic would run a bit faster for big iolists

chrzaszcz

Looks good now, great work 👍

…- final commit So, we can more easily print it Also, ets-lookup logic would run a bit faster for big iolists

Store statement as binary in prepared_statements ETS table So, we can more easily print it Also, ets-lookup logic would run a bit faster for big iolists

chrzaszcz reviewed Dec 2, 2020

View reviewed changes

arcusfelis changed the title ~~Prepared queries for one2one MAM~~ Prepared queries for MAM Dec 2, 2020

chrzaszcz reviewed Dec 7, 2020

View reviewed changes

chrzaszcz reviewed Dec 8, 2020

View reviewed changes

src/mam/mam_lookup_sql.erl Outdated Show resolved Hide resolved

arcusfelis force-pushed the mu-prepared-queries-mam branch from f819c09 to f8f3b84 Compare December 10, 2020 09:19

arcusfelis closed this Dec 10, 2020

arcusfelis reopened this Dec 10, 2020

arcusfelis force-pushed the mu-prepared-queries-mam branch from f8f3b84 to 72d5c9a Compare December 10, 2020 09:45

arcusfelis closed this Dec 14, 2020

arcusfelis reopened this Dec 14, 2020

arcusfelis added 18 commits December 14, 2020 10:44

Use prepared queries for message retraction

09eb8b6

Make lookups prepared in mod_mam

7e195bb

Reorder function in mod_mam_rdbms_arch.erl

0fa6a9f

Convert gdpr query to prepared

c77739e

ADd maybe_reserve helper

2990d19

Compactify code for do_lookup_messages

9235285

Adress review commits

d300b56

Use lookup_field record in mod_mam_rdbms_arch

c212904

Refactor opt_count logic (less black magic)

701d952

Add Index Hint

1e34520

Reorder code

45d94ee

Add db_mappings

60bb199

Remove GDPR helpers - use standard functions

86d8cbb

Make prepare_value pure by introducing env_vars

b1f1b8d

Use env_vars for lookups

68a169e

Use env_vars for retraction

afc0b96

Use generic lookup for GDPR data extraction in mod_mam

3f0da9c

Shorten calc_count specs

6f87b56

arcusfelis added 15 commits December 14, 2020 10:44

Reorder code in mod_mam_rdbms_arch

4bb40e9

Use generic logic for mod_mam_muc_rdbms_arch

b863595

Address review comments

67890b5

Pass Offset and Limit as a separate arg

2eaff17

Fix GDPR MAM MUC query

fb02635

Improve type specs

724be34

Remove unused column ids

3f1dffe

Order MAM MUC GDPR messages by ID

b1f7f43

Rename limit_offset_to_args to offset_limit_to_args

36bcc84

Fix mam_muc_extract_gdpr_messages on MSSQL

308785b

Define mod_mam_rdbms_arch:env_vars() type

c8c2e5d

Replace lower with less

cb92c4a

Use boolean type for lookup_field.required field

ee03871

ADd specs for decoder/encoder

14e8803

Remove mod_mam_utils:get_retract_id/3

be8ba0d

arcusfelis force-pushed the mu-prepared-queries-mam branch from 72d5c9a to cc3b30f Compare December 14, 2020 09:45

chrzaszcz reviewed Dec 14, 2020

View reviewed changes

Apply code review comments

5bef9cc

arcusfelis force-pushed the mu-prepared-queries-mam branch from cc3b30f to 5bef9cc Compare December 14, 2020 10:08

arcusfelis added 3 commits December 14, 2020 12:03

Add mongoose_rdbms:execute_successfully

5a1ca51

Address review comments

Pass binary into mongoose_rdbms:prepare/4 from mam modules

7f01379

Store statement as binary in prepared_statements ETS table

031e841

So, we can more easily print it Also, ets-lookup logic would run a bit faster for big iolists

arcusfelis force-pushed the mu-prepared-queries-mam branch from f73c84d to 031e841 Compare December 14, 2020 15:08

chrzaszcz approved these changes Dec 14, 2020

View reviewed changes

chrzaszcz merged commit 3a5d1bc into mu-prepared-queries Dec 14, 2020

chrzaszcz deleted the mu-prepared-queries-mam branch December 14, 2020 15:19

arcusfelis mentioned this pull request Mar 1, 2021

Prepared queries for modules #3039

Merged

arcusfelis added a commit that referenced this pull request Mar 1, 2021

MAM: Use prepared queries for message retraction #2961 - first commit

0afdd09

arcusfelis added a commit that referenced this pull request Mar 1, 2021

MAM: Store statement as binary in prepared_statements ETS table #2961 …

125715a

…- final commit So, we can more easily print it Also, ets-lookup logic would run a bit faster for big iolists

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepared queries for MAM #2961

Prepared queries for MAM #2961

arcusfelis commented Dec 1, 2020

codecov bot commented Dec 1, 2020 •

edited

Loading

chrzaszcz left a comment

chrzaszcz Dec 1, 2020

arcusfelis Dec 2, 2020

This comment was marked as resolved.

arcusfelis Dec 7, 2020

chrzaszcz left a comment

chrzaszcz Dec 7, 2020

arcusfelis Dec 7, 2020

chrzaszcz Dec 8, 2020

chrzaszcz Dec 7, 2020

chrzaszcz Dec 7, 2020

arcusfelis Dec 11, 2020 •

edited

Loading

arcusfelis Dec 14, 2020

This comment was marked as resolved.

chrzaszcz commented Dec 10, 2020

chrzaszcz left a comment

chrzaszcz Dec 14, 2020

arcusfelis Dec 14, 2020 •

edited

Loading

chrzaszcz Dec 14, 2020

arcusfelis Dec 14, 2020

chrzaszcz Dec 14, 2020

arcusfelis Dec 14, 2020

chrzaszcz left a comment

		@@ -0,0 +1,2 @@
		-record(db_mapping, {column :: atom(), param :: atom(), format :: atom()}).
		-record(lookup_field, {op :: atom(), column :: atom(), param :: atom(), required :: atom(), value_maker :: atom()}).


		%% Private logic below

		%% There is no optimizations for these queries yet:

	%% There is no optimizations for these queries yet:
	%% There are no optimizations for these queries yet:

Prepared queries for MAM #2961

Prepared queries for MAM #2961

Conversation

arcusfelis commented Dec 1, 2020

codecov bot commented Dec 1, 2020 • edited Loading

Codecov Report

chrzaszcz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

chrzaszcz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcusfelis Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

chrzaszcz commented Dec 10, 2020

chrzaszcz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcusfelis Dec 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrzaszcz left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 1, 2020 •

edited

Loading

arcusfelis Dec 11, 2020 •

edited

Loading

arcusfelis Dec 14, 2020 •

edited

Loading