From 62c0e112ea3805708b39de0e39423c2545df3406 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Mon, 19 Nov 2018 00:43:52 +0100 Subject: [PATCH] Extract counters from accounts table to account_stats table (#9295) --- app/controllers/admin/instances_controller.rb | 2 +- .../accounts/follower_accounts_controller.rb | 2 +- .../accounts/following_accounts_controller.rb | 2 +- app/controllers/api/v1/blocks_controller.rb | 2 +- .../api/v1/endorsements_controller.rb | 2 +- .../api/v1/follow_requests_controller.rb | 2 +- .../api/v1/lists/accounts_controller.rb | 4 +- .../favourited_by_accounts_controller.rb | 2 +- .../reblogged_by_accounts_controller.rb | 2 +- app/models/account.rb | 17 +++--- app/models/account_stat.rb | 26 +++++++++ app/models/concerns/account_counters.rb | 31 +++++++++++ app/models/follow.rb | 19 +++++-- app/models/notification.rb | 2 +- app/models/status.rb | 28 ++++------ app/presenters/instance_presenter.rb | 2 +- .../20181116165755_create_account_stats.rb | 12 +++++ .../20181116173541_copy_account_stats.rb | 54 +++++++++++++++++++ ...181116184611_copy_account_stats_cleanup.rb | 13 +++++ db/schema.rb | 16 ++++-- spec/fabricators/account_stat_fabricator.rb | 6 +++ spec/models/account_spec.rb | 18 ------- spec/models/account_stat_spec.rb | 4 ++ spec/models/notification_spec.rb | 2 +- spec/models/status_stat_spec.rb | 1 - 25 files changed, 203 insertions(+), 68 deletions(-) create mode 100644 app/models/account_stat.rb create mode 100644 app/models/concerns/account_counters.rb create mode 100644 db/migrate/20181116165755_create_account_stats.rb create mode 100644 db/migrate/20181116173541_copy_account_stats.rb create mode 100644 db/post_migrate/20181116184611_copy_account_stats_cleanup.rb create mode 100644 spec/fabricators/account_stat_fabricator.rb create mode 100644 spec/models/account_stat_spec.rb diff --git a/app/controllers/admin/instances_controller.rb b/app/controllers/admin/instances_controller.rb index 8ed0ea4219..6f8eaf65c4 100644 --- a/app/controllers/admin/instances_controller.rb +++ b/app/controllers/admin/instances_controller.rb @@ -31,7 +31,7 @@ module Admin end def subscribeable_accounts - Account.with_followers.remote.where(domain: params[:by_domain]) + Account.remote.where(protocol: :ostatus).where(domain: params[:by_domain]) end def filter_params diff --git a/app/controllers/api/v1/accounts/follower_accounts_controller.rb b/app/controllers/api/v1/accounts/follower_accounts_controller.rb index daa35769eb..ec15debb0c 100644 --- a/app/controllers/api/v1/accounts/follower_accounts_controller.rb +++ b/app/controllers/api/v1/accounts/follower_accounts_controller.rb @@ -25,7 +25,7 @@ class Api::V1::Accounts::FollowerAccountsController < Api::BaseController end def default_accounts - Account.includes(:active_relationships).references(:active_relationships) + Account.includes(:active_relationships, :account_stat).references(:active_relationships) end def paginated_follows diff --git a/app/controllers/api/v1/accounts/following_accounts_controller.rb b/app/controllers/api/v1/accounts/following_accounts_controller.rb index 6be97b87ec..f3e112f2c3 100644 --- a/app/controllers/api/v1/accounts/following_accounts_controller.rb +++ b/app/controllers/api/v1/accounts/following_accounts_controller.rb @@ -25,7 +25,7 @@ class Api::V1::Accounts::FollowingAccountsController < Api::BaseController end def default_accounts - Account.includes(:passive_relationships).references(:passive_relationships) + Account.includes(:passive_relationships, :account_stat).references(:passive_relationships) end def paginated_follows diff --git a/app/controllers/api/v1/blocks_controller.rb b/app/controllers/api/v1/blocks_controller.rb index 99c53d59af..4cff04cad7 100644 --- a/app/controllers/api/v1/blocks_controller.rb +++ b/app/controllers/api/v1/blocks_controller.rb @@ -19,7 +19,7 @@ class Api::V1::BlocksController < Api::BaseController end def paginated_blocks - @paginated_blocks ||= Block.eager_load(:target_account) + @paginated_blocks ||= Block.eager_load(target_account: :account_stat) .where(account: current_account) .paginate_by_max_id( limit_param(DEFAULT_ACCOUNTS_LIMIT), diff --git a/app/controllers/api/v1/endorsements_controller.rb b/app/controllers/api/v1/endorsements_controller.rb index 0f04b488f5..2770c7aef6 100644 --- a/app/controllers/api/v1/endorsements_controller.rb +++ b/app/controllers/api/v1/endorsements_controller.rb @@ -27,7 +27,7 @@ class Api::V1::EndorsementsController < Api::BaseController end def endorsed_accounts - current_account.endorsed_accounts + current_account.endorsed_accounts.includes(:account_stat) end def insert_pagination_headers diff --git a/app/controllers/api/v1/follow_requests_controller.rb b/app/controllers/api/v1/follow_requests_controller.rb index e9aca5f8a7..e6888154e2 100644 --- a/app/controllers/api/v1/follow_requests_controller.rb +++ b/app/controllers/api/v1/follow_requests_controller.rb @@ -33,7 +33,7 @@ class Api::V1::FollowRequestsController < Api::BaseController end def default_accounts - Account.includes(:follow_requests).references(:follow_requests) + Account.includes(:follow_requests, :account_stat).references(:follow_requests) end def paginated_follow_requests diff --git a/app/controllers/api/v1/lists/accounts_controller.rb b/app/controllers/api/v1/lists/accounts_controller.rb index ec44770344..23078263e7 100644 --- a/app/controllers/api/v1/lists/accounts_controller.rb +++ b/app/controllers/api/v1/lists/accounts_controller.rb @@ -37,9 +37,9 @@ class Api::V1::Lists::AccountsController < Api::BaseController def load_accounts if unlimited? - @list.accounts.all + @list.accounts.includes(:account_stat).all else - @list.accounts.paginate_by_max_id(limit_param(DEFAULT_ACCOUNTS_LIMIT), params[:max_id], params[:since_id]) + @list.accounts.includes(:account_stat).paginate_by_max_id(limit_param(DEFAULT_ACCOUNTS_LIMIT), params[:max_id], params[:since_id]) end end diff --git a/app/controllers/api/v1/statuses/favourited_by_accounts_controller.rb b/app/controllers/api/v1/statuses/favourited_by_accounts_controller.rb index 8f4070bc78..657e578319 100644 --- a/app/controllers/api/v1/statuses/favourited_by_accounts_controller.rb +++ b/app/controllers/api/v1/statuses/favourited_by_accounts_controller.rb @@ -22,7 +22,7 @@ class Api::V1::Statuses::FavouritedByAccountsController < Api::BaseController def default_accounts Account - .includes(:favourites) + .includes(:favourites, :account_stat) .references(:favourites) .where(favourites: { status_id: @status.id }) end diff --git a/app/controllers/api/v1/statuses/reblogged_by_accounts_controller.rb b/app/controllers/api/v1/statuses/reblogged_by_accounts_controller.rb index 93b83ce485..4315b02832 100644 --- a/app/controllers/api/v1/statuses/reblogged_by_accounts_controller.rb +++ b/app/controllers/api/v1/statuses/reblogged_by_accounts_controller.rb @@ -21,7 +21,7 @@ class Api::V1::Statuses::RebloggedByAccountsController < Api::BaseController end def default_accounts - Account.includes(:statuses).references(:statuses) + Account.includes(:statuses, :account_stat).references(:statuses) end def paginated_statuses diff --git a/app/models/account.rb b/app/models/account.rb index 44963f3e63..593ee29f76 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -32,9 +32,6 @@ # suspended :boolean default(FALSE), not null # locked :boolean default(FALSE), not null # header_remote_url :string default(""), not null -# statuses_count :integer default(0), not null -# followers_count :integer default(0), not null -# following_count :integer default(0), not null # last_webfingered_at :datetime # inbox_url :string default(""), not null # outbox_url :string default(""), not null @@ -58,6 +55,7 @@ class Account < ApplicationRecord include AccountInteractions include Attachmentable include Paginable + include AccountCounters enum protocol: [:ostatus, :activitypub] @@ -119,8 +117,6 @@ class Account < ApplicationRecord scope :remote, -> { where.not(domain: nil) } scope :local, -> { where(domain: nil) } - scope :without_followers, -> { where(followers_count: 0) } - scope :with_followers, -> { where('followers_count > 0') } scope :expiring, ->(time) { remote.where.not(subscription_expires_at: nil).where('subscription_expires_at < ?', time) } scope :partitioned, -> { order(Arel.sql('row_number() over (partition by domain)')) } scope :silenced, -> { where(silenced: true) } @@ -385,7 +381,9 @@ class Account < ApplicationRecord LIMIT ? SQL - find_by_sql([sql, limit]) + records = find_by_sql([sql, limit]) + ActiveRecord::Associations::Preloader.new.preload(records, :account_stat) + records end def advanced_search_for(terms, account, limit = 10, following = false) @@ -412,7 +410,7 @@ class Account < ApplicationRecord LIMIT ? SQL - find_by_sql([sql, account.id, account.id, account.id, limit]) + records = find_by_sql([sql, account.id, account.id, account.id, limit]) else sql = <<-SQL.squish SELECT @@ -428,8 +426,11 @@ class Account < ApplicationRecord LIMIT ? SQL - find_by_sql([sql, account.id, account.id, limit]) + records = find_by_sql([sql, account.id, account.id, limit]) end + + ActiveRecord::Associations::Preloader.new.preload(records, :account_stat) + records end private diff --git a/app/models/account_stat.rb b/app/models/account_stat.rb new file mode 100644 index 0000000000..d5715268eb --- /dev/null +++ b/app/models/account_stat.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +# == Schema Information +# +# Table name: account_stats +# +# id :bigint(8) not null, primary key +# account_id :bigint(8) not null +# statuses_count :bigint(8) default(0), not null +# following_count :bigint(8) default(0), not null +# followers_count :bigint(8) default(0), not null +# created_at :datetime not null +# updated_at :datetime not null +# + +class AccountStat < ApplicationRecord + belongs_to :account, inverse_of: :account_stat + + def increment_count!(key) + update(key => public_send(key) + 1) + end + + def decrement_count!(key) + update(key => [public_send(key) - 1, 0].max) + end +end diff --git a/app/models/concerns/account_counters.rb b/app/models/concerns/account_counters.rb new file mode 100644 index 0000000000..fa3ec9a3da --- /dev/null +++ b/app/models/concerns/account_counters.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +module AccountCounters + extend ActiveSupport::Concern + + included do + has_one :account_stat, inverse_of: :account + after_save :save_account_stat + end + + delegate :statuses_count, + :statuses_count=, + :following_count, + :following_count=, + :followers_count, + :followers_count=, + :increment_count!, + :decrement_count!, + to: :account_stat + + def account_stat + super || build_account_stat + end + + private + + def save_account_stat + return unless account_stat&.changed? + account_stat.save + end +end diff --git a/app/models/follow.rb b/app/models/follow.rb index 7ad56eb78d..87fa114253 100644 --- a/app/models/follow.rb +++ b/app/models/follow.rb @@ -16,11 +16,8 @@ class Follow < ApplicationRecord include Paginable include RelationshipCacheable - belongs_to :account, counter_cache: :following_count - - belongs_to :target_account, - class_name: 'Account', - counter_cache: :followers_count + belongs_to :account + belongs_to :target_account, class_name: 'Account' has_one :notification, as: :activity, dependent: :destroy @@ -39,7 +36,9 @@ class Follow < ApplicationRecord end before_validation :set_uri, only: :create + after_create :increment_cache_counters after_destroy :remove_endorsements + after_destroy :decrement_cache_counters private @@ -50,4 +49,14 @@ class Follow < ApplicationRecord def remove_endorsements AccountPin.where(target_account_id: target_account_id, account_id: account_id).delete_all end + + def increment_cache_counters + account&.increment_count!(:following_count) + target_account&.increment_count!(:followers_count) + end + + def decrement_cache_counters + account&.decrement_count!(:following_count) + target_account&.decrement_count!(:followers_count) + end end diff --git a/app/models/notification.rb b/app/models/notification.rb index 4233532d08..2f0a9b78c7 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -75,7 +75,7 @@ class Notification < ApplicationRecord return if account_ids.empty? - accounts = Account.where(id: account_ids).each_with_object({}) { |a, h| h[a.id] = a } + accounts = Account.where(id: account_ids).includes(:account_stat).each_with_object({}) { |a, h| h[a.id] = a } cached_items.each do |item| item.from_account = accounts[item.from_account_id] diff --git a/app/models/status.rb b/app/models/status.rb index 680081724e..0449d33e12 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -94,17 +94,16 @@ class Status < ApplicationRecord end } - cache_associated :account, - :application, + cache_associated :application, :media_attachments, :conversation, :status_stat, :tags, :preview_cards, :stream_entry, - active_mentions: :account, + account: :account_stat, + active_mentions: { account: :account_stat }, reblog: [ - :account, :application, :stream_entry, :tags, @@ -112,9 +111,10 @@ class Status < ApplicationRecord :media_attachments, :conversation, :status_stat, - active_mentions: :account, + account: :account_stat, + active_mentions: { account: :account_stat }, ], - thread: :account + thread: { account: :account_stat } delegate :domain, to: :account, prefix: true @@ -348,7 +348,7 @@ class Status < ApplicationRecord return if account_ids.empty? - accounts = Account.where(id: account_ids).each_with_object({}) { |a, h| h[a.id] = a } + accounts = Account.where(id: account_ids).includes(:account_stat).each_with_object({}) { |a, h| h[a.id] = a } cached_items.each do |item| item.account = accounts[item.account_id] @@ -475,12 +475,7 @@ class Status < ApplicationRecord 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 - + account&.increment_count!(:statuses_count) reblog&.increment_count!(:reblogs_count) if reblog? thread&.increment_count!(:replies_count) if in_reply_to_id.present? && (public_visibility? || unlisted_visibility?) end @@ -488,12 +483,7 @@ class Status < ApplicationRecord 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 - + account&.decrement_count!(:statuses_count) reblog&.decrement_count!(:reblogs_count) if reblog? thread&.decrement_count!(:replies_count) if in_reply_to_id.present? && (public_visibility? || unlisted_visibility?) end diff --git a/app/presenters/instance_presenter.rb b/app/presenters/instance_presenter.rb index 9a9157f1ba..457257e264 100644 --- a/app/presenters/instance_presenter.rb +++ b/app/presenters/instance_presenter.rb @@ -22,7 +22,7 @@ class InstancePresenter end def status_count - Rails.cache.fetch('local_status_count') { Account.local.sum(:statuses_count) } + Rails.cache.fetch('local_status_count') { Account.local.joins(:account_stat).sum('account_stats.statuses_count') } end def domain_count diff --git a/db/migrate/20181116165755_create_account_stats.rb b/db/migrate/20181116165755_create_account_stats.rb new file mode 100644 index 0000000000..a798e8166e --- /dev/null +++ b/db/migrate/20181116165755_create_account_stats.rb @@ -0,0 +1,12 @@ +class CreateAccountStats < ActiveRecord::Migration[5.2] + def change + create_table :account_stats do |t| + t.belongs_to :account, null: false, foreign_key: { on_delete: :cascade }, index: { unique: true } + t.bigint :statuses_count, null: false, default: 0 + t.bigint :following_count, null: false, default: 0 + t.bigint :followers_count, null: false, default: 0 + + t.timestamps + end + end +end diff --git a/db/migrate/20181116173541_copy_account_stats.rb b/db/migrate/20181116173541_copy_account_stats.rb new file mode 100644 index 0000000000..bb523fbbd3 --- /dev/null +++ b/db/migrate/20181116173541_copy_account_stats.rb @@ -0,0 +1,54 @@ +class CopyAccountStats < ActiveRecord::Migration[5.2] + disable_ddl_transaction! + + def up + safety_assured do + if supports_upsert? + up_fast + else + up_slow + end + end + end + + def down + # Nothing + end + + private + + def supports_upsert? + version = select_one("SELECT current_setting('server_version_num') AS v")['v'].to_i + version >= 90500 + end + + def up_fast + say 'Upsert is available, importing counters using the fast method' + + Account.unscoped.select('id').find_in_batches(batch_size: 5_000) do |accounts| + execute <<-SQL.squish + INSERT INTO account_stats (account_id, statuses_count, following_count, followers_count, created_at, updated_at) + SELECT id, statuses_count, following_count, followers_count, created_at, updated_at + FROM accounts + WHERE id IN (#{accounts.map(&:id).join(', ')}) + ON CONFLICT (account_id) DO UPDATE + SET statuses_count = EXCLUDED.statuses_count, following_count = EXCLUDED.following_count, followers_count = EXCLUDED.followers_count + SQL + end + end + + def up_slow + say 'Upsert is not available in PostgreSQL below 9.5, falling back to slow import of counters' + + # We cannot use bulk INSERT or overarching transactions here because of possible + # uniqueness violations that we need to skip over + Account.unscoped.select('id, statuses_count, following_count, followers_count, created_at, updated_at').find_each do |account| + begin + params = [[nil, account.id], [nil, account.statuses_count], [nil, account.following_count], [nil, account.followers_count], [nil, account.created_at], [nil, account.updated_at]] + exec_insert('INSERT INTO account_stats (account_id, statuses_count, following_count, followers_count, created_at, updated_at) VALUES ($1, $2, $3, $4, $5, $6)', nil, params) + rescue ActiveRecord::RecordNotUnique + next + end + end + end +end diff --git a/db/post_migrate/20181116184611_copy_account_stats_cleanup.rb b/db/post_migrate/20181116184611_copy_account_stats_cleanup.rb new file mode 100644 index 0000000000..9267e9b2c6 --- /dev/null +++ b/db/post_migrate/20181116184611_copy_account_stats_cleanup.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class CopyAccountStatsCleanup < ActiveRecord::Migration[5.2] + disable_ddl_transaction! + + def change + safety_assured do + remove_column :accounts, :statuses_count, :integer, default: 0, null: false + remove_column :accounts, :following_count, :integer, default: 0, null: false + remove_column :accounts, :followers_count, :integer, default: 0, null: false + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 731a84521e..b0f14954f8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2018_10_26_034033) do +ActiveRecord::Schema.define(version: 2018_11_16_184611) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -56,6 +56,16 @@ ActiveRecord::Schema.define(version: 2018_10_26_034033) do t.index ["target_account_id"], name: "index_account_pins_on_target_account_id" end + create_table "account_stats", force: :cascade do |t| + t.bigint "account_id", null: false + t.bigint "statuses_count", default: 0, null: false + t.bigint "following_count", default: 0, null: false + t.bigint "followers_count", default: 0, null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["account_id"], name: "index_account_stats_on_account_id", unique: true + end + create_table "accounts", force: :cascade do |t| t.string "username", default: "", null: false t.string "domain" @@ -85,9 +95,6 @@ ActiveRecord::Schema.define(version: 2018_10_26_034033) do t.boolean "suspended", default: false, null: false t.boolean "locked", default: false, null: false t.string "header_remote_url", default: "", null: false - t.integer "statuses_count", default: 0, null: false - t.integer "followers_count", default: 0, null: false - t.integer "following_count", default: 0, null: false t.datetime "last_webfingered_at" t.string "inbox_url", default: "", null: false t.string "outbox_url", default: "", null: false @@ -629,6 +636,7 @@ ActiveRecord::Schema.define(version: 2018_10_26_034033) do add_foreign_key "account_moderation_notes", "accounts", column: "target_account_id" add_foreign_key "account_pins", "accounts", column: "target_account_id", on_delete: :cascade add_foreign_key "account_pins", "accounts", on_delete: :cascade + add_foreign_key "account_stats", "accounts", on_delete: :cascade add_foreign_key "accounts", "accounts", column: "moved_to_account_id", on_delete: :nullify add_foreign_key "admin_action_logs", "accounts", on_delete: :cascade add_foreign_key "backups", "users", on_delete: :nullify diff --git a/spec/fabricators/account_stat_fabricator.rb b/spec/fabricators/account_stat_fabricator.rb new file mode 100644 index 0000000000..2b06b47909 --- /dev/null +++ b/spec/fabricators/account_stat_fabricator.rb @@ -0,0 +1,6 @@ +Fabricator(:account_stat) do + account nil + statuses_count "" + following_count "" + followers_count "" +end diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb index 60d13d32e9..9133616a2a 100644 --- a/spec/models/account_spec.rb +++ b/spec/models/account_spec.rb @@ -754,24 +754,6 @@ RSpec.describe Account, type: :model do expect(Account.suspended).to match_array([account_1]) end end - - describe 'without_followers' do - it 'returns a relation of accounts without followers' do - account_1 = Fabricate(:account) - account_2 = Fabricate(:account) - Fabricate(:follow, account: account_1, target_account: account_2) - expect(Account.without_followers).to match_array([account_1]) - end - end - - describe 'with_followers' do - it 'returns a relation of accounts with followers' do - account_1 = Fabricate(:account) - account_2 = Fabricate(:account) - Fabricate(:follow, account: account_1, target_account: account_2) - expect(Account.with_followers).to match_array([account_2]) - end - end end context 'when is local' do diff --git a/spec/models/account_stat_spec.rb b/spec/models/account_stat_spec.rb new file mode 100644 index 0000000000..a94185109c --- /dev/null +++ b/spec/models/account_stat_spec.rb @@ -0,0 +1,4 @@ +require 'rails_helper' + +RSpec.describe AccountStat, type: :model do +end diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index be5545646f..403eb8c336 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -101,7 +101,7 @@ RSpec.describe Notification, type: :model do before do allow(accounts_with_ids).to receive(:[]).with(stale_account1.id).and_return(account1) allow(accounts_with_ids).to receive(:[]).with(stale_account2.id).and_return(account2) - allow(Account).to receive_message_chain(:where, :each_with_object).and_return(accounts_with_ids) + allow(Account).to receive_message_chain(:where, :includes, :each_with_object).and_return(accounts_with_ids) end let(:cached_items) do diff --git a/spec/models/status_stat_spec.rb b/spec/models/status_stat_spec.rb index 5e9351aff4..af1a6f288b 100644 --- a/spec/models/status_stat_spec.rb +++ b/spec/models/status_stat_spec.rb @@ -1,5 +1,4 @@ require 'rails_helper' RSpec.describe StatusStat, type: :model do - pending "add some examples to (or delete) #{__FILE__}" end