From a7d726c3836a87006cedcdc4bd186f8aff89d093 Mon Sep 17 00:00:00 2001
From: Eugen Rochko <eugen@zeonfederated.com>
Date: Wed, 30 May 2018 02:50:23 +0200
Subject: [PATCH] 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 | 25 ++++++++-
app/models/status.rb | 51 ++++++++++++++++++-
app/services/batched_remove_status_service.rb | 5 +-
app/services/suspend_account_service.rb | 7 +--
spec/models/account_spec.rb | 31 +++++++++++
spec/models/status_spec.rb | 14 +++++
6 files changed, 126 insertions(+), 7 deletions(-)
diff --git a/app/models/favourite.rb b/app/models/favourite.rb
index c998a67eb5..0fce82f6f9 100644
--- a/app/models/favourite.rb
+++ b/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
diff --git a/app/models/status.rb b/app/models/status.rb
index 54f3f68f54..08ec36f381 100644
--- a/app/models/status.rb
+++ b/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
diff --git a/app/services/batched_remove_status_service.rb b/app/services/batched_remove_status_service.rb
index dab1c47940..ebb4034aaf 100644
--- a/app/services/batched_remove_status_service.rb
+++ b/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|
diff --git a/app/services/suspend_account_service.rb b/app/services/suspend_account_service.rb
index 56fa2d8dd1..708d15e37d 100644
--- a/app/services/suspend_account_service.rb
+++ b/app/services/suspend_account_service.rb
@@ -41,9 +41,10 @@ class SuspendAccountService < BaseService
end
def purge_profile!
- @account.suspended = true
- @account.display_name = ''
- @account.note = ''
+ @account.suspended = true
+ @account.display_name = ''
+ @account.note = ''
+ @account.statuses_count = 0
@account.avatar.destroy
@account.header.destroy
@account.save!
diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb
index 3aaaa55eb1..cce659a8a1 100644
--- a/spec/models/account_spec.rb
+++ b/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
diff --git a/spec/models/status_spec.rb b/spec/models/status_spec.rb
index aee4f49b47..5113b652fc 100644
--- a/spec/models/status_spec.rb
+++ b/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
--
GitLab