db, valsep: do not separate mvcc values if DisableSeparationBySuffix is true

Move this field into the ValueStoragePolicy struct and make sure it's being read
at the time of value separation.

Author: xinhaoz

compaction.go

@@ -3438,7 +3438,7 @@ func (d *DB) compactAndWrite(
 			spanPolicyValid = true
 		}
 		writerOpts := d.makeWriterOptions(c.outputLevel.level)
-		if spanPolicy.DisableValueSeparationBySuffix {
+		if spanPolicy.ValueStoragePolicy.DisableSeparationBySuffix {
 			writerOpts.DisableValueBlocks = true
 		}
 		if spanPolicy.PreferFastCompression && writerOpts.Compression != block.NoCompression {
@@ -3454,7 +3454,8 @@ func (d *DB) compactAndWrite(
 			// This span of keyspace is more tolerant of latency, so set a more
 			// aggressive value separation policy for this output.
 			vSep.SetNextOutputConfig(valsep.ValueSeparationOutputConfig{
-				MinimumSize: spanPolicy.ValueStoragePolicy.MinimumSize,
+				MinimumSize:                    spanPolicy.ValueStoragePolicy.MinimumSize,
+				DisableValueSeparationBySuffix: spanPolicy.ValueStoragePolicy.DisableSeparationBySuffix,
 			})
 		}
 		objMeta, tw, err := d.newCompactionOutputTable(jobID, c, writerOpts)

compaction_test.go

@@ -1526,6 +1526,15 @@ func runCompactionTest(
 				for _, arg := range args {
 					parts := strings.Split(arg, "=")
 					switch parts[0] {
+					case "disable-separation-by-suffix":
+						if len(parts) != 1 {
+							td.Fatalf(t, "expected disable-separation-by-suffix with no value, got: %s", arg)
+						}
+						if policy.ValueStoragePolicy.PolicyAdjustment == NoValueSeparation {
+							td.Fatalf(t, "conflicting value storage policies for span: %s", line)
+						}
+						policy.ValueStoragePolicy.PolicyAdjustment = OverrideValueStorage
+						policy.ValueStoragePolicy.DisableSeparationBySuffix = true
 					case "val-sep-minimum-size":
 						if len(parts) != 2 {
 							td.Fatalf(t, "expected val-sep-minimum-size=<size>, got: %s", arg)
@@ -1540,6 +1549,8 @@ func runCompactionTest(
 						} else if int(size) != d.opts.Experimental.ValueSeparationPolicy().MinimumSize {
 							policy.ValueStoragePolicy.PolicyAdjustment = OverrideValueStorage
 						}
+					default:
+						td.Fatalf(t, "unknown span policy arg: %s", arg)
 					}
 				}
 				spanPolicies = append(spanPolicies, SpanAndPolicy{

data_test.go

@@ -1783,8 +1783,7 @@ func parseDBOptionsArgs(opts *Options, args []datadriven.CmdArg) error {
 				End:   []byte(cmdArg.Vals[1]),
 			}
 			policy := SpanPolicy{
-				DisableValueSeparationBySuffix: true,
-				ValueStoragePolicy:             ValueStorageLowReadLatency,
+				ValueStoragePolicy: ValueStorageLowReadLatency,
 			}
 			spanPolicies = append(spanPolicies, SpanAndPolicy{
 				KeyRange: span,

options.go

@@ -1283,15 +1283,6 @@ type SpanPolicy struct {
 	// amount to a significant amount of space.
 	PreferFastCompression bool
 
-	// DisableValueSeparationBySuffix disables discriminating KVs depending on
-	// suffix.
-	//
-	// Among a set of keys with the same prefix, Pebble's default heuristics
-	// optimize access to the KV with the smallest suffix. This is useful for MVCC
-	// keys (where the smallest suffix is the latest version), but should be
-	// disabled for keys where the suffix does not correspond to a version.
-	DisableValueSeparationBySuffix bool
-
 	// ValueStoragePolicy is a hint used to determine where to store the values
 	// for KVs.
 	ValueStoragePolicy ValueStoragePolicyAdjustment
@@ -1303,7 +1294,7 @@ func (p SpanPolicy) String() string {
 	if p.PreferFastCompression {
 		sb.WriteString("fast-compression,")
 	}
-	if p.DisableValueSeparationBySuffix {
+	if p.ValueStoragePolicy.DisableSeparationBySuffix {
 		sb.WriteString("disable-value-separation-by-suffix,")
 	}
 	switch p.ValueStoragePolicy.PolicyAdjustment {
@@ -1324,6 +1315,18 @@ type ValueStoragePolicyAdjustment struct {
 
 	// Remaining fields are ignored, unless the PolicyAdjustment is OverrideValueStorage.
 
+	// DisableSeparationBySuffix disables discriminating KVs depending on
+	// suffix.
+	//
+	// Among a set of keys with the same prefix, Pebble's default heuristics
+	// optimize access to the KV with the smallest suffix. This is useful for MVCC
+	// keys (where the smallest suffix is the latest version), but should be
+	// disabled for keys where the suffix does not correspond to a version.
+	//
+	// TODO(xinhaoz): Persist this setting when writing sstables so we can
+	// compare against current span policy configs during compaction to determine
+	// whether we can preserve blob references.
+	DisableSeparationBySuffix bool
 	// MinimumSize is the minimum size of the value.
 	MinimumSize int
 }

testdata/compaction/mvcc_garbage_blob

@@ -63,3 +63,40 @@ Blob files:
 flush-log
 ----
 [JOB 1] flushed 1 memtable (100B) to L0 [000010] (837B) blob [000011 (MVCCGarbage: 17%)] (102B), in 1.0s (1.0s total), output rate 837B/s
+
+
+# Another test with the same value separation config, but this time we set a span
+# policy for the span y-zz that disables value separation by suffix. This should
+# prevent MVCC garbage from being written to blob files.
+
+define value-separation=(enabled, min-size=5, max-ref-depth=3, garbage-ratios=1.0:1.0)
+----
+
+
+# Unspecified minimum value size should result in using the global min size of 5.
+set-span-policies
+y,zz disable-separation-by-suffix
+----
+
+
+batch
+set apple@3 a
+set apple@2 ab
+set yay@3 a
+set yay@2 ab
+set zoo@4 b
+set zoo@3 ba
+del zoo@2
+set zoo@2 bag
+set zoo@1 bah
+----
+
+# We expect 1 blob file to be created for the MVCC garbage values from  apple but not
+# for the values from y-z.
+flush
+----
+L0.0:
+  000005:[apple@3#10,SET-apple@2#11,SET] seqnums:[10-11] points:[apple@3#10,SET-apple@2#11,SET] size:809 blobrefs:[(B000006: 2); depth:1]
+  000007:[yay@3#12,SET-zoo@1#18,SET] seqnums:[12-18] points:[yay@3#12,SET-zoo@1#18,SET] size:768
+Blob files:
+  B000006 physical:{000006 size:[91 (91B)] vals:[2 (2B)]}

valsep/value_separation.go

@@ -15,9 +15,15 @@ import (
 // ValueSeparationOutputConfig is used to configure value separation for an
 // individual compaction output.
 type ValueSeparationOutputConfig struct {
-	// MinimumSize is the minimum size of a value that will be separated into a
-	// blob file.
+	// MinimumSize imposes a lower bound on the size of values that can be
+	// separated into a blob file. Values smaller than this are always written
+	// to the sstable (but may still be written to a value block within the
+	// sstable).
 	MinimumSize int
+	// DisableValueSeparationBySuffix indicates whether we should consider the
+	// KV suffix when separating values (e.g. for MVCC garbage). See
+	// pebble.ValueStoragePolicyAdjustment for details.
+	DisableValueSeparationBySuffix bool
 }
 
 // ValueSeparation defines an interface for writing some values to separate blob

valsep/value_separator.go

@@ -33,8 +33,11 @@ const (
 	rewriteAllHotBlobReferences
 )
 
-// ValueSeparator can function in any of the valueSeparationModes,
-// It will be extended in the future to support the writing of hot and cold
+// ValueSeparator can function in any of the valueSeparationModes to write
+// new or preserve blob references when writing an sstable. FinishOutput
+// should be called when the output sstable is complete. The ValueSeparator
+// can then be reused for the next output sstable.
+// This will be extended in the future to support the writing of hot and cold
 // blob files. All blob references are currently written to the hot tier.
 type ValueSeparator struct {
 	mode valueSeparationMode
@@ -48,24 +51,21 @@ type ValueSeparator struct {
 	shortAttrExtractor base.ShortAttributeExtractor
 	// writerOpts is used to configure all constructed blob writers.
 	writerOpts blob.FileWriterOptions
-	// minimumSize imposes a lower bound on the size of values that can be
-	// separated into a blob file. Values smaller than this are always written
-	// to the sstable (but may still be written to a value block within the
-	// sstable).
 	//
-	// minimumSize is set to globalMinimumSize by default and on every call to
-	// FinishOutput. It may be overriden by SetNextOutputConfig (i.e, if a
-	// SpanPolicy dictates a different minimum size for a span of the keyspace).
-	minimumSize int
-	// globalMinimumSize is the size threshold for separating values into blob
-	// files globally across the keyspace.
-	globalMinimumSize int
+	// currentConfig holds the configuration for the current output sstable.
+	// It is set to globalConfig by default and on every call to FinishOutput.
+	// It may be overridden by SetNextOutputConfig (i.e, if a SpanPolicy
+	// dictates a different minimum size for a span of the keyspace).
+	currentConfig ValueSeparationOutputConfig
+	// globalConfig holds settings that are applied globally across all outputs.
+	globalConfig ValueSeparationOutputConfig
 	// invalidValueCallback is called when a value is encountered for which the
 	// short attribute extractor returns an error.
 	invalidValueCallback func(userKey []byte, value []byte, err error)
 
 	// state.
 	buf []byte
+
 	// currPendingReferences holds the pending references that have been referenced by
 	// the current output sstable. The index of a reference with a given blob
 	// file ID is the value of the base.BlobReferenceID used by its value handles
@@ -109,22 +109,26 @@ func NewPreserveAllHotBlobReferences(
 	outputBlobReferenceDepth manifest.BlobReferenceDepth,
 	globalMinimumSize int,
 ) *ValueSeparator {
+	config := ValueSeparationOutputConfig{
+		MinimumSize: globalMinimumSize,
+	}
 	return &ValueSeparator{
 		mode:                     preserveAllHotBlobReferences,
 		inputBlobPhysicalFiles:   inputBlobPhysicalFiles,
 		outputBlobReferenceDepth: outputBlobReferenceDepth,
-		minimumSize:              globalMinimumSize,
-		globalMinimumSize:        globalMinimumSize,
+		globalConfig:             config,
+		currentConfig:            config,
 	}
 }
 
 type WriteNewBlobFilesOptions struct {
 	// InputBlobPhysicalFiles holds the *PhysicalBlobFile for every unique blob
 	// file referenced by input sstables. This may be nil if there are no input
 	// blob files to preserve.
-	InputBlobPhysicalFiles map[base.BlobFileID]*manifest.PhysicalBlobFile
-	ShortAttrExtractor     base.ShortAttributeExtractor
-	InvalidValueCallback   func(userKey []byte, value []byte, err error)
+	InputBlobPhysicalFiles         map[base.BlobFileID]*manifest.PhysicalBlobFile
+	ShortAttrExtractor             base.ShortAttributeExtractor
+	InvalidValueCallback           func(userKey []byte, value []byte, err error)
+	DisableValueSeparationBySuffix bool
 }
 
 func NewWriteNewBlobFiles(
@@ -138,6 +142,10 @@ func NewWriteNewBlobFiles(
 	if inputBlobPhysicalFiles == nil {
 		inputBlobPhysicalFiles = make(map[base.BlobFileID]*manifest.PhysicalBlobFile)
 	}
+	config := ValueSeparationOutputConfig{
+		MinimumSize:                    globalMinimumSize,
+		DisableValueSeparationBySuffix: opts.DisableValueSeparationBySuffix,
+	}
 	return &ValueSeparator{
 		mode:                     rewriteAllHotBlobReferences,
 		inputBlobPhysicalFiles:   inputBlobPhysicalFiles,
@@ -146,26 +154,31 @@ func NewWriteNewBlobFiles(
 		newBlobObject:            newBlobObject,
 		shortAttrExtractor:       opts.ShortAttrExtractor,
 		writerOpts:               writerOpts,
-		minimumSize:              globalMinimumSize,
-		globalMinimumSize:        globalMinimumSize,
+		globalConfig:             config,
+		currentConfig:            config,
 		invalidValueCallback:     opts.InvalidValueCallback,
 	}
 }
 
 // SetNextOutputConfig implements the ValueSeparation interface.
 func (vs *ValueSeparator) SetNextOutputConfig(config ValueSeparationOutputConfig) {
-	vs.minimumSize = config.MinimumSize
+	if config.MinimumSize == 0 {
+		// This indicates that MinimumSize was unset, so fall back
+		// to the global minimum size.
+		config.MinimumSize = vs.globalConfig.MinimumSize
+	}
+	vs.currentConfig = config
 }
 
 func (vs *ValueSeparator) Kind() sstable.ValueSeparationKind {
-	if vs.minimumSize != vs.globalMinimumSize {
+	if vs.currentConfig != vs.globalConfig {
 		return sstable.ValueSeparationSpanPolicy
 	}
 	return sstable.ValueSeparationDefault
 }
 
 func (vs *ValueSeparator) MinimumSize() int {
-	return vs.minimumSize
+	return vs.currentConfig.MinimumSize
 }
 
 // EstimatedFileSize returns an estimate of the disk space consumed by the current
@@ -241,9 +254,7 @@ func (vs *ValueSeparator) Add(
 
 	// Values that are too small are never separated; however, MVCC keys are
 	// separated if they are a SET key kind, as long as the value is not empty.
-	// TODO(xinhaoz): Handle the case where DisableValueSeparationBySuffix=true,
-	// for which do not want to separate MVCC garbage values.
-	if len(v) < vs.minimumSize && !isLikelyMVCCGarbage {
+	if len(v) < vs.currentConfig.MinimumSize && (vs.currentConfig.DisableValueSeparationBySuffix || !isLikelyMVCCGarbage) {
 		return tw.Add(kv.K, v, forceObsolete)
 	}
 
@@ -462,7 +473,7 @@ func (vs *ValueSeparator) FinishOutput() (ValueSeparationMetadata, error) {
 	referenceSize := vs.blobTiers[base.HotTier].totalPreservedValueSize + newReferencedValueSize
 
 	// Reset the minimum size for the next output.
-	vs.minimumSize = vs.globalMinimumSize
+	vs.currentConfig = vs.globalConfig
 	// Reset the remaining state.
 	vs.currPendingReferences = vs.currPendingReferences[:0]
 	for i := range vs.blobTiers {