Extract counters from accounts table to account_stats table (#9295)

25 files changed, 203 insertions, 68 deletions

diff --git a/app/controllers/admin/instances_controller.rb b/app/controllers/admin/instances_controller.rb
index 8ed0ea421..6f8eaf65c 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 daa35769e..ec15debb0 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 6be97b87e..f3e112f2c 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 99c53d59a..4cff04cad 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 0f04b488f..2770c7aef 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 e9aca5f8a..e6888154e 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 ec4477034..23078263e 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 8f4070bc7..657e57831 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 93b83ce48..4315b0283 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 44963f3e6..593ee29f7 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 000000000..d5715268e
--- /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 000000000..fa3ec9a3d
--- /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 7ad56eb78..87fa11425 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 4233532d0..2f0a9b78c 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 680081724..0449d33e1 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 9a9157f1b..457257e26 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 000000000..a798e8166
--- /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 000000000..bb523fbbd
--- /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 000000000..9267e9b2c
--- /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 731a84521..b0f14954f 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 000000000..2b06b4790
--- /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 60d13d32e..9133616a2 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 000000000..a94185109
--- /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 be5545646..403eb8c33 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 5e9351aff..af1a6f288 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