Repository: cockroachdb/pebble
Pull request #4067 (2024-10-17 15:15:01)

options: add EnableDeleteOnlyCompactionExcises Option
This change adds a dynamic knob in Options to be able to toggle the functionality added in #3926. If the method is undefined or returns false, delete-only compactions only do full-file deletes and not excises.

cockroach-teamcity: 

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/cockroachdb/pebble/4067)
<!-- Reviewable:end -->

itsbilal: TFTR! Mind taking another look? I made some more changes to make the actual behaviour of having `!exciseEnabled` closer to the pre-#3926 behaviour.
itsbilal: *[`compaction.go` line 2580 at r2](https://reviewable.io/reviews/cockroachdb/pebble/4067#-O9QjjRoEcqIZOeRh6Pr:-O9QtDkD7N5vCDh-9fT7:b-hr5bkt) ([raw file](https://github.com/cockroachdb/pebble/blob/504d0a337423a73885c730709d6321ca02923bfb/compaction.go#L2580)):*
<details><summary><i>Previously, jbowens (Jackson Owens) wrote…</i></summary><blockquote>

I was thinking of the changes that were made to the table stats collector when you adapted it to permit excises. I re-read the code and I think those changes would just potentially cause us to wait longer to resolve the hint, because the associated `fileSmallestSeqNum` may have been made smaller by a file that only partially overlaps (and previously would've been excluded from consideration). I don't think this is a concern.

Relying on altered semantics in two locations based on a shared flag feels too fragile. If `!exciseEnabled` can we just construct fragments that map 1:1 with hints, so we still check down below to ensure this sstable can be deleted outright?
</blockquote></details>

We already have a panic if `exciseEnabled` and there are no fragments. I just wanted to avoid the `O(hints x files)` iteration in the compaction as that can really get out of control; the old code didn't propagate hints at all and just blindly deleted every file in the compaction, so I was trying to mimic that here.

However on a closer inspection it seems like that's likely a non-issue because we already do a bit of an `O(hints x files)` iteration _with_ the db mutex held in `checkDeleteCompactionHints` and have always done that; the trick is to just process hints asap. Alongside that logic we could probably do the hint fragmentation and file iteration here no problem, so I've made that change and now the code is a lot closer to what you had approved earlier.


<!-- Sent from Reviewable.io -->

itsbilal: TFTR!

Patch:
diff --git a/compaction.go b/compaction.go
index ca3d7749..d2af198d 100644
--- a/compaction.go
+++ b/compaction.go
@@ -257,6 +257,10 @@ type compaction struct {
 	// virtual sstables.
 	deletionHints []deleteCompactionHint
 
+	// exciseEnabled is set to true if this is a compactionKindDeleteOnly and
+	// this compaction is allowed to excise files.
+	exciseEnabled bool
+
 	metrics map[int]*LevelMetrics
 
 	pickerMetrics compactionPickerMetrics
@@ -401,6 +405,7 @@ func newDeleteOnlyCompaction(
 	inputs []compactionLevel,
 	beganAt time.Time,
 	hints []deleteCompactionHint,
+	exciseEnabled bool,
 ) *compaction {
 	c := &compaction{
 		kind:          compactionKindDeleteOnly,
@@ -413,6 +418,7 @@ func newDeleteOnlyCompaction(
 		beganAt:       beganAt,
 		inputs:        inputs,
 		deletionHints: hints,
+		exciseEnabled: exciseEnabled,
 	}
 
 	// Set c.smallest, c.largest.
@@ -1773,11 +1779,15 @@ func (d *DB) maybeScheduleCompactionPicker(
 func (d *DB) tryScheduleDeleteOnlyCompaction() {
 	v := d.mu.versions.currentVersion()
 	snapshots := d.mu.snapshots.toSlice()
-	inputs, resolvedHints, unresolvedHints := checkDeleteCompactionHints(d.cmp, v, d.mu.compact.deletionHints, snapshots, d.FormatMajorVersion())
+	// We need to save the value of exciseEnabled in the compaction itself, as
+	// it can change dynamically between now and when the compaction runs.
+	exciseEnabled := d.FormatMajorVersion() >= FormatVirtualSSTables &&
+		d.opts.Experimental.EnableDeleteOnlyCompactionExcises != nil && d.opts.Experimental.EnableDeleteOnlyCompactionExcises()
+	inputs, resolvedHints, unresolvedHints := checkDeleteCompactionHints(d.cmp, v, d.mu.compact.deletionHints, snapshots, exciseEnabled)
 	d.mu.compact.deletionHints = unresolvedHints
 
 	if len(inputs) > 0 {
-		c := newDeleteOnlyCompaction(d.opts, v, inputs, d.timeNow(), resolvedHints)
+		c := newDeleteOnlyCompaction(d.opts, v, inputs, d.timeNow(), resolvedHints, exciseEnabled)
 		d.mu.compact.compactingCount++
 		d.addInProgressCompaction(c)
 		go d.compact(c, nil)
@@ -1941,7 +1951,7 @@ func (h deleteCompactionHint) String() string {
 }
 
 func (h *deleteCompactionHint) canDeleteOrExcise(
-	cmp Compare, m *fileMetadata, snapshots compact.Snapshots, fmv FormatMajorVersion,
+	cmp Compare, m *fileMetadata, snapshots compact.Snapshots, exciseEnabled bool,
 ) deletionHintOverlap {
 	// The file can only be deleted if all of its keys are older than the
 	// earliest tombstone aggregated into the hint. Note that we use
@@ -1990,7 +2000,7 @@ func (h *deleteCompactionHint) canDeleteOrExcise(
 		base.UserKeyExclusive(h.end).CompareUpperBounds(cmp, m.UserKeyBounds().End) >= 0 {
 		return hintDeletesFile
 	}
-	if fmv < FormatVirtualSSTables {
+	if !exciseEnabled {
 		// The file's keys must be completely contained within the hint range; excises
 		// aren't allowed.
 		return hintDoesNotApply
@@ -2015,7 +2025,7 @@ func checkDeleteCompactionHints(
 	v *version,
 	hints []deleteCompactionHint,
 	snapshots compact.Snapshots,
-	fmv FormatMajorVersion,
+	exciseEnabled bool,
 ) (levels []compactionLevel, resolved, unresolved []deleteCompactionHint) {
 	var files map[*fileMetadata]bool
 	var byLevel [numLevels][]*fileMetadata
@@ -2023,7 +2033,8 @@ func checkDeleteCompactionHints(
 	// Delete-only compactions can be quadratic (O(mn)) in terms of runtime
 	// where m = number of files in the delete-only compaction and n = number
 	// of resolved hints. To prevent these from growing unbounded, we cap
-	// the number of hints we resolve for one delete-only compaction.
+	// the number of hints we resolve for one delete-only compaction. This
+	// cap only applies if exciseEnabled == true.
 	const maxHintsPerDeleteOnlyCompaction = 10
 
 	unresolvedHints := hints[:0]
@@ -2071,7 +2082,7 @@ func checkDeleteCompactionHints(
 		//     a b c d e f g h i j k l m n o p q r s t u v w x y z
 
 		if snapshots.Index(h.tombstoneLargestSeqNum) != snapshots.Index(h.fileSmallestSeqNum) ||
-			len(resolvedHints) >= maxHintsPerDeleteOnlyCompaction {
+			(len(resolvedHints) >= maxHintsPerDeleteOnlyCompaction && exciseEnabled) {
 			// Cannot resolve yet.
 			unresolvedHints = append(unresolvedHints, h)
 			continue
@@ -2086,8 +2097,9 @@ func checkDeleteCompactionHints(
 		for l := h.tombstoneLevel + 1; l < numLevels; l++ {
 			overlaps := v.Overlaps(l, base.UserKeyBoundsEndExclusive(h.start, h.end))
 			iter := overlaps.Iter()
+
 			for m := iter.First(); m != nil; m = iter.Next() {
-				doesHintApply := h.canDeleteOrExcise(cmp, m, snapshots, fmv)
+				doesHintApply := h.canDeleteOrExcise(cmp, m, snapshots, exciseEnabled)
 				if m.IsCompacting() || doesHintApply == hintDoesNotApply || files[m] {
 					continue
 				}
@@ -2546,6 +2558,7 @@ func (d *DB) runDeleteOnlyCompactionForLevel(
 	ve *versionEdit,
 	snapshots compact.Snapshots,
 	fragments []deleteCompactionHintFragment,
+	exciseEnabled bool,
 ) error {
 	curFragment := 0
 	iter := cl.files.Iter()
@@ -2582,7 +2595,7 @@ func (d *DB) runDeleteOnlyCompactionForLevel(
 					// above it.
 					continue
 				}
-				hintOverlap := h.canDeleteOrExcise(d.cmp, curFile, snapshots, d.FormatMajorVersion())
+				hintOverlap := h.canDeleteOrExcise(d.cmp, curFile, snapshots, exciseEnabled)
 				if hintOverlap == hintDoesNotApply {
 					continue
 				}
@@ -2669,7 +2682,7 @@ func (d *DB) runDeleteOnlyCompaction(
 	}
 	for _, cl := range c.inputs {
 		levelMetrics := &LevelMetrics{}
-		if err := d.runDeleteOnlyCompactionForLevel(cl, levelMetrics, ve, snapshots, fragments); err != nil {
+		if err := d.runDeleteOnlyCompactionForLevel(cl, levelMetrics, ve, snapshots, fragments, c.exciseEnabled); err != nil {
 			return nil, stats, err
 		}
 		c.metrics[cl.level] = levelMetrics
diff --git a/metamorphic/options.go b/metamorphic/options.go
index 94f34d8e..25939575 100644
--- a/metamorphic/options.go
+++ b/metamorphic/options.go
@@ -163,6 +163,12 @@ func parseOptions(
 			case "TestOptions.use_excise":
 				opts.useExcise = true
 				return true
+			case "TestOptions.use_delete_only_compaction_excises":
+				opts.useDeleteOnlyCompactionExcises = true
+				opts.Opts.Experimental.EnableDeleteOnlyCompactionExcises = func() bool {
+					return opts.useDeleteOnlyCompactionExcises
+				}
+				return true
 			case "TestOptions.use_jemalloc_size_classes":
 				opts.Opts.AllocatorSizeClasses = pebble.JemallocSizeClasses
 				return true
@@ -259,6 +265,9 @@ func optionsToString(opts *TestOptions) string {
 	if opts.useExcise {
 		fmt.Fprintf(&buf, "  use_excise=%v\n", opts.useExcise)
 	}
+	if opts.useDeleteOnlyCompactionExcises {
+		fmt.Fprintf(&buf, "  use_delete_only_compaction_excises=%v\n", opts.useDeleteOnlyCompactionExcises)
+	}
 	if opts.Opts.AllocatorSizeClasses != nil {
 		if fmt.Sprint(opts.Opts.AllocatorSizeClasses) != fmt.Sprint(pebble.JemallocSizeClasses) {
 			panic(fmt.Sprintf("unexpected AllocatorSizeClasses %v", opts.Opts.AllocatorSizeClasses))
@@ -401,6 +410,9 @@ type TestOptions struct {
 	// excises. However !useExcise && !useSharedReplicate can be used to guarantee
 	// lack of excises.
 	useExcise bool
+	// useDeleteOnlyCompactionExcises turns on the ability for delete-only compactions
+	// to do excises. Note that this can be true even when useExcise is false.
+	useDeleteOnlyCompactionExcises bool
 }
 
 // InitRemoteStorageFactory initializes Opts.Experimental.RemoteStorage.
@@ -837,6 +849,10 @@ func RandomOptions(
 			testOpts.Opts.FormatMajorVersion = pebble.FormatVirtualSSTables
 		}
 	}
+	testOpts.useDeleteOnlyCompactionExcises = rng.IntN(2) == 0
+	opts.Experimental.EnableDeleteOnlyCompactionExcises = func() bool {
+		return testOpts.useDeleteOnlyCompactionExcises
+	}
 	testOpts.InitRemoteStorageFactory()
 	testOpts.Opts.EnsureDefaults()
 	return testOpts
diff --git a/options.go b/options.go
index 4c8dee2b..db89b25f 100644
--- a/options.go
+++ b/options.go
@@ -840,6 +840,12 @@ type Options struct {
 		//   which will later be consumed by SingleDelete#3. The violation will
 		//   not be detected and the DB will be correct.
 		SingleDeleteInvariantViolationCallback func(userKey []byte)
+
+		// EnableDeleteOnlyCompactionExcises enables delete-only compactions to also
+		// apply delete-only compaction hints on sstables that partially overlap
+		// with it. This application happens through an excise, similar to
+		// the excise phase of IngestAndExcise.
+		EnableDeleteOnlyCompactionExcises func() bool
 	}
 
 	// Filters is a map from filter policy name to filter policy. It is used for


Test patch:
diff --git a/compaction_test.go b/compaction_test.go
index 00a1d30d..89b82606 100644
--- a/compaction_test.go
+++ b/compaction_test.go
@@ -1357,6 +1357,7 @@ func TestCompactionDeleteOnlyHints(t *testing.T) {
 			},
 			FormatMajorVersion: internalFormatNewest,
 		}).WithFSDefaults()
+		opts.Experimental.EnableDeleteOnlyCompactionExcises = func() bool { return true }
 		opts.Experimental.EnableColumnarBlocks = func() bool { return true }
 		return opts, nil
 	}
@@ -1617,6 +1618,7 @@ func TestCompactionTombstones(t *testing.T) {
 					},
 					FormatMajorVersion: internalFormatNewest,
 				}).WithFSDefaults()
+				opts.Experimental.EnableDeleteOnlyCompactionExcises = func() bool { return true }
 				opts.Experimental.EnableColumnarBlocks = func() bool { return true }
 				var err error
 				d, err = runDBDefineCmd(td, opts)
diff --git a/metamorphic/options_test.go b/metamorphic/options_test.go
index ef3187fe..c53e902e 100644
--- a/metamorphic/options_test.go
+++ b/metamorphic/options_test.go
@@ -79,6 +79,7 @@ func TestOptionsRoundtrip(t *testing.T) {
 		"Experimental.IngestSplit:",
 		"Experimental.RemoteStorage:",
 		"Experimental.SingleDeleteInvariantViolationCallback:",
+		"Experimental.EnableDeleteOnlyCompactionExcises:",
 		"Levels[0].Compression:",
 		"Levels[1].Compression:",
 		"Levels[2].Compression:",