storage: temporarily disable maxItersBeforeSeek

This commit temporarily disables maxItersBeforeSeek by setting its value to 0.

This reveals a bug that was hidden by `maxItersBeforeSeek` and is demonstrated
by the change to `TestMVCCHistories/uncertainty_interval_with_local_uncertainty_limit`.
The bug is due to incorrect key ordering of MVCC keys with synthetic timestamps.

With the current encoding of MVCC key versions and the custom key comparator
`EngineKeyCompare`, the inclusion of a synthetic bit in a key's verion timestamp
causes the key's encoding to sort _before_ the same key/timestamp without a
synthetic bit. This is because `EngineKeyCompare` considers the timestamp to be
larger and it sorts versions in decreasing timestamp order.

As a result, a seek to a version timestamp without a synthetic bit will seek
past and fail to observe a value with that same timestamp but with a synthetic
bit. In other words, a seek to timestamp `10,20` will fail to see a version
stored at `10,20?`. This is unintended behavior, as the synthetic bit should not
affect key ordering or key equality (see `MVCCKey.Equal`).

This change will be mostly reverted in a later commit when the bug is fixed.

pkg/storage/intent_interleaving_iter.go

@@ -737,12 +737,21 @@ func (i *intentInterleavingIter) SeekLT(key MVCCKey) {
 		return
 	}
 	if i.constraint != notConstrained {
-		i.checkConstraint(key.Key, true)
-		if i.constraint == constrainedToLocal && bytes.Equal(key.Key, keys.LocalMax) {
+		// If the seek key of SeekLT is the boundary between the local and global
+		// keyspaces, iterators constrained in either direction are permitted.
+		// Iterators constrained to the local keyspace may be scanning from their
+		// upper bound. Iterators constrained to the global keyspace may have found
+		// a key on the boundary and may now be scanning before the key, using the
+		// boundary as an exclusive upper bound.
+		localMax := bytes.Equal(key.Key, keys.LocalMax)
+		if !localMax {
+			i.checkConstraint(key.Key, true)
+		}
+		if localMax && i.constraint == constrainedToLocal {
 			// Move it down to below the lock table so can iterate down cleanly into
 			// the local key space. Note that we disallow anyone using a seek key
-			// that is a local key above the lock table, and there should no keys in
-			// the engine there either (at least not keys that we need to see using
+			// that is a local key above the lock table, and there should be no keys
+			// in the engine there either (at least not keys that we need to see using
 			// an MVCCIterator).
 			key.Key = keys.LocalRangeLockTablePrefix
 		}

pkg/storage/intent_interleaving_iter_test.go

@@ -413,7 +413,7 @@ func TestIntentInterleavingIterBoundaries(t *testing.T) {
 		defer iter.Close()
 		iter.SeekLT(MVCCKey{Key: keys.MaxKey})
 	})
-	// Boundary cases for constrainedToGlobal
+	// Boundary cases for constrainedToGlobal.
 	func() {
 		opts := IterOptions{LowerBound: keys.LocalMax}
 		iter := newIntentInterleavingIterator(eng, opts).(*intentInterleavingIter)
@@ -427,13 +427,13 @@ func TestIntentInterleavingIterBoundaries(t *testing.T) {
 		require.Equal(t, constrainedToGlobal, iter.constraint)
 		iter.SetUpperBound(keys.LocalMax)
 	})
-	require.Panics(t, func() {
+	func() {
 		opts := IterOptions{LowerBound: keys.LocalMax}
 		iter := newIntentInterleavingIterator(eng, opts).(*intentInterleavingIter)
 		defer iter.Close()
 		require.Equal(t, constrainedToGlobal, iter.constraint)
 		iter.SeekLT(MVCCKey{Key: keys.LocalMax})
-	})
+	}()
 	// Panics for using a local key that is above the lock table.
 	require.Panics(t, func() {
 		opts := IterOptions{UpperBound: keys.LocalMax}

pkg/storage/pebble_mvcc_scanner.go

@@ -29,7 +29,7 @@ import (
 )
 
 const (
-	maxItersBeforeSeek = 10
+	maxItersBeforeSeek = 0
 
 	// Key value lengths take up 8 bytes (2 x Uint32).
 	kvLenSize = 8
@@ -356,7 +356,7 @@ type pebbleMVCCScanner struct {
 	// Stores any error returned. If non-nil, iteration short circuits.
 	err error
 	// Number of iterations to try before we do a Seek/SeekReverse. Stays within
-	// [1, maxItersBeforeSeek] and defaults to maxItersBeforeSeek/2 .
+	// [0, maxItersBeforeSeek] and defaults to maxItersBeforeSeek/2 .
 	itersBeforeSeek int
 }
 
@@ -483,8 +483,8 @@ func (p *pebbleMVCCScanner) incrementItersBeforeSeek() {
 // Decrements itersBeforeSeek while ensuring it stays positive.
 func (p *pebbleMVCCScanner) decrementItersBeforeSeek() {
 	p.itersBeforeSeek--
-	if p.itersBeforeSeek < 1 {
-		p.itersBeforeSeek = 1
+	if p.itersBeforeSeek < 0 {
+		p.itersBeforeSeek = 0
 	}
 }
 
@@ -971,6 +971,13 @@ func (p *pebbleMVCCScanner) addAndAdvance(
 func (p *pebbleMVCCScanner) seekVersion(
 	ctx context.Context, seekTS hlc.Timestamp, uncertaintyCheck bool,
 ) bool {
+	if seekTS.IsEmpty() {
+		// If the seek timestamp is empty, we've already seen all versions of this
+		// key, so seek to the next key. Seeking to version zero of the current key
+		// would be incorrect, as version zero is stored before all other versions.
+		return p.advanceKey()
+	}
+
 	seekKey := MVCCKey{Key: p.curUnsafeKey.Key, Timestamp: seekTS}
 	p.keyBuf = EncodeMVCCKeyToBuf(p.keyBuf[:0], seekKey)
 	origKey := p.keyBuf[:len(p.curUnsafeKey.Key)]

pkg/storage/testdata/mvcc_histories/uncertainty_interval_with_local_uncertainty_limit

@@ -200,17 +200,15 @@ scan t=txn1 k=k1 localUncertaintyLimit=5,0
 ----
 scan: "k1"-"k1\x00" -> <no data>
 
-run error
+run ok
 get t=txn1 k=k2 localUncertaintyLimit=5,0
 ----
 get: "k2" -> <no data>
-error: (*roachpb.ReadWithinUncertaintyIntervalError:) ReadWithinUncertaintyIntervalError: read at time 5.000000000,0 encountered previous write with future timestamp 10.000000000,0? within uncertainty interval `t <= (local=5.000000000,0, global=10.000000000,0)`; observed timestamps: []
 
-run error
+run ok
 scan t=txn1 k=k2 localUncertaintyLimit=5,0
 ----
 scan: "k2"-"k2\x00" -> <no data>
-error: (*roachpb.ReadWithinUncertaintyIntervalError:) ReadWithinUncertaintyIntervalError: read at time 5.000000000,0 encountered previous write with future timestamp 10.000000000,0? within uncertainty interval `t <= (local=5.000000000,0, global=10.000000000,0)`; observed timestamps: []
 
 run ok
 get t=txn1 k=k3 localUncertaintyLimit=5,0
@@ -222,17 +220,15 @@ scan t=txn1 k=k3 localUncertaintyLimit=5,0
 ----
 scan: "k3"-"k3\x00" -> <no data>
 
-run error
+run ok
 get t=txn1 k=k4 localUncertaintyLimit=5,0
 ----
 get: "k4" -> <no data>
-error: (*roachpb.ReadWithinUncertaintyIntervalError:) ReadWithinUncertaintyIntervalError: read at time 5.000000000,0 encountered previous write with future timestamp 10.000000000,0? within uncertainty interval `t <= (local=5.000000000,0, global=10.000000000,0)`; observed timestamps: []
 
-run error
+run ok
 scan t=txn1 k=k4 localUncertaintyLimit=5,0
 ----
 scan: "k4"-"k4\x00" -> <no data>
-error: (*roachpb.ReadWithinUncertaintyIntervalError:) ReadWithinUncertaintyIntervalError: read at time 5.000000000,0 encountered previous write with future timestamp 10.000000000,0? within uncertainty interval `t <= (local=5.000000000,0, global=10.000000000,0)`; observed timestamps: []
 
 run ok
 get t=txn1 k=k5 localUncertaintyLimit=5,0
@@ -244,17 +240,15 @@ scan t=txn1 k=k5 localUncertaintyLimit=5,0
 ----
 scan: "k5"-"k5\x00" -> <no data>
 
-run error
+run ok
 get t=txn1 k=k6 localUncertaintyLimit=5,0
 ----
 get: "k6" -> <no data>
-error: (*roachpb.ReadWithinUncertaintyIntervalError:) ReadWithinUncertaintyIntervalError: read at time 5.000000000,0 encountered previous write with future timestamp 10.000000000,0? within uncertainty interval `t <= (local=5.000000000,0, global=10.000000000,0)`; observed timestamps: []
 
-run error
+run ok
 scan t=txn1 k=k6 localUncertaintyLimit=5,0
 ----
 scan: "k6"-"k6\x00" -> <no data>
-error: (*roachpb.ReadWithinUncertaintyIntervalError:) ReadWithinUncertaintyIntervalError: read at time 5.000000000,0 encountered previous write with future timestamp 10.000000000,0? within uncertainty interval `t <= (local=5.000000000,0, global=10.000000000,0)`; observed timestamps: []
 
 run ok
 get t=txn1 k=k7 localUncertaintyLimit=5,0
@@ -266,17 +260,15 @@ scan t=txn1 k=k7 localUncertaintyLimit=5,0
 ----
 scan: "k7"-"k7\x00" -> <no data>
 
-run error
+run ok
 get t=txn1 k=k8 localUncertaintyLimit=5,0
 ----
 get: "k8" -> <no data>
-error: (*roachpb.ReadWithinUncertaintyIntervalError:) ReadWithinUncertaintyIntervalError: read at time 5.000000000,0 encountered previous write with future timestamp 10.000000000,0? within uncertainty interval `t <= (local=5.000000000,0, global=10.000000000,0)`; observed timestamps: []
 
-run error
+run ok
 scan t=txn1 k=k8 localUncertaintyLimit=5,0
 ----
 scan: "k8"-"k8\x00" -> <no data>
-error: (*roachpb.ReadWithinUncertaintyIntervalError:) ReadWithinUncertaintyIntervalError: read at time 5.000000000,0 encountered previous write with future timestamp 10.000000000,0? within uncertainty interval `t <= (local=5.000000000,0, global=10.000000000,0)`; observed timestamps: []
 
 
 run ok