Transaction lock time reduction investigation

[peppy]

Following up from discussion last week after we ran into some pretty dire deadlock scenarios involving partition rotation metadata locks, I wanted to investigate whether it would be worthwhile to change the way score-statistics-processor, potentially avoiding opening a transaction in the first place, or if doing so, minimising the amount of time spent within the transaction.

The imminent issue was fixed by reducing the timeout on osu-web rank lookup requests, but I don't see this as a permanent fix because:

• It is relying on things "eventually succeeding". Individual scores will fail to complete and enter the retry queue during partition switching.
• Any non-database component failure which is accessed inside the transaction could result in similar database-wise locks, causing full system downtime.

Let's assess the issues at hand:

UserStats write usages

These processor-external usages can be easily fixes, and probably should be as right now they are writing full row data even when they don't need to.

TotalScoreProcessorTests.cs (1 usages)
Commands\Maintenance\VerifyUserRankCounts.cs (5 usages)
Commands\Maintenance\VerifyUserRankedScore.cs (1 usages)

The cases below are a touch more delicate. Because of codependencies and medal awarders, we need to be updating the UserStats tracking object, or be re-querying on subsequent read usages.

Processors\HitStatisticsProcessor.cs (4 usages)

• countMiss ¹
• count50 ¹
• count100 ¹
• count300 ¹
  Processors\LastPlayedDateProcessor.cs (1 usages)
• last_played
Processors\MaxComboProcessor.cs (1 usages)
• max_combo
Processors\PlayCountProcessor.cs (2 usages)
• playcount ^{1 2}
  Processors\PlayTimeProcessor.cs (2 usages)
• total_seconds_played
Processors\RankedScoreProcessor.cs (1 usages)
• ranked_score
Processors\TotalScoreProcessor.cs (6 usages)
• total_score
• level
Processors\UserRankCountProcessor.cs (5 usages)
• xh_rank_count
• x_rank_count
• sh_rank_count
• s_rank_count
• a_rank_count
Processors\UserTotalPerformanceProcessor.cs (3 usages)
• rank_score
• accuracy_new
• rank_score_index

READ COMMITTED is not safe

We switched to this transaction mode previously to avoid actual database deadlocks. I knew at the time that this was not robust, but it did fix the issue and in majority of cases would not cause an issue.

While I was investigating this new issue and writing up documentation, I wanted to write this:

// Opening a transaction here guarantees a mutex over the user which is being processed.
//
// This is important as queue processors are made to be run in parallel at high scale,
// which means two queue processors could end up operating on the same user in an edge case.
//
// Transaction mode READ COMMITTED means that subsequent queries may see different underlying
// data, but...
using (var transaction = conn.BeginTransaction(IsolationLevel.ReadCommitted))

And that's where I stopped because I realised that the part I wrote before the transaction explanation was actually not true. We would need to do SELECT .. FOR UPDATE or change the isolation mode to guarantee this.

Proposed paths forward

Here are our options:

• Do nothing. Things are working until they aren't.
  • Will potentially still cause sentry error reports each day during partition cycling
  • Any longer failures that take more than a few seconds to timeout could definitely cause system-wide failures.
  • Some updates may not be correct due to the transaction mode. For instance, if two scores by the same user are processed in parallel, a single playcount increment may be missed.
• Move all write operations to local UPDATE calls
  • This fixes transaction issues for the majority case as the moment an update occurs on user_stats, a row lock will be taken.
  • Requires consideration for subsequent reads (ie medal awarder). We'd need to update the local mode in addition to the database, probably using a SELECT to ensure we have a correct state post-lock.
  • More RTT to database due to increased query count.
  • Does not fix whole system falling over from holding long transactions.
• Don't start transaction during processing. Store delegates of changes and apply in one go at end.
  • As we're no longer taking out a transaction, there's no guarantee of data consistency during processing. But this is probably fine as long as we are reading from the UserStats object.
  • Fixes cases such as playcount incrementing, as long as UserStats is re-retrieved before applying delegate changes.
  • With the caveat that there could still be two playcount medals awarded for the same user due to lack of 100% transaction coverage. Imagine two scores being processed in parallel from the same base playcount number.
  • We will need to do a pass of UPDATE calls which are inline in apply/revert methods.
  • Proof of concept here. Have not tested yet.

Footnotes

1. has dependent medal ↩ ↩² ↩³ ↩⁴ ↩⁵

2. has codependent processor ↩

[bdach]

Since you asked for thoughts:

Do nothing. Things are working until they aren't.

I wouldn't do this for obvious reasons.

Move all write operations to local UPDATE calls

I probably would not do this either because it's a lot of work for seemingly not much gain. Like a lot of things will need to get re-written to ensure the data dependencies are clean and even then the concurrency control is really diffused and it'd probably be really difficult for me to tell if the end product is concurrency safe.

Don't start transaction during processing. Store delegates of changes and apply in one go at end.

I probably agree with your presumed conclusion that this is probably the least-worst option but also I worry a little. You mentioned the following:

With the caveat that there could still be two playcount medals awarded for the same user due to lack of 100% transaction coverage. Imagine two scores being processed in parallel from the same base playcount number.

If we're worrying about this sort of thing then there's more of that where that comes from. Think about helpers like GetBestScoreForUser() or whatever and everything that depends on them, and what that does to every single consumer of that method. You could get stuff like double-counting user rank totals, double-awarding some hush-hush medals, etc.

---

That said, another thought that I had is that maybe we can do something extremely similar, which - if it goes well - may be even less work than the last approach, possibly. I'm talking about optimistic concurrency control, which would basically be

• Put a timestamp row on user_stats (if one doesn't exist already)
• Determine what changes we need to make for the user whose score is incoming for processing
• Before committing any changes to the db, double-check the timestamp row on user_stats.
  • If the timestamp hasn't changed, commit the changes as they were calculated and fire all side-effects (like LIO calls, score indexing, etc.)
  • If the timestamp has changed, abort the changes and re-queue the score again.

I always kind of thought this concurrency control mode was kind of pointless, but now that I think about it a little harder, maybe we can make it work here if the contention rate on a single user's stats is low enough to keep the number of retries relatively self-contained.

The one worry I'd have with that is that this concurrency control method has some potential of entering a failure feedback loop if things get far behind, which may be alleviated a little if we can introduce some queue partitioning scheme to ensure a single user is always assigned to a single processor instance. Think something like "queue processor instance M of N only processes scores for users whose userId % N == M", which would end up being inherently serialising for a single user. (That technique can also be useful in your proposed scheme to reduce the number of concurrency conflicts on a single user, by the way - however I'm not sure whether that sort of scheme can work with redis or be kubernetesified at all, though.)

That said what we would need to do to try what I'm proposing here, would be to make sure we delay all side effects that all processors are currently (wrongly, by the way) doing within the transaction - like LIO calls or inline SQL updates or whatever - until the write is actually confirmed, because currently we're relying on the fact that transactions basically never roll back, which isn't going to be the case with optimistic concurrency.

[smoogipoo]

a lot of things will need to get re-written to ensure the data dependencies are clean

Can you give an example of a data dependency that meets this? Moving all writes and transactions locally sounds like the cleanest solution to me. Though it sounds chaotic it'll force processors to be implemented with a diffing perspective.

That is, I would see the processors being implemented as just async ProcessScore() and chaotically doing things the way they want locally - whether it is taking a transaction for the entire period or storing a change and applying it in a transaction at the very end.

[bdach]

Can you give an example of a data dependency that meets this?

Supposing we want to move every processor to a local transaction, then you can't e.g. pass UserStats between processors like we currently do - we sometimes rely on being able to read a change made to UserStats by a processor earlier down the processing chain, later down the chain. Moving all writes/txns locally means you'd need to re-query it every time in every processor.

Which is kinda wasteful given how little some processors really do (see something like LastPlayedDateProcessor). And there's the question of whether you re-query it outside of a txn or inside, if inside then with what isolation level, at which point there are concerns about potential lock contention, etc...

I'm really wary of this approach because I fear that the 'chaos' approach in the worst case scenario leads to then things blowing up in a way that we cannot easily detect because 20 txns in a circular lock-waiting pattern are involved or something. Or just not that much better performance in general because the overhead of acquiring locks becomes much larger.

[smoogipoo]

I wouldn't pass a UserStats between processors, just re-query every time if deemed appropriate by the processor. Something like LastPlayedDateProcessor I'd implement it as something like:

START_TRANSACTION;
UPDATE osu_user_stats SET last_played = MAX(last_played, {score.ended_at}) WHERE user_id = {score.user_id};
COMMIT;

Everything else - fair. I don't know the precise cost of acquiring locks, though I hope it'd be amortised against the cost of actually running the processors... Would prefer to not have to think about transaction isolation tbh, so am only considering the default mode.

[bdach]

Would prefer to not have to think about transaction isolation tbh

The thing is that txn isolation level is also key here. READ COMMITTED which we're currently using in production (because of partitioning locking woes IIRC) as mentioned in the OP is not safe. The only reason we're not seeing it break as hard as it probably should is likely because of the current coarseness of queue processor transactions causing low contention / concurrency. If we're to move to fine-grained txns and short updates of user_stats in every processor, we likely would want to go back to REPEATABLE READ, because staying on READ COMMITTED means you can essentially have lost updates.

[smoogipoo]

Yeah, when I say "default" I mean the MySQL default REPEATABLE READ. I'd hope that to be a general goal throughout all of this...