storage/batcheval: clean up span on non-poisoning, aborting EndTransa…

…ction request

Fixes #29128.

Before this change, an EndTransaction request sent to rollback a transaction record
would not remove any AbortSpan entries, even if its own Poison flag was set to false.
This allowed AbortSpan entries to leak. This commit fixes this behavior by removing
the AbortSpan entry in this case.

There were concerns raised in #29128 about this being safe. It turns out that we already
do this on every Range except the transaction record's Range, so this isn't introducing
any new concerns.

Release note (bug fix): AbortSpan records are now cleaned up more
aggresively when doing so is known to be safe.

pkg/storage/batcheval/cmd_end_transaction.go

@@ -519,13 +519,12 @@ func resolveLocalIntents(
  return nil, errors.Wrapf(err, "resolving intent at %s on end transaction [%s]", span, txn.Status)
  }
  }
- // 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 {
+
+ if WriteAbortSpanOnResolve(txn.Status) {
+ if err := SetAbortSpan(ctx, evalCtx, batch, ms, txn.TxnMeta, args.Poison); err != nil {
  return nil, err
  }
  }
-
  return externalIntents, nil
 }
 

pkg/storage/batcheval/cmd_end_transaction_test.go

@@ -17,6 +17,7 @@ import (
 
  "github.com/cockroachdb/cockroach/pkg/keys"
  "github.com/cockroachdb/cockroach/pkg/roachpb"
+ "github.com/cockroachdb/cockroach/pkg/storage/abortspan"
  "github.com/cockroachdb/cockroach/pkg/storage/engine"
  "github.com/cockroachdb/cockroach/pkg/testutils"
  "github.com/cockroachdb/cockroach/pkg/util/hlc"
@@ -37,6 +38,7 @@ func TestEndTxnUpdatesTransactionRecord(t *testing.T) {
  StartKey: roachpb.RKey(startKey),
  EndKey: roachpb.RKey(endKey),
  }
+ as := abortspan.New(desc.RangeID)
 
  k, k2 := roachpb.Key("a"), roachpb.Key("b")
  ts, ts2, ts3 := hlc.Timestamp{WallTime: 1}, hlc.Timestamp{WallTime: 2}, hlc.Timestamp{WallTime: 3}
@@ -1012,7 +1014,8 @@ func TestEndTxnUpdatesTransactionRecord(t *testing.T) {
  var resp roachpb.EndTxnResponse
  _, err := EndTxn(ctx, batch, CommandArgs{
  EvalCtx: &mockEvalCtx{
- desc: &desc,
+ desc: &desc,
+ abortSpan: as,
  canCreateTxnFn: func() (bool, hlc.Timestamp, roachpb.TransactionAbortedReason) {
  require.NotNil(t, c.canCreateTxn, "CanCreateTxnRecord unexpectedly called")
  if can, minTS := c.canCreateTxn(); can {

pkg/storage/batcheval/transaction_test.go

@@ -158,9 +158,8 @@ func TestSetAbortSpan(t *testing.T) {
  run: func(b engine.ReadWriter, rec EvalContext) error {
  return endTxn(b, rec, false /* commit */, false /* poison */)
  },
- // Not poisoning, should clean up abort span entry. However, it currently
- // doesn't! See https://github.com/cockroachdb/cockroach/issues/29128.
- exp: &prevTxnAbortSpanEntry,
+ // Not poisoning, should clean up abort span entry.
+ exp: nil,
  },
  {
  name: "end txn, rollback, poison, abort span missing",