From d4a90ce899e501b2a07a3836f5539bf3b622f0ff Mon Sep 17 00:00:00 2001 From: sumeerbhola Date: Thu, 4 Jan 2024 10:42:19 -0500 Subject: [PATCH] metamorphic: don't panic on SingleDeleteInvariantViolationCallback The code comment motivating this change is copied from https://github.com/cockroachdb/cockroach/pull/116889. Fixes #3183 --- metamorphic/options.go | 7 ----- options.go | 58 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 7 deletions(-) diff --git a/metamorphic/options.go b/metamorphic/options.go index af82c75d14..8a9b8d25b7 100644 --- a/metamorphic/options.go +++ b/metamorphic/options.go @@ -242,13 +242,6 @@ func defaultOptions() *pebble.Options { }}, BlockPropertyCollectors: blockPropertyCollectorConstructors, } - // TODO(sumeer): add IneffectualSingleDeleteCallback that panics by - // supporting a test option that does not generate ineffectual single - // deletes. - opts.Experimental.SingleDeleteInvariantViolationCallback = func( - userKey []byte) { - panic(errors.AssertionFailedf("single del invariant violations on key %q", userKey)) - } return opts } diff --git a/options.go b/options.go index 8086363537..63a07a9729 100644 --- a/options.go +++ b/options.go @@ -705,6 +705,64 @@ type Options struct { // on shared storage in bytes. If it is 0, no cache is used. SecondaryCacheSizeBytes int64 + // NB: DO NOT crash on SingleDeleteInvariantViolationCallback or + // IneffectualSingleDeleteCallback, since these can be false positives + // even if SingleDel has been used correctly. + // + // Pebble's delete-only compactions can cause a recent RANGEDEL to peek + // below an older SINGLEDEL and delete an arbitrary subset of data below + // that SINGLEDEL. When that SINGLEDEL gets compacted (without the + // RANGEDEL), any of these callbacks can happen, without it being a real + // correctness problem. + // + // Example 1: + // RANGEDEL [a, c)#10 in L0 + // SINGLEDEL b#5 in L1 + // SET b#3 in L6 + // + // If the L6 file containing the SET is narrow and the L1 file containing + // the SINGLEDEL is wide, a delete-only compaction can remove the file in + // L2 before the SINGLEDEL is compacted down. Then when the SINGLEDEL is + // compacted down, it will not find any SET to delete, resulting in the + // ineffectual callback. + // + // Example 2: + // RANGEDEL [a, z)#60 in L0 + // SINGLEDEL g#50 in L1 + // SET g#40 in L2 + // RANGEDEL [g,h)#30 in L3 + // SET g#20 in L6 + // + // In this example, the two SETs represent the same user write, and the + // RANGEDELs are caused by the CockroachDB range being dropped. That is, + // the user wrote to g once, range was dropped, then added back, which + // caused the SET again, then at some point g was validly deleted using a + // SINGLEDEL, and then the range was dropped again. The older RANGEDEL can + // get fragmented due to compactions it has been part of. Say this L3 file + // containing the RANGEDEL is very narrow, while the L1, L2, L6 files are + // wider than the RANGEDEL in L0. Then the RANGEDEL in L3 can be dropped + // using a delete-only compaction, resulting in an LSM with state: + // + // RANGEDEL [a, z)#60 in L0 + // SINGLEDEL g#50 in L1 + // SET g#40 in L2 + // SET g#20 in L6 + // + // A multi-level compaction involving L1, L2, L6 will cause the invariant + // violation callback. This example doesn't need multi-level compactions: + // say there was a Pebble snapshot at g#21 preventing g#20 from being + // dropped when it meets g#40 in a compaction. That snapshot will not save + // RANGEDEL [g,h)#30, so we can have: + // + // SINGLEDEL g#50 in L1 + // SET g#40, SET g#20 in L6 + // + // And say the snapshot is removed and then the L1 and L6 compaction + // happens, resulting in the invariant violation callback. + // + // TODO(sumeer): rename SingleDeleteInvariantViolationCallback to remove + // the word "invariant". + // IneffectualPointDeleteCallback is called in compactions/flushes if any // single delete is being elided without deleting a point set/merge. IneffectualSingleDeleteCallback func(userKey []byte)