From 9b75d95fc616ea51181d622182b0f74dea8694ac Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Thu, 3 Sep 2020 14:02:15 +0800 Subject: [PATCH] PERF: Keep track of first unread PM and first unread group PM for user. This optimization helps to filter away topics so that the joins on related tables when querying for unread messages is not expensive. --- app/jobs/scheduled/ensure_db_consistency.rb | 3 + app/models/group_user.rb | 46 ++++++++++++++ app/models/post_timing.rb | 25 +++++++- app/models/user_stat.rb | 53 +++++++++++++++- ...531_add_first_unread_pm_a_to_group_user.rb | 16 +++++ ...03_add_first_unread_pm_at_to_user_stats.rb | 18 ++++++ ..._add_index_topics_on_timestamps_private.rb | 18 ++++++ lib/topic_query.rb | 16 ++++- spec/models/group_user_spec.rb | 60 +++++++++++++++++++ spec/models/post_timing_spec.rb | 37 ++++++++++++ spec/models/user_stat_spec.rb | 40 +++++++++++++ 11 files changed, 326 insertions(+), 6 deletions(-) create mode 100644 db/migrate/20200902054531_add_first_unread_pm_a_to_group_user.rb create mode 100644 db/migrate/20200902082203_add_first_unread_pm_at_to_user_stats.rb create mode 100644 db/post_migrate/20200903045539_add_index_topics_on_timestamps_private.rb diff --git a/app/jobs/scheduled/ensure_db_consistency.rb b/app/jobs/scheduled/ensure_db_consistency.rb index 6c21814fa01..c687cbec348 100644 --- a/app/jobs/scheduled/ensure_db_consistency.rb +++ b/app/jobs/scheduled/ensure_db_consistency.rb @@ -35,6 +35,9 @@ module Jobs UserStat.ensure_consistency!(13.hours.ago) measure(UserStat) + GroupUser.ensure_consistency!(13.hours.ago) + measure(GroupUser) + Rails.logger.debug(format_measure) nil end diff --git a/app/models/group_user.rb b/app/models/group_user.rb index 921641d4c64..d83b0606b5a 100644 --- a/app/models/group_user.rb +++ b/app/models/group_user.rb @@ -19,6 +19,52 @@ class GroupUser < ActiveRecord::Base NotificationLevels.all end + def self.ensure_consistency!(last_seen = 1.hour.ago) + update_first_unread_pm(last_seen) + end + + def self.update_first_unread_pm(last_seen, limit: 10_000) + DB.exec(<<~SQL, archetype: Archetype.private_message, last_seen: last_seen, limit: limit) + UPDATE group_users gu + SET first_unread_pm_at = Y.min_date + FROM ( + SELECT + X.group_id, + X.user_id, + X.min_date + FROM ( + SELECT + gu2.group_id, + gu2.user_id, + MIN(t.updated_at) min_date + FROM group_users gu2 + INNER JOIN topic_allowed_groups tag ON tag.group_id = gu2.group_id + INNER JOIN topics t ON t.id = tag.topic_id + INNER JOIN users u ON u.id = gu2.user_id + LEFT JOIN topic_users tu ON t.id = tu.topic_id AND tu.user_id = gu2.user_id + WHERE t.deleted_at IS NULL + AND t.archetype = :archetype + AND tu.last_read_post_number < CASE + WHEN u.admin OR u.moderator + THEN t.highest_staff_post_number + ELSE t.highest_post_number + END + AND (COALESCE(tu.notification_level, 1) >= 2) + GROUP BY gu2.user_id, gu2.group_id + ) AS X + WHERE X.user_id IN ( + SELECT id + FROM users + WHERE last_seen_at IS NOT NULL + AND last_seen_at > :last_seen + ORDER BY last_seen_at DESC + LIMIT :limit + ) + ) Y + WHERE gu.user_id = Y.user_id AND gu.group_id = Y.group_id + SQL + end + protected def set_notification_level diff --git a/app/models/post_timing.rb b/app/models/post_timing.rb index 1bf1c3f4149..8251d429074 100644 --- a/app/models/post_timing.rb +++ b/app/models/post_timing.rb @@ -75,7 +75,9 @@ class PostTiming < ActiveRecord::Base topic.posts.find_by(post_number: post_number).decrement!(:reads) - if !topic.private_message? + if topic.private_message? + set_minimum_first_unread_pm!(topic: topic, user_id: user.id, date: topic.updated_at) + else set_minimum_first_unread!(user_id: user.id, date: topic.updated_at) end end @@ -101,6 +103,27 @@ class PostTiming < ActiveRecord::Base end end + def self.set_minimum_first_unread_pm!(topic:, user_id:, date:) + if topic.topic_allowed_users.exists?(user_id: user_id) + UserStat.where("first_unread_pm_at > ? AND user_id = ?", date, user_id) + .update_all(first_unread_pm_at: date) + else + DB.exec(<<~SQL, date: date, user_id: user_id, topic_id: topic.id) + UPDATE group_users gu + SET first_unread_pm_at = :date + FROM ( + SELECT + gu2.user_id, + gu2.group_id + FROM group_users gu2 + INNER JOIN topic_allowed_groups tag ON tag.group_id = gu2.group_id AND tag.topic_id = :topic_id + WHERE gu2.user_id = :user_id + ) Y + WHERE gu.user_id = Y.user_id AND gu.group_id = Y.group_id + SQL + end + end + def self.set_minimum_first_unread!(user_id:, date:) DB.exec(<<~SQL, date: date, user_id: user_id) UPDATE user_stats diff --git a/app/models/user_stat.rb b/app/models/user_stat.rb index 881b14ae69a..bc2a9d251ca 100644 --- a/app/models/user_stat.rb +++ b/app/models/user_stat.rb @@ -12,10 +12,59 @@ class UserStat < ActiveRecord::Base update_distinct_badge_count update_view_counts(last_seen) update_first_unread(last_seen) + update_first_unread_pm(last_seen) end - def self.update_first_unread(last_seen, limit: 10_000) - DB.exec(<<~SQL, min_date: last_seen, limit: limit, now: 10.minutes.ago) + UPDATE_UNREAD_MINUTES_AGO = 10 + UPDATE_UNREAD_USERS_LIMIT = 10_000 + + def self.update_first_unread_pm(last_seen, limit: UPDATE_UNREAD_USERS_LIMIT) + DB.exec(<<~SQL, archetype: Archetype.private_message, now: UPDATE_UNREAD_MINUTES_AGO.minutes.ago, last_seen: last_seen, limit: limit) + UPDATE user_stats us + SET first_unread_pm_at = COALESCE(Z.min_date, :now) + FROM ( + SELECT + Y.user_id, + Y.min_date + FROM ( + SELECT + u1.id user_id, + X.min_date + FROM users u1 + LEFT JOIN ( + SELECT + tau.user_id, + MIN(t.updated_at) min_date + FROM topic_allowed_users tau + INNER JOIN topics t ON t.id = tau.topic_id + INNER JOIN users u ON u.id = tau.user_id + LEFT JOIN topic_users tu ON t.id = tu.topic_id AND tu.user_id = tau.user_id + WHERE t.deleted_at IS NULL + AND t.archetype = :archetype + AND tu.last_read_post_number < CASE + WHEN u.admin OR u.moderator + THEN t.highest_staff_post_number + ELSE t.highest_post_number + END + AND (COALESCE(tu.notification_level, 1) >= 2) + GROUP BY tau.user_id + ) AS X ON X.user_id = u1.id + ) AS Y + WHERE Y.user_id IN ( + SELECT id + FROM users + WHERE last_seen_at IS NOT NULL + AND last_seen_at > :last_seen + ORDER BY last_seen_at DESC + LIMIT :limit + ) + ) AS Z + WHERE us.user_id = Z.user_id + SQL + end + + def self.update_first_unread(last_seen, limit: UPDATE_UNREAD_USERS_LIMIT) + DB.exec(<<~SQL, min_date: last_seen, limit: limit, now: UPDATE_UNREAD_MINUTES_AGO.minutes.ago) UPDATE user_stats us SET first_unread_at = COALESCE(Y.min_date, :now) FROM ( diff --git a/db/migrate/20200902054531_add_first_unread_pm_a_to_group_user.rb b/db/migrate/20200902054531_add_first_unread_pm_a_to_group_user.rb new file mode 100644 index 00000000000..8cfb624a113 --- /dev/null +++ b/db/migrate/20200902054531_add_first_unread_pm_a_to_group_user.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class AddFirstUnreadPmAToGroupUser < ActiveRecord::Migration[6.0] + def up + add_column :group_users, :first_unread_pm_at, :datetime, null: false, default: -> { 'CURRENT_TIMESTAMP' } + + execute <<~SQL + UPDATE group_users gu + SET first_unread_pm_at = gu.created_at + SQL + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/migrate/20200902082203_add_first_unread_pm_at_to_user_stats.rb b/db/migrate/20200902082203_add_first_unread_pm_at_to_user_stats.rb new file mode 100644 index 00000000000..6f4c8e812ac --- /dev/null +++ b/db/migrate/20200902082203_add_first_unread_pm_at_to_user_stats.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class AddFirstUnreadPmAtToUserStats < ActiveRecord::Migration[6.0] + def up + add_column :user_stats, :first_unread_pm_at, :datetime, null: false, default: -> { 'CURRENT_TIMESTAMP' } + + execute <<~SQL + UPDATE user_stats us + SET first_unread_pm_at = u.created_at + FROM users u + WHERE u.id = us.user_id + SQL + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/post_migrate/20200903045539_add_index_topics_on_timestamps_private.rb b/db/post_migrate/20200903045539_add_index_topics_on_timestamps_private.rb new file mode 100644 index 00000000000..9479dce40a8 --- /dev/null +++ b/db/post_migrate/20200903045539_add_index_topics_on_timestamps_private.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class AddIndexTopicsOnTimestampsPrivate < ActiveRecord::Migration[6.0] + disable_ddl_transaction! + + def up + execute <<~SQL + CREATE INDEX CONCURRENTLY IF NOT EXISTS + index_topics_on_timestamps_private + ON topics (bumped_at, created_at, updated_at) + WHERE deleted_at IS NULL AND archetype = 'private_message' + SQL + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/topic_query.rb b/lib/topic_query.rb index 9d2ee6ac82c..7e0a7b8786b 100644 --- a/lib/topic_query.rb +++ b/lib/topic_query.rb @@ -960,9 +960,19 @@ class TopicQuery def unread_messages(params) query = TopicQuery.unread_filter( messages_for_groups_or_user(params[:my_group_ids]), - @user&.id, - staff: @user&.staff?) - .limit(params[:count]) + @user.id, + staff: @user.staff? + ) + + first_unread_pm_at = + if params[:my_group_ids].present? + GroupUser.where(user_id: @user.id, group_id: params[:my_group_ids]).minimum(:first_unread_pm_at) + else + UserStat.where(user_id: @user.id).pluck(:first_unread_pm_at).first + end + + query = query.where("topics.updated_at >= ?", first_unread_pm_at) if first_unread_pm_at + query = query.limit(params[:count]) if params[:count] query end diff --git a/spec/models/group_user_spec.rb b/spec/models/group_user_spec.rb index 6b3029bab54..fca2c2c7e6d 100644 --- a/spec/models/group_user_spec.rb +++ b/spec/models/group_user_spec.rb @@ -160,4 +160,64 @@ describe GroupUser do expect(TagUser.lookup(user, :watching_first_post).pluck(:tag_id)).to eq([tag4.id]) end end + + describe '#ensure_consistency!' do + fab!(:group) { Fabricate(:group) } + + fab!(:pm_post) { Fabricate(:private_message_post) } + + fab!(:pm_topic) do + pm_post.topic.tap { |t| t.allowed_groups << group } + end + + fab!(:user) do + Fabricate(:user, last_seen_at: Time.zone.now).tap do |u| + group.add(u) + + TopicUser.change(u.id, pm_topic.id, + notification_level: TopicUser.notification_levels[:tracking], + last_read_post_number: pm_post.post_number + ) + end + end + + # User that is not tracking topic + fab!(:user_2) do + Fabricate(:user, last_seen_at: Time.zone.now).tap do |u| + group.add(u) + + TopicUser.change(u.id, pm_topic.id, + notification_level: TopicUser.notification_levels[:regular], + last_read_post_number: pm_post.post_number + ) + end + end + + # User that has not been seen + fab!(:user_3) do + Fabricate(:user).tap do |u| + group.add(u) + + TopicUser.change(u.id, pm_topic.id, + notification_level: TopicUser.notification_levels[:tracking], + last_read_post_number: pm_post.post_number + ) + end + end + + it 'updates first unread pm timestamp correctly' do + freeze_time 10.minutes.from_now + + post = create_post( + user: pm_topic.user, + topic_id: pm_topic.id + ) + + GroupUser.ensure_consistency! + + expect(group.group_users.find_by(user_id: user.id).first_unread_pm_at).to eq_time(post.topic.updated_at) + expect(group.group_users.find_by(user_id: user_2.id).first_unread_pm_at).to_not eq_time(post.topic.updated_at) + expect(group.group_users.find_by(user_id: user_3.id).first_unread_pm_at).to_not eq_time(post.topic.updated_at) + end + end end diff --git a/spec/models/post_timing_spec.rb b/spec/models/post_timing_spec.rb index cb18d50bbd1..43e3f051b65 100644 --- a/spec/models/post_timing_spec.rb +++ b/spec/models/post_timing_spec.rb @@ -184,4 +184,41 @@ describe PostTiming do expect(post.reload.reads).to eq initial_read_count end end + + describe '.destroy_last_for' do + it 'updates first unread for a user correctly when topic is public' do + post = Fabricate(:post) + post.topic.update!(updated_at: 10.minutes.ago) + PostTiming.process_timings(post.user, post.topic_id, 1, [[post.post_number, 100]]) + + PostTiming.destroy_last_for(post.user, post.topic_id) + + expect(post.user.user_stat.reload.first_unread_at).to eq_time(post.topic.updated_at) + end + + it 'updates first unread for a user correctly when topic is a pm' do + post = Fabricate(:private_message_post) + post.topic.update!(updated_at: 10.minutes.ago) + PostTiming.process_timings(post.user, post.topic_id, 1, [[post.post_number, 100]]) + + PostTiming.destroy_last_for(post.user, post.topic_id) + + expect(post.user.user_stat.reload.first_unread_pm_at).to eq_time(post.topic.updated_at) + end + + it 'updates first unread for a user correctly when topic is a group pm' do + topic = Fabricate(:private_message_topic, updated_at: 10.minutes.ago) + post = Fabricate(:post, topic: topic) + user = Fabricate(:user) + group = Fabricate(:group) + group.add(user) + topic.allowed_groups << group + PostTiming.process_timings(user, topic.id, 1, [[post.post_number, 100]]) + + PostTiming.destroy_last_for(user, topic.id) + + expect(GroupUser.find_by(user: user, group: group).first_unread_pm_at) + .to eq_time(post.topic.updated_at) + end + end end diff --git a/spec/models/user_stat_spec.rb b/spec/models/user_stat_spec.rb index 946e0fe3a14..ef0ddc3f62e 100644 --- a/spec/models/user_stat_spec.rb +++ b/spec/models/user_stat_spec.rb @@ -88,6 +88,46 @@ describe UserStat do post.user.user_stat.reload expect(post.user.user_stat.first_unread_at).to eq_time(Time.zone.now) end + + it 'updates first unread pm timestamp correctly' do + pm_topic = Fabricate(:private_message_topic) + user = pm_topic.user + user.update!(last_seen_at: Time.zone.now) + create_post(user: user, topic_id: pm_topic.id) + + TopicUser.change(user.id, pm_topic.id, + notification_level: TopicUser.notification_levels[:tracking] + ) + + # user that is not tracking PM topic + user_2 = Fabricate(:user, last_seen_at: Time.zone.now) + pm_topic.allowed_users << user_2 + + TopicUser.change(user_2.id, pm_topic.id, + notification_level: TopicUser.notification_levels[:regular] + ) + + # User that has not been seen + user_3 = Fabricate(:user) + pm_topic.allowed_users << user_3 + + TopicUser.change(user_3.id, pm_topic.id, + notification_level: TopicUser.notification_levels[:tracking] + ) + + freeze_time 10.minutes.from_now + + post = create_post( + user: Fabricate(:admin), + topic_id: pm_topic.id + ) + + UserStat.ensure_consistency! + + expect(user.user_stat.reload.first_unread_pm_at).to eq_time(post.topic.updated_at) + expect(user_2.user_stat.reload.first_unread_pm_at).to_not eq_time(post.topic.updated_at) + expect(user_3.user_stat.reload.first_unread_pm_at).to_not eq_time(post.topic.updated_at) + end end describe 'update_time_read!' do