storage: improve EndTransaction's AbortSpan declaration#28799
storage: improve EndTransaction's AbortSpan declaration#28799craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
Fixes cockroachdb#24285. We no longer declare a write on the abort span key if the EndTransaction is not trying to abort the transaction with poison=true. Release note: None
|
This change is |
|
TFTR! bors r+ |
|
bors r- |
Canceled |
|
This actually brings up an interesting question. Why isn't cockroach/pkg/storage/batcheval/cmd_end_transaction.go Lines 492 to 497 in f435f61 written as: if txn.Status == roachpb.ABORTED {
if err := SetAbortSpan(ctx, evalCtx, batch, ms, txn.TxnMeta, args.Poison); err != nil {
return nil, err
}
}The effect of this is that we currently never remove abort span entries if they exist when the |
|
That question doesn't need to hold up this PR though, so... bors r+ |
28737: opt: add colstats flag to opttester r=RaduBerinde a=RaduBerinde Adding a flag that triggers the calculation of a column statistic for the root expression. This is useful for testing statistics that otherwise aren't calculated; in particular, we have code for multi-column stats but they are not used yet. Release note: None 28799: storage: improve EndTransaction's AbortSpan declaration r=nvanbenschoten a=nvanbenschoten Fixes #24285. We no longer declare a write on the abort span key if the EndTransaction is not trying to abort the transaction with poison=true. Release note: None Co-authored-by: Radu Berinde <radu@cockroachlabs.com> Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Build succeeded |
|
I think this comes (came?) down to a lack of trust in that TxnCoordSender was doing the right thing with respect to its "async aborts". Could a txn be aborted (unbeknownst to it) and dispatch a read (which needs to hit the abort span for correctness) that would be "passed" by an async abort but would still return its value to the client? If so, clearing the abort span from the async abort would let the read return something incorrect (i.e. not read what you wrote). I agree that we should revisit this and that it has a likely connection to #25233! |
Fixes #24285.
We no longer declare a write on the abort span key if the
EndTransaction is not trying to abort the transaction with
poison=true.
Release note: None