sstable: in props, remove ValueSeparationKind, add ValueSeparationBySuffixDisabled

Remove ValueSeparationKind, which was originally added for compaction decisions
but was not necessary in the end.

Add the property ValueSeparationBySuffixDisabled, which indicates if the table
used special value separation policies for mvcc garbage to inform value
separation decisions during compactions.

Author: xinhaoz

compaction_value_separation.go

@@ -123,31 +123,39 @@ func shouldWriteBlobFiles(
 				continue
 			}
 
-			var expectedMinSize int
+			// Set expected policy to global policy values to start, and
+			// extract the expected values from the span policy.
+			expectedMinSize := policy.MinimumSize
+			expectedValSepBySuffixDisabled := false
 			bounds := t.UserKeyBounds()
 			spanPolicy, spanPolicyEndKey, err := spanPolicyFunc(bounds.Start)
-			if err != nil {
-				// For now, if we can't determine the span policy, we should just assume
-				// the default policy is in effect for this table.
-				expectedMinSize = policy.MinimumSize
-			} else {
+			// For now, if we can't determine the span policy, we should just assume
+			// the default policy is in effect for this table.
+			if err == nil {
 				if len(spanPolicyEndKey) > 0 && cmp(bounds.End.Key, spanPolicyEndKey) >= 0 {
 					// The table's key range now uses multiple span policies. Rewrite to new
 					// blob files so values are stored according to the current policy.
 					return true, 0
 				}
 				switch spanPolicy.ValueStoragePolicy.PolicyAdjustment {
 				case UseDefaultValueStorage:
-					// Use the global policy's minimum size.
-					expectedMinSize = policy.MinimumSize
+					// Use the global policy.
 				case OverrideValueStorage:
 					expectedMinSize = spanPolicy.ValueStoragePolicy.MinimumSize
+					if expectedMinSize == 0 {
+						// A 0 minimum value size on the span policy indicates the field
+						// was unset, but other parts of value separation are being
+						// overridden. Use the default min size.
+						expectedMinSize = policy.MinimumSize
+					}
+					expectedValSepBySuffixDisabled = spanPolicy.ValueStoragePolicy.DisableSeparationBySuffix
 				case NoValueSeparation:
 					expectedMinSize = 0
 				}
 			}
 
-			if int(backingProps.ValueSeparationMinSize) != expectedMinSize {
+			if int(backingProps.ValueSeparationMinSize) != expectedMinSize ||
+				(expectedMinSize > 0 && backingProps.ValueSeparationBySuffixDisabled != expectedValSepBySuffixDisabled) {
 				return true, 0
 			}
 		}

data_test.go

@@ -1939,14 +1939,11 @@ var _ valsep.ValueSeparation = (*defineDBValueSeparator)(nil)
 // SetNextOutputConfig implements the valsep.ValueSeparation interface.
 func (vs *defineDBValueSeparator) SetNextOutputConfig(config valsep.ValueSeparationOutputConfig) {}
 
-// Kind implements the ValueSeparation interface.
-func (vs *defineDBValueSeparator) Kind() sstable.ValueSeparationKind {
-	return vs.pbr.Kind()
+// OutputConfig implements the valsep.ValueSeparation interface.
+func (vs *defineDBValueSeparator) OutputConfig() valsep.ValueSeparationOutputConfig {
+	return vs.pbr.OutputConfig()
 }
 
-// MinimumSize implements the ValueSeparation interface.
-func (vs *defineDBValueSeparator) MinimumSize() int { return vs.pbr.MinimumSize() }
-
 // EstimatedFileSize returns an estimate of the disk space consumed by the current
 // blob file if it were closed now.
 func (vs *defineDBValueSeparator) EstimatedFileSize() uint64 {

internal/compact/run.go

@@ -190,8 +190,9 @@ func (r *Runner) WriteTable(
 		panic("error already encountered")
 	}
 
-	// Set the value separation kind on the writer based on the strategy being used.
-	tw.SetValueSeparationProps(valueSeparation.Kind(), uint64(valueSeparation.MinimumSize()))
+	// Set the value separation props on the writer.
+	config := valueSeparation.OutputConfig()
+	tw.SetValueSeparationProps(uint64(config.MinimumSize), config.DisableValueSeparationBySuffix)
 
 	r.tables = append(r.tables, OutputTable{
 		CreationTime: time.Now(),

internal/manifest/table_metadata.go

@@ -414,12 +414,14 @@ type TableBackingProperties struct {
 
 	CompressionStats block.CompressionStats
 
-	// ValueSeparationKind is the value separation policy used when writing the table.
-	ValueSeparationKind sstable.ValueSeparationKind
 	// ValueSeparationMinSize is the minimum value size for which values were
 	// separated when writing the table. This value is 0 if the policy used
 	// does not write blob files.
 	ValueSeparationMinSize uint64
+	// ValueSeparationBySuffixDisabled indicates if disabled applying special
+	// value separation rules by KV suffix when writing the table. Note that
+	// if value separation was disabled, this field is not meaningful.
+	ValueSeparationBySuffixDisabled bool
 }
 
 // NumPointDeletions is the number of point deletions in the sstable. For virtual
@@ -443,19 +445,19 @@ func (b *TableBacking) Properties() (_ *TableBackingProperties, ok bool) {
 // TableBacking.
 func (b *TableBacking) PopulateProperties(props *sstable.Properties) *TableBackingProperties {
 	b.props = TableBackingProperties{
-		NumEntries:                 props.NumEntries,
-		RawKeySize:                 props.RawKeySize,
-		RawValueSize:               props.RawValueSize,
-		RawPointTombstoneKeySize:   props.RawPointTombstoneKeySize,
-		RawPointTombstoneValueSize: props.RawPointTombstoneValueSize,
-		NumSizedDeletions:          props.NumSizedDeletions,
-		NumDeletions:               props.NumDeletions,
-		NumRangeDeletions:          props.NumRangeDeletions,
-		NumRangeKeyDels:            props.NumRangeKeyDels,
-		NumRangeKeySets:            props.NumRangeKeySets,
-		ValueBlocksSize:            props.ValueBlocksSize,
-		ValueSeparationKind:        sstable.ValueSeparationKind(props.ValueSeparationKind),
-		ValueSeparationMinSize:     props.ValueSeparationMinSize,
+		NumEntries:                      props.NumEntries,
+		RawKeySize:                      props.RawKeySize,
+		RawValueSize:                    props.RawValueSize,
+		RawPointTombstoneKeySize:        props.RawPointTombstoneKeySize,
+		RawPointTombstoneValueSize:      props.RawPointTombstoneValueSize,
+		NumSizedDeletions:               props.NumSizedDeletions,
+		NumDeletions:                    props.NumDeletions,
+		NumRangeDeletions:               props.NumRangeDeletions,
+		NumRangeKeyDels:                 props.NumRangeKeyDels,
+		NumRangeKeySets:                 props.NumRangeKeySets,
+		ValueBlocksSize:                 props.ValueBlocksSize,
+		ValueSeparationMinSize:          props.ValueSeparationMinSize,
+		ValueSeparationBySuffixDisabled: props.ValueSeparationBySuffixDisabled,
 	}
 	if props.NumDataBlocks != 0 {
 		b.props.TombstoneDenseBlocksRatio = float64(props.NumTombstoneDenseBlocks) / float64(props.NumDataBlocks)

options.go

@@ -1322,10 +1322,6 @@ type ValueStoragePolicyAdjustment struct {
 	// 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

replay/testdata/corpus/simple_val_sep

@@ -103,6 +103,6 @@ stat simple_val_sep/MANIFEST-000013 simple_val_sep/000015.sst simple_val_sep/000
 simple_val_sep/MANIFEST-000013:
   size: 250
 simple_val_sep/000015.sst:
-  size: 802
+  size: 792
 simple_val_sep/000016.blob:
   size: 97

replay/testdata/replay_val_sep

@@ -5,14 +5,14 @@ tree
 ----
           /
             build/
-     829      000005.sst
+     819      000005.sst
      101      000006.blob
-     808      000008.sst
+     798      000008.sst
       97      000009.blob
       92      000011.log
-     707      000012.sst
+     687      000012.sst
       63      000014.log
-     802      000015.sst
+     792      000015.sst
       97      000016.blob
        0      LOCK
      152      MANIFEST-000010
@@ -21,16 +21,16 @@ tree
        0      marker.format-version.000011.024
        0      marker.manifest.000003.MANIFEST-000013
             simple_val_sep/
-     802      000015.sst
+     792      000015.sst
       97      000016.blob
      250      MANIFEST-000013
               checkpoint/
-     829        000005.sst
+     819        000005.sst
      101        000006.blob
-     808        000008.sst
+     798        000008.sst
       97        000009.blob
       11        000011.log
-     707        000012.sst
+     687        000012.sst
      187        MANIFEST-000013
     2911        OPTIONS-000002
        0        marker.format-version.000001.024

sstable/colblk_writer.go

@@ -1273,9 +1273,11 @@ func (w *RawColumnWriter) copyProperties(props Properties) {
 }
 
 // SetValueSeparationProps implements RawWriter.
-func (w *RawColumnWriter) SetValueSeparationProps(kind ValueSeparationKind, minValueSize uint64) {
-	w.props.ValueSeparationKind = uint8(kind)
+func (w *RawColumnWriter) SetValueSeparationProps(
+	minValueSize uint64, disableSeparationBySuffix bool,
+) {
 	w.props.ValueSeparationMinSize = minValueSize
+	w.props.ValueSeparationBySuffixDisabled = disableSeparationBySuffix
 }
 
 // IsLikelyMVCCGarbage determines whether the given user key is likely MVCC

sstable/properties.go

@@ -122,13 +122,12 @@ type Properties struct {
 	// The compression statistics encoded as a string. The format is:
 	// "<setting1>:<compressed1>/<uncompressed1>,<setting2>:<compressed2>/<uncompressed2>,..."
 	CompressionStats string `prop:"pebble.compression_stats"`
-	// The value separation policy that was used when writing this sstable.
-	// This indicates how values were handled during writing (separated into
-	// blob files, preserved references, or kept in-place). See ValueSeparationKind type
-	// for possible values.
-	ValueSeparationKind uint8 `prop:"pebble.value-separation.kind"`
 	// The minimum size a value must be to be separated into a blob file during writing.
 	ValueSeparationMinSize uint64 `prop:"pebble.value-separation.min-size"`
+	// ValueSeparationBySuffixDisabled indicates if special value separation rules were
+	// applied based on the KV suffix when writing. Pebble attempts to optimize writing of
+	// MVCC garbage values into blob files, which are recognized by the key suffix.
+	ValueSeparationBySuffixDisabled bool `prop:"pebble.value-separation.by-suffix.disabled"`
 	// User collected properties. Currently, we only use them to store block
 	// properties aggregated at the table level.
 	UserProperties map[string]string
@@ -231,25 +230,6 @@ func (p *Properties) toAttributes() Attributes {
 	return attributes
 }
 
-// ValueSeparationKind indicates the value separation policy that was used
-// when writing an sstable.
-type ValueSeparationKind uint8
-
-const (
-	// ValueSeparationNone indicates that no value separation was used.
-	// All values are stored in-place within the sstable.
-	ValueSeparationNone ValueSeparationKind = iota
-
-	// ValueSeparationDefault indicates that values were separated into
-	// new blob files during writing using the default value separation
-	// policy settings for minimum sizes.
-	ValueSeparationDefault
-
-	// ValueSeparationSpanPolicy indicates that values were separated
-	// based on a span policy during writing.
-	ValueSeparationSpanPolicy
-)
-
 var (
 	singleZeroSlice = []byte{0x00}
 	maxInt32Slice   = binary.AppendUvarint([]byte(nil), math.MaxInt32)