storage: improve documentation for MVCCValue encoding

We also clean up some related code a bit.

Epic: none
Release note: None

Author: RaduBerinde

pkg/storage/mvcc.go

@@ -2742,7 +2742,7 @@ func mvccPutInternal(
 	}
 	newMeta.Timestamp = versionKey.Timestamp.ToLegacyTimestamp()
 	newMeta.KeyBytes = MVCCVersionTimestampSize
-	newMeta.ValBytes = int64(encodedMVCCValueSize(versionValue))
+	newMeta.ValBytes = int64(versionValue.encodedSize())
 	newMeta.Deleted = versionValue.IsTombstone()
 	newMeta.IntentHistory = newIntentHistory
 
@@ -5497,7 +5497,7 @@ func mvccResolveWriteIntent(
 			newMeta.Txn.WriteTimestamp = newTimestamp
 			newMeta.Timestamp = newTimestamp.ToLegacyTimestamp()
 			newMeta.KeyBytes = MVCCVersionTimestampSize
-			newMeta.ValBytes = int64(encodedMVCCValueSize(newValue))
+			newMeta.ValBytes = int64(newValue.encodedSize())
 			newMeta.Deleted = newValue.IsTombstone()
 
 			if err = writer.PutMVCC(newKey, newValue); err != nil {

pkg/storage/mvcc_value.go

@@ -36,27 +36,29 @@ const (
 //
 // The mvcc value has a "simple" and an "extended" encoding scheme, depending on
 // whether the value's header is empty or not. If the value's header is empty,
-// it is omitted in the encoding and the mvcc value's encoding is identical to
-// that of roachpb.Value. This provided backwards compatibility and ensures that
-// the MVCCValue optimizes away in the common case. If the value's header is not
-// empty, it is prepended to the roachpb.Value encoding. The encoding scheme's
-// variants are:
+// it is omitted in the encoding and the mvcc value's encoding is just that of
+// roachpb.Value. This provides backwards compatibility and ensures that the
+// MVCCValueHeader optimizes away in the common case. If the value's header is
+// not empty, it is prepended to the roachpb.Value encoding. The encoding
+// scheme's variants are:
 //
-// Simple (identical to the roachpb.Value encoding):
+// Simple (just the roachpb.Value encoding):
 //
 //	<4-byte-checksum><1-byte-tag><encoded-data>
 //
-// Extended (header prepended to roachpb.Value encoding):
+// Extended (header prepended to the roachpb.Value encoding):
 //
-//	<4-byte-header-len><1-byte-sentinel><mvcc-header><4-byte-checksum><1-byte-tag><encoded-data>
+//	<4-byte-header-len><1-byte-sentinel-tag><mvcc-header><4-byte-checksum><1-byte-tag><encoded-data>
+//	                                                     ^                                         ^
+//	                                                     \-------- roachpb.Value encoding ---------/
 //
 // The two encoding scheme variants are distinguished using the 5th byte, which
 // is either the roachpb.Value tag (which has many values) or a sentinel tag not
 // used by the roachpb.Value encoding which indicates the extended encoding
 // scheme.
 //
-// For a deletion tombstone, the encoding of roachpb.Value is special cased to
-// be empty, i.e., no checksum, tag, or encoded-data. In that case the extended
+// For a deletion tombstone, the encoding of roachpb.Value is special-cased to
+// be empty, i.e. no checksum, tag, or encoded-data. In that case the extended
 // encoding above is simply:
 //
 //	<4-byte-header-len><1-byte-sentinel><mvcc-header>
@@ -150,9 +152,9 @@ func EncodeMVCCValueForExport(mvccValue MVCCValue, b []byte) ([]byte, bool, erro
 	return EncodeMVCCValueToBuf(mvccValue, b)
 }
 
-// When running a metamorphic build, disable the simple MVCC value encoding to
-// prevent code from assuming that the MVCCValue encoding is identical to the
-// roachpb.Value encoding.
+// disableSimpleValueEncoding forces encoding of the MVCCValueHeader even when
+// it is empty (see MVCCValue). It is set metamorphically to extend testing
+// coverage.
 var disableSimpleValueEncoding = metamorphic.ConstantWithTestBool(
 	"mvcc-value-disable-simple-encoding", false)
 
@@ -171,9 +173,14 @@ func DisableMetamorphicSimpleValueEncoding(t interface {
 	}
 }
 
-// encodedMVCCValueSize returns the size of the MVCCValue when encoded.
-func encodedMVCCValueSize(v MVCCValue) int {
-	if v.MVCCValueHeader.IsEmpty() && !disableSimpleValueEncoding {
+//gcassert:inline
+func (v *MVCCValue) useSimpleEncoding() bool {
+	return v.MVCCValueHeader.IsEmpty() && !disableSimpleValueEncoding
+}
+
+// encodedSize returns the size of the MVCCValue when encoded.
+func (v *MVCCValue) encodedSize() int {
+	if v.useSimpleEncoding() {
 		return len(v.Value.RawBytes)
 	}
 	return extendedPreludeSize + v.MVCCValueHeader.Size() + len(v.Value.RawBytes)
@@ -204,7 +211,7 @@ func EncodeMVCCValue(v MVCCValue) ([]byte, error) {
 // negates the inlining gain. Reconsider this with Go 1.20. See:
 // https://github.com/cockroachdb/cockroach/issues/88818
 func EncodeMVCCValueToBuf(v MVCCValue, buf []byte) ([]byte, bool, error) {
-	if v.MVCCValueHeader.IsEmpty() && !disableSimpleValueEncoding {
+	if v.useSimpleEncoding() {
 		// Simple encoding. Use the roachpb.Value encoding directly with no
 		// modification. No need to re-allocate or copy.
 		return v.Value.RawBytes, false, nil
@@ -243,21 +250,14 @@ func EncodeMVCCValueToBuf(v MVCCValue, buf []byte) ([]byte, bool, error) {
 	return buf, true, nil
 }
 
-func mvccValueSize(v MVCCValue) (size int, extendedEncoding bool) {
-	if v.MVCCValueHeader.IsEmpty() && !disableSimpleValueEncoding {
-		return len(v.Value.RawBytes), false
-	}
-	return extendedPreludeSize + v.MVCCValueHeader.Size() + len(v.Value.RawBytes), true
-}
-
 // encodeExtendedMVCCValueToSizedBuf encodes an MVCCValue into its encoded form
 // in the provided buffer. The provided buf must be exactly sized, matching the
 // value returned by MVCCValue.encodedMVCCValueSize.
 //
 // See EncodeMVCCValueToBuf for detailed comments on the encoding scheme.
 func encodeExtendedMVCCValueToSizedBuf(v MVCCValue, buf []byte) error {
 	if buildutil.CrdbTestBuild {
-		if sz := encodedMVCCValueSize(v); sz != len(buf) {
+		if sz := v.encodedSize(); sz != len(buf) {
 			panic(errors.AssertionFailedf("provided buf (len=%d) is not sized correctly; expected %d", len(buf), sz))
 		}
 	}

pkg/storage/mvcc_value_test.go

@@ -219,7 +219,7 @@ func TestEncodeDecodeMVCCValue(t *testing.T) {
 			enc, err := EncodeMVCCValue(tc.val)
 			require.NoError(t, err)
 			fmt.Fprintf(&buf, "encoded: %x", enc)
-			assert.Equal(t, encodedMVCCValueSize(tc.val), len(enc))
+			assert.Equal(t, tc.val.encodedSize(), len(enc))
 
 			dec, err := DecodeMVCCValue(enc)
 			require.NoError(t, err)
@@ -239,7 +239,7 @@ func TestEncodeDecodeMVCCValue(t *testing.T) {
 		t.Run("DeocdeValueFromMVCCValue/"+name, func(t *testing.T) {
 			enc, err := EncodeMVCCValue(tc.val)
 			require.NoError(t, err)
-			assert.Equal(t, encodedMVCCValueSize(tc.val), len(enc))
+			assert.Equal(t, tc.val.encodedSize(), len(enc))
 
 			dec, err := DecodeValueFromMVCCValue(enc)
 			require.NoError(t, err)

pkg/storage/pebble_batch.go

@@ -15,6 +15,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/storage/fs"
 	"github.com/cockroachdb/cockroach/pkg/storage/mvccencoding"
 	"github.com/cockroachdb/cockroach/pkg/storage/pebbleiter"
+	"github.com/cockroachdb/cockroach/pkg/util/buildutil"
 	"github.com/cockroachdb/errors"
 	"github.com/cockroachdb/pebble"
 	"github.com/cockroachdb/pebble/rangekey"
@@ -307,21 +308,25 @@ func (wb *writeBatch) putMVCC(key MVCCKey, value MVCCValue) error {
 	// - encode the MVCC key and MVCC value directly into the Batch
 	// - call Finish on the deferred operation (which will index the key if
 	//   wb.batch is indexed)
-	valueLen, isExtended := mvccValueSize(value)
 	keyLen := mvccencoding.EncodedMVCCKeyLength(key.Key, key.Timestamp)
-	o := wb.batch.SetDeferred(keyLen, valueLen)
-	mvccencoding.EncodeMVCCKeyToBufSized(o.Key, key.Key, key.Timestamp, keyLen)
-	if !isExtended {
+	var o *pebble.DeferredBatchOp
+	if value.useSimpleEncoding() {
 		// Fast path; we don't need to use the extended encoding and can copy
 		// RawBytes in verbatim.
+		o = wb.batch.SetDeferred(keyLen, len(value.Value.RawBytes))
 		copy(o.Value, value.Value.RawBytes)
 	} else {
-		// Slow path; we need the MVCC value header.
-		err := encodeExtendedMVCCValueToSizedBuf(value, o.Value)
-		if err != nil {
+		// Slow path; we need the MVCC value header. Inline the relevant part of encodedSize().
+		valueLen := extendedPreludeSize + value.MVCCValueHeader.Size() + len(value.Value.RawBytes)
+		if buildutil.CrdbTestBuild && valueLen != value.encodedSize() {
+			panic("incorrect valueLen calculation")
+		}
+		o = wb.batch.SetDeferred(keyLen, valueLen)
+		if err := encodeExtendedMVCCValueToSizedBuf(value, o.Value); err != nil {
 			return err
 		}
 	}
+	mvccencoding.EncodeMVCCKeyToBufSized(o.Key, key.Key, key.Timestamp, keyLen)
 	return o.Finish()
 }