PERF: Improve query performance all inbox private messages. (#14304)

PERF: Improve query performance all inbox private messages. (#14304)

First reported in Slow loading on private-message-topic-tracking-state.json - #19 by forkythetoy - bug - Discourse Meta

There are two optimizations being applied here:

  1. Fetch a user’s group ids in a seperate query instead of including it as a sub-query. When I tried a subquery, the query plan becomes very inefficient.

  2. Join against the topic_allowed_users and topic_allowed_groups table instead of doing an IN against a subquery where we UNION the topic_ids from the two tables. From my profiling, this enables PG to do a backwards index scan on the index_topics_on_timestamps_private index.

This commit fixes a bug where listing all messages was incorrectly excluding topics if a topic has been archived by a group even if the user did not belong to the group.

This commit also fixes another bug where dismissing private messages selectively was subjected to the default limit of 30.

diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb
index 710632b..4de9618 100644
--- a/app/controllers/topics_controller.rb
+++ b/app/controllers/topics_controller.rb
@@ -952,7 +952,7 @@ class TopicsController < ApplicationController
   end
 
   def private_message_reset_new
-    topic_query = TopicQuery.new(current_user)
+    topic_query = TopicQuery.new(current_user, limit: false)
 
     if params[:topic_ids].present?
       unless Array === params[:topic_ids]
@@ -968,13 +968,12 @@ class TopicsController < ApplicationController
       params.require(:inbox)
       inbox = params[:inbox].to_s
       filter = private_message_filter(topic_query, inbox)
-      topic_query.options[:limit] = false
       topic_scope = topic_query.filter_private_message_new(current_user, filter)
     end
 
     topic_ids = TopicsBulkAction.new(
       current_user,
-      topic_scope.pluck(:id),
+      topic_scope.distinct(false).pluck(:id),
       type: "dismiss_topics"
     ).perform!
 
@@ -1246,7 +1245,11 @@ class TopicsController < ApplicationController
     if inbox = params[:private_message_inbox]
       filter = private_message_filter(topic_query, inbox)
       topic_query.options[:limit] = false
-      topics = topic_query.filter_private_messages_unread(current_user, filter)
+
+      topic_query
+        .filter_private_messages_unread(current_user, filter)
+        .distinct(false)
+        .pluck(:id)
     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"
@@ -1265,9 +1268,9 @@ class TopicsController < ApplicationController
       if params[:tag_name].present?
         topics = topics.joins(:tags).where("tags.name": params[:tag_name])
       end
-    end
 
-    topics.pluck(:id)
+      topics.pluck(:id)
+    end
   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 9aaadba..1a0c770 100644
--- a/lib/topic_query/private_message_lists.rb
+++ b/lib/topic_query/private_message_lists.rb
@@ -143,16 +143,20 @@ class TopicQuery
       elsif type == :user
         result = result.where("topics.id IN (SELECT topic_id FROM topic_allowed_users WHERE user_id = #{user.id.to_i})")
       elsif type == :all
-        result = result.where("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 (
-                SELECT group_id FROM group_users WHERE user_id = #{user.id.to_i}
-              )
-      )")
+        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
       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})")
@@ -229,8 +233,15 @@ class TopicQuery
     end
 
     def filter_archived(list, user, archived: true)
+      # Executing an extra query instead of a sub-query because it is more
+      # efficient for the PG planner. Caution should be used when changing the
+      # 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
+      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
@@ -264,5 +275,15 @@ class TopicQuery
     def user_first_unread_pm_at(user)
       UserStat.where(user: user).pluck_first(:first_unread_pm_at)
     end
+
+    def group_with_messages_ids(user)
+      @group_with_messages_ids ||= {}
+
+      if ids = @group_with_messages_ids[user.id]
+        return ids
+      end
+
+      @group_with_messages_ids[user.id] = user.groups.where(has_messages: true).pluck(:id)
+    end
   end
 end
diff --git a/spec/lib/topic_query/private_message_lists_spec.rb b/spec/lib/topic_query/private_message_lists_spec.rb
index 6c02589..681a4b8 100644
--- a/spec/lib/topic_query/private_message_lists_spec.rb
+++ b/spec/lib/topic_query/private_message_lists_spec.rb
@@ -43,7 +43,7 @@ describe TopicQuery::PrivateMessageLists do
 
       expect(topics).to eq([])
 
-      GroupArchivedMessage.archive!(user_2.id, group_message)
+      GroupArchivedMessage.archive!(group.id, group_message)
 
       topics = TopicQuery.new(nil).list_private_messages_all(user_2).topics
 
@@ -75,7 +75,7 @@ describe TopicQuery::PrivateMessageLists do
       create_post(user: user_2, topic: group_message)
 
       UserArchivedMessage.archive!(user_2.id, private_message)
-      GroupArchivedMessage.archive!(user_2.id, group_message)
+      GroupArchivedMessage.archive!(group.id, group_message)
 
       topics = TopicQuery.new(nil).list_private_messages_all_sent(user_2).topics
 
@@ -86,7 +86,7 @@ describe TopicQuery::PrivateMessageLists do
   describe '#list_private_messages_all_archive' do
     it 'returns a list of all private messages that has been archived' do
       UserArchivedMessage.archive!(user_2.id, private_message)
-      GroupArchivedMessage.archive!(user_2.id, group_message)
+      GroupArchivedMessage.archive!(group.id, group_message)
 
       topics = TopicQuery.new(nil).list_private_messages_all_archive(user_2).topics
 

GitHub sha: ddb458343dc39a7a8c99467dcd809b444514fe2c

This commit appears in #14304 which was approved by eviltrout. It was merged by tgxworld.

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