storage: randomly mangle range key buffers in test

pkg/storage/engine.go

@@ -1606,14 +1606,6 @@ func assertSimpleMVCCIteratorInvariants(iter SimpleMVCCIterator) error {
 			}
 		}
 
-	} else {
-		// Bounds and range keys must be empty.
-		if bounds := iter.RangeBounds(); !bounds.Equal(roachpb.Span{}) {
-			return errors.AssertionFailedf("hasRange=false but RangeBounds=%s", bounds)
-		}
-		if r := iter.RangeKeys(); !r.IsEmpty() || !r.Bounds.Equal(roachpb.Span{}) {
-			return errors.AssertionFailedf("hasRange=false but RangeKeys=%s", r)
-		}
 	}
 	if hasPoint {
 		value, err := iter.UnsafeValue()

pkg/storage/mvcc_history_metamorphic_iterator_test.go

@@ -173,7 +173,7 @@ func (m *metamorphicIterator) moveAround() {
 	}
 
 	hasPoint, _ := m.it.HasPointAndRange()
-	rangeKeys := m.it.RangeKeys().Clone()
+	rangeKeys := rangeKeysIfExist(m.it).Clone()
 	var rangeKeysIgnoringTime storage.MVCCRangeKeyStack
 	if iit != nil {
 		rangeKeysIgnoringTime = iit.RangeKeysIgnoringTime()
@@ -225,7 +225,7 @@ func (m *metamorphicIterator) moveAround() {
 					if m.it.UnsafeKey().Equal(cur) {
 						break // made it
 					}
-					printfln("step: %s %s [changed=%t]", m.it.UnsafeKey(), m.it.RangeKeys(), m.it.RangeKeyChanged())
+					printfln("step: %s %s [changed=%t]", m.it.UnsafeKey(), rangeKeysIfExist(m.it), m.it.RangeKeyChanged())
 					if iit != nil {
 						// If we're an incremental iterator with time bounds, and `cur` is not within bounds,
 						// would miss it if we used Next. So call NextIgnoringTime unconditionally.
@@ -248,7 +248,7 @@ func (m *metamorphicIterator) moveAround() {
 					valid, err := m.it.Valid()
 					require.Nil(m.t, err)
 					require.True(m.t, valid, "unable to recover original position following SeekLT")
-					printfln("rev-step: %s %s [changed=%t]", m.it.UnsafeKey(), m.it.RangeKeys(), m.it.RangeKeyChanged())
+					printfln("rev-step: %s %s [changed=%t]", m.it.UnsafeKey(), rangeKeysIfExist(m.it), m.it.RangeKeyChanged())
 					if m.it.UnsafeKey().Equal(cur) {
 						printfln("done")
 						break // made it
@@ -269,13 +269,13 @@ func (m *metamorphicIterator) moveAround() {
 			rangeKeysIgnoringTime2 = iit.RangeKeysIgnoringTime()
 		}
 		printfln("recovered position: %s hasPoint=%t, rangeKeys=%s, rangeKeysIgnoringTime=%s",
-			m.it.UnsafeKey(), hasPoint2, m.it.RangeKeys(), rangeKeysIgnoringTime2)
+			m.it.UnsafeKey(), hasPoint2, rangeKeysIfExist(m.it), rangeKeysIgnoringTime2)
 	}
 	// Back where we started and hopefully in an indistinguishable state.
 	// When the stack is empty, sometimes it's a nil slice and sometimes zero
 	// slice. A similar problem exists with MVCCRangeKeyVersion.Value. Sidestep
 	// them by comparing strings.
-	require.Equal(m.t, fmt.Sprint(rangeKeys), fmt.Sprint(m.it.RangeKeys()))
+	require.Equal(m.t, fmt.Sprint(rangeKeys), fmt.Sprint(rangeKeysIfExist(m.it)))
 	if iit != nil {
 		require.Equal(m.t, fmt.Sprint(rangeKeysIgnoringTime), fmt.Sprint(iit.RangeKeysIgnoringTime()))
 	}

pkg/storage/mvcc_history_test.go

@@ -1982,9 +1982,18 @@ func printIter(e *evalCtx) {
 	}
 }
 
+func rangeKeysIfExist(it storage.SimpleMVCCIterator) storage.MVCCRangeKeyStack {
+	if valid, err := it.Valid(); !valid || err != nil {
+		return storage.MVCCRangeKeyStack{}
+	} else if _, hasRange := it.HasPointAndRange(); !hasRange {
+		return storage.MVCCRangeKeyStack{}
+	}
+	return it.RangeKeys()
+}
+
 func checkAndUpdateRangeKeyChanged(e *evalCtx) bool {
 	rangeKeyChanged := e.iter.RangeKeyChanged()
-	rangeKeys := e.iter.RangeKeys()
+	rangeKeys := rangeKeysIfExist(e.iter)
 
 	if incrIter := e.tryMVCCIncrementalIter(); incrIter != nil {
 		// For MVCCIncrementalIterator, make sure RangeKeyChangedIgnoringTime() fires

pkg/storage/mvcc_key.go

@@ -512,6 +512,13 @@ type MVCCRangeKeyVersion struct {
 	Value     []byte
 }
 
+// CloneInto copies the version into the provided destination
+// MVCCRangeKeyVersion, reusing and overwriting its value slice.
+func (v MVCCRangeKeyVersion) CloneInto(dst *MVCCRangeKeyVersion) {
+	dst.Timestamp = v.Timestamp
+	dst.Value = append(dst.Value[:0], v.Value...)
+}
+
 // AsRangeKey returns an MVCCRangeKey for the given version. Byte slices
 // are shared with the stack.
 func (s MVCCRangeKeyStack) AsRangeKey(v MVCCRangeKeyVersion) MVCCRangeKey {

pkg/storage/pebble_mvcc_scanner.go

@@ -24,6 +24,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/util"
+	"github.com/cockroachdb/cockroach/pkg/util/buildutil"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/mon"
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
@@ -441,14 +442,15 @@ type pebbleMVCCScanner struct {
 	// cur* variables store the "current" record we're pointing to. Updated in
 	// updateCurrent. Note that the timestamp can be clobbered in the case of
 	// adding an intent from the intent history but is otherwise meaningful.
-	curUnsafeKey   MVCCKey
-	curRawKey      []byte
-	curUnsafeValue MVCCValue
-	curRawValue    pebble.LazyValue
-	curRangeKeys   MVCCRangeKeyStack
-	savedRangeKeys MVCCRangeKeyStack
-	results        results
-	intents        pebble.Batch
+	curUnsafeKey      MVCCKey
+	curRawKey         []byte
+	curUnsafeValue    MVCCValue
+	curRawValue       pebble.LazyValue
+	curRangeKeys      MVCCRangeKeyStack
+	savedRangeKeys    MVCCRangeKeyStack
+	savedRangeKeyVers MVCCRangeKeyVersion
+	results           results
+	intents           pebble.Batch
 	// mostRecentTS stores the largest timestamp observed that is equal to or
 	// above the scan timestamp. Only applicable if failOnMoreRecent is true. If
 	// set and no other error is hit, a WriteToOld error will be returned from
@@ -546,20 +548,40 @@ func (p *pebbleMVCCScanner) get(ctx context.Context) {
 	if !p.iterValid() {
 		return
 	}
+
+	// Unlike scans, if tombstones are enabled, we synthesize point tombstones
+	// for MVCC range tombstones even if there is no existing point key below
+	// it. These are often needed for e.g. conflict checks. However, both
+	// processRangeKeys and getOne may need to advance the iterator,
+	// moving away from range key we originally landed on. If we're in tombstone
+	// mode and there's a range key, save the most recent visible value so that
+	// we can use it to synthesize a tombstone if we fail to find a KV.
+	var hadMVCCRangeTombstone bool
+	if p.tombstones {
+		if _, hasRange := p.parent.HasPointAndRange(); hasRange {
+			rangeKeys := p.parent.RangeKeys()
+			if rkv, ok := rangeKeys.FirstAtOrBelow(p.ts); ok {
+				hadMVCCRangeTombstone = true
+				rkv.CloneInto(&p.savedRangeKeyVers)
+			}
+		}
+	}
+
 	var added bool
 	if p.processRangeKeys(true /* seeked */, false /* reverse */) {
 		if p.updateCurrent() {
 			_, added = p.getOne(ctx)
 		}
 	}
 	p.maybeFailOnMoreRecent()
-	// Unlike scans, if tombstones are enabled, we synthesize point tombstones for
-	// MVCC range tombstones even if there is no existing point key below it.
-	// These are often needed for e.g. conflict checks.
-	if p.tombstones && !added && p.err == nil {
-		if rkv, ok := p.coveredByRangeKey(hlc.MinTimestamp); ok {
-			p.addSynthetic(ctx, p.curRangeKeys.Bounds.Key, rkv)
-		}
+
+	// In tombstone mode, if there was no existing point key we may need to
+	// synthesize a point tombstone if we saved a range key before
+	// Unlike scans, if tombstones are enabled, we synthesize point tombstones
+	// for MVCC range tombstones even if there is no existing point key below
+	// it. These are often needed for e.g. conflict checks.
+	if p.tombstones && hadMVCCRangeTombstone && !added && p.err == nil {
+		p.addSynthetic(ctx, p.start, p.savedRangeKeyVers)
 	}
 }
 
@@ -1658,6 +1680,9 @@ func (p *pebbleMVCCScanner) iterNext() bool {
 		p.parent.Next()
 		// We don't need to process range key changes here, because curRangeKeys
 		// already contains the range keys at this position from before the peek.
+		if buildutil.CrdbTestBuild {
+			p.assertOwnedRangeKeys()
+		}
 		if !p.iterValid() {
 			return false
 		}
@@ -1834,3 +1859,16 @@ func (p *pebbleMVCCScanner) intentsRepr() []byte {
 	}
 	return p.intents.Repr()
 }
+
+// assertOwnedRangeKeys asserts that p.curRangeKeys is empty, or backed by
+// p.savedRangeKeys's buffers.
+func (p *pebbleMVCCScanner) assertOwnedRangeKeys() {
+	if p.curRangeKeys.IsEmpty() {
+		return
+	}
+	// NB: We compare on the EndKey in case the start key is /Min, the empty
+	// key.
+	if &p.curRangeKeys.Bounds.EndKey[0] != &p.savedRangeKeys.Bounds.EndKey[0] {
+		panic(errors.AssertionFailedf("current range keys are not scanner-owned"))
+	}
+}

pkg/storage/pebbleiter/BUILD.bazel

@@ -14,6 +14,7 @@ go_library(
     importpath = "github.com/cockroachdb/cockroach/pkg/storage/pebbleiter",
     visibility = ["//visibility:public"],
     deps = [
+        "//pkg/util",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_cockroachdb_pebble//:pebble",
     ],