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 <xfalcox@gmail.com>

Co-authored-by: Rafael dos Santos Silva <xfalcox@gmail.com>

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
 

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

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?

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

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)

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

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)