Fix range lock manager assertion crash on reverse-comparator CF point locks#14880
Closed
jaykorean wants to merge 1 commit into
Closed
Fix range lock manager assertion crash on reverse-comparator CF point locks#14880jaykorean wants to merge 1 commit into
jaykorean wants to merge 1 commit into
Conversation
… locks Summary: The RocksDB 11.5.0 reverse-comparator range-lock fix (the HISTORY.md entry "Fixed a bug where the range lock manager would crash with an assertion failure when using a reverse comparator column family") made `RangeTreeLockManager::CompareDbtEndpoints` reverse-aware by wrapping the endpoint-suffix tie-break results in an `is_reverse` flip. The length-disparity (different user key) branches need that flip, but the equal-length / equal-user-key branch was flipped too, and that is incorrect. Range-lock endpoints are encoded as `[suffix_byte][user_key]` where the suffix is `SUFFIX_INFIMUM` (0x0) or `SUFFIX_SUPREMUM` (0x1). When two endpoints share the same user key and differ only in the suffix byte, the suffix is a positional boundary marker for that key, not key content, so the infimum endpoint must sort before the supremum endpoint in any total order, regardless of the column family sort direction. Flipping it for a reverse comparator makes `m_cmp(infimum(K), supremum(K)) > 0`. MyRocks issues an equality lock (e.g. `UPDATE t1 SET a=123 WHERE a=35`) as the range `[infimum(K), supremum(K)]` over the same user key K. On a reverse (`rev:`) column family — which uses the stock `ReverseBytewiseComparator()`, so `is_reverse` is true — the flipped comparator reports `left > right`, tripping `paranoid_invariant(m_cmp(left_key, right_key) <= 0)` in `toku::locktree::try_acquire_lock` and aborting the server with SIGABRT. This broke many MyRocks `---range_locking` MTR tests (partial_index*, bypass_select_range_sk*, alter_table_online_debug, allow_no_primary_key_with_sk) after RocksDB was bumped to 11.5.0 in D108936924. Fix: in the equal-length / equal-user-key branch of `CompareDbtEndpoints`, compare the suffix bytes without the `is_reverse` flip so infimum always precedes supremum. The length-disparity branches keep the reverse-aware flip (still covered by `RangeLockWithReverseComparator`). Note: this lands in `internal_repo_rocksdb/repo`, the RocksDB development tree; it reaches `fbcode/rocksdb/src` (what MyRocks/ZippyDB link) through the RocksDB release sync. Tracked in T276980728. Differential Revision: D109591908
|
@jaykorean has exported this pull request. If you are a Meta employee, you can view the originating Diff in D109591908. |
✅ clang-tidy: No findings on changed linesCompleted in 45.6s. |
🟡 Codex Code ReviewAuto-triggered after CI passed — reviewing commit 5cdb2bb ❌ Codex review failed before producing findings. ℹ️ About this responseGenerated by Codex CLI. Limitations:
Commands:
|
✅ Claude Code ReviewAuto-triggered after CI passed — reviewing commit 5cdb2bb SummaryThis is a well-targeted, correct bug fix for a regression introduced by the 11.5.0 reverse-comparator range-lock fix. The change correctly removes the High-severity findings (0): Full review (click to expand)Findings🟡 MEDIUMM1. Custom reverse comparators are not detected by
|
|
This pull request has been merged in b0ecf86. |
jaykorean
added a commit
that referenced
this pull request
Jun 24, 2026
… locks (#14880) Summary: Pull Request resolved: #14880 The RocksDB 11.5.0 reverse-comparator range-lock fix (the HISTORY.md entry "Fixed a bug where the range lock manager would crash with an assertion failure when using a reverse comparator column family") made `RangeTreeLockManager::CompareDbtEndpoints` reverse-aware by wrapping the endpoint-suffix tie-break results in an `is_reverse` flip. The length-disparity (different user key) branches need that flip, but the equal-length / equal-user-key branch was flipped too, and that is incorrect. Range-lock endpoints are encoded as `[suffix_byte][user_key]` where the suffix is `SUFFIX_INFIMUM` (0x0) or `SUFFIX_SUPREMUM` (0x1). When two endpoints share the same user key and differ only in the suffix byte, the suffix is a positional boundary marker for that key, not key content, so the infimum endpoint must sort before the supremum endpoint in any total order, regardless of the column family sort direction. Flipping it for a reverse comparator makes `m_cmp(infimum(K), supremum(K)) > 0`. MyRocks issues an equality lock (e.g. `UPDATE t1 SET a=123 WHERE a=35`) as the range `[infimum(K), supremum(K)]` over the same user key K. On a reverse (`rev:`) column family — which uses the stock `ReverseBytewiseComparator()`, so `is_reverse` is true — the flipped comparator reports `left > right`, tripping `paranoid_invariant(m_cmp(left_key, right_key) <= 0)` in `toku::locktree::try_acquire_lock` and aborting the server with SIGABRT. This broke many MyRocks `---range_locking` MTR tests (partial_index*, bypass_select_range_sk*, alter_table_online_debug, allow_no_primary_key_with_sk) after RocksDB was bumped to 11.5.0 in D108936924. Fix: in the equal-length / equal-user-key branch of `CompareDbtEndpoints`, compare the suffix bytes without the `is_reverse` flip so infimum always precedes supremum. The length-disparity branches keep the reverse-aware flip (still covered by `RangeLockWithReverseComparator`). Note: this lands in `internal_repo_rocksdb/repo`, the RocksDB development tree; it reaches `fbcode/rocksdb/src` (what MyRocks/ZippyDB link) through the RocksDB release sync. Tracked in T276980728. Reviewed By: xingbowang Differential Revision: D109591908 fbshipit-source-id: 078b16e6e51f4f365b4a8d539ac5b968a7461729
jaykorean
added a commit
to jaykorean/rocksdb
that referenced
this pull request
Jun 25, 2026
…ok#14887) Summary: This reverts two changes to the range-lock endpoint comparator and restores the prior, direction-agnostic behavior: - facebook#14831 ("Fix range lock manager crash with reverse comparator"), which made RangeTreeLockManager::CompareDbtEndpoints reverse-aware, and - facebook#14880 (a follow-up that only corrected the equal-length / point-range branch). Why: facebook#14831 taught the endpoint comparator about reverse-ordered column families by flipping the suffix tie-break based on comparator direction. However, callers that use range locking on a reverse-ordered CF already flip the start/end endpoints themselves before calling lock_range() -- for example, MyRocks does this in ha_rocksdb::set_range_lock() (see its "RangeFlagsShouldBeFlippedForRevCF" comment). The two corrections stack and produce left > right again, which: (a) trips paranoid_invariant(compare(left, right) <= 0) in toku::locktree::try_acquire_lock() and aborts on equality/point locks, and (b) for range scans, mis-orders the endpoints and produces an incorrect lock range, leading to silent correctness issues under concurrency. facebook#14880 only addressed (a) (the equal-length / point-range branch), not (b), so it was an incomplete fix at the wrong layer. Decision: keep CompareDbtEndpoints direction-agnostic and make it an explicit caller requirement to pass a flipped (swapped start/end, with negated infimum/supremum flags) range for range locking to work on a reverse-ordered column family. A function-level comment documenting this contract is added so reverse-awareness is not reintroduced here. Removes the RangeLockWithReverseComparator unit test added by facebook#14831 (it asserts the reverted direction-flipped ordering). Keeps a regression test that a point range lock [infimum(K), supremum(K)] works on a reverse-comparator column family. Differential Revision: D109713989
meta-codesync Bot
pushed a commit
that referenced
this pull request
Jun 25, 2026
Summary: Pull Request resolved: #14887 This reverts two changes to the range-lock endpoint comparator and restores the prior, direction-agnostic behavior: - #14831 ("Fix range lock manager crash with reverse comparator"), which made RangeTreeLockManager::CompareDbtEndpoints reverse-aware, and - #14880 (a follow-up that only corrected the equal-length / point-range branch). Why: #14831 taught the endpoint comparator about reverse-ordered column families by flipping the suffix tie-break based on comparator direction. However, callers that use range locking on a reverse-ordered CF already flip the start/end endpoints themselves before calling lock_range() -- for example, MyRocks does this in ha_rocksdb::set_range_lock() (see its "RangeFlagsShouldBeFlippedForRevCF" comment). The two corrections stack and produce left > right again, which: (a) trips paranoid_invariant(compare(left, right) <= 0) in toku::locktree::try_acquire_lock() and aborts on equality/point locks, and (b) for range scans, mis-orders the endpoints and produces an incorrect lock range, leading to silent correctness issues under concurrency. #14880 only addressed (a) (the equal-length / point-range branch), not (b), so it was an incomplete fix at the wrong layer. Decision: keep CompareDbtEndpoints direction-agnostic and make it an explicit caller requirement to pass a flipped (swapped start/end, with negated infimum/supremum flags) range for range locking to work on a reverse-ordered column family. A function-level comment documenting this contract is added so reverse-awareness is not reintroduced here. Removes the RangeLockWithReverseComparator unit test added by #14831 (it asserts the reverted direction-flipped ordering). Keeps a regression test that a point range lock [infimum(K), supremum(K)] works on a reverse-comparator column family. Reviewed By: xingbowang Differential Revision: D109713989 fbshipit-source-id: 4ea0999c92645e1adc9cd1114d924cad142f2001
jaykorean
added a commit
that referenced
this pull request
Jun 25, 2026
Summary: Pull Request resolved: #14887 This reverts two changes to the range-lock endpoint comparator and restores the prior, direction-agnostic behavior: - #14831 ("Fix range lock manager crash with reverse comparator"), which made RangeTreeLockManager::CompareDbtEndpoints reverse-aware, and - #14880 (a follow-up that only corrected the equal-length / point-range branch). Why: #14831 taught the endpoint comparator about reverse-ordered column families by flipping the suffix tie-break based on comparator direction. However, callers that use range locking on a reverse-ordered CF already flip the start/end endpoints themselves before calling lock_range() -- for example, MyRocks does this in ha_rocksdb::set_range_lock() (see its "RangeFlagsShouldBeFlippedForRevCF" comment). The two corrections stack and produce left > right again, which: (a) trips paranoid_invariant(compare(left, right) <= 0) in toku::locktree::try_acquire_lock() and aborts on equality/point locks, and (b) for range scans, mis-orders the endpoints and produces an incorrect lock range, leading to silent correctness issues under concurrency. #14880 only addressed (a) (the equal-length / point-range branch), not (b), so it was an incomplete fix at the wrong layer. Decision: keep CompareDbtEndpoints direction-agnostic and make it an explicit caller requirement to pass a flipped (swapped start/end, with negated infimum/supremum flags) range for range locking to work on a reverse-ordered column family. A function-level comment documenting this contract is added so reverse-awareness is not reintroduced here. Removes the RangeLockWithReverseComparator unit test added by #14831 (it asserts the reverted direction-flipped ordering). Keeps a regression test that a point range lock [infimum(K), supremum(K)] works on a reverse-comparator column family. Reviewed By: xingbowang Differential Revision: D109713989 fbshipit-source-id: 4ea0999c92645e1adc9cd1114d924cad142f2001
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
The RocksDB 11.5.0 reverse-comparator range-lock fix (#14831) made
RangeTreeLockManager::CompareDbtEndpointsreverse-aware by wrapping the endpoint-suffix tie-break results in anis_reverseflip. The length-disparity (different user key) branches need that flip, but the equal-length / equal-user-key branch was flipped too, and that is incorrect.Range-lock endpoints are encoded as
[suffix_byte][user_key]where the suffix isSUFFIX_INFIMUM(0x0) orSUFFIX_SUPREMUM(0x1). When two endpoints share the same user key and differ only in the suffix byte, the suffix is a positional boundary marker for that key, not key content, so the infimum endpoint must sort before the supremum endpoint in any total order, regardless of the column family sort direction. Flipping it for a reverse comparator makesm_cmp(infimum(K), supremum(K)) > 0.MyRocks issues an equality lock (e.g.
UPDATE t1 SET a=123 WHERE a=35) as the range[infimum(K), supremum(K)]over the same user key K. On a reverse (rev:) column family — which uses the stockReverseBytewiseComparator(), sois_reverseis true — the flipped comparator reportsleft > right, trippingparanoid_invariant(m_cmp(left_key, right_key) <= 0)intoku::locktree::try_acquire_lockand aborting the server with SIGABRT. This broke many MyRocks---range_lockingMTR tests (partial_index*, bypass_select_range_sk*, alter_table_online_debug, allow_no_primary_key_with_sk) after RocksDB was bumped to 11.5.0.Fix: in the equal-length / equal-user-key branch of
CompareDbtEndpoints, compare the suffix bytes without theis_reverseflip so infimum always precedes supremum. The length-disparity branches keep the reverse-aware flip (still covered byRangeLockWithReverseComparator).Differential Revision: D109591908