storage: strengthen time-bound iteration guarantees

Previously, the minimum and maximum timestamps used to configure a "time-bound"
iterator were only hints. The iterator would surface keys outside those bounds
frequently if the keys were contained within a sstable block that also
contained keys within bounds. This commit adapts the iterator construction to
use Pebble's IterOptions.SkipPoint option to filter out-of-range keys without
yielding them to the caller.

This adds a small additional overhead to processing KVs that are ultimately
yielded to the user of a MVCCIncrementalIterator, but removes a large overhead
to proessing KVs that are ultimately skipped. In the current
MVCCIncrementalIterator implementation, keys that are outside the time range
still reposition the iterator without time bound. This repositioning no longer
happens, and removing this overhead more than reclaims the RefreshRange
benchmark's regression from the introduction value blocks packing keys more
densely.

This also serves as a step towards cockroachdb#66869.

```
goos: linux
goarch: amd64
cpu: Intel(R) Xeon(R) CPU @ 2.80GHz
                                                         │    old.txt    │               new.txt               │
                                                         │    sec/op     │   sec/op     vs base                │
RefreshRange/linear-keys/refresh_window=[0.00,0.00]-24      150.73µ ± 1%   55.75µ ± 1%  -63.01% (p=0.000 n=10)
RefreshRange/linear-keys/refresh_window=[0.00,50.00]-24      16.52µ ± 1%   16.19µ ± 2%   -2.00% (p=0.005 n=10)
RefreshRange/linear-keys/refresh_window=[0.00,75.00]-24      13.94µ ± 1%   13.59µ ± 1%   -2.55% (p=0.000 n=10)
RefreshRange/linear-keys/refresh_window=[0.00,95.00]-24      13.97µ ± 1%   13.56µ ± 3%   -2.99% (p=0.002 n=10)
RefreshRange/linear-keys/refresh_window=[0.00,99.00]-24      13.96µ ± 1%   13.45µ ± 3%   -3.69% (p=0.000 n=10)
RefreshRange/linear-keys/refresh_window=[50.00,50.00]-24    149.95µ ± 1%   55.67µ ± 1%  -62.88% (p=0.000 n=10)
RefreshRange/linear-keys/refresh_window=[50.00,75.00]-24     22.05µ ± 1%   21.70µ ± 0%   -1.57% (p=0.000 n=10)
RefreshRange/linear-keys/refresh_window=[50.00,95.00]-24     22.08µ ± 1%   21.61µ ± 0%   -2.14% (p=0.000 n=10)
RefreshRange/linear-keys/refresh_window=[50.00,99.00]-24     21.91µ ± 1%   21.69µ ± 1%   -0.99% (p=0.000 n=10)
RefreshRange/linear-keys/refresh_window=[75.00,75.00]-24    139.27µ ± 1%   55.82µ ± 1%  -59.92% (p=0.000 n=10)
RefreshRange/linear-keys/refresh_window=[75.00,95.00]-24     23.76µ ± 1%   23.36µ ± 0%   -1.66% (p=0.000 n=10)
RefreshRange/linear-keys/refresh_window=[75.00,99.00]-24     23.85µ ± 1%   23.25µ ± 1%   -2.48% (p=0.000 n=10)
RefreshRange/linear-keys/refresh_window=[95.00,95.00]-24    138.94µ ± 1%   55.52µ ± 0%  -60.04% (p=0.000 n=10)
RefreshRange/linear-keys/refresh_window=[95.00,99.00]-24     26.68µ ± 1%   26.30µ ± 1%   -1.42% (p=0.005 n=10)
RefreshRange/linear-keys/refresh_window=[99.00,99.00]-24    141.39µ ± 1%   55.02µ ± 1%  -61.09% (p=0.000 n=10)
RefreshRange/random-keys/refresh_window=[0.00,0.00]-24      322.35µ ± 0%   51.21µ ± 0%  -84.11% (p=0.000 n=10)
RefreshRange/random-keys/refresh_window=[0.00,50.00]-24      16.35µ ± 1%   16.63µ ± 2%        ~ (p=0.105 n=10)
RefreshRange/random-keys/refresh_window=[0.00,75.00]-24      17.42µ ± 1%   17.29µ ± 3%        ~ (p=0.796 n=10)
RefreshRange/random-keys/refresh_window=[0.00,95.00]-24      13.40µ ± 1%   13.45µ ± 1%        ~ (p=0.529 n=10)
RefreshRange/random-keys/refresh_window=[0.00,99.00]-24      13.59µ ± 2%   13.41µ ± 0%        ~ (p=0.239 n=10)
RefreshRange/random-keys/refresh_window=[50.00,50.00]-24     707.5m ± 1%   111.7m ± 1%  -84.21% (p=0.000 n=10)
RefreshRange/random-keys/refresh_window=[50.00,75.00]-24     21.94µ ± 1%   17.86µ ± 0%  -18.59% (p=0.000 n=10)
RefreshRange/random-keys/refresh_window=[50.00,95.00]-24     13.95µ ± 1%   13.54µ ± 1%   -2.94% (p=0.000 n=10)
RefreshRange/random-keys/refresh_window=[50.00,99.00]-24     13.96µ ± 0%   13.53µ ± 1%   -3.06% (p=0.000 n=10)
RefreshRange/random-keys/refresh_window=[75.00,75.00]-24    395.22m ± 0%   33.81m ± 1%  -91.45% (p=0.000 n=10)
RefreshRange/random-keys/refresh_window=[75.00,95.00]-24     19.42µ ± 1%   19.40µ ± 1%        ~ (p=0.912 n=10)
RefreshRange/random-keys/refresh_window=[75.00,99.00]-24     19.37µ ± 1%   19.42µ ± 1%        ~ (p=0.247 n=10)
RefreshRange/random-keys/refresh_window=[95.00,95.00]-24    546.66m ± 3%   48.48m ± 3%  -91.13% (p=0.000 n=10)
RefreshRange/random-keys/refresh_window=[95.00,99.00]-24     21.69µ ± 1%   19.68µ ± 1%   -9.24% (p=0.000 n=10)
RefreshRange/random-keys/refresh_window=[99.00,99.00]-24    546.27m ± 0%   48.73m ± 3%  -91.08% (p=0.000 n=10)
RefreshRange/mixed-case/refresh_window=[0.00,0.00]-24        410.1µ ± 0%   221.1µ ± 1%  -46.09% (p=0.000 n=10)
RefreshRange/mixed-case/refresh_window=[0.00,50.00]-24       14.84µ ± 1%   14.84µ ± 1%        ~ (p=0.739 n=10)
RefreshRange/mixed-case/refresh_window=[0.00,75.00]-24       14.84µ ± 1%   14.83µ ± 1%        ~ (p=0.615 n=10)
RefreshRange/mixed-case/refresh_window=[0.00,95.00]-24       17.39µ ± 2%   17.51µ ± 2%   +0.68% (p=0.029 n=10)
RefreshRange/mixed-case/refresh_window=[0.00,99.00]-24       16.41µ ± 1%   16.38µ ± 1%        ~ (p=0.971 n=10)
RefreshRange/mixed-case/refresh_window=[50.00,50.00]-24      660.9m ± 0%   140.6m ± 3%  -78.73% (p=0.000 n=10)
RefreshRange/mixed-case/refresh_window=[50.00,75.00]-24      20.43µ ± 1%   15.46µ ± 1%  -24.31% (p=0.000 n=10)
RefreshRange/mixed-case/refresh_window=[50.00,95.00]-24      18.03µ ± 1%   17.66µ ± 1%   -2.01% (p=0.000 n=10)
RefreshRange/mixed-case/refresh_window=[50.00,99.00]-24      16.86µ ± 1%   16.70µ ± 1%   -0.93% (p=0.011 n=10)
RefreshRange/mixed-case/refresh_window=[75.00,75.00]-24      659.5m ± 1%   137.1m ± 3%  -79.21% (p=0.000 n=10)
RefreshRange/mixed-case/refresh_window=[75.00,95.00]-24      17.96µ ± 1%   17.65µ ± 1%   -1.77% (p=0.000 n=10)
RefreshRange/mixed-case/refresh_window=[75.00,99.00]-24      16.86µ ± 1%   16.66µ ± 1%   -1.23% (p=0.008 n=10)
RefreshRange/mixed-case/refresh_window=[95.00,95.00]-24     900.95µ ± 0%   32.18µ ± 0%  -96.43% (p=0.000 n=10)
RefreshRange/mixed-case/refresh_window=[95.00,99.00]-24      14.35µ ± 1%   14.50µ ± 2%   +1.03% (p=0.002 n=10)
RefreshRange/mixed-case/refresh_window=[99.00,99.00]-24    227.235m ± 1%   3.361m ± 1%  -98.52% (p=0.000 n=10)
geomean                                                      136.9µ        73.57µ       -46.24%

goos: linux
goarch: amd64
cpu: Intel(R) Xeon(R) CPU @ 2.80GHz
                                                    │  old2.txt   │              new2.txt               │
                                                    │   sec/op    │   sec/op     vs base                │
MVCCIncrementalIterator/ts=5-24                       24.93m ± 3%   24.53m ± 2%   -1.61% (p=0.004 n=10)
MVCCIncrementalIterator/ts=480-24                     560.4µ ± 1%   453.8µ ± 0%  -19.02% (p=0.000 n=10)
MVCCIncrementalIteratorForOldData/valueSize=100-24    1.444m ± 0%   1.434m ± 0%   -0.72% (p=0.000 n=10)
MVCCIncrementalIteratorForOldData/valueSize=500-24    1.994m ± 0%   1.974m ± 1%   -1.02% (p=0.000 n=10)
MVCCIncrementalIteratorForOldData/valueSize=1000-24   2.672m ± 1%   2.649m ± 1%        ~ (p=0.063 n=10)
MVCCIncrementalIteratorForOldData/valueSize=2000-24   4.059m ± 1%   4.009m ± 1%   -1.23% (p=0.011 n=10)
geomean                                               2.754m        2.635m        -4.33%
```

Epic: none
Informs cockroachdb#66869
Close cockroachdb#98881
Release note: none

pkg/ccl/storageccl/engineccl/bench_test.go

@@ -187,9 +187,9 @@ func BenchmarkTimeBoundIterate(b *testing.B) {
 			b.Run("TimeBoundIterator", func(b *testing.B) {
 				runIterate(b, loadFactor, func(e storage.Engine, startTime, endTime hlc.Timestamp) (storage.MVCCIterator, error) {
 					return e.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{
-						MinTimestampHint: startTime,
-						MaxTimestampHint: endTime,
-						UpperBound:       roachpb.KeyMax,
+						MinTimestamp: startTime,
+						MaxTimestamp: endTime,
+						UpperBound:   roachpb.KeyMax,
 					})
 				})
 			})

pkg/kv/kvserver/batcheval/cmd_revert_range.go

@@ -79,11 +79,11 @@ 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, err := readWriter.NewMVCCIterator(storage.MVCCKeyIterKind, storage.IterOptions{
-		KeyTypes:         storage.IterKeyTypePointsAndRanges,
-		LowerBound:       from,
-		UpperBound:       to,
-		MinTimestampHint: since.Next(), // make exclusive
-		MaxTimestampHint: until,
+		KeyTypes:     storage.IterKeyTypePointsAndRanges,
+		LowerBound:   from,
+		UpperBound:   to,
+		MinTimestamp: since.Next(), // make exclusive
+		MaxTimestamp: until,
 	})
 	if err != nil {
 		return false, err

pkg/kv/kvserver/batcheval/cmd_revert_range_test.go

@@ -55,7 +55,7 @@ func TestCmdRevertRange(t *testing.T) {
 
 	ctx := context.Background()
 
-	// Run this test on both RocksDB and Pebble. Regression test for:
+	// Regression test for:
 	// https://github.com/cockroachdb/cockroach/pull/42386
 	eng := storage.NewDefaultInMemForTesting()
 	defer eng.Close()
@@ -118,10 +118,11 @@ func TestCmdRevertRange(t *testing.T) {
 		ts       hlc.Timestamp
 		expected []byte
 		resumes  int
+		empty    bool
 	}{
-		{"revert revert to time A", tsA, sumA, 4},
-		{"revert revert to time B", tsB, sumB, 4},
-		{"revert revert to time C (nothing)", tsC, sumC, 0},
+		{"revert revert to time A", tsA, sumA, 4, false},
+		{"revert revert to time B", tsB, sumB, 4, false},
+		{"revert revert to time C (nothing)", tsC, sumC, 0, true},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			batch := &wrappedBatch{Batch: eng.NewBatch()}
@@ -140,9 +141,13 @@ func TestCmdRevertRange(t *testing.T) {
 				if err != nil {
 					t.Fatal(err)
 				}
-				require.NotNil(t, result.Replicated.MVCCHistoryMutation)
-				require.Equal(t, result.Replicated.MVCCHistoryMutation.Spans,
-					[]roachpb.Span{{Key: req.RequestHeader.Key, EndKey: req.RequestHeader.EndKey}})
+				// If there's nothing to revert and the fast-path is hit,
+				// MVCCHistoryMutation will be empty.
+				if !tc.empty {
+					require.NotNil(t, result.Replicated.MVCCHistoryMutation)
+					require.Equal(t, result.Replicated.MVCCHistoryMutation.Spans,
+						[]roachpb.Span{{Key: req.RequestHeader.Key, EndKey: req.RequestHeader.EndKey}})
+				}
 				if reply.ResumeSpan == nil {
 					break
 				}

pkg/storage/batch_test.go

@@ -192,9 +192,9 @@ func TestReadOnlyBasics(t *testing.T) {
 		},
 		func() {
 			iter, _ := ro.NewMVCCIterator(MVCCKeyIterKind, IterOptions{
-				MinTimestampHint: hlc.MinTimestamp,
-				MaxTimestampHint: hlc.MaxTimestamp,
-				UpperBound:       roachpb.KeyMax,
+				MinTimestamp: hlc.MinTimestamp,
+				MaxTimestamp: hlc.MaxTimestamp,
+				UpperBound:   roachpb.KeyMax,
 			})
 			iter.Close()
 		},

pkg/storage/engine.go

@@ -420,38 +420,22 @@ type IterOptions struct {
 	// the iterator. UpperBound must be provided unless Prefix is true, in which
 	// case the end of the prefix will be used as the upper bound.
 	UpperBound roachpb.Key
-	// MinTimestampHint and MaxTimestampHint, if set, indicate that keys outside
-	// of the time range formed by [MinTimestampHint, MaxTimestampHint] do not
-	// need to be presented by the iterator. The underlying iterator may be able
-	// to efficiently skip over keys outside of the hinted time range, e.g., when
-	// an SST indicates that it contains no keys within the time range. Intents
+	// MinTimestamp and MaxTimestamp, if set, indicate that only keys
+	// within the time range formed by [MinTimestamp, MaxTimestamp] should be
+	// returned. The underlying iterator may be able to efficiently skip over
+	// keys outside of the hinted time range, e.g., when a block handle
+	// indicates that the block contains no keys within the time range. Intents
 	// will not be visible to such iterators at all. This is only relevant for
 	// MVCCIterators.
 	//
-	// Note that time bound hints are strictly a performance optimization, and
-	// iterators with time bounds hints will frequently return keys outside of the
-	// [start, end] time range. If you must guarantee that you never see a key
-	// outside of the time bounds, perform your own filtering.
-	//
-	// NB: The iterator may surface stale data. Pebble range tombstones do not have
-	// timestamps and thus may be ignored entirely depending on whether their SST
-	// happens to satisfy the filter. Furthermore, keys outside the timestamp
-	// range may be stale and must be ignored -- for example, consider a key foo@5
-	// written in an SST with timestamp range [3-7], and then a non-MVCC removal
-	// or update of this key in a different SST with timestamp range [3-5]. Using
-	// an iterator with range [6-9] would surface the old foo@5 key because it
-	// would return all keys in the old [3-7] SST but not take into account the
-	// separate [3-5] SST where foo@5 was removed or updated. See also:
-	// https://github.com/cockroachdb/pebble/issues/1786
+	// Note that time-bound iterators previously were only a performance
+	// optimization but now guarantee that no keys outside of the [start, end]
+	// time range will be returned.
 	//
 	// NB: Range keys are not currently subject to timestamp filtering due to
 	// complications with MVCCIncrementalIterator. See:
 	// https://github.com/cockroachdb/cockroach/issues/86260
-	//
-	// Currently, the only way to correctly use such an iterator is to use it in
-	// concert with an iterator without timestamp hints, as done by
-	// MVCCIncrementalIterator.
-	MinTimestampHint, MaxTimestampHint hlc.Timestamp
+	MinTimestamp, MaxTimestamp hlc.Timestamp
 	// KeyTypes specifies the types of keys to surface: point and/or range keys.
 	// Use HasPointAndRange() to determine which key type is present at a given
 	// iterator position, and RangeBounds() and RangeKeys() to access range keys.

pkg/storage/engine_test.go

@@ -580,72 +580,77 @@ func TestEngineTimeBound(t *testing.T) {
 		"right not touching": {
 			iter: func() (MVCCIterator, error) {
 				return batch.NewMVCCIterator(MVCCKeyIterKind, IterOptions{
-					MinTimestampHint: maxTimestamp.WallNext(),
-					MaxTimestampHint: maxTimestamp.WallNext().WallNext(),
-					UpperBound:       roachpb.KeyMax,
+					MinTimestamp: maxTimestamp.WallNext(),
+					MaxTimestamp: maxTimestamp.WallNext().WallNext(),
+					UpperBound:   roachpb.KeyMax,
 				})
 			},
 			keys: 0,
 		},
 		"left not touching": {
 			iter: func() (MVCCIterator, error) {
 				return batch.NewMVCCIterator(MVCCKeyIterKind, IterOptions{
-					MinTimestampHint: minTimestamp.WallPrev().WallPrev(),
-					MaxTimestampHint: minTimestamp.WallPrev(),
-					UpperBound:       roachpb.KeyMax,
+					MinTimestamp: minTimestamp.WallPrev().WallPrev(),
+					MaxTimestamp: minTimestamp.WallPrev(),
+					UpperBound:   roachpb.KeyMax,
 				})
 			},
 			keys: 0,
 		},
 		"right touching": {
 			iter: func() (MVCCIterator, error) {
 				return batch.NewMVCCIterator(MVCCKeyIterKind, IterOptions{
-					MinTimestampHint: maxTimestamp,
-					MaxTimestampHint: maxTimestamp,
-					UpperBound:       roachpb.KeyMax,
+					MinTimestamp: maxTimestamp,
+					MaxTimestamp: maxTimestamp,
+					UpperBound:   roachpb.KeyMax,
 				})
 			},
-			keys: len(times),
+			keys: 1, // only one key exists at the max timestamp @7
 		},
-		"right touching ignores logical": {
+		// Although the block-property and table filters have historically
+		// ignored logical timestamps (and synthetic bits), the
+		// MVCCIncrementalIterator does not. It performs a strict hlc.Timestamp
+		// comparison. Both @7,1 and @7,2 are greater than @7, so no keys are
+		// visible.
+		"right touching enfoces logical": {
 			iter: func() (MVCCIterator, error) {
 				return batch.NewMVCCIterator(MVCCKeyIterKind, IterOptions{
-					MinTimestampHint: maxTimestamp.Next(),
-					MaxTimestampHint: maxTimestamp.Next().Next(),
-					UpperBound:       roachpb.KeyMax,
+					MinTimestamp: maxTimestamp.Next(),        // @7,1
+					MaxTimestamp: maxTimestamp.Next().Next(), // @7,2
+					UpperBound:   roachpb.KeyMax,
 				})
 			},
-			keys: len(times),
+			keys: 0,
 		},
 		"left touching": {
 			iter: func() (MVCCIterator, error) {
 				return batch.NewMVCCIterator(MVCCKeyIterKind, IterOptions{
-					MinTimestampHint: minTimestamp,
-					MaxTimestampHint: minTimestamp,
-					UpperBound:       roachpb.KeyMax,
+					MinTimestamp: minTimestamp,
+					MaxTimestamp: minTimestamp,
+					UpperBound:   roachpb.KeyMax,
 				})
 			},
-			keys: len(times),
+			keys: 1,
 		},
 		"left touching upperbound": {
 			iter: func() (MVCCIterator, error) {
 				return batch.NewMVCCIterator(MVCCKeyIterKind, IterOptions{
-					MinTimestampHint: minTimestamp,
-					MaxTimestampHint: minTimestamp,
-					UpperBound:       []byte("02"),
+					MinTimestamp: minTimestamp,
+					MaxTimestamp: minTimestamp,
+					UpperBound:   []byte("02"),
 				})
 			},
-			keys: 2,
+			keys: 1,
 		},
 		"between": {
 			iter: func() (MVCCIterator, error) {
 				return batch.NewMVCCIterator(MVCCKeyIterKind, IterOptions{
-					MinTimestampHint: minTimestamp.Next(),
-					MaxTimestampHint: minTimestamp.Next(),
-					UpperBound:       roachpb.KeyMax,
+					MinTimestamp: minTimestamp.Next(),
+					MaxTimestamp: minTimestamp.Next(),
+					UpperBound:   roachpb.KeyMax,
 				})
 			},
-			keys: len(times),
+			keys: 0,
 		},
 	}
 

pkg/storage/intent_interleaving_iter.go

@@ -69,7 +69,7 @@ var intentInterleavingReaderPool = sync.Pool{
 func (imr *intentInterleavingReader) NewMVCCIterator(
 	iterKind MVCCIterKind, opts IterOptions,
 ) (MVCCIterator, error) {
-	if (!opts.MinTimestampHint.IsEmpty() || !opts.MaxTimestampHint.IsEmpty()) &&
+	if (!opts.MinTimestamp.IsEmpty() || !opts.MaxTimestamp.IsEmpty()) &&
 		iterKind == MVCCKeyAndIntentsIterKind {
 		panic("cannot ask for interleaved intents when specifying timestamp hints")
 	}
@@ -233,7 +233,7 @@ func isLocal(k roachpb.Key) bool {
 }
 
 func newIntentInterleavingIterator(reader Reader, opts IterOptions) (MVCCIterator, error) {
-	if !opts.MinTimestampHint.IsEmpty() || !opts.MaxTimestampHint.IsEmpty() {
+	if !opts.MinTimestamp.IsEmpty() || !opts.MaxTimestamp.IsEmpty() {
 		panic("intentInterleavingIter must not be used with timestamp hints")
 	}
 	var lowerIsLocal, upperIsLocal bool

pkg/storage/mvcc_incremental_iterator.go

@@ -219,9 +219,9 @@ func NewMVCCIncrementalIterator(
 			LowerBound: opts.StartKey,
 			UpperBound: opts.EndKey,
 			// The call to startTime.Next() converts our exclusive start bound into
-			// the inclusive start bound that MinTimestampHint expects.
-			MinTimestampHint:     opts.StartTime.Next(),
-			MaxTimestampHint:     opts.EndTime,
+			// the inclusive start bound that MinTimestampt expects.
+			MinTimestamp:         opts.StartTime.Next(),
+			MaxTimestamp:         opts.EndTime,
 			RangeKeyMaskingBelow: tbiRangeKeyMasking,
 		})
 		if err != nil {