Revert "batcheval: add MVCC-compliant RevertRange variant"

This reverts commit e1acd75.

Release note: None

pkg/kv/kvserver/batcheval/cmd_revert_range.go

@@ -51,12 +51,8 @@ func isEmptyKeyTimeRange(
 	// that there is *a* key in the SST that is in the time range. Thus we should
 	// proceed to iteration that actually checks timestamps on each key.
 	iter := readWriter.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{
-		// TODO(erikgrinaker): Make sure TBIs respect range keys too.
-		KeyTypes:         storage.IterKeyTypePointsAndRanges, // revert any range keys as well
-		LowerBound:       from,
-		UpperBound:       to,
-		MinTimestampHint: since.Next(), // exclusive
-		MaxTimestampHint: until,
+		LowerBound: from, UpperBound: to,
+		MinTimestampHint: since.Next() /* make exclusive */, MaxTimestampHint: until,
 	})
 	defer iter.Close()
 	iter.SeekGE(storage.MVCCKey{Key: from})
@@ -82,39 +78,29 @@ func RevertRange(
 
 	args := cArgs.Args.(*roachpb.RevertRangeRequest)
 	reply := resp.(*roachpb.RevertRangeResponse)
+	pd := result.Result{
+		Replicated: kvserverpb.ReplicatedEvalResult{
+			MVCCHistoryMutation: &kvserverpb.ReplicatedEvalResult_MVCCHistoryMutation{
+				Spans: []roachpb.Span{{Key: args.Key, EndKey: args.EndKey}},
+			},
+		},
+	}
 
 	if empty, err := isEmptyKeyTimeRange(
 		readWriter, args.Key, args.EndKey, args.TargetTime, cArgs.Header.Timestamp,
 	); err != nil {
 		return result.Result{}, err
 	} else if empty {
-		log.VEventf(ctx, 2, "no keys to revert in specified time range")
+		log.VEventf(ctx, 2, "no keys to clear in specified time range")
 		return result.Result{}, nil
 	}
 
-	log.VEventf(ctx, 2, "reverting keys with timestamp (%v, %v]",
-		args.TargetTime, cArgs.Header.Timestamp)
+	log.VEventf(ctx, 2, "clearing keys with timestamp (%v, %v]", args.TargetTime, cArgs.Header.Timestamp)
 
-	var pd result.Result
-	var resume *roachpb.Span
-	var err error
-	if args.ExperimentalPreserveHistory {
-		const deleteRangeThreshold = 100
-		maxIntents := storage.MaxIntentsPerWriteIntentError.Get(&cArgs.EvalCtx.ClusterSettings().SV)
-		// TODO(erikgrinaker): Write a test for this once MVCC range tombstones are
-		// properly written to batches and replicated.
-		// TODO(erikgrinaker): Test that this records MVCC logical ops correctly.
-		resume, err = storage.ExperimentalMVCCRevertRange(ctx, readWriter, cArgs.Stats,
-			args.Key, args.EndKey, cArgs.Header.Timestamp, args.TargetTime, deleteRangeThreshold,
-			cArgs.Header.MaxSpanRequestKeys, maxRevertRangeBatchBytes, maxIntents)
-	} else {
-		resume, err = storage.MVCCClearTimeRange(ctx, readWriter, cArgs.Stats, args.Key, args.EndKey,
-			args.TargetTime, cArgs.Header.Timestamp, cArgs.Header.MaxSpanRequestKeys,
-			maxRevertRangeBatchBytes, args.EnableTimeBoundIteratorOptimization)
-		pd.Replicated.MVCCHistoryMutation = &kvserverpb.ReplicatedEvalResult_MVCCHistoryMutation{
-			Spans: []roachpb.Span{{Key: args.Key, EndKey: args.EndKey}},
-		}
-	}
+	resume, err := storage.MVCCClearTimeRange(ctx, readWriter, cArgs.Stats, args.Key, args.EndKey,
+		args.TargetTime, cArgs.Header.Timestamp, cArgs.Header.MaxSpanRequestKeys,
+		maxRevertRangeBatchBytes,
+		args.EnableTimeBoundIteratorOptimization)
 	if err != nil {
 		return result.Result{}, err
 	}

pkg/roachpb/api.go

@@ -778,8 +778,8 @@ func (crr *ClearRangeRequest) ShallowCopy() Request {
 }
 
 // ShallowCopy implements the Request interface.
-func (rrr *RevertRangeRequest) ShallowCopy() Request {
-	shallowCopy := *rrr
+func (crr *RevertRangeRequest) ShallowCopy() Request {
+	shallowCopy := *crr
 	return &shallowCopy
 }
 
@@ -1270,14 +1270,9 @@ func (*ClearRangeRequest) flags() flag {
 	return isWrite | isRange | isAlone | bypassesReplicaCircuitBreaker
 }
 
-// Note that RevertRange commands cannot be part of a transaction, as they
-// either clear MVCC versions or write MVCC range tombstones, neither of which
-// is supported within transactions.
-func (rrr *RevertRangeRequest) flags() flag {
-	if rrr.ExperimentalPreserveHistory {
-		return isRead | isWrite | isRange | isAlone | updatesTSCache | appliesTSCache |
-			bypassesReplicaCircuitBreaker
-	}
+// Note that RevertRange commands cannot be part of a transaction as
+// they clear all MVCC versions above their target time.
+func (*RevertRangeRequest) flags() flag {
 	return isWrite | isRange | isAlone | bypassesReplicaCircuitBreaker
 }
 

pkg/roachpb/api.proto

@@ -402,34 +402,18 @@ message ClearRangeResponse {
 }
 
 
-// A RevertRangeRequest specifies a range of keys to revert to some past time.
-// By default, it will clear all revision more recent that TargetTime from the
-// underlying engine. However, this violates several guarantees including MVCC
-// immutability, the closed timestamp, timestamp cache, and others. See the
-// ExperimentalPreserveHistory parameter which will uphold these guarantees.
+// A RevertRangeRequest specifies a range of keys in which to clear all MVCC
+// revisions more recent than some TargetTime from the underlying engine, thus
+// reverting the range (from the perspective of an MVCC scan) to that time.
 message RevertRangeRequest {
   RequestHeader header = 1 [(gogoproto.nullable) = false, (gogoproto.embed) = true];
 
-  // TargetTime specifies a the time to which to "revert" the range to. Any
-  // versions later than TargetTime will be undone. TargetTime must be higher
+  // TargetTime specifies a the time to which to "revert" the range by clearing
+  // any MVCC key with a strictly higher timestamp. TargetTime must be higher
   // than the GC Threshold for the replica - so that it is assured that the keys
   // for that time are still there — or the request will fail.
   util.hlc.Timestamp target_time = 2 [(gogoproto.nullable) = false];
 
-  // ExperimentalPreserveHistory will preserve MVCC history by, rather than
-  // clearing newer versions, deleting them using tombstones or updating them
-  // back to their original value as of the target time. Long runs of key
-  // deletions will use an MVCC range tombstone instead. This respects the
-  // closed timestamp and timestamp cache.
-  //
-  // The caller must check storage.CanUseExperimentalMVCCRangeTombstones()
-  // before enabling this parameter.
-  //
-  // This parameter is EXPERIMENTAL: range tombstones are under active
-  // development, and have severe limitations including being ignored by all
-  // KV and MVCC APIs and only being stored in memory.
-  bool experimental_preserve_history = 5;
-
   bool enable_time_bound_iterator_optimization = 3;
 
   // IgnoreGcThreshold can be set by a caller to ignore the target-time when

pkg/roachpb/api_test.go

@@ -321,7 +321,6 @@ func TestFlagCombinations(t *testing.T) {
 		&DeleteRangeRequest{UseExperimentalRangeTombstone: true},
 		&GetRequest{KeyLocking: lock.Exclusive},
 		&ReverseScanRequest{KeyLocking: lock.Exclusive},
-		&RevertRangeRequest{ExperimentalPreserveHistory: true},
 		&ScanRequest{KeyLocking: lock.Exclusive},
 	}