From 4491813d2247cb6648caa7145f11532de25fb501 Mon Sep 17 00:00:00 2001 From: Sam Date: Tue, 21 Jul 2015 21:48:07 +1000 Subject: [PATCH] Revert "Revert "PERF: optimise query that gathers topic tracking state"" This reverts commit 909be09f1a0c1817177e61f947b7567a37aa84a5. --- app/controllers/application_controller.rb | 2 +- app/models/topic_tracking_state.rb | 91 +++++++++++++++-------- spec/models/topic_tracking_state_spec.rb | 25 ++++--- 3 files changed, 77 insertions(+), 41 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index aa29468fb26..7e48c5c45da 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -307,7 +307,7 @@ class ApplicationController < ActionController::Base def preload_current_user_data store_preloaded("currentUser", MultiJson.dump(CurrentUserSerializer.new(current_user, scope: guardian, root: false))) - serializer = ActiveModel::ArraySerializer.new(TopicTrackingState.report([current_user.id]), each_serializer: TopicTrackingStateSerializer) + serializer = ActiveModel::ArraySerializer.new(TopicTrackingState.report(current_user.id), each_serializer: TopicTrackingStateSerializer) store_preloaded("topicTrackingStates", MultiJson.dump(serializer)) end diff --git a/app/models/topic_tracking_state.rb b/app/models/topic_tracking_state.rb index 0bf3fa7096c..a98ceb9bf1e 100644 --- a/app/models/topic_tracking_state.rb +++ b/app/models/topic_tracking_state.rb @@ -114,7 +114,7 @@ class TopicTrackingState ).where_values[0] end - def self.report(user_ids, topic_id = nil) + def self.report(user_id, topic_id = nil) # Sam: this is a hairy report, in particular I need custom joins and fancy conditions # Dropping to sql_builder so I can make sense of it. @@ -130,46 +130,77 @@ class TopicTrackingState new = TopicQuery.new_filter(Topic, "xxx").where_values.join(" AND ").gsub!("'xxx'", treat_as_new_topic_clause) sql = < 'private_message' AND - ((#{unread}) OR (#{new})) AND - (topics.visible OR u.admin OR u.moderator) AND - topics.deleted_at IS NULL AND - ( category_id IS NULL OR NOT c.read_restricted OR u.admin OR category_id IN ( - SELECT c2.id FROM categories c2 - JOIN category_groups cg ON cg.category_id = c2.id - JOIN group_users gu ON gu.user_id = u.id AND cg.group_id = gu.group_id - WHERE c2.read_restricted ) - ) - AND NOT EXISTS( SELECT 1 FROM category_users cu - WHERE last_read_post_number IS NULL AND - cu.user_id = u.id AND - cu.category_id = topics.category_id AND - cu.notification_level = #{CategoryUser.notification_levels[:muted]}) + FROM topics + JOIN topic_users tu ON tu.topic_id = topics.id AND tu.user_id = :user_id AND tu.last_read_post_number IS NOT NULL + JOIN allowed_categories c ON c.id = topics.category_id + JOIN users u on u.id = :user_id + WHERE topics.archetype <> 'private_message' AND + (#{unread}) AND + (topics.visible OR u.admin OR u.moderator) AND + topics.deleted_at IS NULL + /*topic_filter*/ + ORDER BY topics.bumped_at DESC + LIMIT 200 + ) X + + UNION ALL + + SELECT * FROM ( + SELECT :user_id user_id, + topics.id topic_id, + topics.created_at, + highest_post_number, + NULL::int last_read_post_number, + topics.category_id, + tu.notification_level + FROM topics + JOIN users u on u.id = :user_id + JOIN user_stats AS us ON us.user_id = u.id + JOIN allowed_categories c ON c.id = topics.category_id + LEFT JOIN topic_users tu ON tu.topic_id = topics.id AND tu.user_id = :user_id AND tu.last_read_post_number IS NOT NULL + + WHERE tu.id IS NULL AND + (#{new}) AND + (topics.visible OR u.admin OR u.moderator) AND + topics.deleted_at IS NULL + /*topic_filter*/ + ORDER BY topics.bumped_at DESC + LIMIT 200 + ) Y SQL if topic_id - sql << " AND topics.id = :topic_id" + sql.gsub! "/*topic_filter*/", " AND topics.id = :topic_id" end - sql << " ORDER BY topics.bumped_at DESC ) SELECT * FROM x LIMIT 500" - SqlBuilder.new(sql) - .map_exec(TopicTrackingState, user_ids: user_ids, topic_id: topic_id) + .map_exec(TopicTrackingState, user_id: user_id, topic_id: topic_id) end diff --git a/spec/models/topic_tracking_state_spec.rb b/spec/models/topic_tracking_state_spec.rb index 6897a2550ac..4729b706d36 100644 --- a/spec/models/topic_tracking_state_spec.rb +++ b/spec/models/topic_tracking_state_spec.rb @@ -20,7 +20,7 @@ describe TopicTrackingState do user = Fabricate(:user) post - report = TopicTrackingState.report([user.id]) + report = TopicTrackingState.report(user.id) expect(report.length).to eq(1) CategoryUser.create!(user_id: user.id, @@ -30,22 +30,23 @@ describe TopicTrackingState do create_post(topic_id: post.topic_id) - report = TopicTrackingState.report([user.id]) + report = TopicTrackingState.report(user.id) expect(report.length).to eq(0) TopicUser.create!(user_id: user.id, topic_id: post.topic_id, last_read_post_number: 1, notification_level: 3) - report = TopicTrackingState.report([user.id]) - expect(report.length).to eq(1) + report = TopicTrackingState.report(user.id) + # no read state for muted categories, query is faster + expect(report.length).to eq(0) end it "correctly gets the tracking state" do - report = TopicTrackingState.report([user.id]) + report = TopicTrackingState.report(user.id) expect(report.length).to eq(0) post.topic.notifier.watch_topic!(post.topic.user_id) - report = TopicTrackingState.report([user.id]) + report = TopicTrackingState.report(user.id) expect(report.length).to eq(1) row = report[0] @@ -56,15 +57,18 @@ describe TopicTrackingState do expect(row.user_id).to eq(user.id) # lets not leak out random users - expect(TopicTrackingState.report([post.user_id])).to be_empty + expect(TopicTrackingState.report(post.user_id)).to be_empty # lets not return anything if we scope on non-existing topic - expect(TopicTrackingState.report([user.id], post.topic_id + 1)).to be_empty + expect(TopicTrackingState.report(user.id, post.topic_id + 1)).to be_empty # when we reply the poster should have an unread row create_post(user: user, topic: post.topic) - report = TopicTrackingState.report([post.user_id, user.id]) + report = TopicTrackingState.report(user.id) + expect(report.length).to eq(0) + + report = TopicTrackingState.report(post.user_id) expect(report.length).to eq(1) row = report[0] @@ -80,6 +84,7 @@ describe TopicTrackingState do post.topic.category_id = category.id post.topic.save - expect(TopicTrackingState.report([post.user_id, user.id]).count).to eq(0) + expect(TopicTrackingState.report(post.user_id)).to be_empty + expect(TopicTrackingState.report(user.id)).to be_empty end end