cockroachdb / cockroachdb/cockroach
kv: stateful transaction retries considered harmful
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
In https://github.com/cockroachdb/cockroach/issues/5935, we discussed whether SAVEPOINT-based retries were more performant than naive, stateless retries. In this issue, I'll argue that SAVEPOINT-based retries where locks are held across transaction retries are harmful (limited utility + unsafe) and should be eliminated.
Much of this is forked from a conversation in [#transactions-theory](https://cockroachlabs.slack.com/archives/C876WUAFM/p1703221336060979):
----
Nathan VanBenschoten (@nvanbenschoten)
>I’ve become increasingly disillusioned with stateful transaction retries (“epochs”) and the role they play in cockroach’s transaction model. Now that transaction’s are mostly pessimistic, priority ratcheting isn’t really a thing. So the remaining benefits of stateful transaction retries are:
>1. transactions hold intents/locks across retries to reduce the chance that future attempts will hit serialization errors.
>2. transactions retain their uncertainty interval and observed timestamps across retries, so that uncertainty errors become less likely and eventually impossible (e.g. to avoid starvation of large scans).
>
>I’ll start by saying that the second benefit still feels real to me. We need some way to avoid starvation by uncertainty error, so we need some way to scope a logical application transaction (the scope that we want to provide linearizability) to one or more database transactions. If we assume every database transaction is a separate application transaction, then each one needs to establish a new uncertainty interval. So this is important, but there are other ways that we could solve this problem, so let’s ignore it for now.
>
>I’d like to talk about the first benefit, which feels dubious to me at this point. The purpose of retaining locks across retries was originally to block other readers and writers from bumping the timestamp cache over spans that the transaction planned to write to, to avoid the transaction getting pushed and forced to restart repeatedly. A transaction would write some intents in an epoch, hit a serialization error, restart, re-write those intents with the expectation that they would still be locked, then proceed to write new intents, proceeding from there. I’m questioning whether this is a good idea in today’s cockroach for all of the following reasons:
>1. This has always relied on a transaction writing to the same set of keys across retries. So it’s never been fully general.
>2. Read Refreshing has made this less of a concern for serializable transactions, because a read-write conflict does not necessarily mean that the transaction will retry.
>3. The closed timestamp doesn’t care about intents, so these intents only prevent read-write conflicts that would force a refresh across epochs for txns that take < 3s per epoch.
>4. This is making the same “fairness” vs. “predictability” tradeoff that was discussed in the [non-blocking write-read conflicts doc](https://docs.google.com/document/d/1ji6C0aDI6n61sVKPjf5-YUucbtgBlwfpsrNdcidW5a0/edit?usp=sharing). It’s assuming that a user wants a large transaction which is having trouble serializing to incrementally block as many other transactions as necessary to complete, regardless of how many retries that takes or how long it blocks those other transactions.
>5. This is a common source of outages, like we just saw in [support/#2775](https://github.com/cockroachlabs/support/issues/2775) and like we saw in [support/#2685](https://github.com/cockroachlabs/support/issues/2685) a few weeks ago. Retaining locks across retries leads to subtle lock ordering inversions, which can be a cause of self-deadlocks. It also means that a transaction stuck retrying can wedge others for the full duration that it is starved (hours in some of our worst cases), instead of blocking for short durations during each retry. These stuck transactions then block others, allowing a single transaction in a retry loop to take down a cluster.
>6. We will soon be offering weaker isolation levels, which will give users a tool to run bulk transactions that might have needed multiple retries to complete.
>
>Reason 4 and 5 are the main reasons why I’m questioning this. I think the mechanism is making a poor tradeoff and is unsafe.
>
>What do you think? Are there other benefits to transaction epochs? Has the mechanism outlived its utility?
Benjamin Darnell (@bdarnell)
>I agree with all of this (except maybe point 6 - I'd want to see some examples of this). I think point 4 is particularly important - we now know that predictability of small frequent transactions is far more important in our ideal workloads than fairness to larger transactions.
>
>The benefit of this protocol was unproven even in the optimistic concurrency era (e.g. https://github.com/cockroachdb/cockroach/issues/5935). I hadn't realized we were starting to attribute production problems to this mechanism, but I'm not surprised. And I'm unaware of any situations where we've gotten a notable benefit from it (except in contrived artificial situations, and even there I think explicit pessimistic locks turn out to be better).
>
>Assuming we can build an alternative solution for clock uncertainty starvation, I think I'd be in favor of phasing this out.
Jira issue: CRDB-34909
Contributor guide
Assessment
This issue has not been assessed yet.