release-20.1: kvclient/kvcoord: inhibit parallel commit when retrying EndTxn request #46848
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Release note: None Release justification: Comment only.
Release note: None Release justification: Minor new log messages.
This function was used in sql, but I want to start using it elsewhere when dealing with traces. Release note: None Release justification: Minor code move, in support of the next commit.
The scenario that this patch addresses is the following (from cockroachdb#46431): 1. txn1 sends Put(a) + Put(b) + EndTxn 2. DistSender splits the Put(a) from the rest. 3. Put(a) succeeds, but the rest catches some retriable error. 4. TxnCoordSender gets the retriable error. The fact that a sub-batch succeeded is lost. We used to care about that fact, but we've successively gotten rid of that tracking across cockroachdb#35140 and cockroachdb#44661. 5. we refresh everything that came before this batch. The refresh succeeds. 6. we re-send the batch. It gets split again. The part with the EndTxn executes first. The transaction is now STAGING. More than that, the txn is in fact implicitly committed - the intent on a is already there since the previous attempt and, because it's at a lower timestamp than the txn record, it counts as golden for the purposes of verifying the implicit commit condition. 7. some other transaction wonders in, sees that txn1 is in its way, and transitions it to explicitly committed. 8. the Put(a) now tries to evaluate. It gets really confused. I guess that different things can happen; none of them good. One thing that I believe we've observed in cockroachdb#46299 is that, if there's another txn's intent there already, the Put will try to push it, enter the txnWaitQueue, eventually observe that its own txn is committed and return an error. The client thus gets an error (and a non-ambiguous one to boot) although the txn is committed. Even worse perhaps, I think it's possible for a request to return wrong results instead of an error. This patch fixes it by inhibiting the parallel commit when the EndTxn batch is retried. This way, there's never a STAGING record. Release note (bug fix): A rare bug causing errors to be returned for successfully committed transactions was fixed. The most common error message was "TransactionStatusError: already committed". Release justification: serious bug fix Fixes cockroachdb#46341
Before this patch, a trace (recording) was String()ed by sorting log messages by timestamp, across spans. This proved to be a terrible idea. This patch moves to keeping requests from the same span together, but still interspersing children spans in the parent. So messages from siblings are no longer mixed together. Release note: None Release justification: testing only
Release note: None Release justification: low risk, just logging
|
This change is |
nvanbenschoten
approved these changes
Apr 1, 2020
|
bors r+ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport 6/6 commits from #46596.
/cc @cockroachdb/release
The scenario that this patch addresses is the following (from #46431):
succeeded is lost. We used to care about that fact, but we've
successively gotten rid of that tracking across storage: employ transactional idempotency to refresh mixed-success batches #35140 and kv: refresh less and retry more #44661.
succeeds.
executes first. The transaction is now STAGING. More than that, the txn
is in fact implicitly committed - the intent on a is already there since
the previous attempt and, because it's at a lower timestamp than the txn
record, it counts as golden for the purposes of verifying the implicit
commit condition.
transitions it to explicitly committed.
that different things can happen; none of them good. One thing that I
believe we've observed in roachtest: kv/contention/nodes=4 failed #46299 is that, if there's another txn's
intent there already, the Put will try to push it, enter the
txnWaitQueue, eventually observe that its own txn is committed and
return an error. The client thus gets an error (and a non-ambiguous one
to boot) although the txn is committed. Even worse perhaps, I think it's
possible for a request to return wrong results instead of an error.
This patch fixes it by inhibiting the parallel commit when the EndTxn
batch is retried. This way, there's never a STAGING record.
Release note (bug fix): A rare bug causing errors to be returned for
successfully committed transactions was fixed. The most common error
message was "TransactionStatusError: already committed".
Release justification: serious bug fix
Fixes #46341