-
Notifications
You must be signed in to change notification settings - Fork 428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add remove_domain handler for MAM #3092
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3092 +/- ##
==========================================
+ Coverage 79.02% 79.04% +0.02%
==========================================
Files 386 386
Lines 31830 31829 -1
==========================================
+ Hits 25153 25160 +7
+ Misses 6677 6669 -8
Continue to review full report at Codecov.
|
41d460c
to
f22496e
Compare
src/mam/mod_mam_rdbms_arch.erl
Outdated
@@ -102,6 +103,7 @@ start_hooks(Host, _Opts) -> | |||
ejabberd_hooks:add(mam_archive_size, Host, ?MODULE, archive_size, 50), | |||
ejabberd_hooks:add(mam_lookup_messages, Host, ?MODULE, lookup_messages, 50), | |||
ejabberd_hooks:add(mam_remove_archive, Host, ?MODULE, remove_archive, 50), | |||
ejabberd_hooks:add(remove_domain, Host, ?MODULE, remove_domain, 50), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please, make a hooks/0
function that returns a list of hooks and use ejabbed_hooks:add/1
and ejabberd_hooks:delete/1
functions to start/stop module
src/mam/mod_mam_muc_rdbms_arch.erl
Outdated
@@ -86,6 +87,7 @@ start_hooks(Host, _Opts) -> | |||
ejabberd_hooks:add(mam_muc_archive_size, Host, ?MODULE, archive_size, 50), | |||
ejabberd_hooks:add(mam_muc_lookup_messages, Host, ?MODULE, lookup_messages, 50), | |||
ejabberd_hooks:add(mam_muc_remove_archive, Host, ?MODULE, remove_archive, 50), | |||
ejabberd_hooks:add(remove_domain, Host, ?MODULE, remove_domain, 50), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please, make a hooks/0 function that returns a list of hooks and use ejabbed_hooks:add/1 and ejabberd_hooks:delete/1 functions to start/stop module
@@ -112,6 +115,11 @@ register_prepared_queries() -> | |||
mongoose_rdbms:prepare(mam_muc_archive_remove, mam_muc_message, [room_id], | |||
<<"DELETE FROM mam_muc_message " | |||
"WHERE room_id = ?">>), | |||
mongoose_rdbms:prepare(mam_muc_remove_domain, mam_muc_message, ['mam_server_user.server'], | |||
<<"DELETE FROM mam_muc_message " | |||
"WHERE room_id IN (SELECT id FROM mam_server_user where server = ?)">>), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how optimal is this query? what if we have over 100k records selected by SELECT id FROM mam_server_user where server = ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should work.
ids would go into a temp table in this case. And use similar logic as left joins.
It would be a pretty long transaction though. And it could deadlock still (as always).
|
||
run_remove_domain() -> | ||
Acc = #{}, | ||
rpc(mim(), ejabberd_hooks, run_fold, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove_domain
hook code is already merged into master. I know it was not in original scope, and it's not critical to be a part of this PR.
but please change this rpc call to mongoose_hooks:remove_domain/2
in the following PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. The changes look good, I've added just a few minor comments
f22496e
to
4db482c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved
This PR addresses "Ensure efficient indexing and removal of persistent data by domain name".
MAM already have proper indexes in the mam_server_user table. We should just LEFT JOIN it. OK, we can't LEFT JOIN in DELETEs. But we can use
WHERE IN
instead.Proposed changes include: