From 800c6e1a6853784bd11f1561812c97d91ccc01d3 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Fri, 9 Jul 2021 20:50:24 +0100 Subject: [PATCH] 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 --- app/models/post_mover.rb | 3 +- app/models/topic_user.rb | 52 ++++++++++++++++++++-------------- spec/models/post_mover_spec.rb | 13 +++++++++ 3 files changed, 45 insertions(+), 23 deletions(-) diff --git a/app/models/post_mover.rb b/app/models/post_mover.rb index fcbfdb845fa..7762f149f01 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 1c3e64169b2..5eaecd27163 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 e5e26d2c331..3df580a71fd 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