Merge cockroachdb#84961

84961: storage: require valid iterator for `HasPointAndRange()` r=jbowens a=erikgrinaker

This patch requires the caller to check `Valid()` before calling
`SimpleMVCCIterator.HasPointAndRange()`. This avoids making redundant
`Valid()` calls internally in `HasPointAndRange()`, which has a
non-negligible cost, especially considering callers may often make
multiple calls to `HasPointAndRange()` but only a single call to
`Valid()` per step. This improves scan performance by as much as
2.5% in the no-range-key case.

This also allows combining `EngineIterator.HasEnginePointAndRange()` with
`HasPointAndRange()`, since `pebbleIterator.Valid()` could not be called
from `EngineIterator` methods (due to lock table checks).

```
MVCCScan_Pebble/rows=1/versions=1/valueSize=64-24         4.75µs ± 2%    4.78µs ± 2%    ~     (p=0.210 n=10+10)
MVCCScan_Pebble/rows=1/versions=10/valueSize=64-24        6.63µs ± 1%    6.62µs ± 1%    ~     (p=0.590 n=9+10)
MVCCScan_Pebble/rows=100/versions=1/valueSize=64-24       38.9µs ± 1%    38.4µs ± 1%  -1.45%  (p=0.001 n=10+10)
MVCCScan_Pebble/rows=100/versions=10/valueSize=64-24       124µs ± 1%     121µs ± 1%  -2.52%  (p=0.000 n=10+10)
MVCCScan_Pebble/rows=10000/versions=1/valueSize=64-24     2.91ms ± 1%    2.84ms ± 1%  -2.39%  (p=0.000 n=10+10)
MVCCScan_Pebble/rows=10000/versions=10/valueSize=64-24    10.8ms ± 1%    10.5ms ± 1%  -2.17%  (p=0.000 n=10+9)
MVCCGet_Pebble/batch=false/versions=1/valueSize=8-24      4.38µs ± 1%    4.37µs ± 1%    ~     (p=0.920 n=10+9)
MVCCGet_Pebble/batch=false/versions=10/valueSize=8-24     5.41µs ± 1%    5.39µs ± 2%    ~     (p=0.170 n=10+10)
MVCCGet_Pebble/batch=false/versions=100/valueSize=8-24    13.4µs ± 2%    13.2µs ± 1%  -1.01%  (p=0.016 n=10+9)
MVCCGet_Pebble/batch=true/versions=1/valueSize=8-24       2.78µs ± 1%    2.77µs ± 1%  -0.50%  (p=0.021 n=10+9)
MVCCGet_Pebble/batch=true/versions=10/valueSize=8-24      3.98µs ± 1%    3.98µs ± 1%    ~     (p=0.921 n=9+10)
MVCCGet_Pebble/batch=true/versions=100/valueSize=8-24     10.7µs ± 3%    10.6µs ± 5%    ~     (p=0.684 n=10+10)
```

Resolves cockroachdb#83801.

Release note: None

Co-authored-by: Erik Grinaker <[email protected]>

pkg/kv/kvserver/rangefeed/task_test.go

@@ -196,8 +196,7 @@ func (s *testIterator) curKV() storage.MVCCKeyValue {
 
 // HasPointAndRange implements SimpleMVCCIterator.
 func (s *testIterator) HasPointAndRange() (bool, bool) {
-	ok, err := s.Valid()
-	return ok && err == nil, false
+	return true, false
 }
 
 // RangeBounds implements SimpleMVCCIterator.

pkg/kv/kvserver/spanset/batch.go

@@ -358,9 +358,9 @@ func (i *EngineIterator) checkKeyAllowed() (valid bool, err error) {
 	return true, nil
 }
 
-// HasEnginePointAndRange is part of the storage.EngineIterator interface.
-func (i *EngineIterator) HasEnginePointAndRange() (bool, bool) {
-	return i.i.HasEnginePointAndRange()
+// HasPointAndRange is part of the storage.EngineIterator interface.
+func (i *EngineIterator) HasPointAndRange() (bool, bool) {
+	return i.i.HasPointAndRange()
 }
 
 // EngineRangeBounds is part of the storage.EngineIterator interface.

pkg/storage/engine.go

@@ -160,9 +160,9 @@ type SimpleMVCCIterator interface {
 	// invalidated on the next call to {Next,NextKey,Prev,SeekGE,SeekLT,Close}.
 	UnsafeValue() []byte
 	// HasPointAndRange returns whether the current iterator position has a point
-	// key and/or a range key. If Valid() returns true, one of these will be true,
-	// otherwise they are both false. For details on range keys, see comment on
-	// SimpleMVCCIterator.
+	// key and/or a range key. Must check Valid() first. At least one of these
+	// will always be true for a valid iterator. For details on range keys, see
+	// comment on SimpleMVCCIterator.
 	HasPointAndRange() (bool, bool)
 	// RangeBounds returns the range bounds for the current range key, or an
 	// empty span if there are none. The returned keys are only valid until the
@@ -309,16 +309,9 @@ type EngineIterator interface {
 	// the iteration. After this call, valid will be true if the iterator was
 	// not originally positioned at the first key.
 	PrevEngineKey() (valid bool, err error)
-	// HasEnginePointAndRange returns whether the iterator is positioned on a
-	// point or range key.
-	//
-	// TODO(erikgrinaker): Consider renaming this HasPointAndRange and merge with
-	// the SimpleMVCCIterator implementation. However, HasPointAndRange() needs to
-	// imply Valid() for MVCC iterators, which in turns prevents it from being
-	// used e.g. across the lock table. Once we revamp the MVCCIterator interface
-	// we can probably merge these:
-	// https://github.com/cockroachdb/cockroach/issues/82589
-	HasEnginePointAndRange() (bool, bool)
+	// HasPointAndRange returns whether the iterator is positioned on a point or
+	// range key (shared with MVCCIterator interface).
+	HasPointAndRange() (bool, bool)
 	// EngineRangeBounds returns the current range key bounds.
 	EngineRangeBounds() (roachpb.Span, error)
 	// EngineRangeKeys returns the engine range keys at the current position.

pkg/storage/engine_test.go

@@ -2202,7 +2202,7 @@ func TestEngineRangeKeysUnsupported(t *testing.T) {
 					if keyType == IterKeyTypeRangesOnly {
 						// With RangesOnly, the iterator must be empty.
 						require.False(t, ok)
-						hasPoint, hasRange := iter.HasEnginePointAndRange()
+						hasPoint, hasRange := iter.HasPointAndRange()
 						require.False(t, hasPoint)
 						require.False(t, hasRange)
 						return
@@ -2214,7 +2214,7 @@ func TestEngineRangeKeysUnsupported(t *testing.T) {
 					require.Equal(t, engineKey("a", 1), key)
 					require.Equal(t, stringValueRaw("a1"), iter.UnsafeValue())
 
-					hasPoint, hasRange := iter.HasEnginePointAndRange()
+					hasPoint, hasRange := iter.HasPointAndRange()
 					require.True(t, hasPoint)
 					require.False(t, hasRange)
 					rangeBounds, err := iter.EngineRangeBounds()

pkg/storage/intent_interleaving_iter.go

@@ -886,9 +886,6 @@ func (i *intentInterleavingIter) Value() []byte {
 
 // HasPointAndRange implements SimpleMVCCIterator.
 func (i *intentInterleavingIter) HasPointAndRange() (bool, bool) {
-	if !i.valid {
-		return false, false
-	}
 	var hasPoint, hasRange bool
 	if i.iterValid {
 		hasPoint, hasRange = i.iter.HasPointAndRange()

pkg/storage/mvcc.go

@@ -3672,14 +3672,15 @@ func mvccResolveWriteIntent(
 			valid, err := iter.Valid()
 			if err != nil {
 				return false, err
-			}
-			if hasPoint, hasRange := iter.HasPointAndRange(); hasRange && !hasPoint {
-				// If the seek lands on a bare range key, attempt to step to a point.
-				iter.Next()
-				if valid, err = iter.Valid(); err != nil {
-					return false, err
-				} else if valid {
-					valid, _ = iter.HasPointAndRange()
+			} else if valid {
+				if hasPoint, hasRange := iter.HasPointAndRange(); hasRange && !hasPoint {
+					// If the seek lands on a bare range key, attempt to step to a point.
+					iter.Next()
+					if valid, err = iter.Valid(); err != nil {
+						return false, err
+					} else if valid {
+						valid, _ = iter.HasPointAndRange()
+					}
 				}
 			}
 			if !valid || !iter.UnsafeKey().Equal(oldKey) {
@@ -3826,14 +3827,15 @@ func mvccResolveWriteIntent(
 		iter.SeekGE(nextKey)
 		if ok, err = iter.Valid(); err != nil {
 			return false, err
-		}
-		// If the seek lands on a bare range key, attempt to step to a point.
-		if hasPoint, hasRange := iter.HasPointAndRange(); hasRange && !hasPoint {
-			iter.Next()
-			if ok, err = iter.Valid(); err != nil {
-				return false, err
-			} else if ok {
-				ok, _ = iter.HasPointAndRange()
+		} else if ok {
+			// If the seek lands on a bare range key, attempt to step to a point.
+			if hasPoint, hasRange := iter.HasPointAndRange(); hasRange && !hasPoint {
+				iter.Next()
+				if ok, err = iter.Valid(); err != nil {
+					return false, err
+				} else if ok {
+					ok, _ = iter.HasPointAndRange()
+				}
 			}
 		}
 		if ok = ok && iter.UnsafeKey().Key.Equal(latestKey.Key); ok {

pkg/storage/mvcc_history_test.go

@@ -1560,19 +1560,16 @@ func printIter(e *evalCtx) {
 	e.results.buf.Printf("%s:", e.td.Cmd)
 	defer e.results.buf.Printf("\n")
 
-	hasPoint, hasRange := e.iter.HasPointAndRange()
 	ok, err := e.iter.Valid()
 	if err != nil {
 		e.results.buf.Printf(" err=%v", err)
 		return
 	}
 	if !ok {
-		if hasPoint || hasRange {
-			e.t.Fatalf("invalid iterator gave hasPoint=%t hasRange=%t", hasPoint, hasRange)
-		}
 		e.results.buf.Print(" .")
 		return
 	}
+	hasPoint, hasRange := e.iter.HasPointAndRange()
 	if !hasPoint && !hasRange {
 		e.t.Fatalf("valid iterator at %s without point nor range keys", e.iter.UnsafeKey())
 	}

pkg/storage/mvcc_incremental_iterator.go

@@ -535,20 +535,20 @@ func (i *MVCCIncrementalIterator) UnsafeKey() MVCCKey {
 
 // HasPointAndRange implements SimpleMVCCIterator.
 func (i *MVCCIncrementalIterator) HasPointAndRange() (bool, bool) {
-	return i.hasPoint && i.valid, i.hasRange && i.valid
+	return i.hasPoint, i.hasRange
 }
 
 // RangeBounds implements SimpleMVCCIterator.
 func (i *MVCCIncrementalIterator) RangeBounds() roachpb.Span {
-	if !i.hasRange || !i.valid {
+	if !i.hasRange {
 		return roachpb.Span{}
 	}
 	return i.iter.RangeBounds()
 }
 
 // RangeKeys implements SimpleMVCCIterator.
 func (i *MVCCIncrementalIterator) RangeKeys() []MVCCRangeKeyValue {
-	if !i.hasRange || !i.valid {
+	if !i.hasRange {
 		return []MVCCRangeKeyValue{}
 	}
 

pkg/storage/pebble_iterator.go

@@ -449,10 +449,6 @@ func (p *pebbleIterator) NextKey() {
 		// NB: We have to be careful to use p.iter methods below, rather than
 		// pebbleIterator methods, since seekKey is an already-encoded roachpb.Key
 		// in raw Pebble key form.
-		//
-		// TODO(erikgrinaker): It's possible for Pebble to return true from
-		// HasPointAndRange when Valid() returns false, so we check Valid first. We
-		// should make this part of the Pebble API contract.
 		if p.iter.Valid() {
 			if hasPoint, hasRange := p.iter.HasPointAndRange(); !hasPoint && hasRange {
 				if startKey, _ := p.iter.RangeBounds(); bytes.Compare(startKey, seekKey) < 0 {
@@ -619,17 +615,6 @@ func (p *pebbleIterator) ValueProto(msg protoutil.Message) error {
 
 // HasPointAndRange implements the MVCCIterator interface.
 func (p *pebbleIterator) HasPointAndRange() (bool, bool) {
-	// TODO(erikgrinaker): The MVCCIterator contract mandates returning false for
-	// an invalid iterator. We should improve pebbleIterator validity and error
-	// checking by doing it once per iterator operation and propagating errors.
-	if ok, err := p.Valid(); !ok || err != nil {
-		return false, false
-	}
-	return p.iter.HasPointAndRange()
-}
-
-// HasEnginePointAndRange implements the EngineIterator interface.
-func (p *pebbleIterator) HasEnginePointAndRange() (bool, bool) {
 	return p.iter.HasPointAndRange()
 }