Forked from #28799 (comment).

Why isn't

cockroach/pkg/storage/batcheval/cmd_end_transaction.go

Lines 492 to 497 in f435f61

	// If the poison arg is set, make sure to set the abort span entry.
	if args.Poison && txn.Status == roachpb.ABORTED {
	if err := SetAbortSpan(ctx, evalCtx, batch, ms, txn.TxnMeta, true /* poison */); err != nil {
	return nil, err
	}
	}

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 TxnCoordSender sends an EndTransactionRequest(Commit=false). My understanding of Poison and its interaction with the AbortSpan is that it is safe to remove the entry when Poison=false because that implies an acknowlegement from the TxnCoordSender of the transaction's aborted disposition. I wonder if this has any implications on #25233.

Response from @tschottdorf:

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!

We should explore this further.

@tschottdorf for triage.