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

Improve account counters handling #15913

Merged
merged 3 commits into from
Mar 19, 2021

Conversation

ClearlyClaire
Copy link
Contributor

@ClearlyClaire ClearlyClaire commented Mar 16, 2021

This is kind of a proof of concept for now, using upserts to avoid the convoluted and broken ruby-based handling of race conditions.

Before this change, Mastodon would load and instantiate the AccountStat object associated with an Account whenever changing any of the account stats, resulting in at least 2 SQL queries with a Ruby round-trip in the best-case scenario, and handling conflicts by doing more SQL queries and Ruby round-trips. Furthermore, this conflict resolution mechanism was broken in Rails 6.

This code ensures that updating the counters always take exactly one SQL query, plus a second in the rare case where account_stats was already loaded (and thus needs reloading). It also fixes these methods on Rails 6.

  • review lock_version-based locking
  • update last_status_at during the upsert
  • disallow dirty records
  • rewrite the upsert query in a saner way (Rails 6 has an upsert method but it's not flexible enough, nor stock ARel either apparently)

@ClearlyClaire ClearlyClaire force-pushed the fixes/stats-counters branch 4 times, most recently from 150bcba to 2a7a298 Compare March 16, 2021 13:38
@ClearlyClaire ClearlyClaire changed the title [WiP] Improve account counters handling Improve account counters handling Mar 16, 2021
@ClearlyClaire ClearlyClaire marked this pull request as ready for review March 16, 2021 14:04
@ClearlyClaire
Copy link
Contributor Author

Not too happy with the raw SQL, but ActiveRecord isn't expressive enough. ARel doesn't seem to be either (although it could be extended for our purposes, but that would overcomplicate the whole thing quite a bit).

Copy link
Member

@Gargron Gargron left a comment

Choose a reason for hiding this comment

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

Upsert requires postgres 9.5. Do we have anyone stuck on 9.4 still?

SQLi is not possible with the way the values are chosen but string evaluation is still a risky code practice

@ClearlyClaire
Copy link
Contributor Author

ClearlyClaire commented Mar 19, 2021

Upsert requires postgres 9.5. Do we have anyone stuck on 9.4 still?

Very good point. I'd say that everyone should use 9.5 at least by now, but I'm pretty sure one could find people using older versions of Postgres.

Bumping the Postgres version requirement might be worth it, but I'll look into whether people are using 9.4, or if we've already bumped the requirement somehow (I doubt it, but maybe).

SQLi is not possible with the way the values are chosen but string evaluation is still a risky code practice

Yeah, though there is no way to write that query in plain ActiveRecord or even unpatched ARel (the closest I could find is https://rubygems.org/gems/active_record_upsert which I'm not even sure exactly covers the query here). In this case, I far prefer two easy-to-read SQL queries than convoluted use of ARel or confidential gems.

EDIT: I edited the code a bit to build part of the query with Rails' sanitize_sql method, but that leaves the #{key} interpolation, as only sanitize_sql_hash_for_assignment seem to come close, and it would require stitching bits of SQL in a way that isn't any easier to read and check safety of.

@ClearlyClaire
Copy link
Contributor Author

ClearlyClaire commented Mar 19, 2021

As far as current requirements go, I think the current required minimum PostgreSQL version for Mastodon is 9.4, since Mastodon 3.3.0, with the inclusion of the scenic gem and the use of the concurrently option. It might have required 9.4 at least for a longer while, and it may require 9.5 but I don't think it actually does.

Regarding people still using PostgreSQL 9.4, I checked a few common distributions:

  • the oldest supported Ubuntu release, Ubuntu 16.04 LTS (support ends next month), ships with PostgreSQL 9.5
  • Debian stable has PostgreSQL 11, oldstable has PostgreSQL 9.6
  • the oldest supported Fedora release, Fedora 32, ships with PostgreSQL 12

In addition to that, I made a quick Mastodon poll. So far, it seems that out of 10 voters, 1 is still using PostgreSQL 9.4 or older.

Please also note that both PostgreSQL 9.4 and 9.5 are currently EOL: https://www.postgresql.org/support/versioning/

I think bumping the required version to 9.5 at least is very reasonable, although it'd probably warrant a note in the release notes, and maybe a database migration hook or something to inform users running older PostgreSQL versions of it.

EDIT: Added a hook to abort before running db:migrate if running PostgreSQL < 9.5.

…olating them

Keep using string interpolation for `key` as it is safe and using
“ActiveRecord::Base::sanitize_sql_hash_for_assignment” would require stitching
bits of SQL in a way that is not more easily checked for safety.
@ClearlyClaire ClearlyClaire force-pushed the fixes/stats-counters branch 2 times, most recently from 9f02196 to 6231fcf Compare March 19, 2021 10:06
@Gargron Gargron merged commit 741d095 into mastodon:main Mar 19, 2021
ClearlyClaire added a commit to ClearlyClaire/mastodon that referenced this pull request May 26, 2021
In some rare occasions[1], deleting accounts would fail with a
`StaleObjectError` exception.

Indeed, account deletion manually sets the `AccountStat` values without
handling cases where the optimistic locking on `AccountStat` would fail.

To my knowledge, with the rewrite of account counters in mastodon#15913, the
`DeleteAccountService` is now the only place that changes the counters in
a way that is not atomic.

Since in this specific case, we do not care about the previous values of the
account counters, it appears we don't need locking at all for this table
anymore.

[1]: https://discourse.joinmastodon.org/t/account-cant-be-deleted/3602
Gargron pushed a commit that referenced this pull request Jun 2, 2021
…16317)

* Fix account deletion sometimes failing because of optimistic locks

In some rare occasions[1], deleting accounts would fail with a
`StaleObjectError` exception.

Indeed, account deletion manually sets the `AccountStat` values without
handling cases where the optimistic locking on `AccountStat` would fail.

To my knowledge, with the rewrite of account counters in #15913, the
`DeleteAccountService` is now the only place that changes the counters in
a way that is not atomic.

Since in this specific case, we do not care about the previous values of the
account counters, it appears we don't need locking at all for this table
anymore.

[1]: https://discourse.joinmastodon.org/t/account-cant-be-deleted/3602

* Bump MAX_SUPPORTED_VERSION in maintenance script
GensouSakuya pushed a commit to GensouSakuya/mastodon that referenced this pull request Jun 3, 2021
…astodon#16317)

* Fix account deletion sometimes failing because of optimistic locks

In some rare occasions[1], deleting accounts would fail with a
`StaleObjectError` exception.

Indeed, account deletion manually sets the `AccountStat` values without
handling cases where the optimistic locking on `AccountStat` would fail.

To my knowledge, with the rewrite of account counters in mastodon#15913, the
`DeleteAccountService` is now the only place that changes the counters in
a way that is not atomic.

Since in this specific case, we do not care about the previous values of the
account counters, it appears we don't need locking at all for this table
anymore.

[1]: https://discourse.joinmastodon.org/t/account-cant-be-deleted/3602

* Bump MAX_SUPPORTED_VERSION in maintenance script
chrisguida pushed a commit to Start9Labs/mastodon that referenced this pull request Feb 26, 2022
* Improve account counters handling

* Use ActiveRecord::Base::sanitize_sql to pass values instead of interpolating them

Keep using string interpolation for `key` as it is safe and using
“ActiveRecord::Base::sanitize_sql_hash_for_assignment” would require stitching
bits of SQL in a way that is not more easily checked for safety.

* Add migration hook to catch PostgreSQL versions earlier than 9.5
chrisguida pushed a commit to Start9Labs/mastodon that referenced this pull request Feb 26, 2022
…astodon#16317)

* Fix account deletion sometimes failing because of optimistic locks

In some rare occasions[1], deleting accounts would fail with a
`StaleObjectError` exception.

Indeed, account deletion manually sets the `AccountStat` values without
handling cases where the optimistic locking on `AccountStat` would fail.

To my knowledge, with the rewrite of account counters in mastodon#15913, the
`DeleteAccountService` is now the only place that changes the counters in
a way that is not atomic.

Since in this specific case, we do not care about the previous values of the
account counters, it appears we don't need locking at all for this table
anymore.

[1]: https://discourse.joinmastodon.org/t/account-cant-be-deleted/3602

* Bump MAX_SUPPORTED_VERSION in maintenance script
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants