mirror of
https://github.com/facebook/rocksdb.git
synced 2026-07-07 14:47:40 +08:00
Josh Kang
26a501b5d1
Summary: Fixes an off-by-one bug in how sequence numbers are handled when splitting range tombstones across output levels in per-key-placement (tiered) compaction. The bug was either introduced or propagated in https://github.com/facebook/rocksdb/issues/13256. Point entries move to proximal output only when their sequence number is strictly greater than `proximal_after_seqno_`, but range tombstones were previously split using an inclusive lower bound at that same seqno. That allowed a range tombstone at the boundary to be emitted to the proximal level while point keys at the same seqno stayed in the last level, which could create overlapping files in the proximal level (caught by `force_consistency_checks` as `L<n> has overlapping ranges`). This matters when `proximal_output_range_type_` is `kNonLastRange`: the compaction only owns the selected proximal-level input range, so existing last-level data at the split boundary must stay in the last level. In `kFullRange`, the compaction owns the relevant proximal-level range, so newer last-level data can be safely emitted to proximal output. The fix splits range tombstones at `proximal_after_seqno_ + 1` (saturating at `kMaxSequenceNumber`), so the half-open `[lower, upper)` tombstone filter lands on the same boundary as the strict `seqno > proximal_after_seqno_` rule used for point keys. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14795 Test Plan: New regression test `PrecludeLastLevelTestBase.RangeDelAtProximalSeqnoBoundaryStaysInLastLevel` uses a targeted manual compaction to exercise the `kNonLastRange` case directly, verifying a boundary range tombstone stays in the last level instead of widening the proximal output range. Worked example: ```text Initial state: L6 (last level): Put(Key 2)s1, Put(Key 12)s2, RangeDel[Key 2, Key 12)s3 L5 (proximal): file A [Key 0 .. Key 4]s4-s5 file B [Key 5 .. Key 9]s6-s7 preclude_last_level_min_seqno is forced to 0 via sync point. Manual CompactFiles selects only L5 file B + the L6 file (output level 6). Only part of the proximal level is selected, so this is the kNonLastRange case: max_last_level_seqno = 3 proximal_after_seqno_ = max(0, 3) = 3 Point keys Key 5 (s6) and Key 9 (s7): both seqno > 3 -> proximal output (L5) OK Range tombstone s3: Old (buggy): proximal keep range [3, MAX) includes s3, so the tombstone is emitted to the proximal output. The output is built from L5 input file B [Key 5 .. Key 9], but the tombstone covers [Key 2, Key 12), so the proximal output file starts at Key 2 and spills past Key 5 into the existing, untouched L5 file A [Key 0 .. Key 4]. Two overlapping files in L5 -> Corruption. New (fixed): split at proximal_after_seqno_ + 1 = 4. proximal keep [4, MAX) excludes s3; last-level keep [0, 4) includes s3 -> tombstone stays in the last level (L6). OK ``` Verification (debug build, `make -j64 tiered_compaction_test`): - Without the fix, the test fails at the `CompactFiles` call: ``` tiered_compaction_test.cc:2940: Failure Corruption: force_consistency_checks(DEBUG): VersionBuilder: L5 has overlapping ranges: file https://github.com/facebook/rocksdb/issues/11 largest key: Key(4) seq:5, type:1 (Put) vs. file https://github.com/facebook/rocksdb/issues/17 smallest key: Key(2) seq:3, type:15 (range deletion) ``` - With the fix, the test passes (range deletions absent from L5, still present in L6). Reviewed By: pdillinger Differential Revision: D106528471 Pulled By: joshkang97 fbshipit-source-id: a5f99b426d3a7a6253bc1972cf8cb60d1cb85089
Adding release notes -------------------- When adding release notes for the next release, add a file to one of these directories: unreleased_history/new_features unreleased_history/behavior_changes unreleased_history/public_api_changes unreleased_history/bug_fixes with a unique name that makes sense for your change, preferably using the .md extension for syntax highlighting. There is a script to help, as in $ unreleased_history/add.sh unreleased_history/bug_fixes/crash_in_feature.md or simply $ unreleased_history/add.sh will take you through some prompts. The file should usually contain one line of markdown, and "* " is not required, as it will automatically be inserted later if not included at the start of the first line in the file. Extra newlines or missing trailing newlines will also be corrected. The only times release notes should be added directly to HISTORY are if * A release is being amended or corrected after it is already "cut" but not tagged, which should be rare. * A single commit contains a noteworthy change and a patch release version bump Ordering of entries ------------------- Within each group, entries will be included using ls sort order, so important entries could start their file name with a small three digit number like 100pretty_important.md. The ordering of groups such as new_features vs. public_api_changes is hard-coded in unreleased_history/release.sh Updating HISTORY.md with release notes -------------------------------------- The script unreleased_history/release.sh does this. Run the script before updating version.h to the next development release, so that the script will pick up the version being released. You might want to start with $ DRY_RUN=1 unreleased_history/release.sh | less to check for problems and preview the output. Then run $ unreleased_history/release.sh which will git rm some files and modify HISTORY.md. You still need to commit the changes, or revert with the command reported in the output. Why not update HISTORY.md directly? ----------------------------------- First, it was common to hit unnecessary merge conflicts when adding entries to HISTORY.md, which slowed development. Second, when a PR was opened before a release cut and landed after the release cut, it was easy to add the HISTORY entry to the wrong version's history. This new setup completely fixes both of those issues, with perhaps slightly more initial work to create each entry. There is also now an extra step in using `git blame` to map a release note to its source code implementation, but that is a relatively rare operation.