From fa0277509564a8d663fcd7878709ab4130b07f2f Mon Sep 17 00:00:00 2001
From: Dan Ungureanu <dan@ungureanu.me>
Date: Fri, 11 Jun 2021 03:55:50 +0300
Subject: [PATCH] PERF: Perform user filtering in SQL (#13358)
Notifying about a tag change sometimes resulted in loading a large
number of users in memory just to perform an exclusion. This commit
prefers to do inclusion (i.e. instead of exclude users X, do include
users in groups Y) and does it in SQL to avoid fetching unnecessary
data that is later discarded.
---
app/jobs/regular/notify_tag_change.rb | 17 +++++++----------
app/services/post_alerter.rb | 10 +++++++---
2 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/app/jobs/regular/notify_tag_change.rb b/app/jobs/regular/notify_tag_change.rb
index 4b513dd53b7..e503287810c 100644
--- a/app/jobs/regular/notify_tag_change.rb
+++ b/app/jobs/regular/notify_tag_change.rb
@@ -7,23 +7,20 @@ module Jobs
if post&.topic&.visible?
post_alerter = PostAlerter.new
- post_alerter.notify_post_users(post, excluded_users(args), include_topic_watchers: !post.topic.private_message?, include_category_watchers: false)
+ post_alerter.notify_post_users(post, User.where(id: args[:notified_user_ids]),
+ group_ids: all_tags_in_hidden_groups?(args) ? tag_group_ids(args) : nil,
+ include_topic_watchers: !post.topic.private_message?,
+ include_category_watchers: false
+ )
post_alerter.notify_first_post_watchers(post, post_alerter.tag_watchers(post.topic))
end
end
private
- def excluded_users(args)
- if !args[:diff_tags] || !all_tags_in_hidden_groups?(args)
- return User.where(id: args[:notified_user_ids])
- end
- group_users_join = DB.sql_fragment("LEFT JOIN group_users ON group_users.user_id = users.id AND group_users.group_id IN (:group_ids)", group_ids: tag_group_ids(args))
- condition = DB.sql_fragment("group_users.id IS NULL OR users.id IN (:notified_user_ids)", notified_user_ids: args[:notified_user_ids])
- User.joins(group_users_join).where(condition)
- end
-
def all_tags_in_hidden_groups?(args)
+ return false if args[:diff_tags].blank?
+
Tag
.where(name: args[:diff_tags])
.joins(tag_groups: :tag_group_permissions)
diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb
index 5051070eda3..297a97257dd 100644
--- a/app/services/post_alerter.rb
+++ b/app/services/post_alerter.rb
@@ -651,13 +651,13 @@ class PostAlerter
emails_to_skip_send
end
- def notify_post_users(post, notified, include_topic_watchers: true, include_category_watchers: true, include_tag_watchers: true, new_record: false)
+ def notify_post_users(post, notified, group_ids: nil, include_topic_watchers: true, include_category_watchers: true, include_tag_watchers: true, new_record: false)
return unless post.topic
warn_if_not_sidekiq
condition = +<<~SQL
- id IN (
+ users.id IN (
SELECT id FROM users WHERE false
/*topic*/
/*category*/
@@ -711,12 +711,16 @@ class PostAlerter
tag_ids: tag_ids
)
+ if group_ids.present?
+ notify = notify.joins(:group_users).where("group_users.group_id IN (?)", group_ids)
+ end
+
if post.topic.private_message?
notify = notify.where(staged: false).staff
end
exclude_user_ids = notified.map(&:id)
- notify = notify.where("id NOT IN (?)", exclude_user_ids) if exclude_user_ids.present?
+ notify = notify.where("users.id NOT IN (?)", exclude_user_ids) if exclude_user_ids.present?
DiscourseEvent.trigger(:before_create_notifications_for_users, notify, post)