db: misc LazyValue tweaks, for testing, and simpler Clone invariant

- Fix metamorphic test to actually test value blocks.
- Do buffer mangling when retrieving from valueBlockReader to ensure
  callers are not relying on excessive memory stability.
- Relax the invariant of LazyValue.Clone to allow cloning even if the
  value has been fetched.

Informs cockroachdb#1170

Epic: CRDB-20378

internal/base/lazy_value.go

@@ -69,12 +69,13 @@ type AttributeAndLen struct {
 //
 // A LazyValue returned by an InternalIterator or Iterator is unstable in that
 // repositioning the iterator will invalidate the memory inside it. A caller
-// wishing to maintain that LazyValue needs to call LazyValue.Clone(). Note that
-// this does not fetch the value if it is not in-place. Additionally, Clone()
-// *must* only be called if LazyValue.Value() has *not* been called, since (a)
-// it only makes a promise about stability of LazyValue and not the underlying
-// value, (b) if LazyValue.Value() has been called already, the caller has the
-// value []byte slice so it is unnecessary to fiddle around with LazyValue.
+// wishing to maintain that LazyValue needs to call LazyValue.Clone(). Note
+// that this does not fetch the value if it is not in-place. Clone() should
+// ideally not be called if LazyValue.Value() has been called, since the
+// cloned LazyValue will forget the extracted/fetched value, and calling
+// Value() on this clone will cause the value to be extracted again. That is,
+// Clone() does not make any promise about the memory stability of the
+// underlying value.
 //
 // A user of an iterator that calls LazyValue.Value() wants as much as
 // possible for the returned value []byte to point to iterator owned memory.
@@ -246,21 +247,28 @@ func (lv *LazyValue) TryGetShortAttribute() (ShortAttribute, bool) {
 // not viable since we have no code trigger for returning to the pool
 // (LazyValues are simply GC'd).
 //
-// REQUIRES: LazyValue.Value() has not been called.
-// Ensure that this is only called before LazyValue.Value is called. We don't
-// actually care if it was in-place but we don't want to complicate behavior
-// regarding whether we need to clone the value inside the fetcher.
+// NB: It is highly preferable that LazyValue.Value() has not been called,
+// since the Clone will forget any previously extracted value, and a future
+// call to Value will cause it to be fetched again. We do this since we don't
+// want to reason about whether or not to clone an already extracted value
+// inside the Fetcher (we don't). Property P1 applies here too: if lv1.Value()
+// has been called, and then lv2 is created as a clone of lv1, then calling
+// lv2.Value() can invalidate any backing memory maintained inside the fetcher
+// for lv1 (even though these are the same values). We initially prohibited
+// calling LazyValue.Clone() if LazyValue.Value() has been called, but there
+// is at least one complex caller (pebbleMVCCScanner inside CockroachDB) where
+// it is not easy to prove this invariant.
 func (lv *LazyValue) Clone(buf []byte, fetcher *LazyFetcher) (LazyValue, []byte) {
-	if invariants.Enabled && lv.Fetcher != nil && lv.Fetcher.fetched {
-		panic("value has already been fetched")
-	}
-	// INVARIANT: LazyFetcher.value is nil, so there is nothing to copy there.
-	vLen := len(lv.ValueOrHandle)
 	var lvCopy LazyValue
 	if lv.Fetcher != nil {
-		*fetcher = *lv.Fetcher
+		*fetcher = LazyFetcher{
+			Fetcher:   lv.Fetcher.Fetcher,
+			Attribute: lv.Fetcher.Attribute,
+			// Not copying anything that has been extracted.
+		}
 		lvCopy.Fetcher = fetcher
 	}
+	vLen := len(lv.ValueOrHandle)
 	if vLen == 0 {
 		return lvCopy, buf
 	}
@@ -274,13 +282,3 @@ func (lv *LazyValue) Clone(buf []byte, fetcher *LazyFetcher) (LazyValue, []byte)
 func MakeInPlaceValue(val []byte) LazyValue {
 	return LazyValue{ValueOrHandle: val}
 }
-
-// TODO(sumeer): test that callers are adhering to promise P1. In the sstable
-// iterators, when invariants are enabled, create a testingFetcher struct that
-// contains the sstable name and the []byte backing for the last fetched
-// value. The sstable iterator, instead of returning an in-place value, will
-// return a copy of the key in ValueOrHandle, and a ValueFetcher implemented
-// by testingFetcher.valueFetcher. Each time this method is called, it will
-// mangle the last fetched value []byte slice, allocate a new one, open the
-// sstable, seek to the key, and copy the value into this new byte slice and
-// return.

internal/metamorphic/options.go

@@ -61,6 +61,7 @@ func parseOptions(opts *testOptions, data string) error {
 				opts.opts.BlockPropertyCollectors = blockPropertyCollectorConstructors
 				return true
 			case "TestOptions.enable_value_blocks":
+				opts.enableValueBlocks = true
 				opts.opts.Experimental.EnableValueBlocks = func() bool { return true }
 				return true
 			default:
@@ -98,6 +99,9 @@ func optionsToString(opts *testOptions) string {
 	if opts.useBlockPropertyCollector {
 		fmt.Fprintf(&buf, "  use_block_property_collector=%t\n", opts.useBlockPropertyCollector)
 	}
+	if opts.enableValueBlocks {
+		fmt.Fprintf(&buf, "  enable_value_blocks=%t\n", opts.enableValueBlocks)
+	}
 
 	s := opts.opts.String()
 	if buf.Len() == 0 {
@@ -145,6 +149,8 @@ type testOptions struct {
 	// Use a block property collector, which may be used by block property
 	// filters.
 	useBlockPropertyCollector bool
+	// Enable the use of value blocks.
+	enableValueBlocks bool
 }
 
 func standardOptions() []*testOptions {
@@ -304,9 +310,7 @@ func randomOptions(rng *rand.Rand) *testOptions {
 	// FormatMajorVersion than pebble.FormatRangeKeys.
 	opts.FormatMajorVersion = pebble.FormatRangeKeys
 	n := int(pebble.FormatNewest - opts.FormatMajorVersion)
-	if n > 0 {
-		opts.FormatMajorVersion += pebble.FormatMajorVersion(rng.Intn(n))
-	}
+	opts.FormatMajorVersion += pebble.FormatMajorVersion(rng.Intn(n + 1))
 	opts.Experimental.L0CompactionConcurrency = 1 + rng.Intn(4)    // 1-4
 	opts.Experimental.LevelMultiplier = 5 << rng.Intn(7)           // 5 - 320
 	opts.Experimental.MinDeletionRate = 1 << uint(20+rng.Intn(10)) // 1MB - 1GB
@@ -368,6 +372,11 @@ func randomOptions(rng *rand.Rand) *testOptions {
 	if testOpts.useBlockPropertyCollector {
 		testOpts.opts.BlockPropertyCollectors = blockPropertyCollectorConstructors
 	}
+	testOpts.enableValueBlocks = opts.FormatMajorVersion >= pebble.FormatSSTableValueBlocks &&
+		rng.Intn(2) != 0
+	if testOpts.enableValueBlocks {
+		testOpts.opts.Experimental.EnableValueBlocks = func() bool { return true }
+	}
 	return testOpts
 }
 

sstable/value_block.go

@@ -727,6 +727,7 @@ type valueBlockReader struct {
 	valueCache    cache.Handle
 	lazyFetcher   base.LazyFetcher
 	closed        bool
+	bufToMangle   []byte
 }
 
 func (r *valueBlockReader) getLazyValueForPrefixAndValueHandle(handle []byte) base.LazyValue {
@@ -774,6 +775,9 @@ func (r *valueBlockReader) Fetch(
 ) (val []byte, callerOwned bool, err error) {
 	if !r.closed {
 		val, err := r.getValueInternal(handle, valLen)
+		if invariants.Enabled {
+			val = r.doValueMangling(val)
+		}
 		return val, false, err
 	}
 
@@ -790,13 +794,27 @@ func (r *valueBlockReader) Fetch(
 	if err != nil {
 		return nil, false, err
 	}
-	if invariants.Enabled && callerOwned {
-		panic("callerOwned must be false")
-	}
 	buf = append(buf[:0], v...)
 	return buf, true, nil
 }
 
+// doValueMangling attempts to uncover violations of the contract listed in
+// the declaration comment of LazyValue. It is expensive, hence only called
+// when invariants.Enabled.
+func (r *valueBlockReader) doValueMangling(v []byte) []byte {
+	// Randomly set the bytes in the previous retrieved value to 0, since
+	// property P1 only requires the valueBlockReader to maintain the memory of
+	// one fetched value.
+	if rand.Intn(2) == 0 {
+		for i := range r.bufToMangle {
+			r.bufToMangle[i] = 0
+		}
+	}
+	// Store the current value in a new buffer for future mangling.
+	r.bufToMangle = append([]byte(nil), v...)
+	return r.bufToMangle
+}
+
 func (r *valueBlockReader) getValueInternal(handle []byte, valLen int32) (val []byte, err error) {
 	vh := decodeRemainingValueHandle(handle)
 	vh.valueLen = uint32(valLen)