PERF: Perform user filtering in SQL (#13358)

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.

diff --git a/app/jobs/regular/notify_tag_change.rb b/app/jobs/regular/notify_tag_change.rb
index 4b513dd..e503287 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 5051070..297a972 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)
 

GitHub sha: fa0277509564a8d663fcd7878709ab4130b07f2f

This commit appears in #13358 which was approved by riking and lis2. It was merged by SamSaffron.