Merge #76921

76921: storage: revert experimental MVCC range tombstones r=aliher1911 a=erikgrinaker This reverts most of #76131, #76203, and #76478 -- except minor changes that were unrelated to the range tombstones themselves. This leaves a gap for cluster version `Internal:78` -- I think that's probably fine, but I've left a comment. Co-authored-by: Erik Grinaker <[email protected]>
cockroachdb · Feb 23, 2022 · d35ac3a · d35ac3a
2 parents 5522769 + 4b79046
commit d35ac3a
Show file tree

Hide file tree

Showing 31 changed files with 51 additions and 2,084 deletions.
diff --git a/pkg/clusterversion/cockroach_versions.go b/pkg/clusterversion/cockroach_versions.go
@@ -303,9 +303,6 @@ const (
 	// BackupResolutionInJob defaults to resolving backup destinations during the
 	// execution of a backup job rather than during planning.
 	BackupResolutionInJob
-	// ExperimentalMVCCRangeTombstones enables the use of highly experimental MVCC
-	// range tombstones.
-	ExperimentalMVCCRangeTombstones
 	// LooselyCoupledRaftLogTruncation allows the cluster to reduce the coupling
 	// for raft log truncation, by allowing each replica to treat a truncation
 	// proposal as an upper bound on what should be truncated.
@@ -494,14 +491,12 @@ var versionsSingleton = keyedVersions{
 		Key:     EnablePebbleFormatVersionRangeKeys,
 		Version: roachpb.Version{Major: 21, Minor: 2, Internal: 74},
 	},
+
 	{
 		Key:     BackupResolutionInJob,
 		Version: roachpb.Version{Major: 21, Minor: 2, Internal: 76},
 	},
-	{
-		Key:     ExperimentalMVCCRangeTombstones,
-		Version: roachpb.Version{Major: 21, Minor: 2, Internal: 78},
-	},
+	// Internal: 78 was reverted (ExperimentalMVCCRangeTombstones)
 	{
 		Key:     LooselyCoupledRaftLogTruncation,
 		Version: roachpb.Version{Major: 21, Minor: 2, Internal: 80},

diff --git a/pkg/clusterversion/key_string.go b/pkg/clusterversion/key_string.go
diff --git a/pkg/kv/batch.go b/pkg/kv/batch.go
@@ -649,34 +649,6 @@ func (b *Batch) DelRange(s, e interface{}, returnKeys bool) {
 	b.initResult(1, 0, notRaw, nil)
 }
 
-// ExperimentalDelRangeUsingTombstone deletes the rows between begin (inclusive)
-// and end (exclusive) using an MVCC range tombstone. The caller must check
-// storage.CanUseExperimentalMVCCRangeTombstones() before using this.
-//
-// This method 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.
-func (b *Batch) ExperimentalDelRangeUsingTombstone(s, e interface{}) {
-	start, err := marshalKey(s)
-	if err != nil {
-		b.initResult(0, 0, notRaw, err)
-		return
-	}
-	end, err := marshalKey(e)
-	if err != nil {
-		b.initResult(0, 0, notRaw, err)
-		return
-	}
-	b.appendReqs(&roachpb.DeleteRangeRequest{
-		RequestHeader: roachpb.RequestHeader{
-			Key:    start,
-			EndKey: end,
-		},
-		UseExperimentalRangeTombstone: true,
-	})
-	b.initResult(1, 0, notRaw, nil)
-}
-
 // adminMerge is only exported on DB. It is here for symmetry with the
 // other operations.
 func (b *Batch) adminMerge(key interface{}) {

diff --git a/pkg/kv/db.go b/pkg/kv/db.go
@@ -546,22 +546,6 @@ func (db *DB) DelRange(
 	return r.Keys, err
 }
 
-// ExperimentalDelRangeUsingTombstone deletes the rows between begin (inclusive)
-// and end (exclusive) using an MVCC range tombstone. The caller must check
-// storage.CanUseExperimentalMVCCRangeTombstones() before using this.
-//
-// This method 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.
-func (db *DB) ExperimentalDelRangeUsingTombstone(
-	ctx context.Context, begin, end interface{},
-) error {
-	b := &Batch{}
-	b.ExperimentalDelRangeUsingTombstone(begin, end)
-	_, err := getOneResult(db.Run(ctx, b), b)
-	return err
-}
-
 // AdminMerge merges the range containing key and the subsequent range. After
 // the merge operation is complete, the range containing key will contain all of
 // the key/value pairs of the subsequent range and the subsequent range will no

diff --git a/pkg/kv/kvserver/batcheval/cmd_delete_range.go b/pkg/kv/kvserver/batcheval/cmd_delete_range.go
@@ -19,7 +19,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
-	"github.com/cockroachdb/errors"
 )
 
 func init() {
@@ -50,27 +49,6 @@ func DeleteRange(
 	h := cArgs.Header
 	reply := resp.(*roachpb.DeleteRangeResponse)
 
-	// Use experimental MVCC range tombstone if requested. The caller is expected
-	// to have checked storage.CanUseExperimentalMVCCRangeTombstones() first.
-	//
-	// TODO(erikgrinaker): Add integration tests for this.
-	if args.UseExperimentalRangeTombstone {
-		if cArgs.Header.Txn != nil {
-			return result.Result{}, ErrTransactionUnsupported
-		}
-		if args.Inline {
-			return result.Result{}, errors.AssertionFailedf("Inline can't be used with range tombstones")
-		}
-		if args.ReturnKeys {
-			return result.Result{}, errors.AssertionFailedf(
-				"ReturnKeys can't be used with range tombstones")
-		}
-		maxIntents := storage.MaxIntentsPerWriteIntentError.Get(&cArgs.EvalCtx.ClusterSettings().SV)
-		err := storage.ExperimentalMVCCDeleteRangeUsingTombstone(
-			ctx, readWriter, cArgs.Stats, args.Key, args.EndKey, h.Timestamp, maxIntents)
-		return result.Result{}, err
-	}
-
 	var timestamp hlc.Timestamp
 	if !args.Inline {
 		timestamp = h.Timestamp

diff --git a/pkg/kv/kvserver/batcheval/cmd_revert_range.go b/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
 	}

diff --git a/pkg/kv/kvserver/rangefeed/task_test.go b/pkg/kv/kvserver/rangefeed/task_test.go
@@ -190,21 +190,6 @@ func (s *testIterator) curKV() storage.MVCCKeyValue {
 	return s.kvs[s.cur]
 }
 
-// HasPointAndRange implements SimpleMVCCIterator.
-func (s *testIterator) HasPointAndRange() (bool, bool) {
-	panic("not implemented")
-}
-
-// RangeBounds implements SimpleMVCCIterator.
-func (s *testIterator) RangeBounds() (roachpb.Key, roachpb.Key) {
-	panic("not implemented")
-}
-
-// RangeTombstones implements SimpleMVCCIterator.
-func (s *testIterator) RangeKeys() []storage.MVCCRangeKeyValue {
-	panic("not implemented")
-}
-
 func TestInitResolvedTSScan(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	startKey := roachpb.RKey("d")

diff --git a/pkg/kv/kvserver/spanset/batch.go b/pkg/kv/kvserver/spanset/batch.go
@@ -176,21 +176,6 @@ func (i *MVCCIterator) UnsafeValue() []byte {
 	return i.i.UnsafeValue()
 }
 
-// HasPointAndRange implements SimpleMVCCIterator.
-func (i *MVCCIterator) HasPointAndRange() (bool, bool) {
-	return i.i.HasPointAndRange()
-}
-
-// RangeBounds implements SimpleMVCCIterator.
-func (i *MVCCIterator) RangeBounds() (roachpb.Key, roachpb.Key) {
-	return i.i.RangeBounds()
-}
-
-// RangeKeys implements SimpleMVCCIterator.
-func (i *MVCCIterator) RangeKeys() []storage.MVCCRangeKeyValue {
-	return i.i.RangeKeys()
-}
-
 // ComputeStats is part of the storage.MVCCIterator interface.
 func (i *MVCCIterator) ComputeStats(
 	start, end roachpb.Key, nowNanos int64,
@@ -614,22 +599,6 @@ func (s spanSetWriter) ClearIterRange(iter storage.MVCCIterator, start, end roac
 	return s.w.ClearIterRange(iter, start, end)
 }
 
-func (s spanSetWriter) ExperimentalPutMVCCRangeKey(
-	rangeKey storage.MVCCRangeKey, value []byte,
-) error {
-	if err := s.checkAllowedRange(rangeKey.StartKey, rangeKey.EndKey); err != nil {
-		return err
-	}
-	return s.w.ExperimentalPutMVCCRangeKey(rangeKey, value)
-}
-
-func (s spanSetWriter) ExperimentalClearMVCCRangeKey(rangeKey storage.MVCCRangeKey) error {
-	if err := s.checkAllowedRange(rangeKey.StartKey, rangeKey.EndKey); err != nil {
-		return err
-	}
-	return s.w.ExperimentalClearMVCCRangeKey(rangeKey)
-}
-
 func (s spanSetWriter) Merge(key storage.MVCCKey, value []byte) error {
 	if s.spansOnly {
 		if err := s.spans.CheckAllowed(SpanReadWrite, roachpb.Span{Key: key.Key}); err != nil {

diff --git a/pkg/roachpb/api.go b/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
 }
 
@@ -1233,10 +1233,6 @@ func (*DeleteRequest) flags() flag {
 }
 
 func (drr *DeleteRangeRequest) flags() flag {
-	// DeleteRangeRequest using MVCC range tombstones cannot be transactional.
-	if drr.UseExperimentalRangeTombstone {
-		return isWrite | isRange | isAlone | appliesTSCache
-	}
 	// DeleteRangeRequest has different properties if the "inline" flag is set.
 	// This flag indicates that the request is deleting inline MVCC values,
 	// which cannot be deleted transactionally - inline DeleteRange will thus
@@ -1270,14 +1266,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
 }
 

diff --git a/pkg/roachpb/api.proto b/pkg/roachpb/api.proto
@@ -348,17 +348,6 @@ message DeleteRangeRequest {
   // Inline values cannot be deleted transactionally; a DeleteRange with
   // "inline" set to true will fail if it is executed within a transaction.
   bool inline = 4;
-  // If enabled, the range is deleted using an MVCC range tombstone, which is a
-  // cheap constant-time operation. This option cannot be used in a transaction,
-  // and it cannot be combined with Inline or ReturnKeys.
-  //
-  // 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 use_experimental_range_tombstone = 5;
 }
 
 // A DeleteRangeResponse is the return value from the DeleteRange()
@@ -402,34 +391,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

diff --git a/pkg/roachpb/api_test.go b/pkg/roachpb/api_test.go
@@ -318,10 +318,8 @@ func TestFlagCombinations(t *testing.T) {
 	reqVariants := []Request{
 		&AddSSTableRequest{SSTTimestampToRequestTimestamp: hlc.Timestamp{Logical: 1}},
 		&DeleteRangeRequest{Inline: true},
-		&DeleteRangeRequest{UseExperimentalRangeTombstone: true},
 		&GetRequest{KeyLocking: lock.Exclusive},
 		&ReverseScanRequest{KeyLocking: lock.Exclusive},
-		&RevertRangeRequest{ExperimentalPreserveHistory: true},
 		&ScanRequest{KeyLocking: lock.Exclusive},
 	}
 

diff --git a/pkg/storage/BUILD.bazel b/pkg/storage/BUILD.bazel
@@ -21,7 +21,6 @@ go_library(
         "mvcc_incremental_iterator.go",
         "mvcc_key.go",
         "mvcc_logical_ops.go",
-        "mvcc_range_key_iterator.go",
         "open.go",
         "pebble.go",
         "pebble_batch.go",
@@ -109,7 +108,6 @@ go_test(
         "mvcc_incremental_iterator_test.go",
         "mvcc_key_test.go",
         "mvcc_logical_ops_test.go",
-        "mvcc_range_key_iterator_test.go",
         "mvcc_stats_test.go",
         "mvcc_test.go",
         "pebble_file_registry_test.go",