sstable: allow SetWithDelete keys to be considered as MVCC garbage

This patch allows the `SetWithDelete` key kind to be eligible for
MVCC garbage consideration. This effectively allows `SetWithDelete`
keys to be separated into value blocks and blob files.

Fixes: #4424

Author: annrpom

internal/compact/run.go

@@ -338,7 +338,6 @@ func (r *Runner) writeKeysToTable(
 		isLikelyMVCCGarbage := func() bool {
 			return sstable.IsLikelyMVCCGarbage(kv.K.UserKey, prevKeyKind, kv.K.Kind(), valueLen, prefixEqual)
 		}
-		prevKeyKind = kv.K.Kind()
 		// Add the value to the sstable, possibly separating its value into a
 		// blob file. The ValueSeparation implementation is responsible for
 		// writing the KV to the sstable.
@@ -348,6 +347,7 @@ func (r *Runner) writeKeysToTable(
 		if err := valueSeparation.Add(tw, kv, r.iter.ForceObsoleteDueToRangeDel(), isLikelyMVCCGarbage); err != nil {
 			return nil, err
 		}
+		prevKeyKind = kv.K.Kind()
 		if r.iter.SnapshotPinned() {
 			// The kv pair we just added to the sstable was only surfaced by
 			// the compaction iterator because an open snapshot prevented

sstable/colblk_writer.go

@@ -1277,8 +1277,8 @@ func (w *RawColumnWriter) copyProperties(props Properties) {
 //
 // We require:
 //
-//	. The previous key to be a SET.
-//	. The current key to be a SET.
+//	. The previous key to be a SET/SETWITHDEL.
+//	. The current key to be a SET/SETWITHDEL.
 //	. The value to be sufficiently large. (Currently we simply require a
 //	  non-zero length, so all non-empty values are eligible for storage
 //	  out-of-band in a value block.)
@@ -1294,8 +1294,11 @@ func IsLikelyMVCCGarbage(
 	prefixEqual func(k []byte) bool,
 ) bool {
 	const tinyValueThreshold = 0
-	return prevKeyKind == InternalKeyKindSet &&
-		keyKind == InternalKeyKindSet &&
+	isSetStarKind := func(k base.InternalKeyKind) bool {
+		return k == InternalKeyKindSet || k == InternalKeyKindSetWithDelete
+	}
+	return isSetStarKind(prevKeyKind) &&
+		isSetStarKind(keyKind) &&
 		valueLen > tinyValueThreshold &&
 		prefixEqual(k)
 }

sstable/testdata/writer_value_blocks

@@ -1,73 +1,95 @@
 # Size of value index is 3 bytes plus 5 + 5 = 10 bytes of trailer of the value
-# block and value index block. So size 18 - 13 = 5 size of the value in the
+# block and value index block. So size 17 - 13 = 4 size of the value in the
 # value block.
+# N.B. The only key kind eligible for separation is `SET`.
 build table-format=Pebble,v4
 a@2.SET.1:a2
 b@5.SET.7:b5
 b@4.DEL.3:
 b@3.SET.2:bat3
-b@2.SET.1:vbat2
+b@2.SETWITHDEL.1:bat2
+b@1.SET.1:bat1
+b@0.SET.0:bat0
 ----
-value-blocks: num-values 1, num-blocks: 1, size: 18
+value-blocks: num-values 1, num-blocks: 1, size: 17
 
 scan-raw
 ----
 a@2#1,SET:in-place a2, same-pre false
 b@5#7,SET:in-place b5, same-pre false
 b@4#3,DEL:
 b@3#2,SET:in-place bat3, same-pre false
-b@2#1,SET:value-handle len 5 block 0 offset 0, att 5, same-pre true
+b@2#1,SETWITHDEL:bat2
+b@1#1,SET:in-place bat1, same-pre false
+b@0#0,SET:value-handle len 4 block 0 offset 0, att 4, same-pre true
 
 scan
 ----
 a@2#1,SET:a2
 b@5#7,SET:b5
 b@4#3,DEL:
 b@3#2,SET:bat3
-b@2#1,SET:vbat2
+b@2#1,SETWITHDEL:bat2
+b@1#1,SET:bat1
+b@0#0,SET:bat0
 
 scan-cloned-lazy-values
 ----
 0(in-place: len 2): a2
 1(in-place: len 2): b5
 2(in-place: len 0):
 3(in-place: len 4): bat3
-4(lazy: len 5, attr: 5): vbat2
+4(in-place: len 4): bat2
+5(in-place: len 4): bat1
+6(lazy: len 4, attr: 4): bat0
 
 # Repeat the above test with (Pebble,v5) [columnar blocks].
+# N.B. The key kinds eligible for separation when writing columnar blocks are
+# `SET` and `SETWITHDEL`.
 
+# Since we end up separating the SETWITHDEL key, this means 2 more values are
+# stored (each with a 4 byte value). This increases the size of the value block
+# by 8 bytes, making the total size 25.
 build table-format=Pebble,v5
 a@2.SET.1:a2
 b@5.SET.7:b5
 b@4.DEL.3:
 b@3.SET.2:bat3
-b@2.SET.1:vbat2
+b@2.SETWITHDEL.1:bat2
+b@1.SET.1:bat1
+b@0.SET.0:bat0
 ----
-value-blocks: num-values 1, num-blocks: 1, size: 18
+value-blocks: num-values 3, num-blocks: 1, size: 25
 
 scan
 ----
 a@2#1,SET:a2
 b@5#7,SET:b5
 b@4#3,DEL:
 b@3#2,SET:bat3
-b@2#1,SET:vbat2
+b@2#1,SETWITHDEL:bat2
+b@1#1,SET:bat1
+b@0#0,SET:bat0
 
 scan-cloned-lazy-values
 ----
 0(in-place: len 2): a2
 1(in-place: len 2): b5
 2(in-place: len 0):
 3(in-place: len 4): bat3
-4(lazy: len 5, attr: 5): vbat2
+4(lazy: len 4, attr: 4): bat2
+5(lazy: len 4, attr: 4): bat1
+6(lazy: len 4, attr: 4): bat0
 
 # Same data as previous, with disable-value-blocks set to true
 build disable-value-blocks=true table-format=Pebble,v4
 a@2.SET.1:a2
 b@5.SET.7:b5
 b@4.DEL.3:
 b@3.SET.2:bat3
-b@2.SET.1:vbat2
+b@2.SETWITHDEL.1:bat2
+b@1.SET.1:bat1
+b@0.SET.0:bat0
 ----
 value-blocks: num-values 0, num-blocks: 0, size: 0
 
@@ -77,15 +99,19 @@ a@2#1,SET:in-place a2, same-pre false
 b@5#7,SET:in-place b5, same-pre false
 b@4#3,DEL:
 b@3#2,SET:in-place bat3, same-pre false
-b@2#1,SET:in-place vbat2, same-pre true
+b@2#1,SETWITHDEL:bat2
+b@1#1,SET:in-place bat1, same-pre false
+b@0#0,SET:in-place bat0, same-pre true
 
 scan
 ----
 a@2#1,SET:a2
 b@5#7,SET:b5
 b@4#3,DEL:
 b@3#2,SET:bat3
-b@2#1,SET:vbat2
+b@2#1,SETWITHDEL:bat2
+b@1#1,SET:bat1
+b@0#0,SET:bat0
 
 # Same as above but with (Pebble,v5) [columnar blocks].
 
@@ -94,7 +120,9 @@ a@2.SET.1:a2
 b@5.SET.7:b5
 b@4.DEL.3:
 b@3.SET.2:bat3
-b@2.SET.1:vbat2
+b@2.SETWITHDEL.1:bat2
+b@1.SET.1:bat1
+b@0.SET.0:bat0
 ----
 value-blocks: num-values 0, num-blocks: 0, size: 0
 
@@ -104,7 +132,9 @@ a@2#1,SET:a2
 b@5#7,SET:b5
 b@4#3,DEL:
 b@3#2,SET:bat3
-b@2#1,SET:vbat2
+b@2#1,SETWITHDEL:bat2
+b@1#1,SET:bat1
+b@0#0,SET:bat0
 
 # Size of value index is 3 bytes plus 5 + 5 = 10 bytes of trailer of the value
 # block and value index block. So size 33 - 13 = 20 is the total size of the

testdata/compaction/mvcc_garbage_blob

@@ -3,26 +3,36 @@
 define value-separation=(true, 5, 3, 0s, 1.0)
 ----
 
-batch
-set bar bar
-set foo foo
-set fuzz fuzz
-set yaya yaya
-----
-
 batch
 set yay@3 a
 set yay@2 ab
-set zoo@3 b
-set zoo@2 ba
+set zoo@4 b
+set zoo@3 ba
+del zoo@2
+set zoo@2 bag
+set zoo@1 bah
 ----
 
 # This flush *should* write a blob file for our MVCC garbage values, containing
-# 2 values: "ab" and "ba" - totaling 4 bytes of logical values.
+# 4 values: "ab", "ba", "bag", "bah" - totaling 10 bytes of logical values.
+# N.B. `del zoo@2, set zoo@2 bag` transforms into `setwithdel zoo@2 bag`.
+flush
+----
+L0.0:
+  000005:[yay@3#10,SET-zoo@1#16,SET] seqnums:[10-16] points:[yay@3#10,SET-zoo@1#16,SET] size:827 blobrefs:[(B000006: 10); depth:1]
+Blob files:
+  B000006 physical:{000006 size:[102 (102B)] vals:[10 (10B)]}
+
+batch
+del yuumi@2
+set yuumi@1 ba
+----
 
 flush
 ----
+L0.1:
+  000008:[yuumi@2#17,DEL-yuumi@1#18,SET] seqnums:[17-18] points:[yuumi@2#17,DEL-yuumi@1#18,SET] size:707
 L0.0:
-  000005:[bar#10,SET-zoo@2#17,SET] seqnums:[10-17] points:[bar#10,SET-zoo@2#17,SET] size:834 blobrefs:[(B000006: 4); depth:1]
+  000005:[yay@3#10,SET-zoo@1#16,SET] seqnums:[10-16] points:[yay@3#10,SET-zoo@1#16,SET] size:827 blobrefs:[(B000006: 10); depth:1]
 Blob files:
-  B000006 physical:{000006 size:[94 (94B)] vals:[4 (4B)]}
+  B000006 physical:{000006 size:[102 (102B)] vals:[10 (10B)]}

value_separation.go

@@ -251,8 +251,6 @@ func (vs *writeNewBlobFiles) 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(annie): Also allow SetWithDelete keys to be separated.
 	if len(v) < vs.minimumSize && !isLikelyMVCCGarbage() {
 		return tw.Add(kv.K, v, forceObsolete)
 	}