Merge #61649

61649: kvserver: fix removal of separated intents in ClearRange r=sumeerbhola a=erikgrinaker

`ClearRange` removes a key range and its intents. It normally does this
with `Writer.ClearMVCCRangeAndIntents`, but has an optimization for
small ranges where it directly removes the keys using an iterator.
However, this did not take separated intents into account, and could
leave behind stray intents in the lock table, as uncovered in
`TestTxnClearRangeIntents`.

This patch changes the small-range optimization to use
`Writer.ClearIterRange()` over an `MVCCIterator` instead, which will
correctly remove intents regardless of whether separated intents have
been enabled or not.

Resolves #61606.

Release justification: low risk, high benefit changes to existing functionality

Release note (bug fix): Fixed a bug where `ClearRange` could leave
behind stray write intents when separated intents were enabled, which
could cause subsequent storage errors.

Co-authored-by: Erik Grinaker <[email protected]>

pkg/kv/kvserver/batcheval/cmd_clear_range.go

@@ -82,23 +82,16 @@ func ClearRange(
 	cArgs.Stats.Subtract(statsDelta)
 
 	// If the total size of data to be cleared is less than
-	// clearRangeBytesThreshold, clear the individual values manually,
+	// clearRangeBytesThreshold, clear the individual values with an iterator,
 	// instead of using a range tombstone (inefficient for small ranges).
 	if total := statsDelta.Total(); total < ClearRangeBytesThreshold {
 		log.VEventf(ctx, 2, "delta=%d < threshold=%d; using non-range clear", total, ClearRangeBytesThreshold)
-		iter := readWriter.NewEngineIterator(storage.IterOptions{UpperBound: to})
-		valid, err := iter.SeekEngineKeyGE(storage.EngineKey{Key: from})
-		for ; valid; valid, err = iter.NextEngineKey() {
-			var k storage.EngineKey
-			if k, err = iter.UnsafeEngineKey(); err != nil {
-				break
-			}
-			if err = readWriter.ClearEngineKey(k); err != nil {
-				return result.Result{}, err
-			}
-		}
-		iter.Close()
-		if err != nil {
+		iter := readWriter.NewMVCCIterator(storage.MVCCKeyAndIntentsIterKind, storage.IterOptions{
+			LowerBound: from,
+			UpperBound: to,
+		})
+		defer iter.Close()
+		if err = readWriter.ClearIterRange(iter, from, to); err != nil {
 			return result.Result{}, err
 		}
 		return pd, nil

pkg/kv/kvserver/batcheval/cmd_clear_range_test.go

@@ -29,13 +29,13 @@ import (
 
 type wrappedBatch struct {
 	storage.Batch
-	clearCount      int
+	clearIterCount  int
 	clearRangeCount int
 }
 
-func (wb *wrappedBatch) ClearEngineKey(key storage.EngineKey) error {
-	wb.clearCount++
-	return wb.Batch.ClearEngineKey(key)
+func (wb *wrappedBatch) ClearIterRange(iter storage.MVCCIterator, start, end roachpb.Key) error {
+	wb.clearIterCount++
+	return wb.Batch.ClearIterRange(iter, start, end)
 }
 
 func (wb *wrappedBatch) ClearMVCCRangeAndIntents(start, end roachpb.Key) error {
@@ -64,24 +64,24 @@ func TestCmdClearRangeBytesThreshold(t *testing.T) {
 	overFull := ClearRangeBytesThreshold/len(valueStr) + 1
 	tests := []struct {
 		keyCount           int
-		expClearCount      int
+		expClearIterCount  int
 		expClearRangeCount int
 	}{
 		{
 			keyCount:           1,
-			expClearCount:      1,
+			expClearIterCount:  1,
 			expClearRangeCount: 0,
 		},
 		// More than a single key, but not enough to use ClearRange.
 		{
 			keyCount:           halfFull,
-			expClearCount:      halfFull,
+			expClearIterCount:  1,
 			expClearRangeCount: 0,
 		},
 		// With key sizes requiring additional space, this will overshoot.
 		{
 			keyCount:           overFull,
-			expClearCount:      0,
+			expClearIterCount:  0,
 			expClearRangeCount: 1,
 		},
 	}
@@ -131,8 +131,8 @@ func TestCmdClearRangeBytesThreshold(t *testing.T) {
 			}
 
 			// Verify we see the correct counts for Clear and ClearRange.
-			if a, e := batch.clearCount, test.expClearCount; a != e {
-				t.Errorf("expected %d clears; got %d", e, a)
+			if a, e := batch.clearIterCount, test.expClearIterCount; a != e {
+				t.Errorf("expected %d iter range clears; got %d", e, a)
 			}
 			if a, e := batch.clearRangeCount, test.expClearRangeCount; a != e {
 				t.Errorf("expected %d clear ranges; got %d", e, a)

pkg/kv/kvserver/txn_recovery_integration_test.go

@@ -274,24 +274,12 @@ func TestTxnClearRangeIntents(t *testing.T) {
 	// implicitly committed above. 😱
 	get := getArgs(keyA)
 	reply, pErr = kv.SendWrappedWith(ctx, store.TestSender(), roachpb.Header{}, &get)
+	require.Nil(t, pErr, "error: %s", pErr)
+	require.Nil(t, reply.(*roachpb.GetResponse).Value, "unexpected value for key %q", keyA)
 
-	// When separated intents are enabled, ClearRange will only remove the B key
-	// but not its intent (stored in a separate lock table). This sometimes (but
-	// not always) causes an iterator error during txn recovery, and is a
-	// separate bug in ClearRange:
-	// https://github.com/cockroachdb/cockroach/issues/61606
-	if pErr != nil {
-		require.True(t, store.Engine().IsSeparatedIntentsEnabledForTesting(),
-			"unexpected error with separated intents disabled: %s", pErr.String())
-		require.Contains(t, pErr.String(), "intentIter at intent, but iter not at provisional value")
-
-	} else {
-		require.Nil(t, reply.(*roachpb.GetResponse).Value, "unexpected value for key %q", keyA)
-
-		// Query the original transaction, which should now be aborted.
-		queryTxn := queryTxnArgs(txn.TxnMeta, false)
-		reply, pErr = kv.SendWrappedWith(ctx, store.TestSender(), roachpb.Header{}, &queryTxn)
-		require.Nil(t, pErr, "error: %s", pErr)
-		require.Equal(t, roachpb.ABORTED, reply.(*roachpb.QueryTxnResponse).QueriedTxn.Status)
-	}
+	// Query the original transaction, which should now be aborted.
+	queryTxn := queryTxnArgs(txn.TxnMeta, false)
+	reply, pErr = kv.SendWrappedWith(ctx, store.TestSender(), roachpb.Header{}, &queryTxn)
+	require.Nil(t, pErr, "error: %s", pErr)
+	require.Equal(t, roachpb.ABORTED, reply.(*roachpb.QueryTxnResponse).QueriedTxn.Status)
 }