From ddb458343dc39a7a8c99467dcd809b444514fe2c Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Wed, 15 Sep 2021 10:29:42 +0800 Subject: [PATCH] PERF: Improve query performance all inbox private messages. (#14304) First reported in https://meta.discourse.org/t/-/202482/19 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. 1. Join against the `topic_allowed_users` and `topic_allowed_groups` table instead of doing an IN against a subquery where we UNION the `topic_id`s 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. --- app/controllers/topics_controller.rb | 15 ++++--- lib/topic_query/private_message_lists.rb | 43 ++++++++++++++----- .../topic_query/private_message_lists_spec.rb | 6 +-- 3 files changed, 44 insertions(+), 20 deletions(-) diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 710632bae0b..4de9618a941 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 9aaadbabbe6..1a0c770a730 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 6c02589c158..681a4b887a7 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