Skip to content
Permalink
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
tgxworld committed Sep 15, 2021
1 parent d1d2298 commit ddb4583
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 20 deletions.
15 changes: 9 additions & 6 deletions app/controllers/topics_controller.rb
Expand Up @@ -952,7 +952,7 @@ def bulk
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]
Expand All @@ -968,13 +968,12 @@ def private_message_reset_new
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!

Expand Down Expand Up @@ -1246,7 +1245,11 @@ def bulk_unread_topic_ids
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"
Expand All @@ -1265,9 +1268,9 @@ def bulk_unread_topic_ids
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)
Expand Down
43 changes: 32 additions & 11 deletions lib/topic_query/private_message_lists.rb
Expand Up @@ -143,16 +143,20 @@ def private_messages_for(user, type)
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})")
Expand Down Expand Up @@ -229,8 +233,15 @@ def append_read_state(list, group)
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
Expand Down Expand Up @@ -264,5 +275,15 @@ def group
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
6 changes: 3 additions & 3 deletions spec/lib/topic_query/private_message_lists_spec.rb
Expand Up @@ -43,7 +43,7 @@

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

Expand Down Expand Up @@ -75,7 +75,7 @@
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

Expand All @@ -86,7 +86,7 @@
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

Expand Down

1 comment on commit ddb4583

@discoursebot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

https://meta.discourse.org/t/inboxes-list-all-messages/203852/1

Please sign in to comment.