Improve counter caches on Status and Account (#7644)

Do not touch statuses_count on accounts table when mass-destroying
statuses to reduce load when removing accounts, same for
reblogs_count and favourites_count

Do not count statuses with direct visibility in statuses_count

Fix #828

app/models/favourite.rb

@@ -16,7 +16,7 @@ class Favourite < ApplicationRecord
   update_index('statuses#status', :status) if Chewy.enabled?
 
   belongs_to :account, inverse_of: :favourites
-  belongs_to :status,  inverse_of: :favourites, counter_cache: true
+  belongs_to :status,  inverse_of: :favourites
 
   has_one :notification, as: :activity, dependent: :destroy
 
@@ -25,4 +25,27 @@ class Favourite < ApplicationRecord
   before_validation do
     self.status = status.reblog if status&.reblog?
   end
+
+  after_create :increment_cache_counters
+  after_destroy :decrement_cache_counters
+
+  private
+
+  def increment_cache_counters
+    if association(:status).loaded?
+      status.update_attribute(:favourites_count, status.favourites_count + 1)
+    else
+      Status.where(id: status_id).update_all('favourites_count = COALESCE(favourites_count, 0) + 1')
+    end
+  end
+
+  def decrement_cache_counters
+    return if association(:status).loaded? && (status.marked_for_destruction? || status.marked_for_mass_destruction?)
+
+    if association(:status).loaded?
+      status.update_attribute(:favourites_count, [status.favourites_count - 1, 0].max)
+    else
+      Status.where(id: status_id).update_all('favourites_count = GREATEST(COALESCE(favourites_count, 0) - 1, 0)')
+    end
+  end
 end

app/models/status.rb

@@ -41,12 +41,12 @@ class Status < ApplicationRecord
 
   belongs_to :application, class_name: 'Doorkeeper::Application', optional: true
 
-  belongs_to :account, inverse_of: :statuses, counter_cache: true
+  belongs_to :account, inverse_of: :statuses
   belongs_to :in_reply_to_account, foreign_key: 'in_reply_to_account_id', class_name: 'Account', optional: true
   belongs_to :conversation, optional: true
 
   belongs_to :thread, foreign_key: 'in_reply_to_id', class_name: 'Status', inverse_of: :replies, optional: true
-  belongs_to :reblog, foreign_key: 'reblog_of_id', class_name: 'Status', inverse_of: :reblogs, counter_cache: :reblogs_count, optional: true
+  belongs_to :reblog, foreign_key: 'reblog_of_id', class_name: 'Status', inverse_of: :reblogs, optional: true
 
   has_many :favourites, inverse_of: :status, dependent: :destroy
   has_many :reblogs, foreign_key: 'reblog_of_id', class_name: 'Status', inverse_of: :reblog, dependent: :destroy
@@ -167,6 +167,17 @@ class Status < ApplicationRecord
     @emojis ||= CustomEmoji.from_text([spoiler_text, text].join(' '), account.domain)
   end
 
+  def mark_for_mass_destruction!
+    @marked_for_mass_destruction = true
+  end
+
+  def marked_for_mass_destruction?
+    @marked_for_mass_destruction
+  end
+
+  after_create  :increment_counter_caches
+  after_destroy :decrement_counter_caches
+
   after_create_commit :store_uri, if: :local?
   after_create_commit :update_statistics, if: :local?
 
@@ -388,4 +399,40 @@ class Status < ApplicationRecord
     return unless public_visibility? || unlisted_visibility?
     ActivityTracker.increment('activity:statuses:local')
   end
+
+  def increment_counter_caches
+    return if direct_visibility?
+
+    if association(:account).loaded?
+      account.update_attribute(:statuses_count, account.statuses_count + 1)
+    else
+      Account.where(id: account_id).update_all('statuses_count = COALESCE(statuses_count, 0) + 1')
+    end
+
+    return unless reblog?
+
+    if association(:reblog).loaded?
+      reblog.update_attribute(:reblogs_count, reblog.reblogs_count + 1)
+    else
+      Status.where(id: reblog_of_id).update_all('reblogs_count = COALESCE(reblogs_count, 0) + 1')
+    end
+  end
+
+  def decrement_counter_caches
+    return if direct_visibility? || marked_for_mass_destruction?
+
+    if association(:account).loaded?
+      account.update_attribute(:statuses_count, [account.statuses_count - 1, 0].max)
+    else
+      Account.where(id: account_id).update_all('statuses_count = GREATEST(COALESCE(statuses_count, 0) - 1, 0)')
+    end
+
+    return unless reblog?
+
+    if association(:reblog).loaded?
+      reblog.update_attribute(:reblogs_count, [reblog.reblogs_count - 1, 0].max)
+    else
+      Status.where(id: reblog_of_id).update_all('reblogs_count = GREATEST(COALESCE(reblogs_count, 0) - 1, 0)')
+    end
+  end
 end

app/services/batched_remove_status_service.rb

@@ -21,7 +21,10 @@ class BatchedRemoveStatusService < BaseService
     @activity_xml          = {}
 
     # Ensure that rendered XML reflects destroyed state
-    statuses.each(&:destroy)
+    statuses.each do |status|
+      status.mark_for_mass_destruction!
+      status.destroy
+    end
 
     # Batch by source account
     statuses.group_by(&:account_id).each_value do |account_statuses|

app/services/suspend_account_service.rb

@@ -44,6 +44,7 @@ class SuspendAccountService < BaseService
     @account.suspended      = true
     @account.display_name   = ''
     @account.note           = ''
+    @account.statuses_count = 0
     @account.avatar.destroy
     @account.header.destroy
     @account.save!

spec/models/account_spec.rb

@@ -525,6 +525,37 @@ RSpec.describe Account, type: :model do
     end
   end
 
+  describe '#statuses_count' do
+    subject { Fabricate(:account) }
+
+    it 'counts statuses' do
+      Fabricate(:status, account: subject)
+      Fabricate(:status, account: subject)
+      expect(subject.statuses_count).to eq 2
+    end
+
+    it 'does not count direct statuses' do
+      Fabricate(:status, account: subject, visibility: :direct)
+      expect(subject.statuses_count).to eq 0
+    end
+
+    it 'is decremented when status is removed' do
+      status = Fabricate(:status, account: subject)
+      expect(subject.statuses_count).to eq 1
+      status.destroy
+      expect(subject.statuses_count).to eq 0
+    end
+
+    it 'is decremented when status is removed when account is not preloaded' do
+      status = Fabricate(:status, account: subject)
+      expect(subject.reload.statuses_count).to eq 1
+      clean_status = Status.find(status.id)
+      expect(clean_status.association(:account).loaded?).to be false
+      clean_status.destroy
+      expect(subject.reload.statuses_count).to eq 0
+    end
+  end
+
   describe '.following_map' do
     it 'returns an hash' do
       expect(Account.following_map([], 1)).to be_a Hash

spec/models/status_spec.rb

@@ -175,6 +175,13 @@ RSpec.describe Status, type: :model do
 
       expect(subject.reblogs_count).to eq 2
     end
+
+    it 'is decremented when reblog is removed' do
+      reblog = Fabricate(:status, account: bob, reblog: subject)
+      expect(subject.reblogs_count).to eq 1
+      reblog.destroy
+      expect(subject.reblogs_count).to eq 0
+    end
   end
 
   describe '#favourites_count' do
@@ -184,6 +191,13 @@ RSpec.describe Status, type: :model do
 
       expect(subject.favourites_count).to eq 2
     end
+
+    it 'is decremented when favourite is removed' do
+      favourite = Fabricate(:favourite, account: bob, status: subject)
+      expect(subject.favourites_count).to eq 1
+      favourite.destroy
+      expect(subject.favourites_count).to eq 0
+    end
   end
 
   describe '#proper' do