Merge cockroachdb#86093

86093: storage: optimize `pointSynthesizingIter` for `MVCCGet` r=nicktrav a=erikgrinaker

**storage: fix slice extention in `MVCCRangeKeyStack.CloneInto`**

This patch makes `MVCCRangeKeyStack.CloneInto` extend the existing slice
to its full capacity before growing it. Previously, this could build a
slice that was too small and panic.

Release note: None

**storage: add `MVCCIterator.IsPrefix()`**

This method allows detecting whether an iterator is a prefix iterator,
which comes in handy for wrapping iterators to apply optimizations.

Resolves cockroachdb#86104.

Release note: None

**storage: reuse `pointSynthesizingIter` range key slice**

```
name                                                                 old time/op    new time/op    delta
MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=0-24     3.70µs ± 1%    3.67µs ± 1%  -0.89%  (p=0.002 n=10+10)
MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=1-24     7.37µs ± 0%    7.25µs ± 0%  -1.70%  (p=0.000 n=9+10)
MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=10-24    14.7µs ± 1%    14.4µs ± 1%  -2.21%  (p=0.000 n=10+10)
```

Touches cockroachdb#83049.

Release note: None

**storage: add prefix mode for `pointSynthesizingIter`**

This patch adds a `prefix` mode for `pointSynthesizingIter`, detected
from the parent's `IsPrefix()` method. When enabled, this allows
omitting key cloning and comparisons. This replaces the previous
`emitOnSeekGE` parameter.

```
name                                                                 old time/op    new time/op    delta
MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=0-24     3.67µs ± 1%    3.67µs ± 1%    ~     (p=0.517 n=10+10)
MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=1-24     7.25µs ± 0%    7.07µs ± 0%  -2.45%  (p=0.000 n=10+9)
MVCCGet_Pebble/batch=true/versions=1/valueSize=8/numRangeKeys=10-24    14.4µs ± 1%    14.2µs ± 1%  -1.10%  (p=0.000 n=10+10)
```

Touches cockroachdb#83049.

Release note: None

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

pkg/kv/kvserver/spanset/batch.go

@@ -215,6 +215,11 @@ func (i *MVCCIterator) Stats() storage.IteratorStats {
 	return i.i.Stats()
 }
 
+// IsPrefix is part of the storage.MVCCIterator interface.
+func (i *MVCCIterator) IsPrefix() bool {
+	return i.i.IsPrefix()
+}
+
 // SupportsPrev is part of the storage.MVCCIterator interface.
 func (i *MVCCIterator) SupportsPrev() bool {
 	return i.i.SupportsPrev()

pkg/storage/engine.go

@@ -267,6 +267,9 @@ type MVCCIterator interface {
 	FindSplitKey(start, end, minSplitKey roachpb.Key, targetSize int64) (MVCCKey, error)
 	// Stats returns statistics about the iterator.
 	Stats() IteratorStats
+	// IsPrefix returns true if the MVCCIterator is a prefix iterator, i.e.
+	// created with IterOptions.Prefix enabled.
+	IsPrefix() bool
 	// SupportsPrev returns true if MVCCIterator implementation supports reverse
 	// iteration with Prev() or SeekLT().
 	SupportsPrev() bool

pkg/storage/intent_interleaving_iter.go

@@ -1236,6 +1236,11 @@ func (i *intentInterleavingIter) Stats() IteratorStats {
 	return stats
 }
 
+// IsPrefix implements the MVCCIterator interface.
+func (i *intentInterleavingIter) IsPrefix() bool {
+	return i.prefix
+}
+
 func (i *intentInterleavingIter) SupportsPrev() bool {
 	return true
 }

pkg/storage/mvcc.go

@@ -28,6 +28,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
+	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/envutil"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/iterutil"
@@ -969,6 +970,9 @@ func mvccGet(
 	if timestamp.WallTime < 0 {
 		return optionalValue{}, nil, errors.Errorf("cannot write to %q at timestamp %s", key, timestamp)
 	}
+	if util.RaceEnabled && !iter.IsPrefix() {
+		return optionalValue{}, nil, errors.AssertionFailedf("mvccGet called with non-prefix iterator")
+	}
 	if err := opts.validate(); err != nil {
 		return optionalValue{}, nil, err
 	}

pkg/storage/mvcc_history_test.go

@@ -92,7 +92,7 @@ var (
 // scan           [t=<name>] [ts=<int>[,<int>]]                         [resolve [status=<txnstatus>]] k=<key> [end=<key>] [inconsistent] [skipLocked] [tombstones] [reverse] [failOnMoreRecent] [localUncertaintyLimit=<int>[,<int>]] [globalUncertaintyLimit=<int>[,<int>]] [max=<max>] [targetbytes=<target>] [allowEmpty]
 // export         [k=<key>] [end=<key>] [ts=<int>[,<int>]] [kTs=<int>[,<int>]] [startTs=<int>[,<int>]] [maxIntents=<int>] [allRevisions] [targetSize=<int>] [maxSize=<int>] [stopMidKey]
 //
-// iter_new       [k=<key>] [end=<key>] [prefix] [kind=key|keyAndIntents] [types=pointsOnly|pointsWithRanges|pointsAndRanges|rangesOnly] [pointSynthesis [emitOnSeekGE]] [maskBelow=<int>[,<int>]]
+// iter_new       [k=<key>] [end=<key>] [prefix] [kind=key|keyAndIntents] [types=pointsOnly|pointsWithRanges|pointsAndRanges|rangesOnly] [pointSynthesis] [maskBelow=<int>[,<int>]]
 // iter_new_incremental [k=<key>] [end=<key>] [startTs=<int>[,<int>]] [endTs=<int>[,<int>]] [types=pointsOnly|pointsWithRanges|pointsAndRanges|rangesOnly] [maskBelow=<int>[,<int>]] [intents=error|aggregate|emit]
 // iter_seek_ge   k=<key> [ts=<int>[,<int>]]
 // iter_seek_lt   k=<key> [ts=<int>[,<int>]]
@@ -1446,11 +1446,15 @@ func cmdIterNew(e *evalCtx) error {
 	}
 
 	r, closer := metamorphicReader(e)
-	e.iter = &iterWithCloser{r.NewMVCCIterator(kind, opts), closer}
-
+	iter := r.NewMVCCIterator(kind, opts)
 	if e.hasArg("pointSynthesis") {
-		e.iter = newPointSynthesizingIter(e.mvccIter(), e.hasArg("emitOnSeekGE"))
+		iter = newPointSynthesizingIter(iter)
+	}
+	if opts.Prefix != iter.IsPrefix() {
+		return errors.Errorf("prefix iterator returned IsPrefix=false")
 	}
+
+	e.iter = &iterWithCloser{iter, closer}
 	e.iterRangeKeys.Clear()
 	return nil
 }

pkg/storage/mvcc_key.go

@@ -664,7 +664,7 @@ func (v MVCCRangeKeyVersions) CloneInto(c *MVCCRangeKeyVersions) {
 	if length, capacity := len(v), cap(*c); length > capacity {
 		// Extend the slice, keeping the existing versions to reuse their Value byte
 		// slices. The compiler optimizes away the intermediate, appended slice.
-		(*c) = append(*c, make(MVCCRangeKeyVersions, length-capacity)...)
+		*c = append((*c)[:capacity], make(MVCCRangeKeyVersions, length-capacity)...)
 	} else {
 		*c = (*c)[:length]
 	}

pkg/storage/mvcc_key_test.go

@@ -22,6 +22,7 @@ import (
 	"testing/quick"
 
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -457,46 +458,51 @@ func TestMVCCRangeKeyCloneInto(t *testing.T) {
 	}
 	for name, tc := range testcases {
 		t.Run(name, func(t *testing.T) {
-			clone := tc.target
-			orig.CloneInto(&clone)
-
-			// We don't discard empty byte slices when cloning a nil value, so we have
-			// to normalize these back to nil for the purpose of comparison.
-			for i := range clone.Versions {
-				if orig.Versions[i].Value == nil && len(clone.Versions[i].Value) == 0 {
-					clone.Versions[i].Value = nil
+			testutils.RunTrueAndFalse(t, "cleared", func(t *testing.T, cleared bool) {
+				clone := tc.target
+				if cleared {
+					clone.Clear()
 				}
-			}
-			require.Equal(t, orig, clone)
-
-			requireSliceIdentity := func(t *testing.T, a, b []byte, expectSame bool) {
-				t.Helper()
-				a, b = a[:cap(a)], b[:cap(b)]
-				if len(a) > 0 {
-					if expectSame {
-						require.Same(t, &a[0], &b[0])
-					} else {
-						require.NotSame(t, &a[0], &b[0])
+				orig.CloneInto(&clone)
+
+				// We don't discard empty byte slices when cloning a nil value, so we have
+				// to normalize these back to nil for the purpose of comparison.
+				for i := range clone.Versions {
+					if orig.Versions[i].Value == nil && len(clone.Versions[i].Value) == 0 {
+						clone.Versions[i].Value = nil
+					}
+				}
+				require.Equal(t, orig, clone)
+
+				requireSliceIdentity := func(t *testing.T, a, b []byte, expectSame bool) {
+					t.Helper()
+					a, b = a[:cap(a)], b[:cap(b)]
+					if len(a) > 0 {
+						if expectSame {
+							require.Same(t, &a[0], &b[0])
+						} else {
+							require.NotSame(t, &a[0], &b[0])
+						}
 					}
 				}
-			}
 
-			// Assert that slices are actual clones, by asserting the address of the
-			// backing array at [0].
-			requireSliceIdentity(t, orig.Bounds.Key, clone.Bounds.Key, false)
-			requireSliceIdentity(t, orig.Bounds.EndKey, clone.Bounds.EndKey, false)
-			for i := range orig.Versions {
-				requireSliceIdentity(t, orig.Versions[i].Value, clone.Versions[i].Value, false)
-			}
+				// Assert that slices are actual clones, by asserting the address of the
+				// backing array at [0].
+				requireSliceIdentity(t, orig.Bounds.Key, clone.Bounds.Key, false)
+				requireSliceIdentity(t, orig.Bounds.EndKey, clone.Bounds.EndKey, false)
+				for i := range orig.Versions {
+					requireSliceIdentity(t, orig.Versions[i].Value, clone.Versions[i].Value, false)
+				}
 
-			// Assert whether the clone is reusing byte slices from the target.
-			requireSliceIdentity(t, tc.target.Bounds.Key, clone.Bounds.Key, tc.expectReused)
-			requireSliceIdentity(t, tc.target.Bounds.EndKey, clone.Bounds.EndKey, tc.expectReused)
-			for i := range tc.target.Versions {
-				if i < len(clone.Versions) {
-					requireSliceIdentity(t, tc.target.Versions[i].Value, clone.Versions[i].Value, tc.expectReused)
+				// Assert whether the clone is reusing byte slices from the target.
+				requireSliceIdentity(t, tc.target.Bounds.Key, clone.Bounds.Key, tc.expectReused)
+				requireSliceIdentity(t, tc.target.Bounds.EndKey, clone.Bounds.EndKey, tc.expectReused)
+				for i := range tc.target.Versions {
+					if i < len(clone.Versions) {
+						requireSliceIdentity(t, tc.target.Versions[i].Value, clone.Versions[i].Value, tc.expectReused)
+					}
 				}
-			}
+			})
 		})
 	}
 }

pkg/storage/pebble_iterator.go

@@ -863,6 +863,11 @@ func (p *pebbleIterator) Stats() IteratorStats {
 	}
 }
 
+// IsPrefix implements the MVCCIterator interface.
+func (p *pebbleIterator) IsPrefix() bool {
+	return p.prefix
+}
+
 // SupportsPrev implements the MVCCIterator interface.
 func (p *pebbleIterator) SupportsPrev() bool {
 	return true

pkg/storage/pebble_mvcc_scanner.go

@@ -1184,7 +1184,7 @@ func (p *pebbleMVCCScanner) updateCurrent() bool {
 // iterator was valid when called and returns true if there is no change.
 func (p *pebbleMVCCScanner) maybeEnablePointSynthesis() bool {
 	if _, hasRange := p.parent.HasPointAndRange(); hasRange {
-		p.pointIter = newPointSynthesizingIterAtParent(p.parent, p.isGet)
+		p.pointIter = newPointSynthesizingIterAtParent(p.parent)
 		p.parent = p.pointIter
 		return p.iterValid()
 	}