db: disable writing to value blocks if DisableSeparationBySuffix is true

Make sure this field, which has been moved from SpanPolicy to
ValueStoragePolicy, is read for the DisableValueBlocks option in the sst writer.

Author: xinhaoz

compaction.go

@@ -3479,16 +3479,11 @@ func (d *DB) compactAndWrite(
 			writerOpts.Compression = block.FastestCompression
 		}
 		vSep := valueSeparation
-		switch spanPolicy.ValueStoragePolicy.PolicyAdjustment {
-		case UseDefaultValueStorage:
-			// No change to value separation.
-		case NoValueSeparation:
+		if spanPolicy.ValueStoragePolicy.DisableBlobSeparation {
 			vSep = valsep.NeverSeparateValues{}
-		case OverrideValueStorage:
-			// This span of keyspace is more tolerant of latency, so set a more
-			// aggressive value separation policy for this output.
+		} else if spanPolicy.ValueStoragePolicy.ContainsOverrides() {
 			vSep.SetNextOutputConfig(valsep.ValueSeparationOutputConfig{
-				MinimumSize:                    spanPolicy.ValueStoragePolicy.MinimumSize,
+				MinimumSize:                    spanPolicy.ValueStoragePolicy.OverrideBlobSeparationMinimumSize,
 				DisableValueSeparationBySuffix: spanPolicy.ValueStoragePolicy.DisableSeparationBySuffix,
 			})
 		}

compaction_test.go

@@ -1530,24 +1530,18 @@ func runCompactionTest(
 						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)
 						}
 						size, err := strconv.ParseUint(parts[1], 10, 64)
 						if err != nil {
-							td.Fatalf(t, "parsing value-minimum-size: %s", err)
+							td.Fatalf(t, "parsing val-sep-minimum-size: %s", err)
 						}
-						policy.ValueStoragePolicy.MinimumSize = int(size)
+						policy.ValueStoragePolicy.OverrideBlobSeparationMinimumSize = int(size)
 						if size == 0 {
-							policy.ValueStoragePolicy.PolicyAdjustment = NoValueSeparation
-						} else if int(size) != d.opts.Experimental.ValueSeparationPolicy().MinimumSize {
-							policy.ValueStoragePolicy.PolicyAdjustment = OverrideValueStorage
+							policy.ValueStoragePolicy.DisableBlobSeparation = true
 						}
 					default:
 						td.Fatalf(t, "unknown span policy arg: %s", arg)

compaction_value_separation.go

@@ -137,20 +137,17 @@ func shouldWriteBlobFiles(
 					// blob files so values are stored according to the current policy.
 					return true, 0
 				}
-				switch spanPolicy.ValueStoragePolicy.PolicyAdjustment {
-				case UseDefaultValueStorage:
-					// Use the global policy.
-				case OverrideValueStorage:
-					expectedMinSize = spanPolicy.ValueStoragePolicy.MinimumSize
+				if spanPolicy.ValueStoragePolicy.DisableBlobSeparation {
+					expectedMinSize = 0
+				} else if spanPolicy.ValueStoragePolicy.ContainsOverrides() {
+					expectedMinSize = spanPolicy.ValueStoragePolicy.OverrideBlobSeparationMinimumSize
 					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
 				}
 			}
 

options.go

@@ -1297,70 +1297,64 @@ func (p SpanPolicy) String() string {
 	if p.ValueStoragePolicy.DisableSeparationBySuffix {
 		sb.WriteString("disable-value-separation-by-suffix,")
 	}
-	switch p.ValueStoragePolicy.PolicyAdjustment {
-	case NoValueSeparation:
-		sb.WriteString("no-value-separation,")
-	case OverrideValueStorage:
-		sb.WriteString("override,")
+	if p.ValueStoragePolicy.DisableBlobSeparation {
+		sb.WriteString("no-blob-value-separation,")
+	}
+	if p.ValueStoragePolicy.OverrideBlobSeparationMinimumSize > 0 {
+		sb.WriteString("override-value-separation-min-size")
 	}
 	return strings.TrimSuffix(sb.String(), ",")
 }
 
 // ValueStoragePolicyAdjustment is used to determine where to store the values for
-// KVs. If the PolicyAdjustment specified is OverrideValueStorage, the remaining fields
-// are used to override the global configuration for value separation.
+// KVs, overriding global policies. Values can be configured to be stored in-place,
+// in value blocks, or in blob files.
 type ValueStoragePolicyAdjustment struct {
-	// PolicyAdjustment specifies the policy adjustment to apply.
-	PolicyAdjustment ValueStoragePolicyAdjustmentType
-
-	// 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.
+	// See sstable.IsLikelyMVCCGarbage for the exact criteria we use to
+	// determine whether a value is likely MVCC garbage.
+	//
+	// If separation by suffix is enabled, KVs with older suffix values will be
+	// written according to the following rules:
+	// - If the value is empty, no separation is performed.
+	// - If blob separation is enabled the value will be separated into a blob
+	// file even if its size is smaller than the minimum value size.
+	// - If blob separation is disabled, the value will be written to a value
+	// block within the sstable.
+	// - Otherwise, the KV will be subject to regular value separation rules.
 	DisableSeparationBySuffix bool
-	// MinimumSize is the minimum size of the value.
-	MinimumSize int
-}
 
-// ValueStoragePolicyAdjustmentType is a hint used to determine where to store the
-// values for KVs.
-type ValueStoragePolicyAdjustmentType uint8
+	// DisableBlobSeparation disables separating values into blob files.
+	DisableBlobSeparation bool
 
-const (
-	// UseDefaultValueStorage is the default value; Pebble will respect global
-	// configuration for value separation.
-	UseDefaultValueStorage ValueStoragePolicyAdjustmentType = iota
-
-	// NoValueSeparation indicates Pebble should prefer storing values
-	// in-place.
-	NoValueSeparation
-
-	// OverrideValueStorage indicates that value separation thresholds (see
-	// valsep.ValueSeparationOutputConfig) for this key range are being
-	// overridden from a SpanPolicy. If the global Options enable value
-	// separation, Pebble will separate values under the OverrideValueStorage
-	// policy even if they do not meet the minimum size threshold of the
-	// global Options' ValueSeparationPolicy.
-	OverrideValueStorage
-)
+	// OverrideBlobSeparationMinimumSize overrides the minimum size required
+	// for value separation into a blob file. Note that value separation must
+	// be enabled globally for this to take effect.
+	OverrideBlobSeparationMinimumSize int
+}
+
+func (vsp *ValueStoragePolicyAdjustment) ContainsOverrides() bool {
+	return vsp.OverrideBlobSeparationMinimumSize > 0 || vsp.DisableSeparationBySuffix
+}
 
 // ValueStorageLatencyTolerant is the suggested ValueStoragePolicyAdjustment
 // to use for key ranges that can tolerate higher value retrieval
 // latency.
 var ValueStorageLatencyTolerant = ValueStoragePolicyAdjustment{
-	PolicyAdjustment: OverrideValueStorage,
-	MinimumSize:      10,
+	OverrideBlobSeparationMinimumSize: 10,
 }
 
 // ValueStorageLowReadLatency is the suggested ValueStoragePolicyAdjustment
 // to use for key ranges that require low value retrieval latency.
 var ValueStorageLowReadLatency = ValueStoragePolicyAdjustment{
-	PolicyAdjustment: NoValueSeparation,
+	DisableBlobSeparation:     true,
+	DisableSeparationBySuffix: true,
 }
 
 // SpanPolicyFunc is used to determine the SpanPolicy for a key region.

sstable/properties.go

@@ -125,8 +125,9 @@ type Properties struct {
 	// 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.
+	// applied based on the KV suffix when writing blob files. Pebble attempts to optimize
+	// separating MVCC garbage, which are recognized by the key suffix. Note that this
+	// value corresponds only for blob file writing and not value blocks.
 	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.

testdata/compaction/mvcc_garbage_blob

@@ -67,7 +67,7 @@ flush-log
 
 # 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.
+# prevent MVCC garbage from being separeted (both into blob files and value blocks).
 
 define value-separation=(enabled, min-size=5, max-ref-depth=3, garbage-ratios=1.0:1.0)
 ----
@@ -127,3 +127,99 @@ pebble.compression_stats: None:234
 pebble.value-separation.min-size: 5
 pebble.value-separation.by-suffix.disabled: true
 obsolete-key: hex:00
+
+define value-separation=(enabled, min-size=5, max-ref-depth=3, garbage-ratios=1.0:1.0)
+----
+
+set-span-policies
+a,c val-sep-minimum-size=0
+----
+
+
+batch
+set a@2 a2
+set b@5 b5
+del b@4
+set b@3 bat3
+set b@2 bat2
+del b@1
+set b@1 bat1
+set b@0 bat0
+----
+
+# Although we've disabled value separation into blob files, writing to 
+# value blocks should still be enabled.
+flush
+----
+L0.0:
+  000005:[a@2#10,SET-b@0#17,SET] seqnums:[10-17] points:[a@2#10,SET-b@0#17,SET] size:880
+
+sstable-properties file=000005
+----
+rocksdb.num.entries: 7
+rocksdb.raw.key.size: 77
+rocksdb.raw.value.size: 20
+pebble.raw.point-tombstone.key.size: 3
+rocksdb.deleted.keys: 1
+rocksdb.num.range-deletions: 0
+pebble.value-blocks.size: 25
+rocksdb.num.data.blocks: 1
+rocksdb.comparator: pebble.internal.testkeys
+rocksdb.data.size: 175
+rocksdb.filter.size: 0
+rocksdb.index.size: 36
+rocksdb.block.based.table.index.type: 0
+pebble.colblk.schema: DefaultKeySchema(pebble.internal.testkeys,16)
+rocksdb.merge.operator: pebble.concatenate
+rocksdb.merge.operands: 0
+pebble.num.value-blocks: 1
+pebble.num.values.in.value-blocks: 3
+rocksdb.property.collectors: [obsolete-key]
+rocksdb.compression: Snappy
+pebble.compression_stats: None:221
+obsolete-key: hex:00
+
+batch
+set a@2 a2
+set b@5 b5
+del b@4
+set b@3 bat3
+set b@2 bat2
+del b@1
+set b@1 bat1
+set b@0 bat0
+----
+
+# Disabling value separation by suffix should also disable writing to value blocks.
+set-span-policies
+a,c val-sep-minimum-size=0 disable-separation-by-suffix
+----
+
+flush
+----
+L0.1:
+  000007:[a@2#18,SET-b@0#25,SET] seqnums:[18-25] points:[a@2#18,SET-b@0#25,SET] size:770
+L0.0:
+  000005:[a@2#10,SET-b@0#17,SET] seqnums:[10-17] points:[a@2#10,SET-b@0#17,SET] size:880
+
+sstable-properties file=000007
+----
+rocksdb.num.entries: 7
+rocksdb.raw.key.size: 77
+rocksdb.raw.value.size: 20
+pebble.raw.point-tombstone.key.size: 3
+rocksdb.deleted.keys: 1
+rocksdb.num.range-deletions: 0
+rocksdb.num.data.blocks: 1
+rocksdb.comparator: pebble.internal.testkeys
+rocksdb.data.size: 157
+rocksdb.filter.size: 0
+rocksdb.index.size: 36
+rocksdb.block.based.table.index.type: 0
+pebble.colblk.schema: DefaultKeySchema(pebble.internal.testkeys,16)
+rocksdb.merge.operator: pebble.concatenate
+rocksdb.merge.operands: 0
+rocksdb.property.collectors: [obsolete-key]
+rocksdb.compression: Snappy
+pebble.compression_stats: None:188
+obsolete-key: hex:00

testdata/static_span_policy_func

@@ -6,8 +6,8 @@ a b c d e f g
 a ->  until d
 b ->  until d
 c ->  until d
-d -> no-value-separation until f
-e -> no-value-separation until f
+d -> disable-value-separation-by-suffix,no-blob-value-separation until f
+e -> disable-value-separation-by-suffix,no-blob-value-separation until f
 f -> none
 g -> none
 
@@ -17,11 +17,11 @@ g -> none
 test a-z:lowlatency
 a b c d e z
 ----
-a -> no-value-separation until z
-b -> no-value-separation until z
-c -> no-value-separation until z
-d -> no-value-separation until z
-e -> no-value-separation until z
+a -> disable-value-separation-by-suffix,no-blob-value-separation until z
+b -> disable-value-separation-by-suffix,no-blob-value-separation until z
+c -> disable-value-separation-by-suffix,no-blob-value-separation until z
+d -> disable-value-separation-by-suffix,no-blob-value-separation until z
+e -> disable-value-separation-by-suffix,no-blob-value-separation until z
 z -> none
 
 # Abutting policies.
@@ -30,12 +30,12 @@ test b-d:lowlatency d-f:latencytolerant f-h:lowlatency
 a b c d e f g h i z
 ----
 a ->  until b
-b -> no-value-separation until d
-c -> no-value-separation until d
-d -> override until f
-e -> override until f
-f -> no-value-separation until h
-g -> no-value-separation until h
+b -> disable-value-separation-by-suffix,no-blob-value-separation until d
+c -> disable-value-separation-by-suffix,no-blob-value-separation until d
+d -> override-value-separation-min-size until f
+e -> override-value-separation-min-size until f
+f -> disable-value-separation-by-suffix,no-blob-value-separation until h
+g -> disable-value-separation-by-suffix,no-blob-value-separation until h
 h -> none
 i -> none
 z -> none
@@ -46,14 +46,14 @@ test b-d:lowlatency h-j:latencytolerant
 a b c d e f g h i j k l
 ----
 a ->  until b
-b -> no-value-separation until d
-c -> no-value-separation until d
+b -> disable-value-separation-by-suffix,no-blob-value-separation until d
+c -> disable-value-separation-by-suffix,no-blob-value-separation until d
 d ->  until h
 e ->  until h
 f ->  until h
 g ->  until h
-h -> override until j
-i -> override until j
+h -> override-value-separation-min-size until j
+i -> override-value-separation-min-size until j
 j -> none
 k -> none
 l -> none

valsep/value_separator.go

@@ -163,8 +163,7 @@ func NewWriteNewBlobFiles(
 // SetNextOutputConfig implements the ValueSeparation interface.
 func (vs *ValueSeparator) SetNextOutputConfig(config ValueSeparationOutputConfig) {
 	if config.MinimumSize == 0 {
-		// This indicates that MinimumSize was unset, so fall back
-		// to the global minimum size.
+		// No override, so fall back to the global minimum size.
 		config.MinimumSize = vs.globalConfig.MinimumSize
 	}
 	vs.currentConfig = config