PERF: Optimize search in private messages query (#14660)

PERF: Optimize search in private messages query (#14660)

  • PERF: Remove JOIN on categories for PM search

JOIN on categories is not needed when searchin in private messages as PMs are not categorized.

  • DEV: Use == for string comparison

  • PERF: Optimize query for allowed topic groups

There was a query that checked for all topics a user or their groups were allowed to see. This used UNION between topic_allowed_users and topic_allowed_groups which was very inefficient. That was replaced with a OR condition that checks in either tables more efficiently.

diff --git a/app/models/post.rb b/app/models/post.rb
index 86bb006..5a8488e 100644
--- a/app/models/post.rb
+++ b/app/models/post.rb
@@ -84,9 +84,13 @@ class Post < ActiveRecord::Base
 
   register_custom_field_type(NOTICE, :json)
 
-  scope :private_posts_for_user, ->(user) {
-    where("posts.topic_id IN (#{Topic::PRIVATE_MESSAGES_SQL})", user_id: user.id)
-  }
+  scope :private_posts_for_user, ->(user) do
+    where(
+      "posts.topic_id IN (#{Topic::PRIVATE_MESSAGES_SQL_USER})
+      OR posts.topic_id IN (#{Topic::PRIVATE_MESSAGES_SQL_GROUP})",
+      user_id: user.id
+    )
+  end
 
   scope :by_newest, -> { order('created_at DESC, id DESC') }
   scope :by_post_number, -> { order('post_number ASC') }
diff --git a/app/models/topic.rb b/app/models/topic.rb
index 81d0d56..fe369fd 100644
--- a/app/models/topic.rb
+++ b/app/models/topic.rb
@@ -267,19 +267,25 @@ class Topic < ActiveRecord::Base
   # Return private message topics
   scope :private_messages, -> { where(archetype: Archetype.private_message) }
 
-  PRIVATE_MESSAGES_SQL = <<~SQL
+  PRIVATE_MESSAGES_SQL_USER = <<~SQL
     SELECT topic_id
     FROM topic_allowed_users
     WHERE user_id = :user_id
-    UNION ALL
+  SQL
+
+  PRIVATE_MESSAGES_SQL_GROUP = <<~SQL
     SELECT tg.topic_id
     FROM topic_allowed_groups tg
     JOIN group_users gu ON gu.user_id = :user_id AND gu.group_id = tg.group_id
   SQL
 
-  scope :private_messages_for_user, ->(user) {
-    private_messages.where("topics.id IN (#{PRIVATE_MESSAGES_SQL})", user_id: user.id)
-  }
+  scope :private_messages_for_user, ->(user) do
+    private_messages.where(
+      "topics.id IN (#{PRIVATE_MESSAGES_SQL_USER})
+      OR topics.id IN (#{PRIVATE_MESSAGES_SQL_GROUP})",
+      user_id: user.id
+    )
+  end
 
   scope :listable_topics, -> { where('topics.archetype <> ?', Archetype.private_message) }
 
diff --git a/lib/search.rb b/lib/search.rb
index 231b5df..516660b 100644
--- a/lib/search.rb
+++ b/lib/search.rb
@@ -895,12 +895,15 @@ class Search
   def posts_query(limit, type_filter: nil, aggregate_search: false)
     posts = Post.where(post_type: Topic.visible_post_types(@guardian.user))
       .joins(:post_search_data, :topic)
-      .joins("LEFT JOIN categories ON categories.id = topics.category_id")
+
+    if type_filter != "private_messages"
+      posts = posts.joins("LEFT JOIN categories ON categories.id = topics.category_id")
+    end
 
     is_topic_search = @search_context.present? && @search_context.is_a?(Topic)
     posts = posts.where("topics.visible") unless is_topic_search
 
-    if type_filter === "private_messages" || (is_topic_search && @search_context.private_message?)
+    if type_filter == "private_messages" || (is_topic_search && @search_context.private_message?)
       posts = posts
         .where(
           "topics.archetype = ? AND post_search_data.private_message",
@@ -910,7 +913,7 @@ class Search
       unless @guardian.is_admin?
         posts = posts.private_posts_for_user(@guardian.user)
       end
-    elsif type_filter === "all_topics"
+    elsif type_filter == "all_topics"
       private_posts = posts
         .where(
           "topics.archetype = ? AND post_search_data.private_message",
@@ -973,7 +976,7 @@ class Search
     posts =
       if @search_context.present?
         if @search_context.is_a?(User)
-          if type_filter === "private_messages"
+          if type_filter == "private_messages"
             if @guardian.is_admin? && !@search_all_pms
               posts.private_posts_for_user(@search_context)
             else
@@ -1036,57 +1039,61 @@ class Search
       )
       SQL
 
-      category_search_priority = <<~SQL
-      (
-        CASE categories.search_priority
-        WHEN #{Searchable::PRIORITIES[:very_high]}
-        THEN 3
-        WHEN #{Searchable::PRIORITIES[:very_low]}
-        THEN 1
-        ELSE 2
-        END
-      )
-      SQL
+      if type_filter != "private_messages"
+        category_search_priority = <<~SQL
+        (
+          CASE categories.search_priority
+          WHEN #{Searchable::PRIORITIES[:very_high]}
+          THEN 3
+          WHEN #{Searchable::PRIORITIES[:very_low]}
+          THEN 1
+          ELSE 2
+          END
+        )
+        SQL
 
-      category_priority_weights = <<~SQL
-      (
-        CASE categories.search_priority
-        WHEN #{Searchable::PRIORITIES[:low]}
-        THEN #{SiteSetting.category_search_priority_low_weight}
-        WHEN #{Searchable::PRIORITIES[:high]}
-        THEN #{SiteSetting.category_search_priority_high_weight}
-        ELSE
-          CASE WHEN topics.closed
-          THEN 0.9
-          ELSE 1
+        category_priority_weights = <<~SQL
+        (
+          CASE categories.search_priority
+          WHEN #{Searchable::PRIORITIES[:low]}
+          THEN #{SiteSetting.category_search_priority_low_weight}
+          WHEN #{Searchable::PRIORITIES[:high]}
+          THEN #{SiteSetting.category_search_priority_high_weight}
+          ELSE
+            CASE WHEN topics.closed
+            THEN 0.9
+            ELSE 1
+            END
           END
-        END
-      )
-      SQL
+        )
+        SQL
 
-      data_ranking =
-        if @term.blank?
-          "(#{category_priority_weights})"
-        else
-          "(#{rank} * #{category_priority_weights})"
-        end
+        data_ranking =
+          if @term.blank?
+            "(#{category_priority_weights})"
+          else
+            "(#{rank} * #{category_priority_weights})"
+          end
 
-      posts =
-        if aggregate_search
-          posts.order("MAX(#{category_search_priority}) DESC", "MAX(#{data_ranking}) DESC")
-        else
-          posts.order("#{category_search_priority} DESC", "#{data_ranking} DESC")
-        end
+        posts =
+          if aggregate_search
+            posts.order("MAX(#{category_search_priority}) DESC", "MAX(#{data_ranking}) DESC")
+          else
+            posts.order("#{category_search_priority} DESC", "#{data_ranking} DESC")
+          end
+      end
 
       posts = posts.order("topics.bumped_at DESC")
     end
 
-    posts =
-      if secure_category_ids.present?
-        posts.where("(categories.id IS NULL) OR (NOT categories.read_restricted) OR (categories.id IN (?))", secure_category_ids).references(:categories)
-      else
-        posts.where("(categories.id IS NULL) OR (NOT categories.read_restricted)").references(:categories)
-      end
+    if type_filter != "private_messages"
+      posts =
+        if secure_category_ids.present?
+          posts.where("(categories.id IS NULL) OR (NOT categories.read_restricted) OR (categories.id IN (?))", secure_category_ids).references(:categories)
+        else
+          posts.where("(categories.id IS NULL) OR (NOT categories.read_restricted)").references(:categories)
+        end
+    end
 
     if @order
       advanced_order = Search.advanced_orders&.fetch(@order, nil)

GitHub sha: f003e31e2ff0bec962c4e8010a46906c9e789203

This commit appears in #14660 which was approved by ZogStriP. It was merged by udan11.