storage: avoid eager MVCC scanner value retrieval

Previously pebbleMVCCScanner.getOne eagerly retrieved and decoded values of KVs
it considered. If the scanner doesn't need to check for uncertainty, this was
unnecessary. Additionally, future work may avoid the value retrieval even when
performing uncertainty checks if the value is known to not encode a local timestamp.

Epic: none
Informs #144715.
Release note: none

Author: jbowens

pkg/storage/pebble_mvcc_scanner.go

@@ -452,15 +452,16 @@ 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
-	savedRangeKeyVers MVCCRangeKeyVersion
-	results           results
-	intents           pebble.Batch
+	curUnsafeKey       MVCCKey
+	curRawKey          []byte
+	curUnsafeValue     MVCCValue
+	curRawValue        pebble.LazyValue
+	curRawValueFetched []byte
+	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
@@ -832,31 +833,14 @@ func (p *pebbleMVCCScanner) getOne(ctx context.Context) (ok, added bool) {
 			return p.addSynthetic(ctx, p.curUnsafeKey.Key, rkv)
 		}
 
-		// We are eagerly fetching and decoding the value, even though it may be
-		// too recent. With some care, this could be optimized to be lazy.
-		v, valid := p.getFromLazyValue()
-		if !valid {
-			return false, false
-		}
-
-		uncertaintyCheckRequired := p.checkUncertainty && !p.curUnsafeKey.Timestamp.LessEq(p.ts)
-		if !p.mvccHeaderRequired(uncertaintyCheckRequired) {
-			if !p.decodeCurrentValueIgnoringHeader(v) {
-				return false, false
-			}
-		} else if extended, valid := p.tryDecodeCurrentValueSimple(v); !valid {
-			return false, false
-		} else if extended {
-			if !p.decodeCurrentValueExtended(v) {
-				return false, false
-			}
-		}
-
 		// ts < read_ts
 		if p.curUnsafeKey.Timestamp.Less(p.ts) {
 			// 1. Fast path: there is no intent and our read timestamp is newer
 			// than the most recent version's timestamp.
-			return p.add(ctx, p.curUnsafeKey.Key, p.curRawKey, p.curUnsafeValue.Value.RawBytes, v)
+			if !p.decodeCurrentValue(p.decodeMVCCHeaders) {
+				return false, false
+			}
+			return p.add(ctx, p.curUnsafeKey.Key, p.curRawKey, p.curUnsafeValue.Value.RawBytes, p.curRawValueFetched)
 		}
 
 		// ts == read_ts
@@ -889,7 +873,10 @@ func (p *pebbleMVCCScanner) getOne(ctx context.Context) (ok, added bool) {
 
 			// 3. There is no intent and our read timestamp is equal to the most
 			// recent version's timestamp.
-			return p.add(ctx, p.curUnsafeKey.Key, p.curRawKey, p.curUnsafeValue.Value.RawBytes, v)
+			if !p.decodeCurrentValue(p.decodeMVCCHeaders) {
+				return false, false
+			}
+			return p.add(ctx, p.curUnsafeKey.Key, p.curRawKey, p.curUnsafeValue.Value.RawBytes, p.curRawValueFetched)
 		}
 
 		// ts > read_ts
@@ -923,6 +910,9 @@ func (p *pebbleMVCCScanner) getOne(ctx context.Context) (ok, added bool) {
 			// 5. Our txn's read timestamp is less than the max timestamp
 			// seen by the txn. We need to check for clock uncertainty
 			// errors.
+			if !p.decodeCurrentValue(true /* requireHeader */) {
+				return false, false
+			}
 			localTS := p.curUnsafeValue.GetLocalTimestamp(p.curUnsafeKey.Timestamp)
 			if p.uncertainty.IsUncertain(p.curUnsafeKey.Timestamp, localTS) {
 				return p.uncertaintyError(p.curUnsafeKey.Timestamp, localTS), false
@@ -1261,6 +1251,16 @@ func IncludeStartKeyIntoErr(startKey roachpb.Key, err error) error {
 func (p *pebbleMVCCScanner) add(
 	ctx context.Context, key roachpb.Key, rawKey []byte, rawValue []byte, mvccRawBytes []byte,
 ) (ok, added bool) {
+	// TODO(jackson): In some cases, the current code is still too eager with
+	// value retrieval and decoding the value into a MVCCValue:
+	//
+	// - If p.skipLocked, we could check if the key is locked and skip over the
+	//   KV without ever retrieving the value. It's unclear how common this is.
+	// - If p.rawMVCCValues is true, we could skip decoding the value into a
+	//   MVCCValue. If p.tombstones is true, we would still need to use the
+	//   iterator's MVCCValueLenAndIsTombstone() method to determine if the
+	//   value is a tombstone we should skip over.
+
 	// Don't include deleted versions len(val) == 0, unless we've been instructed
 	// to include tombstones in the results.
 	if len(rawValue) == 0 && !p.tombstones {
@@ -1601,9 +1601,33 @@ func (p *pebbleMVCCScanner) getFromLazyValue() (v []byte, valid bool) {
 	if callerOwned {
 		p.lazyValueBuf = v[:0]
 	}
+	p.curRawValueFetched = v
 	return v, true
 }
 
+func (p *pebbleMVCCScanner) decodeCurrentValue(requireHeader bool) (valid bool) {
+	v, callerOwned, err := p.curRawValue.Value(p.lazyValueBuf)
+	if err != nil {
+		p.err = err
+		return false
+	}
+	if callerOwned {
+		p.lazyValueBuf = v[:0]
+	}
+	p.curRawValueFetched = v
+
+	if !requireHeader {
+		return p.decodeCurrentValueIgnoringHeader(p.curRawValueFetched)
+	}
+	extended, valid := p.tryDecodeCurrentValueSimple(p.curRawValueFetched)
+	if !valid {
+		return false
+	} else if !extended {
+		return true
+	}
+	return p.decodeCurrentValueExtended(p.curRawValueFetched)
+}
+
 func (p *pebbleMVCCScanner) decodeCurrentMetadata() bool {
 	val, valid := p.getFromLazyValue()
 	if !valid {