Partially revert "PERF: Improve query performance all inbox private messages. (#14304)" (#14344)

Partially revert “PERF: Improve query performance all inbox private messages. (#14304)” (#14344)

This partially reverts commit ddb458343dc39a7a8c99467dcd809b444514fe2c.

Seeing performance degrade on larger sites so back to drawing board on this one. Instead of the DISTINCT LEFT JOIN, we switch back to IN(subquery).

diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb
index 4de9618..b23245c 100644
--- a/app/controllers/topics_controller.rb
+++ b/app/controllers/topics_controller.rb
@@ -973,7 +973,7 @@ class TopicsController < ApplicationController
 
     topic_ids = TopicsBulkAction.new(
       current_user,
-      topic_scope.distinct(false).pluck(:id),
+      topic_scope.pluck(:id),
       type: "dismiss_topics"
     ).perform!
 
@@ -1245,11 +1245,7 @@ class TopicsController < ApplicationController
     if inbox = params[:private_message_inbox]
       filter = private_message_filter(topic_query, inbox)
       topic_query.options[:limit] = false
-
-      topic_query
-        .filter_private_messages_unread(current_user, filter)
-        .distinct(false)
-        .pluck(:id)
+      topics = topic_query.filter_private_messages_unread(current_user, filter)
     else
       topics = TopicQuery.unread_filter(topic_query.joined_topic_user, staff: guardian.is_staff?).listable_topics
       topics = TopicQuery.tracked_filter(topics, current_user.id) if params[:tracked].to_s == "true"
@@ -1268,9 +1264,9 @@ class TopicsController < ApplicationController
       if params[:tag_name].present?
         topics = topics.joins(:tags).where("tags.name": params[:tag_name])
       end
-
-      topics.pluck(:id)
     end
+
+    topics.pluck(:id)
   end
 
   def private_message_filter(topic_query, inbox)
diff --git a/lib/topic_query/private_message_lists.rb b/lib/topic_query/private_message_lists.rb
index 1a0c770..c056d59 100644
--- a/lib/topic_query/private_message_lists.rb
+++ b/lib/topic_query/private_message_lists.rb
@@ -145,18 +145,25 @@ class TopicQuery
       elsif type == :all
         group_ids = group_with_messages_ids(user)
 
-        result = result.joins(<<~SQL)
-        LEFT JOIN topic_allowed_users tau
-          ON tau.topic_id = topics.id
-          AND tau.user_id = #{user.id.to_i}
-        LEFT JOIN topic_allowed_groups tag
-          ON tag.topic_id = topics.id
-          #{group_ids.present? ? "AND tag.group_id IN (#{group_ids.join(",")})" : ""}
-        SQL
-
-        result = result
-          .where("tag.topic_id IS NOT NULL OR tau.topic_id IS NOT NULL")
-          .distinct
+        result =
+        if group_ids.present?
+          result.where(<<~SQL)
+            topics.id IN (
+              SELECT topic_id
+              FROM topic_allowed_users
+              WHERE user_id = #{user.id.to_i}
+              UNION ALL
+              SELECT topic_id FROM topic_allowed_groups
+              WHERE group_id IN (#{group_ids.join(",")})
+            )
+          SQL
+        else
+          result.joins(<<~SQL)
+          INNER JOIN topic_allowed_users tau
+            ON tau.topic_id = topics.id
+            AND tau.user_id = #{user.id.to_i}
+          SQL
+        end
       end
 
       result = result.joins("LEFT OUTER JOIN topic_users AS tu ON (topics.id = tu.topic_id AND tu.user_id = #{user.id.to_i})")
@@ -238,23 +245,30 @@ class TopicQuery
       # query here as it can easily lead to an inefficient query.
       group_ids = group_with_messages_ids(user)
 
-      list = list.joins(<<~SQL)
-      LEFT JOIN group_archived_messages gm
-        ON gm.topic_id = topics.id
-        #{group_ids.present? ? "AND gm.group_id IN (#{group_ids.join(",")})" : ""}
-      LEFT JOIN user_archived_messages um
-        ON um.user_id = #{user.id.to_i}
-        AND um.topic_id = topics.id
-      SQL
+      if group_ids.present?
+        list = list.joins(<<~SQL)
+          LEFT JOIN group_archived_messages gm
+            ON gm.topic_id = topics.id
+            AND gm.group_id IN (#{group_ids.join(",")})
+          LEFT JOIN user_archived_messages um
+            ON um.user_id = #{user.id.to_i}
+            AND um.topic_id = topics.id
+        SQL
 
-      list =
         if archived
           list.where("um.user_id IS NOT NULL OR gm.topic_id IS NOT NULL")
         else
           list.where("um.user_id IS NULL AND gm.topic_id IS NULL")
         end
+      else
+        list = list.joins(<<~SQL)
+          LEFT JOIN user_archived_messages um
+          ON um.user_id = #{user.id.to_i}
+          AND um.topic_id = topics.id
+        SQL
 
-      list
+        list.where("um.user_id IS #{archived ? "NOT NULL" : "NULL"}")
+      end
     end
 
     def not_archived(list, user)

GitHub sha: 27bad28c530c89acab35a56b945b6a3924280f4b

This commit appears in #14344 which was approved by martin. It was merged by tgxworld.

This commit has been mentioned on Discourse Meta. There might be relevant details there: