batcheval: handle MVCC range tombstones in RefreshRange

This patch add handling of MVCC range tombstones in `RefreshRange`, by
erroring on them as a committed value.

Release note: None

pkg/kv/kvserver/batcheval/cmd_refresh_range.go

@@ -73,6 +73,8 @@ func refreshRange(
 	// iterators will emit MVCC tombstones by default and will emit intents when
 	// configured to do so (see IntentPolicy).
 	iter := storage.NewMVCCIncrementalIterator(reader, storage.MVCCIncrementalIterOptions{
+		KeyTypes:     storage.IterKeyTypePointsAndRanges,
+		StartKey:     span.Key,
 		EndKey:       span.EndKey,
 		StartTime:    refreshFrom, // exclusive
 		EndTime:      refreshTo,   // inclusive
@@ -81,15 +83,24 @@ func refreshRange(
 	defer iter.Close()
 
 	var meta enginepb.MVCCMetadata
-	iter.SeekGE(storage.MakeMVCCMetadataKey(span.Key))
-	for {
+	for iter.SeekGE(storage.MVCCKey{Key: span.Key}); ; iter.Next() {
 		if ok, err := iter.Valid(); err != nil {
 			return err
 		} else if !ok {
 			break
 		}
 
 		key := iter.UnsafeKey().Clone()
+
+		if _, hasRange := iter.HasPointAndRange(); hasRange {
+			rangeKVs := iter.RangeKeys()
+			if len(rangeKVs) == 0 { // defensive
+				return errors.Errorf("expected range key at %s not found", key)
+			}
+			return roachpb.NewRefreshFailedError(roachpb.RefreshFailedError_REASON_COMMITTED_VALUE,
+				key.Key, rangeKVs[0].RangeKey.Timestamp)
+		}
+
 		if !key.IsValue() {
 			// Found an intent. Check whether it is owned by this transaction.
 			// If so, proceed with iteration. Otherwise, return an error.
@@ -99,7 +110,6 @@ func refreshRange(
 			if meta.IsInline() {
 				// Ignore inline MVCC metadata. We don't expect to see this in practice
 				// when performing a refresh of an MVCC keyspace.
-				iter.Next()
 				continue
 			}
 			if meta.Txn.ID == txnID {
@@ -116,7 +126,6 @@ func refreshRange(
 					return errors.Errorf("expected provisional value for intent with ts %s, found %s",
 						meta.Timestamp, iter.UnsafeKey().Timestamp)
 				}
-				iter.Next()
 				continue
 			}
 			return roachpb.NewRefreshFailedError(roachpb.RefreshFailedError_REASON_INTENT,

pkg/kv/kvserver/batcheval/cmd_refresh_range_test.go

@@ -290,3 +290,56 @@ func TestRefreshRangeTimestampBounds(t *testing.T) {
 		}
 	}
 }
+
+// TestRefreshRangeRangeTombstone verifies that we get an error for an MVCC
+// range tombstone.
+func TestRefreshRangeRangeTombstone(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	ctx := context.Background()
+	ts1 := hlc.Timestamp{WallTime: 1}
+	ts2 := hlc.Timestamp{WallTime: 2}
+	ts3 := hlc.Timestamp{WallTime: 3}
+
+	eng := storage.NewDefaultInMemForTesting()
+	defer eng.Close()
+
+	start, end := roachpb.Key("a"), roachpb.Key("z")
+
+	// Write an MVCC range tombstone at ts2.
+	require.NoError(t, storage.ExperimentalMVCCDeleteRangeUsingTombstone(
+		ctx, eng, nil, start, end, ts2, hlc.ClockTimestamp{}, 0))
+
+	// We are trying to refresh from time 1 to 3, but the tombstone was written at
+	// time 2, therefore the refresh should fail.
+	var resp roachpb.RefreshRangeResponse
+	_, err := RefreshRange(ctx, eng, CommandArgs{
+		EvalCtx: (&MockEvalCtx{
+			ClusterSettings: cluster.MakeTestingClusterSettings(),
+		}).EvalContext(),
+		Args: &roachpb.RefreshRangeRequest{
+			RequestHeader: roachpb.RequestHeader{
+				Key:    start,
+				EndKey: end,
+			},
+			RefreshFrom: ts1,
+		},
+		Header: roachpb.Header{
+			Txn: &roachpb.Transaction{
+				TxnMeta: enginepb.TxnMeta{
+					WriteTimestamp: ts3,
+				},
+				ReadTimestamp: ts3,
+			},
+			Timestamp: ts3,
+		},
+	}, &resp)
+	var refreshErr *roachpb.RefreshFailedError
+	require.ErrorAs(t, err, &refreshErr)
+	require.Equal(t, roachpb.RefreshFailedError{
+		Reason:    roachpb.RefreshFailedError_REASON_COMMITTED_VALUE,
+		Key:       start,
+		Timestamp: ts2,
+	}, *refreshErr)
+}