mirror of
https://github.com/facebook/rocksdb.git
synced 2026-07-07 14:47:40 +08:00
update-folly-11.6
13973 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Josh Kang
|
03e5a0b9a8 |
Fix range tombstone conversion + ingest sst and re-enable crash tests (#14654)
Summary:
Fixes a correctness bug in read-path range-tombstone synthesis when it races with `IngestExternalFile`. The synthesis path could insert a tombstone into the active memtable at a snapshot's sequence number, while ingestion installed an L0 SST at `LastSequence + 1` — a higher seqno than the synthesized tombstone. This breaks the main assumption of range tombstone reads that all lower levels have lower seqno.
The fix introduces a per-CF `port::RWMutex` (`ColumnFamilyData::ingest_sst_lock_`) plus a per-memtable `ingest_seqno_barrier_`. Ingestion takes the read lock and range tombstone synthesis **tries** to take a write lock.
If iterator lock is successful, then we have a new updated barrier seqno that we can validate the iterator seqno against. An added benefit is we no longer need to gate against empty memtable. This was originally added as an easy fix to prevent memtables from being inserted into while ingestion was happening.
## The bug, by example
`ReadPathRangeTombstoneTest.NewerPointInOlderFileStillVisible` (`db/db_iterator_test.cc:6929`):
1. L0 file `b@1, c@2, d@3`, then L0 file `Delete(b)4, Delete(c)5`.
2. Active memtable: `Put(z)6`. Snapshot taken at seq 6.
3. `IngestExternalFile({c → "vc_live"})` → installed at L0 with seq 7.
4. Iterator at snap 6 walks the deletion run and synthesizes `[b, d) @ seq 6` into the active memtable via `MemTable::AddLogicallyRedundantRangeTombstone`.
5. `Get("c")` at the latest snapshot: memtable returns covering tombstone (seq 6), `Version::Get` short-circuits, **never reads `c@7`** — returns `NotFound` instead of `"vc_live"`.
The invariant `Version::Get` relies on (memtable seqs ≥ any L0 seq for the same key) is broken because synthesis writes at the *snapshot's* seq while ingestion writes at `LastSequence + 1`.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14654
Test Plan:
- Updated regression test
- Re-enable crashtests and manually run
| Flavor | Jobs |
|---|---:|
| `fbcode_blackbox_crash_test` | 200 |
| `fbcode_whitebox_crash_test` | 30 |
| `fbcode_asan_blackbox_crash_test` | 30 |
| `fbcode_tsan_blackbox_crash_test` | 30 |
| `fbcode_crash_test_with_atomic_flush` | 30 |
| `fbcode_crash_test_with_wc_txn` | 30 |
| `fbcode_crash_test_with_ts` | 30 |
| **Total** | **380** |
Reviewed By: xingbowang
Differential Revision: D102044512
Pulled By: joshkang97
fbshipit-source-id: 0c69187595edc5a5fa80be24bffdba710a92e56e
|
||
|
Yoshinori Matsunobu
|
763401b595 |
Add ParseCompressionNameForDisplay API (#14637)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14637 Add public API ParseCompressionNameForDisplay() to render TableProperties::compression_name in human-readable form for both legacy and format_version >= 7 SST metadata, including BuiltinV2/compression-manager encodings and filtered no-compression markers for display. Reviewed By: xingbowang Differential Revision: D101463511 fbshipit-source-id: 51c44850183cb9757644a323376844adffc7a8c2 |
||
|
Xingbo Wang
|
c4941760c9 |
Keep remote compaction stats serialization compatible with 11.1 (#14656)
Summary: - Keep remote compaction stats serialization compatible with RocksDB 11.1. - Serialize `InternalStats::CompactionStats::counts` using the pre-11.2 compaction-reason count so remote compaction metadata remains readable across the version boundary. - Preserve the existing formatting-only follow-up commit on the branch. ## Notes - Needs a better mechanism to make OptionTypeInfo::Array ser/des friendly. Will follow up after 11.2 release. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14656 Reviewed By: pdillinger Differential Revision: D102071507 Pulled By: xingbowang fbshipit-source-id: 6eeb87d86494f528f0d1ce46ce4c3e8e7dd2da53 |
||
|
xingbowang
|
809ed26be5 |
Revert "Migrate fbcode coro references from folly/experimental to folly/coro (5/6 - fbcode a-m)" (#14652)
Summary: This reverts commit `bad2d5b0af7e160de5ca58869f40941af0f9da95`. The reverted change switched fbcode Buck dependencies from `//folly/experimental/coro:*` to `//folly/coro:*` in: - `BUCK` - `buckifier/buckify_rocksdb.py` That change broke the internal build, so this PR restores the previous dependency paths. ## Testing - Not run locally; this task only fetched, rebased, and verified the existing revert branch. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14652 Reviewed By: archang19 Differential Revision: D101980062 Pulled By: xingbowang fbshipit-source-id: 9a6edf14c226caf87e2c63c7f2806d409dc809ef |
||
|
Hui Xiao
|
13ea89f66b |
Handle DB::Open failure in fault_injection_test better (#14646)
Summary: **Summary:** Assert the DB::Open status before asserting on the DB pointer in FaultInjectionTest::OpenDB(). This avoids aborting the test process when reopen fails and preserves the underlying RocksDB error for ASSERT_OK(OpenDB()). This is to deal with recent failures on a host with nearby tests failing with space issue already. ``` [ RUN ] ExternalSSTFileBasicTest.LargeSizeSstFileWriter db/external_sst_file_basic_test.cc:3303: Failure sst_file_writer.Finish() IO error: No space left on device: While appending to file: /dev/shm/rocksdb_testt/run-external_sst_file_basic_test-shard-3/external_sst_file_basic_test_578578_8746831521190564136/large_key.sst: No space left on device [ FAILED ] ExternalSSTFileBasicTest.LargeSizeSstFileWriter (8551 ms) [ RUN ] FaultTest/FaultInjectionTestSplitted.FaultTest/2 fault_injection_test: db/fault_injection_test.cc:269: rocksdb::Status rocksdb::FaultInjectionTest::OpenDB(): Assertion `db_ != nullptr' failed. Received signal 6 (Aborted) ``` Pull Request resolved: https://github.com/facebook/rocksdb/pull/14646 Test Plan: Test changes only Reviewed By: anand1976 Differential Revision: D101742874 Pulled By: hx235 fbshipit-source-id: de3b9349d9d067dd8f538b44ac08d108924faed3 |
||
|
Hui Xiao
|
4c1ffedb1e |
Disable skip_stats_update_on_db_open with remote compaction in stress test (#14647)
Summary: **Summary:** When remote compaction is enabled with skip_stats_update_on_db_open=true, the remote worker's secondary DB skips UpdateAccumulatedStats(), leaving FileMetaData fields (num_entries, num_range_deletions) at 0. This breaks standalone range deletion file filtering in FilterInputsForCompactionIterator() (https://github.com/facebook/rocksdb/blob/b374ba5968bdd6b16ff58918c561781c8bdab442/db/compaction/compaction.cc#L1085 + https://github.com/facebook/rocksdb/blob/b374ba5968bdd6b16ff58918c561781c8bdab442/db/version_edit.h#L462) , causing the primary and remote worker to disagree on input key count and triggering a "Compaction number of input keys does not match number of keys processed" corruption error. Repro test (should be landed with the fix in the future): https://github.com/hx235/rocksdb/commit/aa1225c69cd6b6e2d2af735e1d1d9b147edfd6ae ``` [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from CompactionServiceTest [ RUN ] CompactionServiceTest.StandaloneRangeDeletionWithSkipStatsUpdateOnOpen db/compaction/compaction_service_test.cc:2970: Failure s Corruption: Compaction number of input keys does not match number of keys processed. Expected 0 but processed 4. Compaction summary: Base version 15 Base level 5, inputs: [20(1280B)], [18(1270B filtered:true) 19(1270B filtered:true)] [ FAILED ] CompactionServiceTest.StandaloneRangeDeletionWithSkipStatsUpdateOnOpen (1257 ms) [----------] 1 test from CompactionServiceTest (1257 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (1257 ms total) [ PASSED ] 0 tests. [ FAILED ] 1 test, listed below: [ FAILED ] CompactionServiceTest.StandaloneRangeDeletionWithSkipStatsUpdateOnOpen 1 FAILED TEST ``` This can happen even when standalone range deletion ingestion is disabled in the current run, because such files may have been ingested in a previous crash test run with different randomized options. So we disable skip_stats_update_on_db_open unconditionally when remote compaction is active until the real fix lands . Pull Request resolved: https://github.com/facebook/rocksdb/pull/14647 Test Plan: 1. Verify skip_stats_update_on_db_open is forced to 0 with remote compaction 2. Verify skip_stats_update_on_db_open can still be 1 without remote compaction: Reviewed By: anand1976 Differential Revision: D101744943 Pulled By: hx235 fbshipit-source-id: c1dcd8943433e8a9dfb7385c998be2c339e3b859 |
||
|
Hui Xiao
|
7f89e5fb09 |
Disable inplace_update_support in stress test (#14645)
Summary: **Summary:** Temporarily disables inplace_update_support in db_crashtest.py. See the db_crashtest.py new comments for mow. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14645 Test Plan: - Run db_crashtest.py without passing inplace_update_support option, with short interval, and verify each run shows inplace_update_support is false in the db_stress command line. Reviewed By: joshkang97 Differential Revision: D101739997 Pulled By: hx235 fbshipit-source-id: 39936b8282e392e688afea5e9bd074b60138f210 |
||
|
Anand Ananthabhotla
|
b374ba5968 |
Rocksdb Unit Test failed: Unit test failed (#14643)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14643 Reviewed By: xingbowang Differential Revision: D101559659 fbshipit-source-id: 56d405f36bfe83c009053796c0d065f4032522b1 |
||
|
Xingbo Wang
|
6ff96384dd |
db_iter: eagerly resolve blob-backed iterator columns (#14632)
Summary:
Restore the iterator contract for blob-backed wide-column entities by eagerly materializing all blob-backed columns before a `DBIter` entry is exposed as valid. Also capture similar error inside compaction filter V4, so that the error is captured a surfaced up like older compaction filter V3.
## Problem
Lazy wide-column blob resolution let `DBIter` report `Valid() == true` before all columns were actually prepared. Callers could read `value()` successfully, then hit a blob-read failure in `columns()`, which flipped the iterator to invalid after the entry had already been observed as valid.
## Solution
- Restore back to old behavior.
- Resolve and materialize all blob-backed wide columns during iterator positioning.
- Keep `columns()` as a pure accessor once `Valid()` is true.
- Add coverage for eager blob resolution on iterator scans.
- Add coverage for blob resolution failures invalidating the iterator before the entry is exposed.
- Capture the same error in compaction filter and set db to bg error status if the error is not retriable.
## Next step
If we want lazy iterator resolution in the future, we will need an API that can surface lazy resolution failures explicitly instead of hiding I/O and status transitions inside value() or columns().
## Testing
- `make -j14 db_wide_blob_direct_write_test`
- `timeout 60s ./db_wide_blob_direct_write_test --gtest_filter='*DirectWriteIteratorValueScanEagerlyResolvesBlobColumns:*DirectWriteIteratorBlobResolutionErrorInvalidatesEntry'`
## Task
T265294130
# Bonus fix
## Non-compaction blob filtering fixes
### 1. Flush-time wide-entity lazy blob resolution now works correctly
Blob direct-write can leave wide-entity blob references in memtables before flush. Flush already runs through `CompactionIterator`, but outside a real compaction it did not have blob read support. As a result, `FilterV4` lazy resolution on flush could fail immediately with
`NotSupported("Blob fetcher not available")` instead of either:
- resolving the blob successfully, or
- surfacing the real blob read error and failing the flush
This stack fixes that by plumbing blob read support into non-compaction table-file creation. `BuildTable()` now gives `CompactionIterator` enough context to construct a `BlobFetcher` for flush/recovery table building, including write-path fallback for direct-write blob files
that are not yet manifest-visible.
With that in place:
- flush-time `FilterV4` can lazily resolve blob-backed wide columns
- if resolution fails, the failure is latched and flush fails after `FilterV4()` returns
- `bg_error` is set instead of silently preserving the entry
### 2. Plain blob fallback outside compaction now uses the resolved user value
For plain blob-backed values, `FilterBlobByKey()` is allowed to return `kUndetermined`, which is supposed to fall back to the normal value-based filter path. That fallback worked in compaction, but the non-compaction `BuildTable()` path still treated plain blob indexes as
unsupported/corrupt because it could not read the blob.
This stack fixes that behavior for flush/recovery table-file creation:
- when `FilterBlobByKey()` returns `kUndetermined`, RocksDB now eagerly resolves the plain blob value
- `FilterV2`/`FilterV3`/`FilterV4` then see the actual user value as `ValueType::kValue`, not the `BlobIndex` encoding
- legacy filters therefore behave the same way outside compaction as they already do inside compaction
Wide-column entities still use the `FilterV4` lazy resolver path. The plain-blob fix is specifically about restoring the documented fallback behavior for blob-backed `kValue` records outside compaction.
## Why this matters
Without these last two commits, flush-time filtering still had two inconsistent behaviors:
- wide-entity lazy resolution could not actually read blob-backed columns in the direct-write case
- plain blob-backed values could not fall back to normal value-based filtering outside compaction
So even after fixing iterator validity and compaction error propagation, non-compaction table-file creation still had remaining contract holes. These commits make flush/recovery behave consistently with compaction.
## Coverage added
The tests added in these commits cover:
- flush-time plain-blob `FilterV3` fallback using the resolved user value
- flush-time plain-blob `FilterV4` fallback using the resolved user value
- flush-time direct-write wide-entity lazy resolver success
- flush-time direct-write wide-entity lazy resolver failure on missing blob file, including flush failure and `bg_error` latching
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14632
Reviewed By: anand1976
Differential Revision: D101425233
Pulled By: xingbowang
fbshipit-source-id: 695b2df5189033d30309b35849815e31fd965664
|
||
|
Anand Ananthabhotla
|
a4139ef9e9 |
IODispatcher: fall back to synchronous coalesced reads when async IO is unavailable (#14633)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14633 Reviewed By: xingbowang Differential Revision: D101299989 fbshipit-source-id: ce30f91da23031b57b2f440f3cdc56f67cc673a0 |
||
|
Andrew Chang
|
3ef8b1b743 |
Guard auto-readahead against exhausted index iter (#14631)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14631 Observed crash from ZippyDB iterate scans: ``` onFatalError: unexpected error : Invariant SignalFatal: fbcode/zippydb/server/Main.cpp:handleSigToFatalCommon:281:FATAL false failed: Signal 11 (SIGSEGV) at address 0x0 ... method: MultiIterate ... RocksDB Activity: iterate ``` Key stack frames: rocksdb::BlockBasedTableIterator::IsNextBlockOutOfReadaheadBound() fbcode/rocksdb/src/table/block_based/block_based_table_iterator.h:471 rocksdb::BlockBasedTableIterator::BlockCacheLookupForReadAheadSize() fbcode/rocksdb/src/table/block_based/block_based_table_iterator.cc:878 rocksdb::FinalizeAsyncRead() rocksdb::FilePrefetchBuffer::PollIfNeeded() rocksdb::BlockFetcher::ReadBlockContents() rocksdb::BlockBasedTableIterator::InitDataBlock() rocksdb::DBIter::Next() Reviewed By: xingbowang Differential Revision: D101217429 fbshipit-source-id: e7657909a0f07526d070b214cb783f581b8e98ca |
||
|
anand76
|
4bc0bff0e4 |
Mark IODispatcher as experimental (#14630)
Summary: As titled Pull Request resolved: https://github.com/facebook/rocksdb/pull/14630 Reviewed By: xingbowang Differential Revision: D101383049 Pulled By: anand1976 fbshipit-source-id: 19fb1c27bee465e691412a30681f9a21c70672f7 |
||
|
xingbowang
|
49ec219955 |
cmake: fix folly nightly build regressions (#14609)
Summary: Fix nightly build regressions introduced by recent CI/toolchain changes. This PR includes two parts: - CMake/link fixes for the Folly and Folly Lite nightly jobs - a targeted nightly workflow fix for the clang-21 ASAN/UBSAN + Folly job Changes: - link gflags explicitly for USE_FOLLY CMake tool and benchmark targets - resolve USE_FOLLY_LITE glog via its installed library path instead of bare -lglog - in the clang-21 nightly job, build Folly/getdeps with gcc/g++ instead of inheriting clang-21 for third-party dependency builds - disable ccache only for that standalone Folly build step so getdeps/CMake does not inject the broken ccache compiler launcher for ASM Root cause: - recent CI/container changes exposed missing explicit link dependencies in the CMake Folly paths - the clang-21 nightly job exported CC/CXX at job scope, so Folly getdeps inherited clang-21 into libiberty/binutils build logic that expects GCC-style driver behavior such as -print-multi-os-directory - the same standalone Folly build path also misbehaved when ccache was auto-detected and used as a compiler launcher for assembler-with-cpp inputs Verification: - make format-auto - git diff --check - local CMake configure sanity check for default config - upstream nightly reruns used to confirm failure signatures before and after the first-round fixes Pull Request resolved: https://github.com/facebook/rocksdb/pull/14609 Reviewed By: anand1976 Differential Revision: D100695693 Pulled By: xingbowang fbshipit-source-id: 703546ad3ddc518ab80c936442709d65fe2d22af |
||
|
Josh Kang
|
790294f3ad |
Disallow table_filters entirely when range tombstone conversion is enabled (#14618)
Summary: The prior fix (https://github.com/facebook/rocksdb/issues/14586) disabled read-path range tombstone synthesis at the `DBIter` level when `table_filter` was set. However, synthesized tombstones persist in the memtable beyond the lifetime of the iterator that created them. A prior unfiltered iterator can synthesize a range tombstone that then silently affects a subsequent filtered iterator's results — the filtered scan sees the tombstone but not the SSTs it was derived from, allowing hidden SST state to corrupt the filtered view. See failed crash test in T264151327. This PR takes the stronger approach of rejecting iterator creation outright (`InvalidArgument`) when `ReadOptions::table_filter` is used on a column family with `min_tombstones_for_range_conversion > 0`. This eliminates the entire class of interaction bugs between the two features rather than trying to suppress conversion in individual code paths. ## Example - `min_tombstones_for_range_conversion = 2` - L1 SST_keep: `Put(a), Put(b), Put(c)` (`c` is the live boundary) - L0 SST_dels: `Delete(a), Delete(b)` (2 contiguous tombstones, newer) ### Step 1 — Iterator A (no filter) A walks `Del(a) Del(b)` (2 contig) then `c` (live). Synthesizes range tombstone `[a, c)` into the active memtable. ### Step 2 — Iterator B (`table_filter` skips SST_dels) B's filter excludes SST_dels, so `a, b` from SST_keep should appear live. But the memtable now holds range tombstone `[a, c)` from step 1, which applies to B regardless of `table_filter`. B sees only `c` — **a, b are silently hidden**. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14618 Test Plan: - `TableFilterNotAllowed` test validates the rejection path end-to-end: unfiltered scan synthesizes the tombstone, filtered iterator is rejected with `InvalidArgument`, and the full-DB view remains correct via `VerifyIteration`. - Crash test sanitization ensures `db_stress` won't hit the incompatible configuration. - Run crash test sanitization unit test: `python3 tools/db_crashtest_test.py` - Run iterator tests: `make db_iterator_test && ./db_iterator_test --gtest_filter="*TableFilterNotAllowed*"` Reviewed By: xingbowang Differential Revision: D100873156 Pulled By: joshkang97 fbshipit-source-id: dd0d123d94b1bfda2a4a5040681fcc7cf14f760e |
||
|
Josh Kang
|
9886d52fc4 |
Disable range tomb conversion when using legacy prefix iterator (#14625)
Summary:
There is an assumption in the code that within a given prefix all keys can be seen after a seek to that prefix. The problem occurs when there is a Next() followed by a Prev() back into the same prefix. The merging iterator actually uses SeekForPrev during Prev for non-child iterators, and as a result changes the prefix being used. Now that we are back to our original prefix, the view is incomplete.
The fix is to simply disable range tombstone conversion in this legacy prefix mode.
### Example
Use a 1-byte prefix extractor, so the prefix is just the first character.
Think of the merged iterator as combining two children:
- Child A: `b8`
- Child B: `b7, c1`
Global sorted order is:
```text
b7, b8, c1
```
So:
- predecessor of `c1` should be `b8`
- predecessor of `b8` should be `b7`
## Sequence
Start in legacy prefix mode:
- `total_order_seek = false`
- `prefix_same_as_start = false`
Now do:
1. `Seek("b8")`
2. `Next()`
3. `Prev()`
## What should happen
```text
Seek("b8") -> b8
Next() -> c1
Prev() -> b8
```
Because in the global order, `b8` is immediately before `c1`.
## What the buggy iterator did
```text
Seek("b8") -> b8
Next() -> c1
Prev() -> b7
```
So it skipped `b8`.
## Why it happens
When `Prev()` switches direction, the merging iterator tells all non-current
children to `SeekForPrev(current_key)`.
At that moment:
- current key is `c1`
- so the other children get `SeekForPrev("c1")`
But in legacy prefix mode, those child seeks are allowed to use prefix
filtering. So a child that only contains `b*` keys can effectively say:
```text
"I was asked for prefix c; I do not have prefix c."
```
That child drops out instead of returning its real global predecessor `b8`.
Then the current child simply moves back from `c1` to `b7`, and the merged
result becomes `b7`.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14625
Test Plan: Update unit tests to disable feature.
Reviewed By: xingbowang
Differential Revision: D101259748
Pulled By: joshkang97
fbshipit-source-id: 23e19c470f931406f96a34a7baebca89e91c7e39
|
||
|
xingbowang
|
36e50a61fc |
db_stress: exclude info logs from metadata read faults (#14616)
Summary: - fix a false positive in `db_stress` PrefixScan fault accounting caused by info-log `FileExists()` probes going through metadata-read fault injection - exclude `kInfoLogFile` from metadata-read injection and extend `FaultInjectionTestFS` filename parsing so LOG/LOG.old.* and `db_log_dir` info-log paths are recognized consistently - add regression coverage showing info-log `FileExists()` bypasses metadata-read injection while manifest lookups still inject ## Testing - make format-auto - make -j48 fault_injection_fs_test db_stress - timeout 60 ./fault_injection_fs_test - timeout 60 ./fault_injection_fs_test --gtest_filter='*MetadataReadFaultExcludesInfoLogFiles*' --gtest_repeat=5 Pull Request resolved: https://github.com/facebook/rocksdb/pull/14616 Reviewed By: hx235 Differential Revision: D100838168 Pulled By: xingbowang fbshipit-source-id: 9ef996908b393aea2a8221fcdda67ed609643dbe |
||
|
xingbowang
|
7b06a9ae7e |
Track blob garbage during flush of direct-write blob references (#14623)
Summary: - meter blob garbage during `BuildTable()` when flush input can contain direct-write blob references, so overwrite elision and flush-time compaction filters register garbage in the manifest - plumb `BlobFileGarbage` through flush and recovery manifest edits even when the flush produces no SST output - add wide-column direct-write tests covering TTL-filtered flushes with both mixed live/expired records and all-expired input Pull Request resolved: https://github.com/facebook/rocksdb/pull/14623 Reviewed By: anand1976 Differential Revision: D101212841 Pulled By: xingbowang fbshipit-source-id: e5803ddaf17b5dfd92312e42191b76e311257f3b |
||
|
xingbowang
|
df5695cf24 |
Fix workflow YAML parsing and add validation (#14624)
Summary: This fixes the Claude review workflow YAML parse error and adds a CI validation step for GitHub Actions workflow YAML via `make check- workflow-yaml`. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14624 Reviewed By: archang19 Differential Revision: D101225058 Pulled By: xingbowang fbshipit-source-id: 0f371dcbd70ece2c25220c96e43305d9665554b1 |
||
|
Xingbo Wang
|
820c4a77c1 |
dump disk usage on no space stress test failure (#14619)
Summary: Detect out-of-space failures from combined db_stress stdout/stderr in the Python wrapper so both OpenAndCompact stdout failures and verification stderr failures trigger the same diagnostics. When a match is found, print filesystem usage for /dev/shm and the db roots, then summarize per-directory and per-extension usage to make it clear which files consumed space. Add unit coverage for the failure matcher and suffix accounting. This help triage any regression in additional file usage in stress test. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14619 Reviewed By: archang19 Differential Revision: D100984310 Pulled By: xingbowang fbshipit-source-id: 23765ff405f0e64382e601b0da173ab6b37dba6d |
||
|
Laurynas Biveinis
|
94b6566505 |
Fix CompareDbtEndpoints crash with timestamp-aware comparators (#14601) (#14611)
Summary: `RangeTreeLockManager::CompareDbtEndpoints()` called `Comparator::Compare()` on range lock endpoint keys that never contain user-defined timestamps. With a timestamp-aware comparator (`timestamp_size() > 0`), this caused assertion failures in debug builds and silent endpoint misordering in release builds. Replace `Compare()` with the 4-argument `CompareWithoutTimestamp()` using `a_has_ts=false, b_has_ts=false`, which is the correct contract for serialized range lock endpoints (format: `[1-byte suffix][key bytes]`, no timestamp). Pull Request resolved: https://github.com/facebook/rocksdb/pull/14611 Test Plan: - New test `RangeLockWithTimestampComparator` reopens with `BytewiseComparatorWithU64Ts` and exercises range lock acquisition, conflict detection, and non-overlapping success with short keys. - Verified RED (assertion failure) before fix, GREEN after. - Full `range_locking_test` suite passes (16/16). - Stress tested with `COERCE_CONTEXT_SWITCH=1 --gtest_repeat=5`. Reviewed By: xingbowang Differential Revision: D100775201 Pulled By: laurynas-biveinis fbshipit-source-id: 58e3846e62e9b3cc5bdc69557458b245f90b3967 |
||
|
Anand Ananthabhotla
|
1cc216a45b |
Fast file open: retrieve, persist, and pass file open metadata (#14596)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14596 When DBOptions::fast_sst_open is enabled, RocksDB retrieves opaque file system metadata for SST files after flush, compaction, and external file ingestion via FSRandomAccessFile::GetFileOpenMetadata(). This metadata is persisted in the MANIFEST using a new forward-compatible NewFileCustomTag (kFileOpenMetadata = 17), and passed back to the file system via FileOptions::file_metadata on subsequent file opens. This accelerates DB open time on remote storage systems by allowing the file system to skip expensive metadata RPCs. The feature is gated by DBOptions::fast_sst_open (default false). Everything works seamlessly regardless of the option setting, file metadata support, or presence/absence of the metadata in the MANIFEST. The MANIFEST change is backward compatible - older RocksDB versions safely ignore the new tag. Reviewed By: xingbowang Differential Revision: D100220973 fbshipit-source-id: f52de9dd853a50653b3297ab4a37a868fe41cc04 |
||
|
Josh Kang
|
978bf67426 |
Disable min_tombstones_for_range_conversion in crash tests (#14617)
Summary: Temporarily disable the feature until all fixes land and crash tests are stabilized. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14617 Reviewed By: xingbowang Differential Revision: D100853151 Pulled By: joshkang97 fbshipit-source-id: f2c9018f0b55891f6c0623902c3b961bc7f623aa |
||
|
Xingbo Wang
|
6b8ca89f45 |
Ignore post-SIGTERM io_uring stderr in stress test (#14613)
Summary: Blackbox crash tests intentionally terminate `db_stress` with `SIGTERM` on timeout. When `io_uring` is enabled, that shutdown can emit the expected `PosixRandomAccessFile::MultiRead: io_uring_submit_and_wait returned terminal error: -9.` message on `stderr`, which currently makes the timeout path look like a real failure. This change filters only that known post-`SIGTERM` stderr after a timeout when the process output confirms the `SIGTERM` handler ran. Any other stderr is still surfaced and fails the test, while the ignored lines are appended to `stdout` so the signal remains visible in logs. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14613 Test Plan: - Added unit tests covering fully ignored post-`SIGTERM` stderr - Added unit tests covering mixed stderr where unrelated lines must still fail - Added unit tests covering guard conditions when the run did not time out or did not print the `SIGTERM` marker - Not run (not requested) Reviewed By: archang19 Differential Revision: D100806204 Pulled By: xingbowang fbshipit-source-id: 01248a371afae91bb43df55f88400baf155373f5 |
||
|
Xingbo Wang
|
50852b5c8d |
db_stress: document expected-state trace/replay contract (#14612)
Summary: - add a `docs/components/` landing page and a stress-test docs index - document the `db_stress` expected-state trace/replay lifecycle, file invariants, and prefix-recovery contract in `expected_state_trace.md` - align `db_stress` comments with the restore semantics: missing trace entries are fatal, while extra tail trace entries are tolerated - keep the new docs tree trackable and point repo instructions at the new component-docs entrypoint ## Testing - Not run (documentation and comment updates only) Pull Request resolved: https://github.com/facebook/rocksdb/pull/14612 Reviewed By: joshkang97 Differential Revision: D100797173 Pulled By: xingbowang fbshipit-source-id: 25be8c6239b9fdd84580818efe7520c371f9a46b |
||
|
Peter Dillinger
|
9f474a1034 |
Enforce < 4GB sizes for keys, values, and some BlockBuilder inputs (#14461)
Summary: BlockBuilder was somewhat inconsistent in its treatment of Slices whose size exceeds 4GB, which in a random corruption case could lead to (for example) serializing only the bottom 32 bits of a value size (uncorrupted) but appending the full multi-GB (corrupted) size. Because this is inner loop code, we don't want to pay CPU for extra conversions, data movements, or Status plumbing (already ruled out by Xingbo). In this change we make BlockBuilder more internally consistent and lift the 32-bit size requirement to callers (of a few functions specifically, for now). To ensure that's satisfied, I've added additional checks near the perimeter of RocksDB to ensure keys and values do not exceed 4GB, plus an extra random corruption or backstop check in BlockBasedTableBuilder. In detail, BlockBuilder (block_builder.cc/h): * Add API comments documenting the < 4GB assumption on Add/AddWithLastKey * Add debug assertions verifying input slice sizes < 4GB * Simplify AddWithLastKeyImpl to use uint32_t locals, reducing static_cast * Use only bottom 32 bits when appending derivative slices for consistency * Update MaybeStripTimestampFromKey to also truncate to 32-bit size * Add FIXME comments where buffer_/values_buffer_ sizes are truncated BlockBasedTableBuilder (block_based_table_builder.cc): * Add value size check (> uint32_t max) as a safety net against random corruption, since we have seen such corruptions in production WriteBatch (write_batch.cc): * Tighten existing key size checks to account for kNumInternalBytes (8-byte internal key suffix), using new kMaxWriteBatchKeySize constant * Add missing size checks to Delete, SingleDelete, and DeleteRange (both Slice and SliceParts variants) SstFileWriter (sst_file_writer.cc): * Add key and value size checks in AddImpl and DeleteRangeImpl, which bypass WriteBatch and go directly to the table builder MergeHelper (merge_helper.cc): * Add merge result size check (> uint32_t max) in both overloads of TimedFullMergeImpl, returning Corruption if exceeded * Add PartialMergeMulti result size check in MergeUntil CompactionIterator (compaction_iterator.cc): * Add size check on compaction filter output values (kChangeValue and kChangeWideColumnEntity), returning Corruption if > 4GB meta_blocks.cc: * Use Slice with uint32_t-truncated sizes in PropertyBlockBuilder::Finish to handle potentially oversized user property collector output Needed follow-up: * Check for blocks that are mis-encoded due to overflowing 32 bits for restart point offsets (and similar). See FIXME comments in block_builder.cc for why this is tricky. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14461 Test Plan: New extreme-size unit tests, along with some testing infrastructure improvements: Added test::HasBigMem() (in test_util/testharness.h) which returns true when the system has ≥128GB RAM (via sysconf(_SC_PHYS_PAGES) on Linux/macOS) or when ROCKSDB_BIGMEM_TESTS is set. All extreme-size tests use HasBigMem() to skip gracefully on smaller machines rather than being permanently disabled. Where possible, tests use MemMapping::AllocateLazyZeroed() to supply large keys/values as Slices backed by anonymous mmap (cleaner with new MemMapping::AsSlice()). On Linux, read-only access to these pages maps to the shared kernel zero page, so the source data consumes no physical RAM — only the destination copy (e.g., WriteBatch::rep_) materializes, cutting peak memory roughly in half vs. std::string. Tests (all enabled, skip via HasBigMem()): ./write_batch_test --gtest_filter='*LargeKeyValueSizeLimit*' ./external_sst_file_basic_test --gtest_filter='*LargeSizeSstFileWriter*' ./merge_test --gtest_filter='*LargeMergeResultRejected*' ./merge_helper_test --gtest_filter='*LargePartialMergeResultRejected*' ./db_compaction_test --gtest_filter='*CompactionFilterLargeValueRejected*' All 5 pass (verified on a 128GB+ machine). On smaller machines, all 5 bypass cleanly with "insufficient memory for reliable continuous testing". Reviewed By: xingbowang Differential Revision: D96521899 Pulled By: pdillinger fbshipit-source-id: 70f4b5e6a23ab074d60e653fbb7ddc5edbe162ab |
||
|
Josh Kang
|
f3a20854e0 |
Revert trace changes (#14600)
Summary: Reverts the two commits https://github.com/facebook/rocksdb/pull/14578 https://github.com/facebook/rocksdb/pull/14575 that changed tracer behavior. The correct behavior is that trace should be written to before WAL. Although a trace file may not be fully consumed, it is recycles after each crash, so there is no need to worry about applying an uncomited write. The original crash test was a false positive for a different error. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14600 Reviewed By: xingbowang Differential Revision: D100397989 Pulled By: joshkang97 fbshipit-source-id: 3da6fb80f682ac6f9529c1a76eaf169e38cf2477 |
||
|
Xingbo Wang
|
c47e35395b |
db: disable read-path tombstone conversion for partial UDT reads (#14595)
Summary: T263957043 reproduces in whitebox crash testing when DBIter converts a run of point deletes into a range tombstone while reading with an older user-defined timestamp. The scan can observe an older delete for the boundary keys but miss newer live versions of the same keys and interior keys, so the synthesized tombstone is based on partial visibility and later hides valid max-timestamp reads. ## Problem Read-path range tombstone conversion (`min_tombstones_for_range_conversion`) assumes the scan observes all interior live keys between the start and end of a contiguous delete run. With user-defined timestamps, a read at an older timestamp can see deletes but miss newer Puts for the same keys. The synthesized range tombstone then incorrectly covers those newer versions, causing data loss on subsequent max-timestamp reads. ## Fix Gate read-path range conversion on full timestamp visibility. The optimization is now only enabled when: 1. There is no `table_filter` (existing guard — SSTs hidden by filter can break the contiguity assumption) 2. There is no `iter_start_ts` (time-travel scans see a subset of versions) 3. Either no read timestamp is set, or the read timestamp is the max timestamp (all `0xff` bytes) This is done via a new helper `HasFullTimestampVisibility()` checked at `DBIter` construction time. ## Test Changes - Updated all existing UDT test variants in `ReadPathRangeTombstoneTest` to read at max timestamp so the optimization remains covered where it is valid. - Added `UDTOlderTimestampDisablesInsertion`: a regression test that writes at ts=1, deletes at ts=2, writes again at ts=3, then reads at ts=2. Verifies no range tombstone is synthesized and a subsequent max-timestamp read still sees all live values. ## Validation - `make -j192 db_iterator_test` - `./db_iterator_test --gtest_filter='*ReadPathRangeTombstoneTest*'` - `./db_iterator_test --gtest_filter='*UDTOlderTimestampDisablesInsertion*' --gtest_repeat=5` - Reran the original whitebox crash-test seed from T263957043 against the patched tree; crash-recovery verification passed Pull Request resolved: https://github.com/facebook/rocksdb/pull/14595 Reviewed By: joshkang97 Differential Revision: D100235999 Pulled By: xingbowang fbshipit-source-id: 7a287f3c7fdc47428fda0ab89eedd5d43f663985 |
||
|
Xingbo Wang
|
a9ac4df630 |
env: avoid unused info in TSAN mapping helper (#14597)
Summary: TsanAnnotateMappedMemory() always builds a TsanMappedMemoryInfo payload for TEST_SYNC_POINT_CALLBACK(), but in builds where sync points compile to a no-op the local variable becomes unused and Clang fails with -Werror,-Wunused-variable. Move the explicit (void)info cast so it applies regardless of whether __SANITIZE_THREAD__ is enabled. This keeps the helper warning-free in: - non-TSAN builds - builds where TEST_SYNC_POINT_CALLBACK expands to nothing - TSAN builds that still want the SyncPoint payload for tests This was missed by CI because the warning only appears in the COMPILE_WITH_TSAN=1 + NDEBUG/release configuration. Our CI matrix covers release builds without TSAN and TSAN builds in debug mode, but not the TSAN+NDEBUG combination. No behavior change is intended. The helper still: - exposes the mapping metadata to SyncPoint-based tests - calls AnnotateNewMemory() in TSAN builds - remains a no-op with respect to runtime behavior in non-TSAN builds Pull Request resolved: https://github.com/facebook/rocksdb/pull/14597 Differential Revision: D100333855 Pulled By: xingbowang fbshipit-source-id: 3ff0b0c8a51b4959b7a36984e40ac31e49da82eb |
||
|
Xingbo Wang
|
4254c705a2 |
env: reset TSAN state for recycled mappings (#14594)
Summary: TSAN was reporting false data-race warnings after the kernel reused a virtual address for a new mapping while TSAN still associated that address range with the old mapping. This showed up in two RocksDB paths: - `io_uring` setup on helper threads vs later `io_uring` `MultiRead` access - `io_uring` setup vs file mmap reads when `use_mmap_reads` is enabled ## Changes Introduce `TsanAnnotateMappedMemory()`, which calls `AnnotateNewMemory()` as soon as a fresh mapping exists so TSAN drops any stale shadow state for the recycled address range. Apply the helper to: - io_uring SQ/CQ/SQE mappings created by `CreateIOUring()` - `PosixFileSystem` mmap-read mappings - `PosixFileSystem` raw memory-mapped file buffers - `PosixMmapFile` writable mmap region growth Also add deterministic regression tests that force virtual-address reuse with `MAP_FIXED` for both the io_uring and mmap-read cases. The tests pass mapping metadata over a pipe so teardown/remap ordering is deterministic without introducing TSAN-visible synchronization that would mask the problem. ## Testing - `COMPILE_WITH_TSAN=1 make -j192 env_test` - `env_test --gtest_filter='EnvPosixTest.SupportedOpsNoAsyncIOOnIOUringInitFailure:EnvPosixTest.IOUringAddressReuseNoTsanFalsePositive:EnvPosixTest.MmapReadAddressReuseNoTsanFalsePositive'` Pull Request resolved: https://github.com/facebook/rocksdb/pull/14594 Reviewed By: archang19 Differential Revision: D100235746 Pulled By: xingbowang fbshipit-source-id: 8ef8d85c08a7646540b9c11c3741978898a5af3d |
||
|
Davis Rollman
|
bad2d5b0af |
Migrate fbcode coro references from folly/experimental to folly/coro (5/6 - fbcode a-m)
Summary: Migrate coro references in remaining fbcode services (a-m). Reviewed By: iahs Differential Revision: D98841546 fbshipit-source-id: 83bb5a2ef6a29a1ff464efa0e4e0f76b48662ab0 |
||
|
xingbowang
|
1fe1b1bd46 |
Reduce claude review failure CI noise (#14591)
Summary:
### Summary
Fix flaky Claude Code Review workflow failures caused by stale workflow_run SHAs.
Problem:
- The auto-review workflow is triggered from pr-jobs via workflow_run.
- In some cases, by the time the review job starts, the PR branch head has already advanced.
- Then Get PR info cannot find an open PR whose head SHA matches the older workflow_run.head_sha, so it fails with:
- Could not find PR for SHA ... after retries
- This creates unnecessary CI noise even though a newer review job will be triggered for the updated commit.
Solution:
- Treat “PR not found for this head SHA” as a stale run, not an error.
- In Get PR info, replace workflow failure with:
- a warning
- skip=true
- skip_reason=stale_sha:<sha>
- Gate all later auto-review steps on steps.pr_info.outputs.skip != 'true'
- Add a small logging step to report the skip reason
Result:
- stale auto-review runs exit successfully instead of failing
- CI becomes less noisy
- the newest commit still gets reviewed by the later workflow run
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14591
Reviewed By: mszeszko-meta
Differential Revision: D100190375
Pulled By: xingbowang
fbshipit-source-id: fa4a0479a10e953e52b3d99c2b483dae01d3e7fb
|
||
|
Peter Dillinger
|
f3f7feba92 |
Document Flush(), Sync(), and Fsync() contracts; remove dead enum (#14590)
Summary: Flush(), Sync(), and Fsync() on WritableFile and FSWritableFile were essentially undocumented. Added comprehensive API contracts covering durability guarantees (process crash vs. power failure), data readability after Flush(), the implication relationship (Sync implies Flush), and thread-safety (referencing IsSyncThreadSafe()). The contracts are written to be implementation-agnostic, avoiding assumptions about OS-level mechanisms, so they apply equally to local, remote, and virtual/wrapper filesystem implementations. Several such implementations have been audited for adherence. Note that WritableFileWriter currently calls FSWritableFile::Flush() UNNECESSARILY for cases like writing SST files. This should be optimized away in follow-up assuming this interpretation of the contract is agreed upon. Also removed the dead FileSystem::WriteLifeTimeHint enum, which was never referenced anywhere. The actively used enum is Env::WriteLifeTimeHint. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14590 Test Plan: Documentation-only change to public headers, plus removal of an unused enum with no references. No functional changes. Verified no compilation errors with existing tests. Reviewed By: xingbowang Differential Revision: D100183288 Pulled By: pdillinger fbshipit-source-id: 7f1982c41e09fe39896dbc6cc328316be559ec4a |
||
|
Peter Dillinger
|
b7395b3fa4 |
Allow Compressor to override parallel_threads for SST building (#14580)
Summary:
Add Compressor::GetRecommendedParallelThreads() virtual method so that the compression subsystem can influence the parallel compression thread count used when building SST files. The base Compressor returns 0 (no opinion), while built-in compressors (via CompressorBase) return the parallel_threads value from their CompressionOptions. This gives CompressionManager a clean mechanism to override parallel_threads by customizing the CompressionOptions passed to GetCompressor() in its GetCompressorForSST() implementation.
The table builder now reads the thread count from the compressor after creation, rather than only from CompressionOptions directly. Hard structural constraints (partition_filters without decoupled mode, user_defined_index_factory) still force single-threaded compression regardless of the compressor's recommendation.
Also adds a sync point in MaybeStartParallelCompression for test observability, and compression_test coverage for the new functionality.
Bonus: extends CompressionManagerCustomCompression test to cover re-opening a DB at format_version=7 with a non-default compression manager that uses the built-in CompatibilityName ("BuiltinV2"). Verifies that data is readable after re-opening without the original manager, and that neither GetId() nor CompatibilityName() resolves to the custom manager via CreateFromString.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14580
Test Plan:
New unit tests in compression_test:
- GetRecommendedParallelThreads: verifies built-in compressors return the parallel_threads from their CompressionOptions for all supported compression types.
- CompressionManagerOverridesParallelThreads: end-to-end test with a custom CompressionManager that overrides parallel_threads from 1 to 4 in GetCompressorForSST, verified via sync point that parallel compression actually activates with the overridden thread count, plus data readback verification.
Existing parallel compression tests (DBCompressionTestMaybeParallel) continue to pass.
Reviewed By: xingbowang
Differential Revision: D99896343
Pulled By: pdillinger
fbshipit-source-id: 6b3a30856a78641714d33ec7ba2099f33533d3af
|
||
|
zaidoon
|
3b38fde293 |
Support UDI as primary index (#14547)
Summary: Add `use_udi_as_primary_index` option to `BlockBasedTableOptions`. When enabled, the UDI becomes the primary index — all reads (including internal operations like compaction and VerifyChecksum) automatically route through the UDI without needing `ReadOptions::table_index_factory`. Both the standard binary search index and the UDI are always fully built. The standard index serves as a safety fallback (e.g., for backup/restore or rollback to a non-UDI configuration). A future refactor will extract the index abstraction to allow skipping the standard index build when the UDI is primary (see discussion below). ## Write path - `UserDefinedIndexBuilderWrapper` always forwards `AddIndexEntry` and `OnKeyAdded` to both the internal standard builder and the UDI builder - New `udi_is_primary_index` table property marks primary-mode SSTs - Validates incompatible options at `DB::Open` and builder creation: partitioned index, partitioned filters, missing `user_defined_index_factory` ## Read path - `UserDefinedIndexReaderWrapper` defaults to UDI when `udi_is_primary_`, even when `ReadOptions::table_index_factory` is null — this handles the 15+ internal call sites that don't set `table_index_factory` - `use_udi_as_primary_index` automatically enforces `fail_if_no_udi_on_open` to prevent silent data loss if SSTs are opened without UDI support ## Rollback Since the standard index is always fully populated, rollback from primary mode is straightforward: set `use_udi_as_primary_index=false`. No compaction required — SSTs written in primary mode are immediately readable through the standard index. ## Public API - `BlockBasedTableOptions::use_udi_as_primary_index` (default: false) - `UserDefinedIndexBuilder::EstimatedSize()` — pure virtual, O(1) via running counter in the trie implementation ## Bug fixes (issues https://github.com/facebook/rocksdb/issues/14560, https://github.com/facebook/rocksdb/issues/14561, https://github.com/facebook/rocksdb/issues/14562) Fixed trie index correctness bugs that caused crash test failures: - **Always-on seqno encoding**: `must_use_separator_with_seq_` is now unconditionally true. Non-boundary separators store tag=0 (sentinel), same-user-key boundaries store the real tag, and the last block stores its real last-key tag. This fixes the `NonBoundaryTag` bug where non-boundary separators with `kMaxSequenceNumber` caused the post-seek correction to incorrectly advance past the correct block. - **Standard index always built in primary mode**: An empty (stub) standard index block caused behavioral divergence in `BlockBasedTableIterator` under concurrent flush/compaction, leading to `test_batches_snapshots` prefix scan inconsistencies. ## Stress test - `use_udi_as_primary_index` flag randomized by `db_crashtest.py` - Both primary and secondary UDI modes exercised in crash tests - Trie index probability halved (~6%) per reviewer request - Re-enables trie crash tests (disabled by https://github.com/facebook/rocksdb/issues/14559) ## Tests - Parameterized `TrieIndexDBTest` on UDI mode (secondary vs primary) - New factory-level tests: non-boundary separator seek, Prev within overflow, SeekToLast with overflow, empty trie, overflow exhaustion, all-scans-exhausted - New DB-level tests: multi-CF coalescing iterator, GetEntity with explicit snapshot, reverse iteration across same-user-key blocks, non-boundary separator seek correctness, rollback from primary Pull Request resolved: https://github.com/facebook/rocksdb/pull/14547 Reviewed By: anand1976 Differential Revision: D99494181 Pulled By: xingbowang fbshipit-source-id: ca52a0d5c0e523770c80e1fe2b9b5d50406b67bc |
||
|
Xingbo Wang
|
3070f73e97 |
Wide-column blob separation: lazy resolution through read, compaction, and write paths (#14386)
Summary: Wide-column blob separation: lazy resolution through read, compaction, and write paths Extend blob direct write to support wide-column entities (PutEntity), and add lazy blob resolution for wide-column values across all read and compaction paths. **Write path -- PutEntity blob separation:** - BlobWriteBatchTransformer::PutEntityCF now extracts large column values (>= min_blob_size) to blob files and serializes V2 entities with BlobIndex references, matching the existing Put behavior. - Add MaybePreprocessWideColumns() static helper to share blob extraction logic between the WriteBatch transformer and the new PutEntity fast path. - Add PutEntityFastPath() in DBImpl that preprocesses columns (sort, blob extract, serialize) before calling WriteImpl, skipping the redundant WriteBatch transformation pass. Trace batch preserves the original columns. **Read path -- blob resolution for Get/MultiGet/Iterator:** - GetContext::SaveValue resolves V2 entity blob columns eagerly: for value (Get), resolves the default column's blob reference; for columns (GetEntity), resolves all blob columns and re-serializes as V1. - DBIter::SetValueAndColumnsFromEntity detects V2 entities, deserializes with DeserializeV2, and eagerly resolves all blob columns via a new ReadPathBlobResolver. Resolved values are cached in the resolver and wide_columns_ Slices point into the cache, avoiding copies. - Add ReadPathBlobResolver (new file) -- on-demand blob fetcher for the read path with per-column caching, used by both DBIter and GetContext. - BlobFetcher gains allow_write_path_fallback to read from in-flight direct-write blob files not yet visible through Version (pre-flush reads). - Memtable lookups for Get(key) on V2 entities with a blob default column now return the blob index with is_blob_index=true, triggering the existing BDW resolution in MaybeResolveWritePathValue. - MaybeResolveWritePathValue (renamed from MaybeResolveDirectWriteBlobIndex) now also resolves V2 entity blob columns for GetEntity/MultiGetEntity, re-serializing as V1 after resolution. **Compaction path -- filter, GC, and extraction:** - CompactionIterator::InvokeFilterIfNeeded handles V2 entities: FilterV3 gets eagerly-resolved column values for backward compatibility; FilterV4 gets a CompactionBlobResolver for lazy on-demand resolution. - Add CompactionFilter::FilterV4 with WideColumnBlobResolver* parameter and SupportsFilterV4() opt-in. Default delegates to FilterV3. - CompactionBlobResolver (new class) implements WideColumnBlobResolver for the compaction path with stats tracking. - ExtractLargeColumnValuesIfNeeded extracts inline columns to blob files during compaction (entities without existing blob columns only). - GarbageCollectEntityBlobsIfNeeded relocates blob values from old blob files to new ones during compaction GC, with helpers FetchBlobsNeedingGC, RelocateBlobValues, and SerializeEntityAfterGC. - PrepareOutput unified entity deserialization: single DeserializeV2 call reused by both filter and GC/extraction paths via entity_deserialized_ flag, avoiding redundant parsing. **Merge path -- V2 entity base value resolution:** - MergeHelper::MergeUntil, GetContext::MergeWithWideColumnBaseValue, and DBIter::MergeWithWideColumnBaseValue resolve V2 blob columns before calling TimedFullMerge, using ResolveEntityForMerge. **Blob garbage accounting:** - BlobGarbageMeter tracks blob file in/out flow for V2 entity blob columns via ForEachBlobFileNumber, used for accurate GC decisions. - FileMetaData::UpdateBoundaries tracks oldest_blob_file_number for V2 entities, ensuring blob files referenced by entities are not prematurely deleted. **Serialization improvements:** - WideColumnSerialization::SerializeV2Impl allocates serialized_blob_indices only for actual blob columns (not all columns) and uses autovector for name/value sizes. - Add ForEachBlobFileNumber for lightweight blob file number extraction without full deserialization. - Add ResolveEntityForMerge helper for merge-path resolution. - Add section-size validation in DeserializeV2Impl. - Add empty blob index and column type validation. - blob_column_resolver_util.h -- shared helpers (FindBlobColumn, FindInCache, CacheInlinedBlob) used by both ReadPathBlobResolver and CompactionBlobResolver. **Testing:** - db_blob_direct_write_test: end-to-end PutEntity with BDW before/after flush, verifying Get, GetEntity, MultiGetEntity, and Iterator. - db_blob_index_test: ~1550 lines covering V2 entity blob resolution through Get, GetEntity, MultiGet, Iterator, compaction filter (V3 compat and V4 lazy), merge with blob base, and compaction GC/extraction. - compaction_iterator_test: ~950 lines testing entity blob GC, extraction, filter interaction, and combined GC+filter scenarios. - db_wide_basic_test: ~1200 lines for wide-column lazy blob resolution through all read paths plus compaction round-trips. - db_open_with_config_test: ~450 lines for BDW entity config validation. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14386 Reviewed By: anand1976 Differential Revision: D99739701 Pulled By: xingbowang fbshipit-source-id: 6badd89b577f3054802eaaa654738468efb9dbdb |
||
|
Anand Ananthabhotla
|
9d609dd6fe |
Fix MultiScanIndexIterator crash on reseek after exhaustion (#14581)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14581 In `MultiScanIndexIterator::Seek()` Case 3, when re-entering a scan range after all ranges were exhausted, `block_idx = std::max(cur_scan_start_idx, cur_idx_)` could produce an out-of-bounds value because `cur_idx_` was left at `block_handles_.size()` from previous exhaustion. `SeekToBlockIdx()` unconditionally set `valid_ = true` without checking bounds, causing the subsequent `value()` call to hit the assertion `cur_idx_ < block_handles_.size()`. Added bounds check before `SeekToBlockIdx()` in Case 3 to correctly report exhaustion instead of crashing. Reviewed By: joshkang97 Differential Revision: D99604049 fbshipit-source-id: 9d5d91afde7c0984a7b4c2f62604f27f19b07922 |
||
|
Xingbo Wang
|
4d8a0acd76 |
Disable read-path range tombstone synthesis when ReadOptions.table_filter is set (#14586)
Summary: **Root cause:** DBIter's read-path range tombstone conversion assumes the iterator can observe *every* interior live key between point tombstones. When `ReadOptions.table_filter` is active, whole SSTs can be hidden from the scan. DBIter may then mistake non-contiguous deletes for a contiguous run and synthesize a range tombstone that covers real, still-live keys — silently corrupting the read path. This is not UDT-specific; UDT just changes the failure surface. The stress test failures that triggered investigation were from `db_stress_tool/no_batched_ops_stress.cc` applying an SQFC-backed `table_filter` for range queries even when `total_order_seek` is forced. ## Fix `db/db_iter.cc`: When initializing `min_tombstones_for_range_conversion_`, set it to 0 (disabling range conversion) whenever `read_options.table_filter` is non-null. This is the conservative and correct behavior — filtered scans don't provide the full key visibility the optimization requires. ## Test `db/db_iterator_test.cc` — new test `ReadPathRangeTombstoneTest.TableFilterHiddenInteriorKey`: - Constructs five flushed SSTs so that the live interior key `"b"` lives in a 2-entry SST that the `table_filter` hides. - Keeps the active memtable non-empty (key `"zz"`) so range conversion has a valid insertion point — without this the test is a false negative. - Asserts `inserted_ranges_.size() == 0` (no synthesis occurred). - Verifies `"b"` is still readable via a normal `Get`. - Runs both with and without UDT (user-defined timestamps) via `SCOPED_TRACE`. On unpatched HEAD this test fails with `inserted_ranges_.size() == 1`; with the fix it passes. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14586 Reviewed By: joshkang97 Differential Revision: D100023487 Pulled By: xingbowang fbshipit-source-id: 5ef885c89a6cfd7a97b814e38bdcc48d3a1ab349 |
||
|
Josh Kang
|
3efe9460ba |
Fix Range Tombstone Entry Accounting Bug (#14579)
Summary: Crash test T263619547 exposed a flush input accounting bug in `FragmentedRangeTombstoneList`. When the constructor sees that the incoming range tombstones are not sorted, it falls back to rereading all tombstones into `keys`/`values` and sorting them via `VectorIterator` before fragmentation. However, `num_unfragmented_tombstones_` was left with the partial count from the aborted first pass. In timestamp stripping flushes, this stale count could make flush verification report `Expected X entries in memtables, but read Y` even though all tombstones were still processed. Fix the slow path by resetting `num_unfragmented_tombstones_` from `keys.size()` after the second pass. Add a regression test that feeds unsorted range tombstones into the fragmenter and verifies the full tombstone count is preserved. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14579 Test Plan: - New unit test that fails without the fix. Reviewed By: xingbowang Differential Revision: D99872374 Pulled By: joshkang97 fbshipit-source-id: 38e2bb73c68dc1c677870b8973c93b933ded9038 |
||
|
Xingbo Wang
|
4dd05023c0 |
Rocksdb Crash Test failed: unknown (#14584)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14584 Reviewed By: mszeszko-meta Differential Revision: D98721280 fbshipit-source-id: 0a0f0da678312424b43dfab449e636cb1782d9cf |
||
|
Maciej Szeszko
|
6a618d0686 |
Rocksdb Warm Storage Test failed: assertion failed - iters[i]->status().ok() (#14583)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14583 Reviewed By: joshkang97, xingbowang Differential Revision: D99576714 fbshipit-source-id: 71f6cccfc2c5f9043e656e6a81e794d555919fe6 |
||
|
Josh Kang
|
ec832ff42c |
Minimize WAL and and tracer gap (#14578)
Summary: https://github.com/facebook/rocksdb/pull/14575 addresses an existing issue where trace record before WAL lead to inconsistent expected state. However, placing the trace record after the WAL is still problematic because. The trace may not have all the records in the WAL, resulting in `Error restoring historical expected values -> Trace ended before replaying all expected write ops` on recovery. This change minimizes the gap to reduce stress test failures, but it is still not a full solution. It is however still better than previously writing the trace before the WAL because the error is much more obvious when writing the trace after WAL. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14578 Test Plan: Existing CI passes. Reviewed By: xingbowang Differential Revision: D99870545 Pulled By: joshkang97 fbshipit-source-id: 024ee3b15e258afd2dd8ff98db7cbe6a01906be7 |
||
|
Josh Kang
|
91fa765f3a |
Rocksdb Crash Test failed: assertion failed - corruption: Sequence number is being set backwards dur (#14567) (#14567)
Summary: Handle the case where a transaction commit with `commit_bypass_memtable` succeeds in writing the commit marker to WAL, but then `IngestWBWIAsMemtable` fails (e.g., WAL creation failure during `SwitchMemtable` or invalid column family). Previously this failure path was not distinguished from a direct WBWI ingest, so the DB could continue operating with committed data durable in WAL but not published to memtables. This resulted in a higher seqno in the WAL that what the DB's latest seqno was. Now `IngestWBWIAsMemtable` takes an `ingest_wbwi_for_commit` parameter. When true and the ingest fails (without a prior memtable update), the DB sets a fatal background error, stopping all writes until the DB is closed and reopened for WAL recovery. A close and reopen is required because the auto-recovery mechanism works by flushing memtables to disk, but here the committed data was never published to memtables in the first place — it only exists as prepare + commit records in the WAL. Only a full WAL replay during `DB::Open` can reconstruct the committed transaction data and insert it into memtables. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14567 Test Plan: - Added `CommitBypassMemtableTest.SwitchMemtableFailureStopsDBUntilReopen`: - Injects an IO error during `SwitchMemtable` WAL creation via `TEST_SYNC_POINT_CALLBACK` - Verifies commit returns `Corruption` status - Verifies DB enters fatal background error state and rejects further writes - Verifies that after close and reopen, committed data is recovered from WAL and the DB resumes normal operation Reviewed By: xingbowang Differential Revision: D98638705 Pulled By: joshkang97 fbshipit-source-id: f116b1f257b19984b0413f6aeba0ba4a7c0a5b25 |
||
|
Peter Dillinger
|
92cedd58af |
Fix table cache leak when flush install fails (#14577)
Summary: When BuildTable succeeds during flush, it caches the output SST file in the table cache for subsequent user reads (builder.cc:460). If the flush then fails to install — e.g., LogAndApply MANIFEST I/O error, CF dropped, or shutdown — the cache entry was never evicted. BuildTable only cleans up its own failures (builder.cc:511), not failures in the install path. The FindObsoleteFiles full-scan backstop would normally catch this by finding the orphan file on disk and evicting the cache entry. However, under crash test metadata read fault injection (open_metadata_read_fault_one_in), GetChildren fails and the orphan is never found, causing the TEST_VerifyNoObsoleteFilesCached assertion to fire during Close(). This is the same class of bug previously fixed in the compaction path by https://github.com/facebook/rocksdb/issues/14469 (Run failure) and https://github.com/facebook/rocksdb/issues/14549 (Install failure), but in the flush path which had no analogous cleanup. Fix: call TableCache::ReleaseObsolete in FlushJob::Run() when the flush fails after BuildTable succeeded (meta_.fd.GetFileSize() > 0). Pull Request resolved: https://github.com/facebook/rocksdb/pull/14577 Test Plan: New unit test DBFlushTest.LeakedTableCacheEntryOnFlushInstallFailure: - Injects failure after BuildTable via sync point, deactivates filesystem to prevent backstop cleanup - Without fix: assertion fires ("Leaked table cache entry") - With fix: passes ``` COMPILE_WITH_ASAN=1 make -j db_flush_test ./db_flush_test --gtest_filter="DBFlushTest.LeakedTableCacheEntry*" [ PASSED ] 1 test. ``` Existing compaction leak tests still pass: ``` COMPILE_WITH_ASAN=1 make -j db_compaction_test ./db_compaction_test --gtest_filter="*LeakedTableCacheEntry*" [ PASSED ] 2 tests. ``` Reviewed By: xingbowang Differential Revision: D99692601 Pulled By: pdillinger fbshipit-source-id: ff5aa1ad165b3abec915844f97f6af9e59d85774 |
||
|
Xingbo Wang
|
3dd6d060e5 |
Upgrade CI Docker images: clang-18→clang-21, ubuntu24 24.0→24.1 (#14576)
Summary: Upgrade the Ubuntu 24 CI Docker image to include clang-21 (from clang-18), and bump all workflow references to the new image tags. Also adds ccache to both images and renames all clang-18 job references to clang-21. This is mainly for upgrading the clang version later used for generating C API automatically. See PR https://github.com/facebook/rocksdb/issues/14572 ### Changes **`build_tools/ubuntu24_image/Dockerfile`** - Add clang-21 installation from LLVM snapshot repo (`apt.llvm.org/noble/llvm-toolchain-noble-21`) - Add ccache - Add comment pointing Meta employees to internal devvm build guide **`build_tools/ubuntu22_image/Dockerfile`** - Add ccache - Add comment pointing Meta employees to internal devvm build guide **`.github/workflows/pr-jobs.yml`** - Rename `build-linux-clang-18-no_test_run` → `build-linux-clang-21-no_test_run` - Rename `build-linux-clang18-asan-ubsan` → `build-linux-clang21-asan-ubsan` - Rename `build-linux-clang18-mini-tsan` → `build-linux-clang21-mini-tsan` - Update all `clang-18`/`clang++-18` references to `clang-21`/`clang++-21` - Update ccache key prefixes: `clang18-asan-ubsan` → `clang21-asan-ubsan`, `clang18-tsan` → `clang21-tsan` - Bump `rocksdb_ubuntu:24.0` → `rocksdb_ubuntu:24.1` **`.github/workflows/nightly.yml`** - Rename `build-linux-clang-18-asan-ubsan-with-folly` → `build-linux-clang-21-asan-ubsan-with-folly` - Update clang-18 → clang-21 compiler references - Bump `rocksdb_ubuntu:24.0` → `rocksdb_ubuntu:24.1` **`.github/workflows/clang-tidy.yml`** - Update clang-18 → clang-21 - Bump `rocksdb_ubuntu:24.0` → `rocksdb_ubuntu:24.1` ### New Docker Images Both images have been built and tested locally. They will be pushed to `ghcr.io/facebook/rocksdb_ubuntu` before this PR is merged. - `ghcr.io/facebook/rocksdb_ubuntu:22.2` — adds ccache, adds devvm build note - `ghcr.io/facebook/rocksdb_ubuntu:24.1` — adds clang-21 from LLVM snapshot repo, adds ccache Pull Request resolved: https://github.com/facebook/rocksdb/pull/14576 Test Plan: Built both images locally and verified CI job names/compiler flags are consistent. Reviewed By: joshkang97 Differential Revision: D99694293 Pulled By: xingbowang fbshipit-source-id: c23c27f5cf870fb2c8b4e3d1cba281d0ce63f9d6 |
||
|
Josh Kang
|
1b817cff45 |
Trace writes only after successful write (#14575)
Summary: Fix trace ordering for `preserve_write_order` mode to only record writes that actually succeeded. Previously, when `preserve_write_order = true`, trace records were emitted **before** confirming the write succeeded — at a point where the write could still fail (e.g., WAL I/O error). This meant the trace could contain records for writes that never actually committed to the DB. When `db_stress` crash testing replays the trace to reconstruct expected state, these phantom records cause the expected state to diverge from reality, leading to false verification failures. This fix moves the `preserve_write_order` tracing to happen **after** the write is confirmed successful (after memtable insert, right before `SetLastSequence`), in all three write paths: `WriteImpl`, `PipelinedWriteImpl`, and `WriteImplWALOnly`. default and pipelined write modes. Replays the trace into a fresh DB and confirms only the successful write is present. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14575 Test Plan: - New unit test: `TracePreserveWriteOrderSkipsFailedWrite` in `db/db_test2.cc` - Tests both `enable_pipelined_write = false` and `true` - Uses `FaultInjectionTestEnv` to force a write failure - Verifies the failed write is absent from both the original DB and the replayed trace Reviewed By: xingbowang Differential Revision: D99624672 Pulled By: joshkang97 fbshipit-source-id: 5793727c91cb01a12fbb4b2cd59615802ae3d394 |
||
|
Anand Ananthabhotla
|
fc85a700cf |
Fix memory accounting leak in IODispatcher ReadIndex() (#14569)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14569 ReadSet::ReadIndex() moves block values out of pinned_blocks_ via std::move, but never releases the associated prefetch memory accounting. This causes ReleaseBlock() and the destructor to skip ReleaseMemory() since they check pinned_blocks_.GetValue() which returns null after the move. Over time, the memory budget is exhausted and no further prefetches can be dispatched when max_prefetch_memory_bytes is set. The bug was introduced in https://github.com/facebook/rocksdb/pull/14401. The fix releases memory accounting in ReadIndex() when moving values out (both for Case 1: block already available, and Case 2: after async IO polling), and zeros block_sizes_ to prevent double-release. Also adds multiscan_max_prefetch_memory_bytes option to db_stress/crashtest for stress testing this code path. Reviewed By: hx235 Differential Revision: D99488961 fbshipit-source-id: 5ddd1f50e2f6ebb357f86e013d781a790e7e558a |
||
|
Anand Ananthabhotla
|
06699cd0fb |
Add IODispatcher memory limiting and multiscanrandom benchmark to db_bench (#14570)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14570 Add support for benchmarking MultiScan with IODispatcher memory limiting in db_bench. This includes: 1. **New flags for existing `multiscan` benchmark:** - `--io_dispatcher_max_prefetch_memory_bytes`: Sets the global memory budget for IODispatcher prefetching across all ReadSets. When this limit is reached, `SubmitJob()` blocks until memory is released. 0 means unlimited (no IODispatcher created). - `--multiscan_max_prefetch_size`: Sets the per-file prefetch size limit (`MultiScanArgs::max_prefetch_size`). 2. **New `multiscanrandom` benchmark with randomized workload:** - Random batch sizes (1-64 ranges per MultiScan call) - Random range sizes (up to ~1MB worth of keys per range) - Generates sorted, non-overlapping ranges each iteration - Supports all IODispatcher flags above - `--use_multiscan` flag: when true (default), uses MultiScan API; when false, uses normal iterators (Seek + Next) for A/B comparison ## Benchmark Results Setup: 5M keys, key=16B, value=256B, 5-level LSM (267 SSTs), direct reads, 8MB block cache, 30s duration. ### Normal Iterators vs MultiScan (`multiscanrandom`) ``` | Mode | ops/sec | |-----------------------------------------|---------| | Normal iterators (use_multiscan=false) | 6 | | MultiScan sync (async=off) | 15 | | MultiScan async (async=on) | 25 | ``` MultiScan sync is **2.5x** faster than normal iterators. MultiScan async is **4.2x** faster. ### MultiScan async=off with Memory Limits (P=4MB peak concurrent memory) ``` | Config | Limit | ops/sec | blocked | |-----------------|--------|---------|---------| | Baseline (10GB) | unlim | 15 | 0 | | Fits budget | 4MB | 15 | 246 | | Exceeds 10% | 3.6MB | 14 | 698 | | Exceeds 50% | 2.67MB | 14 | 1,314 | | Exceeds 100% | 2MB | 15 | 10,724 | ``` Sync IO is disk-bound so memory limiting has negligible throughput impact, even at 2x oversubscription. ### MultiScan async=on with Memory Limits (P=35MB peak concurrent memory) ``` | Config | Limit | ops/sec | blocked | |-----------------|--------|---------|---------| | Baseline (10GB) | unlim | 25 | 0 | | Fits budget | 35MB | 24 | 204,525 | | Exceeds 10% | 31.8MB | 19 | 361,444 | | Exceeds 50% | 23.3MB | 14 | 289,495 | | Exceeds 100% | 17.5MB | 19 | 464,878 | ``` Async IO uses ~9x more peak concurrent memory (35MB vs 4MB) due to multiple in-flight reads. At ~50% oversubscription, async throughput degrades to sync-level, negating the async benefit. Reviewed By: xingbowang Differential Revision: D99489650 fbshipit-source-id: e88d3a980f592e4e86628604e6389a1f2f71cfef |
||
|
Peter Dillinger
|
0186937710 |
Skip AllocateTest block-count checks on btrfs (#14553)
Summary: btrfs accepts fallocate without error but uses copy-on-write, so preallocated extents are not reflected in st_blocks. This caused AllocateTest to fail spuriously on btrfs filesystems. Detect btrfs via statfs and skip only the block-count assertions, keeping the rest of the test (write, flush, close, size checks) intact. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14553 Test Plan: env_test AllocateTest passes on btrfs (skips with message) and would still enforce preallocation checks on ext4. Reviewed By: joshkang97 Differential Revision: D99307962 Pulled By: pdillinger fbshipit-source-id: 8f40c0109aa397e9ca7d9ee4496fa0614e971970 |
||
|
Peter Dillinger
|
8ec2177bd1 |
Fix missing sync point in DeleteScheduler MarkAsTrash failure path (#14556)
Summary:
When MarkAsTrash fails (e.g., rename error due to filesystem conditions), AddFileToDeletionQueue falls back to deleting the file directly via fs_->DeleteFile(). This bypasses DeleteFileImmediately() and its TEST_SYNC_POINT("DeleteScheduler::DeleteFile") callback, causing tests that count deletions via sync points to undercount. This explains flaky failures in DBSSTTest.DeleteSchedulerMultipleDBPaths where bg_delete_file is 4 instead of the expected 8.
Fix by calling DeleteFileImmediately() in the error path instead of fs_->DeleteFile() directly. DeleteFileImmediately already handles the sync point, OnDeleteFile tracking, and FILES_DELETED_IMMEDIATELY stat, so the manual bookkeeping is also removed to avoid duplication.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14556
Test Plan:
- Existing tests: db_sst_test (DeleteSchedulerMultipleDBPaths, DestroyDBWithRateLimitedDelete) and delete_scheduler_test all pass.
- Ran DeleteSchedulerMultipleDBPaths 100x with COERCE_CONTEXT_SWITCH=1.
Reviewed By: joshkang97
Differential Revision: D99308177
Pulled By: pdillinger
fbshipit-source-id: 3918c4800c2420ff1ae667beb321da3f27406420
|
||
|
Josh Kang
|
156328107d |
Fix range deletion crash test errors (#14573)
Summary: Fix two bugs in read-path range tombstone conversion (`min_tombstones_for_range_conversion`): 1. **SeekToLast stale saved_key_**: `SeekToLast()` did not clear `saved_key_` before calling `PrevInternal()`. A stale key from a prior `Seek()` would be swapped into `range_tomb_end_key_`, corrupting the tombstone tracking bounds and triggering an assertion failure when `range_tomb_first_key_ > end_key`. 2. **Tombstone inserted at wrong sequence**: The current behavior uses the "max" seqno of the tombstones as the insertion seqno. This is not correct because it does not take into account the seqno range deletions seen throughout the iteration. Unfortunately, range deletions are not visible to the db_iter as the are filtered at the merging iterator level. A future refactor may try to expose this, but currently the simplest solution is to just use the iterator seqno. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14573 Test Plan: Unit tests for both bugs that fail without the fix. Reviewed By: xingbowang Differential Revision: D99569668 Pulled By: joshkang97 fbshipit-source-id: bb1b154beccced7438831cd5cf2780ad1e11a4cc |