Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move account counters to separate table #9295

Merged
merged 1 commit into from
Nov 18, 2018
Merged

Conversation

Gargron
Copy link
Member

@Gargron Gargron commented Nov 16, 2018

statuses_count is the most updated column probably among all tables, because it goes up every time you post a new status. By extracting it to a table with only a few columns, updates can run faster and the dead tuples left behind until next vacuum are smaller.

Fix #7908

Instructions:

  1. Run SKIP_POST_DEPLOYMENT_MIGRATIONS=true rails db:migrate first
  2. Restart Mastodon
  3. Run rails db:migrate

@Gargron Gargron added the performance Runtime performance label Nov 16, 2018
end

def increment_count!(key)
update_account_stat!(key => public_send(key) + 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this should maybe be attributes[key]? or something similar, i can't remember the precise syntax offhand.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's if the counter was on the same table as before. We want to call the explicitly defined followers_count method here.

@Gargron Gargron force-pushed the feature-account-stat branch 3 times, most recently from 7971ab1 to 12e6781 Compare November 18, 2018 16:10
@@ -31,7 +31,7 @@ def ordered_instances
end

def subscribeable_accounts
Account.with_followers.remote.where(domain: params[:by_domain])
Account.remote.where(protocol: :ostatus).where(domain: params[:by_domain])
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This query has been wrong for a long time. I think it hasn't been updated since ActivityPub was introduced. It would send PuSH subscriptions to ActivityPub accounts. It also would not correctly filter accounts that had local followers. So this is a net improvement.

end

def statuses_count
account_stat&.statuses_count || 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be delegate :statuses_count, :statuses_count=, :following_count, :following_count=, :followers_count, :followers_count=, to: :account_stat

and then handle the case of nil account_stat in the set_account_stat hook

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

%w(statuses following followers).flat_map { |i| [:"#{i}_count", :"#{i}_count=" } is even a little cuter, your call.

private

def set_account_stat
self.account_stat = build_account_stat if account_stat.nil?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per our conversation, this should handle migrating the counts from the existing record if there's no account_stat record.

super || build_account_stat
end

def increment_count!(key)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think these should live on AccountStat and be delegated also


private

def update_account_stat!(attrs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and then we don't need to care about this method

@Gargron Gargron merged commit d6b9a62 into master Nov 18, 2018
@Gargron Gargron deleted the feature-account-stat branch November 18, 2018 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Runtime performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants