storage: write to AbortSpan less, clean up AbortSpan more

pkg/cli/debug.go

@@ -1127,7 +1127,7 @@ func removeDeadReplicas(
 				return nil, err
 			}
 			intent.Status = roachpb.ABORTED
-			if err := engine.MVCCResolveWriteIntent(ctx, batch, &ms, intent); err != nil {
+			if _, err := engine.MVCCResolveWriteIntent(ctx, batch, &ms, intent); err != nil {
 				return nil, err
 			}
 			// With the intent resolved, we can try again.

pkg/roachpb/api.proto

@@ -624,11 +624,11 @@ message EndTxnRequest {
   // accummulated no refresh spans. This allows the executing server
   // to retry it locally on the fast path.
   bool no_refresh_spans = 8;
-  // True to indicate that intent spans should be resolved with
-  // poison=true.  This is used when the transaction is being aborted
-  // independently of the main thread of client operation, as in the
-  // case of an asynchronous abort from the TxnCoordSender on a failed
-  // heartbeat.
+  // True to indicate that intent spans should be resolved with poison=true.
+  // This is used when the transaction is being aborted independently of the
+  // main thread of client operation, as in the case of an asynchronous abort
+  // from the TxnCoordSender on a failed heartbeat. It should only be set to
+  // true when commit=false.
   bool poison = 9;
   reserved 7;
 }

pkg/storage/batcheval/cmd_end_transaction.go

@@ -165,6 +165,8 @@ func declareKeysEndTxn(
 
 // EndTxn either commits or aborts (rolls back) an extant transaction according
 // to the args.Commit parameter. Rolling back an already rolled-back txn is ok.
+// TODO(nvanbenschoten): rename this file to cmd_end_txn.go once some of andrei's
+// recent PRs have landed.
 func EndTxn(
 	ctx context.Context, batch engine.ReadWriter, cArgs CommandArgs, resp roachpb.Response,
 ) (result.Result, error) {
@@ -176,11 +178,13 @@ func EndTxn(
 	if err := VerifyTransaction(h, args, roachpb.PENDING, roachpb.STAGING, roachpb.ABORTED); err != nil {
 		return result.Result{}, err
 	}
-
-	// If a 1PC txn was required and we're in EndTxn, something went wrong.
 	if args.Require1PC {
+		// If a 1PC txn was required and we're in EndTxn, something went wrong.
 		return result.Result{}, roachpb.NewTransactionStatusError("could not commit in one phase as requested")
 	}
+	if args.Commit && args.Poison {
+		return result.Result{}, errors.Errorf("cannot poison during a committing EndTxn request")
+	}
 
 	key := keys.TransactionKey(h.Txn.Key, h.Txn.ID)
 
@@ -488,8 +492,11 @@ func resolveLocalIntents(
 					return nil
 				}
 				resolveMS := ms
-				resolveAllowance--
-				return engine.MVCCResolveWriteIntentUsingIter(ctx, batch, iterAndBuf, resolveMS, intent)
+				ok, err := engine.MVCCResolveWriteIntentUsingIter(ctx, batch, iterAndBuf, resolveMS, intent)
+				if ok {
+					resolveAllowance--
+				}
+				return err
 			}
 			// For intent ranges, cut into parts inside and outside our key
 			// range. Resolve locally inside, delegate the rest. In particular,
@@ -519,13 +526,13 @@ 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 {
+
+	removedAny := resolveAllowance != intentResolutionBatchSize
+	if WriteAbortSpanOnResolve(txn.Status, args.Poison, removedAny) {
+		if err := UpdateAbortSpan(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/cmd_refresh_range_test.go

@@ -83,7 +83,7 @@ func TestRefreshRangeTimeBoundIterator(t *testing.T) {
 	// (committed). The sstable also has a second write at a different (older)
 	// timestamp, because if it were empty other than the deletion tombstone, it
 	// would not have any timestamp bounds and would be selected for every read.
-	if err := engine.MVCCResolveWriteIntent(ctx, db, nil, roachpb.Intent{
+	if _, err := engine.MVCCResolveWriteIntent(ctx, db, nil, roachpb.Intent{
 		Span:   roachpb.Span{Key: k},
 		Txn:    txn.TxnMeta,
 		Status: roachpb.COMMITTED,

pkg/storage/batcheval/cmd_resolve_intent.go

@@ -39,7 +39,9 @@ func declareKeysResolveIntentCombined(
 		status = t.Status
 		txnID = t.IntentTxn.ID
 	}
-	if WriteAbortSpanOnResolve(status) {
+	if status == roachpb.ABORTED {
+		// We don't always write to the abort span when resolving an ABORTED
+		// intent, but we can't tell whether we will or not ahead of time.
 		spans.AddNonMVCC(spanset.SpanReadWrite, roachpb.Span{Key: keys.AbortSpanKey(header.RangeID, txnID)})
 	}
 }
@@ -52,7 +54,7 @@ func declareKeysResolveIntent(
 
 func resolveToMetricType(status roachpb.TransactionStatus, poison bool) *result.Metrics {
 	var typ result.Metrics
-	if WriteAbortSpanOnResolve(status) {
+	if status == roachpb.ABORTED {
 		if poison {
 			typ.ResolvePoison = 1
 		} else {
@@ -82,15 +84,16 @@ func ResolveIntent(
 		Txn:    args.IntentTxn,
 		Status: args.Status,
 	}
-	if err := engine.MVCCResolveWriteIntent(ctx, batch, ms, intent); err != nil {
+	ok, err := engine.MVCCResolveWriteIntent(ctx, batch, ms, intent)
+	if err != nil {
 		return result.Result{}, err
 	}
 
 	var res result.Result
 	res.Local.Metrics = resolveToMetricType(args.Status, args.Poison)
 
-	if WriteAbortSpanOnResolve(args.Status) {
-		if err := SetAbortSpan(ctx, cArgs.EvalCtx, batch, ms, args.IntentTxn, args.Poison); err != nil {
+	if WriteAbortSpanOnResolve(args.Status, args.Poison, ok) {
+		if err := UpdateAbortSpan(ctx, cArgs.EvalCtx, batch, ms, args.IntentTxn, args.Poison); err != nil {
 			return result.Result{}, err
 		}
 	}

pkg/storage/batcheval/cmd_resolve_intent_range.go

@@ -67,8 +67,8 @@ func ResolveIntentRange(
 	var res result.Result
 	res.Local.Metrics = resolveToMetricType(args.Status, args.Poison)
 
-	if WriteAbortSpanOnResolve(args.Status) {
-		if err := SetAbortSpan(ctx, cArgs.EvalCtx, batch, ms, args.IntentTxn, args.Poison); err != nil {
+	if WriteAbortSpanOnResolve(args.Status, args.Poison, numKeys > 0) {
+		if err := UpdateAbortSpan(ctx, cArgs.EvalCtx, batch, ms, args.IntentTxn, args.Poison); err != nil {
 			return result.Result{}, err
 		}
 	}

pkg/storage/batcheval/cmd_resolve_intent_test.go

@@ -200,7 +200,7 @@ func TestDeclareKeysResolveIntent(t *testing.T) {
 				rir.Key = ri.Key
 				rir.EndKey = roachpb.Key("c")
 
-				ac := abortspan.New(desc.RangeID)
+				as := abortspan.New(desc.RangeID)
 
 				var spans spanset.SpanSet
 				batch := engine.NewBatch()
@@ -211,7 +211,7 @@ func TestDeclareKeysResolveIntent(t *testing.T) {
 				h.RangeID = desc.RangeID
 
 				cArgs := CommandArgs{Header: h}
-				cArgs.EvalCtx = &mockEvalCtx{abortSpan: ac}
+				cArgs.EvalCtx = &mockEvalCtx{abortSpan: as}
 
 				if !ranged {
 					cArgs.Args = &ri

pkg/storage/batcheval/transaction.go

@@ -54,16 +54,31 @@ func VerifyTransaction(
 }
 
 // WriteAbortSpanOnResolve returns true if the abort span must be written when
-// the transaction with the given status is resolved.
-func WriteAbortSpanOnResolve(status roachpb.TransactionStatus) bool {
-	return status == roachpb.ABORTED
+// the transaction with the given status is resolved. It avoids instructing the
+// caller to write to the abort span if the caller didn't actually remove any
+// intents but intends to poison.
+func WriteAbortSpanOnResolve(status roachpb.TransactionStatus, poison, removedIntents bool) bool {
+	if status != roachpb.ABORTED {
+		// Only update the AbortSpan for aborted transactions.
+		return false
+	}
+	if !poison {
+		// We can remove any entries from the AbortSpan.
+		return true
+	}
+	// We only need to add AbortSpan entries for transactions that we have
+	// invalidated by removing intents. This avoids leaking AbortSpan entries if
+	// a request raced with txn record GC and mistakenly interpreted a committed
+	// txn as aborted only to return to the intent it wanted to push and find it
+	// already resolved. We're only required to write an entry if we do
+	// something that could confuse/invalidate a zombie transaction.
+	return removedIntents
 }
 
-// SetAbortSpan clears any AbortSpan entry if poison is false.
-// Otherwise, if poison is true, creates an entry for this transaction
-// in the AbortSpan to prevent future reads or writes from
-// spuriously succeeding on this range.
-func SetAbortSpan(
+// UpdateAbortSpan clears any AbortSpan entry if poison is false. Otherwise, if
+// poison is true, it creates an entry for this transaction in the AbortSpan to
+// prevent future reads or writes from spuriously succeeding on this range.
+func UpdateAbortSpan(
 	ctx context.Context,
 	rec EvalContext,
 	batch engine.ReadWriter,