PERF: Improve topic_user.liked update performance when moving posts

PERF: Improve topic_user.liked update performance when moving posts

Previously we would re-calculate topic_user.liked for all users who have ever viewed the source or destination topic. This can be very expensive on large sites. Instead, we can use the array of moved post ids to find which users are actually affected by the move, and restrict the update query to only check/update their records.

On an example site this reduced the update_post_action_cache time from ~27s to 300ms

diff --git a/app/models/post_mover.rb b/app/models/post_mover.rb
index fcbfdb8..7762f14 100644
--- a/app/models/post_mover.rb
+++ b/app/models/post_mover.rb
@@ -418,8 +418,7 @@ class PostMover
   def update_statistics
     destination_topic.update_statistics
     original_topic.update_statistics
-    TopicUser.update_post_action_cache(topic_id: original_topic.id)
-    TopicUser.update_post_action_cache(topic_id: destination_topic.id)
+    TopicUser.update_post_action_cache(topic_id: [original_topic.id, destination_topic.id], post_id: @post_ids)
   end
 
   def update_user_actions
diff --git a/app/models/topic_user.rb b/app/models/topic_user.rb
index 1c3e641..5eaecd2 100644
--- a/app/models/topic_user.rb
+++ b/app/models/topic_user.rb
@@ -372,20 +372,26 @@ class TopicUser < ActiveRecord::Base
 
   end
 
-  def self.update_post_action_cache(opts = {})
-    user_id = opts[:user_id]
-    post_id = opts[:post_id]
-    topic_id = opts[:topic_id]
-    action_type = opts[:post_action_type]
-
-    action_type_name = "liked" if action_type == :like
-
-    raise ArgumentError, "action_type" if action_type && !action_type_name
-
-    unless action_type_name
-      update_post_action_cache(opts.merge(post_action_type: :like))
-      return
-    end
+  # Update the cached topic_user.liked column based on data
+  # from the post_actions table. This is useful when posts
+  # have moved around, or to ensure integrity of the data.
+  #
+  # By default this will update data for all topics and all users.
+  # The parameters can be used to shrink the scope, and make it faster.
+  # user_id, post_id and topic_id can optionally be arrays of ids.
+  #
+  # Providing post_id will automatically scope to the relavent user_id and topic_id.
+  # A provided `topic_id` value will always take presedence, which is
+  # useful when a post has been moved between topics.
+  def self.update_post_action_cache(
+    user_id: nil,
+    post_id: nil,
+    topic_id: nil,
+    post_action_type: :like
+  )
+    raise ArgumentError, "post_action_type must equal :like" if post_action_type != :like
+    raise ArgumentError, "post_id and user_id cannot be supplied together" if user_id && post_id
+    action_type_name = "liked"
 
     builder = DB.build <<~SQL
       UPDATE topic_users tu
@@ -411,21 +417,25 @@ class TopicUser < ActiveRecord::Base
     SQL
 
     if user_id
-      builder.where("tu2.user_id = :user_id", user_id: user_id)
+      builder.where("tu2.user_id IN (:user_id)", user_id: user_id)
     end
 
     if topic_id
-      builder.where("tu2.topic_id = :topic_id", topic_id: topic_id)
+      builder.where("tu2.topic_id IN (:topic_id)", topic_id: topic_id)
     end
 
     if post_id
-      builder.where("tu2.topic_id IN (SELECT topic_id FROM posts WHERE id = :post_id)", post_id: post_id)
-      builder.where("tu2.user_id IN (SELECT user_id FROM post_actions
-                                     WHERE post_id = :post_id AND
-                                           post_action_type_id = :action_type_id)")
+      builder.where("tu2.topic_id IN (SELECT topic_id FROM posts WHERE id IN (:post_id))", post_id: post_id) if !topic_id
+      builder.where(<<~SQL, post_id: post_id)
+        tu2.user_id IN (
+          SELECT user_id FROM post_actions
+          WHERE post_id IN (:post_id)
+          AND post_action_type_id = :action_type_id
+        )
+      SQL
     end
 
-    builder.exec(action_type_id: PostActionType.types[action_type])
+    builder.exec(action_type_id: PostActionType.types[post_action_type])
   end
 
   # cap number of unread topics at count, bumping up last_read if needed
diff --git a/spec/models/post_mover_spec.rb b/spec/models/post_mover_spec.rb
index e5e26d2..3df580a 100644
--- a/spec/models/post_mover_spec.rb
+++ b/spec/models/post_mover_spec.rb
@@ -769,6 +769,19 @@ describe PostMover do
             end
           end
 
+          it "updates topic_user.liked values for both source and destination topics" do
+            expect(TopicUser.find_by(topic: topic, user: user).liked).to eq(false)
+
+            like = Fabricate(:post_action, post: p3, user: user, post_action_type_id: PostActionType.types[:like])
+            expect(TopicUser.find_by(topic: topic, user: user).liked).to eq(true)
+
+            expect(TopicUser.find_by(topic: destination_topic, user: user)).to eq(nil)
+            topic.move_posts(user, [p3.id], destination_topic_id: destination_topic.id)
+
+            expect(TopicUser.find_by(topic: topic, user: user).liked).to eq(false)
+            expect(TopicUser.find_by(topic: destination_topic, user: user).liked).to eq(true)
+          end
+
           context "read state and other stats per user" do
             def create_topic_user(user, topic, opts = {})
               notification_level = opts.delete(:notification_level) || :regular

GitHub sha: 800c6e1a6853784bd11f1561812c97d91ccc01d3

This commit appears in #13687 which was approved by ZogStriP and gschlager. It was merged by davidtaylorhq.