From 7c321d3aadb0d2325e070ead3218975fa2495fd9 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Wed, 30 Nov 2022 22:52:08 +0800 Subject: [PATCH] PERF: Update `Group#user_count` counter cache outside DB transaction (#19256) While load testing our user creation code path in production, we identified that executing the DB statement to update the `Group#user_count` column within a transaction is creating a bottleneck for us. This is because the creation of a user and addition of the user to the relevant groups are done in a transaction. When we execute the DB statement to update `Group#user_count` for the relevant group, a row level lock is held until the transaction completes. This row level lock acts like a global lock when the server is creating users that will be added to the same group in quick succession. Instead of updating the counter cache within a transaction which the default ActiveRecord `counter_cache` option does, we simply update the counter cache outside of the committing transaction. Co-authored-by: Rafael dos Santos Silva Co-authored-by: Rafael dos Santos Silva --- app/controllers/admin/groups_controller.rb | 2 -- .../regular/automatic_group_membership.rb | 2 -- app/models/group.rb | 25 ++++++++++++++++--- app/models/group_user.rb | 13 +++++++++- script/import_scripts/vbulletin.rb | 4 +-- spec/jobs/automatic_group_membership_spec.rb | 1 + spec/models/group_user_spec.rb | 16 ++++++++++++ 7 files changed, 52 insertions(+), 11 deletions(-) diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index 788f5e60063..935931de224 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/app/controllers/admin/groups_controller.rb @@ -101,8 +101,6 @@ class Admin::GroupsController < Admin::StaffController GroupActionLogger.new(current_user, group).log_remove_user_as_group_owner(user) end - Group.reset_counters(group.id, :group_users) - render json: success_json end diff --git a/app/jobs/regular/automatic_group_membership.rb b/app/jobs/regular/automatic_group_membership.rb index bf9f6b213d3..6831078842d 100644 --- a/app/jobs/regular/automatic_group_membership.rb +++ b/app/jobs/regular/automatic_group_membership.rb @@ -19,8 +19,6 @@ module Jobs group.add(user, automatic: true) GroupActionLogger.new(Discourse.system_user, group).log_add_user_to_group(user) end - - Group.reset_counters(group.id, :group_users) end end diff --git a/app/models/group.rb b/app/models/group.rb index aa6fdc0220b..b4dbd85eabb 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -534,7 +534,7 @@ class Group < ActiveRecord::Base end # we want to ensure consistency - Group.reset_counters(group.id, :group_users) + Group.reset_user_count(group) group end @@ -545,12 +545,28 @@ class Group < ActiveRecord::Base refresh_has_messages! end + def self.reset_user_count(group) + reset_groups_user_count!(only_group_ids: [group.id]) + end + def self.reset_all_counters! + reset_groups_user_count! + end + + def self.reset_groups_user_count!(only_group_ids: []) + where_sql = '' + + if only_group_ids.present? + where_sql = "WHERE group_id IN (#{only_group_ids.map(&:to_i).join(",")})" + end + DB.exec <<-SQL WITH X AS ( - SELECT group_id - , COUNT(user_id) users - FROM group_users + SELECT + group_id, + COUNT(user_id) users + FROM group_users + #{where_sql} GROUP BY group_id ) UPDATE groups @@ -560,6 +576,7 @@ class Group < ActiveRecord::Base AND user_count <> X.users SQL end + private_class_method :reset_groups_user_count! def self.refresh_automatic_groups!(*args) args = AUTO_GROUPS.keys if args.empty? diff --git a/app/models/group_user.rb b/app/models/group_user.rb index 5e6b3f7dcba..1c5df59922e 100644 --- a/app/models/group_user.rb +++ b/app/models/group_user.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class GroupUser < ActiveRecord::Base - belongs_to :group, counter_cache: "user_count" + belongs_to :group belongs_to :user after_save :update_title @@ -15,6 +15,9 @@ class GroupUser < ActiveRecord::Base after_save :set_category_notifications after_save :set_tag_notifications + after_commit :increase_group_user_count, on: [:create] + after_commit :decrease_group_user_count, on: [:destroy] + def self.notification_levels NotificationLevels.all end @@ -186,6 +189,14 @@ class GroupUser < ActiveRecord::Base end end end + + def increase_group_user_count + Group.increment_counter(:user_count, self.group_id) + end + + def decrease_group_user_count + Group.decrement_counter(:user_count, self.group_id) + end end # == Schema Information diff --git a/script/import_scripts/vbulletin.rb b/script/import_scripts/vbulletin.rb index 90182e19b9e..8b3de80bdc3 100644 --- a/script/import_scripts/vbulletin.rb +++ b/script/import_scripts/vbulletin.rb @@ -195,13 +195,13 @@ class ImportScripts::VBulletin < ImportScripts::Base DB.exec <<~SQL INSERT INTO group_users (group_id, user_id, created_at, updated_at) VALUES #{values} SQL - - Group.reset_counters(group.id, :group_users) rescue Exception => e puts e.message puts e.backtrace.join("\n") end end + + Group.reset_all_counters! end def import_profile_picture(old_user, imported_user) diff --git a/spec/jobs/automatic_group_membership_spec.rb b/spec/jobs/automatic_group_membership_spec.rb index 5ad6140895e..76efc6cd8ab 100644 --- a/spec/jobs/automatic_group_membership_spec.rb +++ b/spec/jobs/automatic_group_membership_spec.rb @@ -45,6 +45,7 @@ RSpec.describe Jobs::AutomaticGroupMembership do expect(group.users.include?(user4)).to eq(true) expect(group.users.include?(user5)).to eq(false) expect(group.users.include?(user6)).to eq(true) + expect(group.user_count).to eq(2) end end diff --git a/spec/models/group_user_spec.rb b/spec/models/group_user_spec.rb index 5fe790dc5fb..483e214e953 100644 --- a/spec/models/group_user_spec.rb +++ b/spec/models/group_user_spec.rb @@ -1,6 +1,22 @@ # frozen_string_literal: true RSpec.describe GroupUser do + fab!(:group) { Fabricate(:group) } + fab!(:user) { Fabricate(:user) } + + describe 'callbacks' do + it "increments and decrements `Group#user_count` when record is created and destroyed" do + group_user = GroupUser.new(user: user, group: group) + + expect do + group_user.save! + end.to change { group.reload.user_count }.from(0).to(1) + + expect do + group_user.destroy! + end.to change { group.reload.user_count }.from(1).to(0) + end + end it 'correctly sets notification level' do moderator = Fabricate(:moderator)