mirror of
https://github.com/facebook/rocksdb.git
synced 2026-07-07 14:47:40 +08:00
3b446089141659fad25328c5ea3e7ed283df46e4
13735 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Josh Kang
|
3b44608914 | Update HISTORY.md and version for 11.1.2 v11.1.2 | ||
|
Josh Kang
|
83a5b52eda |
Do not allow read only DBs to delete obsolete files (#14881)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14881 Read-only DB instances can share a directory with a live writer, so their view of live files is only a snapshot from the time they opened. After read-only DB open began setting `opened_successfully_` for its intended open-success semantics, that state had an unintended side effect: the common close path treated a successful read-only open like a read-write open and ran obsolete-file cleanup. If the writer created or made files live after the read-only handle opened, that cleanup could use the stale read-only live set and delete files still needed by the writer. This change records whether a `DBImpl` was opened read-only and skips close-time obsolete-file cleanup for read-only DBs instead of simply keeping `opened_successfully_=false`, which could be confusing and easily mistaken in the future again. For completeness I also updated other callsites of `opened_successfully_`, but those should not be real bugs as ReadOnly DBs do not run flushes/compactions or write to WALs. Reviewed By: xingbowang Differential Revision: D109622445 fbshipit-source-id: d7be4b20fce86ccb218a63ac6f5b707b316aac79 |
||
|
anand76
|
6cdeb9d9d0 | Update HISTORY.md and version for 11.1.1 v11.1.1 | ||
|
Josh Kang
|
ea72ee89da |
Support Pinnable Reads In SstFileReader (#14500)
Summary: Add `SstFileReader::Get` (single-key) and `SstFileReader::MultiGet` (PinnableSlice) overloads to enable zero-copy point lookups directly from SST files. The existing `MultiGet(std::string*)` is refactored to delegate to the new `MultiGet(PinnableSlice*)`, which writes results directly into caller-provided `PinnableSlice` values instead of copying through an intermediate buffer. The single-key `Get` uses `TableReader::Get` with a `GetContext` for efficient single-key lookups without the overhead of MultiGet's sorting and batching machinery. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14500 Test Plan: - New unit tests Reviewed By: xingbowang Differential Revision: D97825648 Pulled By: joshkang97 fbshipit-source-id: 17f3edd59bbf4747d17309c44ef12f0d952ea4eb |
||
|
Josh Kang
|
37c3a6de09 |
Support ExternalTable PinnableSlice Get (#14497)
Summary: The ExternalTableReader `Get` API has been modified to use PinnableSlice instead of std::string, this will allow implementations to utilize zero-copy Gets (e.g. reading from mmap or a cache). This is not a compatible change, but the API is marked as experimental, so should be allowed. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14497 Test Plan: - New unit tests Reviewed By: xingbowang Differential Revision: D97782011 Pulled By: joshkang97 fbshipit-source-id: 8a9e8c5bc5ff5e8dee6c0f2ee745521f09042cef |
||
|
Anand Ananthabhotla
|
ede2cbd67b |
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 |
||
|
Anand Ananthabhotla
|
b21eaa91c2 |
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 |
||
|
Jialiang Tan
|
405e1a4ac6 |
Add block_decompress_count to PerfContext (#14557)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14557 RocksDB's PerfContext tracks block_decompress_time but has no counter for the number of block decompressions. The global Statistics ticker NUMBER_BLOCK_DECOMPRESSED exists but is not accessible through PerfContext, which is the thread-local, per-operation metric system used by benchmarks. Add block_decompress_count as a new PerfContext counter (gated at kEnableCount level) incremented in DecompressBlockData alongside the existing NUMBER_BLOCK_DECOMPRESSED ticker. This enables benchmarks and applications to observe how many blocks required decompression per operation, complementing the existing block_read_count. Reviewed By: anand1976 Differential Revision: D99233514 fbshipit-source-id: 40a68c1d9321f560cebdb7c30a544a0c62ae64f0 |
||
|
anand76
|
a201a3c61a | Update HISTORY.md for 11.1.0 | ||
|
Xingbo Wang
|
77a0e347bb |
Add Claude code review CI workflow (#14480)
Summary: Add GitHub Actions workflows for AI-powered code review on PRs using Claude (Anthropic). Follows the clang-tidy security pattern with two separate workflows for privilege separation. ## Trigger Modes **1. Auto** — Runs after `pr-jobs` workflow completes successfully via `workflow_run`. Safe for fork PRs (runs from default branch, never executes PR code). **2. Manual** — Maintainers comment `/claude-review [focus area]` or `/claude-query <question>` on any PR. Restricted to 15 authorized team members. **3. workflow_dispatch** — For manual testing. ## Security Model (Two-Workflow Separation) Same pattern as clang-tidy: **`claude-review.yml`** (analysis): - Runs Claude with `ANTHROPIC_API_KEY` - Has ONLY `contents: read` — no PR write, no issue write - Saves review markdown + metadata as artifact **`claude-review-comment.yml`** (posting): - Triggers on `workflow_run` completion - Downloads artifact and posts/updates PR comment - Has `pull-requests: write` but never runs AI This separation prevents a crafted PR from tricking Claude into exfiltrating write tokens. ## Review Methodology Review prompt in `claude_md/code_review.md` (shared with local Claude Code reviews). Five perspectives: - Call-chain analysis (3-5 levels up/down) - Correctness & edge cases - Cross-component & adversarial (10 execution contexts) - Performance - API compatibility & test coverage ## Shared Scripts - `.github/scripts/post-pr-comment.js` — Create-or-update PR comment with marker-based dedup. Now used by both clang-tidy and Claude review. - `.github/scripts/parse-claude-review.js` — Parses `claude-code-base-action` execution log into markdown. ## Files Changed | File | Description | |------|-------------| | `.github/workflows/claude-review.yml` | Analysis workflow (476 lines) | | `.github/workflows/claude-review-comment.yml` | Comment posting workflow (146 lines) | | `.github/scripts/post-pr-comment.js` | Shared PR comment utility (57 lines) | | `.github/scripts/parse-claude-review.js` | Execution log parser (78 lines) | | `.github/workflows/clang-tidy-comment.yml` | Updated to use shared script | | `claude_md/code_review.md` | Review methodology (104 lines) | ## Setup Required Add `ANTHROPIC_API_KEY` secret to the repo settings. ## Testing Tested end-to-end on `xingbowang/rocksdb` fork — both auto and manual triggers, artifact upload/download, comment posting, and duplicate detection all verified working. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14480 Reviewed By: omkarhgawde Differential Revision: D97832666 Pulled By: xingbowang fbshipit-source-id: f80c7d8683ac980614dc4ca66c1e545deb3be504 |
||
|
Andrew Chang
|
89e384ca6f |
Support GetLiveFiles on secondary DB instances (#14475)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14475 GetLiveFiles was previously blocked on secondary instances with Status::NotSupported, even though the operation is safe to perform. The only reason GetLiveFiles was originally blocked is that it defaults to flushing the memtable (flush_memtable=true), which is a write operation. However, the actual implementation in DBImpl::GetLiveFiles (db_filesnapshot.cc) does two things: 1. Optionally flush the memtable — which we skip by passing flush_memtable=false. The secondary already overrides FlushForGetLiveFiles() as a no-op, so even if true were passed it would not actually flush. 2. Read live file state from versions_ under the mutex — this is purely read-only. It iterates the ColumnFamilySet to collect live table and blob file numbers, builds relative file paths for SST files, blob files, CURRENT, MANIFEST, and OPTIONS, and reads the manifest file size. The secondary maintains its own VersionSet which it keeps up to date via MANIFEST replay in TryCatchUpWithPrimary(). So all of this state is valid and accurate — it reflects exactly which files the secondary considers live at its current replay point. This is the same approach used by DBImplReadOnly and CompactedDBImpl, which both delegate to DBImpl::GetLiveFiles with flush_memtable=false. Reviewed By: xingbowang Differential Revision: D97563143 fbshipit-source-id: 8d5b52e26a478ef190eba598819de6527817bcfc |
||
|
Josh Kang
|
3d99378791 |
Fix table handle leak (#14469)
Summary: Fix leaked table cache entries that cause `TEST_VerifyNoObsoleteFilesCached` assertion failure during `DB::Close()` in ASAN crash test builds (T258745630): ``` File 126519 is not live nor quarantined Assertion `cached_file_is_live_or_quar' failed. ``` When a compaction fails *after* `VerifyOutputFiles` succeeds (e.g., at `VerifyCompactionRecordCounts`), the overall `compact_->status` is set to error but each subcompaction's individual `status` remains OK. `SubcompactionState::Cleanup` only checked the individual subcompaction status, so it skipped calling `ReleaseObsolete` on the output files' table cache entries — leaking them. Normally, `Close()`'s backstop (`FindObsoleteFiles(force=true)` + `PurgeObsoleteFiles`) would catch this by finding the orphan file on disk, evicting the cache entry, and deleting the file. However, `FindObsoleteFiles` calls `FaultInjectionTestFS::GetChildren` which can fail under metadata read fault injection (`--open_metadata_read_fault_one_in=8`). The error is silently ignored (`s.PermitUncheckedError()` in db_impl_files.cc:199), so the orphan file is never found and the leaked cache entry is never evicted. The `Cleanup` bug is latent and predates recent changes. PR https://github.com/facebook/rocksdb/issues/14433 added `verify_output_flags` randomization to the crash test, and PR https://github.com/facebook/rocksdb/issues/14456 fixed false-positive corruptions that https://github.com/facebook/rocksdb/issues/14433 caused. Before https://github.com/facebook/rocksdb/issues/14456, `VerifyOutputFiles` would produce false corruption errors that *accidentally prevented the leak* by setting the subcompaction status to non-OK. ### How it triggers **Step 1 — VerifyOutputFiles adds cache entries for compaction output files:** ``` CompactionJob::Run() RunSubcompactions() // all subcompactions succeed // output file 12 written to disk // each sub_compact.status = OK SyncOutputDirectories() // status = OK VerifyOutputFiles() for each output_file: table_cache()->NewIterator(output_file.meta) FindTable() cache->Insert(file_number=12) // <<< ENTRY ADDED TO TABLE CACHE return iterator holding handle delete iter // releases handle, entry stays in LRU // status = OK ``` **Step 2 — A post-verification step fails, overall status set but NOT subcompaction status:** ``` CompactionJob::Run() VerifyCompactionRecordCounts() // returns Status::Corruption(...) FinalizeCompactionRun(status=Corruption) compact_->status = Corruption // <<< OVERALL status = error // each sub_compact.status is still OK! ``` **Step 3 — Install skips InstallCompactionResults (file 12 never enters a Version):** ``` CompactionJob::Install() status = compact_->status // Corruption if (status.ok()) // FALSE InstallCompactionResults() // <<< SKIPPED — file 12 not in any version ``` **Step 4 — CleanupCompaction skips ReleaseObsolete (THE BUG):** ``` CompactionJob::CleanupCompaction() for each sub_compact: sub_compact.Cleanup(table_cache) if (!status.ok()) // checks sub_compact.status = OK // <<< FALSE — individual status is OK! ReleaseObsolete(...) // <<< NEVER CALLED — cache entry leaked! ``` **Step 5 — Metadata read fault prevents backstop from finding the orphan:** `FindObsoleteFiles(force=true)` calls `GetChildren()` to scan the DB directory. `FaultInjectionTestFS::GetChildren` injects a metadata read error (`--open_metadata_read_fault_one_in=8`). The error is silently ignored (`s.PermitUncheckedError()`), so the directory listing is empty and the orphan file is never found. This happens both in the post-compaction cleanup (`BackgroundCallCompaction`) and in `Close()`'s backstop: ``` FindObsoleteFiles(force=true) fs->GetChildren(path, ...) // returns IOError (fault injected) s.PermitUncheckedError(); // error silently ignored // files vector is empty — orphan file 12 not found PurgeObsoleteFiles() // nothing to do — no Evict called ``` **Step 6 — Assertion fires during Close():** ``` CloseHelper() FindObsoleteFiles(force=true) // GetChildren fails again → orphan missed PurgeObsoleteFiles() // nothing to evict TEST_VerifyNoObsoleteFilesCached() for each cache entry: file_number = 12 live_and_quar_files.find(12) == end() // NOT in any version! >>> assert(cached_file_is_live_or_quar) FAILS <<< ``` **Crash test call stack:** ``` frame https://github.com/facebook/rocksdb/issues/9: __assert_fail_base("cached_file_is_live_or_quar", "db_impl_debug.cc", 389) frame https://github.com/facebook/rocksdb/issues/11: DBImpl::TEST_VerifyNoObsoleteFilesCached()::lambda // finds leaked entry frame https://github.com/facebook/rocksdb/issues/18: LRUCacheShard::ApplyToSomeEntries(...) // iterating cache shard frame https://github.com/facebook/rocksdb/issues/19: ShardedCache::ApplyToAllEntries(...) // iterating all shards frame https://github.com/facebook/rocksdb/issues/20: DBImpl::TEST_VerifyNoObsoleteFilesCached() // the verification frame https://github.com/facebook/rocksdb/issues/21: DBImpl::CloseHelper() // during Close frame https://github.com/facebook/rocksdb/issues/22: DBImpl::CloseImpl() frame https://github.com/facebook/rocksdb/issues/23: DBImpl::Close() frame https://github.com/facebook/rocksdb/issues/24: StressTest::Reopen() // crash test reopen frame https://github.com/facebook/rocksdb/issues/25: StressTest::OperateDb() // worker thread ``` ### Fix Pass the overall `compact_->status` to `SubcompactionState::Cleanup` and call `ReleaseObsolete` when *either* the subcompaction status or the overall status is non-OK: ```cpp // Before: if (!status.ok()) { ReleaseObsolete(...); } // After: if (!status.ok() || !overall_status.ok()) { ReleaseObsolete(...); } ``` ## Key changes - **`SubcompactionState::Cleanup`**: Now takes an `overall_status` parameter and calls `ReleaseObsolete` when *either* the subcompaction status or the overall compaction status is non-OK. - **`CompactionJob::CleanupCompaction`**: Passes `compact_->status` (the overall status) to each subcompaction's `Cleanup`. - **Sync point**: Added `CompactionJob::Run():AfterVerifyOutputFiles` for error injection in tests. - **Unit test**: `DBCompactionTest.LeakedTableCacheEntryOnCompactionFailure` uses `FaultInjectionTestFS` to reproduce the crash test scenario — injects error after `VerifyOutputFiles` and deactivates the filesystem so `GetChildren` fails in `FindObsoleteFiles`, preventing the backstop from evicting the leaked cache entry. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14469 Test Plan: - `DBCompactionTest.LeakedTableCacheEntryOnCompactionFailure`: - **Without fix (ASAN)**: assertion fires during `Close()` — `File 12 is not live nor quarantined` - **With fix (ASAN)**: passes — `ReleaseObsolete` properly cleans up the cache entry - **With fix (non-ASAN)**: passes ``` $ COMPILE_WITH_ASAN=1 make -j db_compaction_test $ ./db_compaction_test --gtest_filter="DBCompactionTest.LeakedTableCacheEntryOnCompactionFailure" [ PASSED ] 1 test. ``` Reviewed By: xingbowang Differential Revision: D97190944 Pulled By: joshkang97 fbshipit-source-id: fdfd481cc1e192803cfb7d64052ccb9162c21b94 |
||
|
Hui Xiao
|
f455ab7bd6 |
Fix flaky BackgroundJobPressure test (#14476)
Summary: **Context/Summary:** Remove assertions on flush_running/flush_scheduled from the BackgroundJobPressure listener test. These checks are inherently racy because Flush() can return before the background thread fires the pressure callback. The race: when BackgroundCallFlush re-acquires the DB mutex after cleanup (line 3661), COERCE_CONTEXT_SWITCH=1 or others may call bg_cv_->SignalAll() and sleep before actually acquiring the lock. This spurious signal wakes WaitForFlushMemTables(), which sees the flush is already installed and returns — so Flush() "completes" seen as foreground in test. The test then starts the next Put()+Flush(), scheduling new flush work. When the previous background thread finally wakes and captures the pressure snapshot, it sees the newly scheduled flush (flush_scheduled > 0). If the race happens on the last flush, its callback may not have fired at all by the time the test reads GetSnapshots(), so snapshots.back() can also show stale data. The remaining assertions (compaction counts, speedup, write stall proximity) are not affected: compaction is blocked by sleeping_task in Phase 1-2, and Phase 3 uses TEST_WaitForCompact() whose wait condition (bg_*_scheduled_ counts) is only satisfied after the mutex is re-acquired and callbacks have fired. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14476 Test Plan: COERCE_CONTEXT_SWITCH=1 make -j56 listener_test Before: ~22/50 failures at line 1715 (flush_running != 0) After: 50/50 pass Reviewed By: xingbowang Differential Revision: D97572545 Pulled By: hx235 fbshipit-source-id: e8a57a7a00e41d17bf47ef8e04cea977225a5909 |
||
|
Hui Xiao
|
63c86160cd |
Add OnBackgroundJobPressureChanged listener callback (#14474)
Summary: **Summary:** Add a new EventListener callback `OnBackgroundJobPressureChanged` that fires after every flush or compaction background job completes. The callback delivers a `BackgroundJobPressure` snapshot containing: - Compaction scheduling counters (scheduled/running, combined and per-priority LOW/BOTTOM breakdown) - Flush scheduling counters (scheduled/running) - Write stall proximity percentage (0=healthy, 100=at stall threshold, can exceed 100 when stalling) - Whether compaction speedup is active `CaptureBackgroundJobPressure()` reads scheduling counters and computes write stall proximity from L0 sorted run count and pending compaction bytes (same inputs as `RecalculateWriteStallConditions()`). TODO: add memory-related write stall triggers later. Introduces `num_running_bottom_compactions_` counter to track BOTTOM- priority compactions separately from LOW, enabling per-pool breakdown in the pressure snapshot. The callback fires on the background thread after counter decrements and `MaybeScheduleFlushOrCompaction()`, so the snapshot reflects post- completion state. Uses the same mutex unlock/lock pattern as `NotifyOnFlushCompleted`. A `bg_pressure_callback_in_progress_` counter ensures destructor safety since the callback fires after `bg_flush_scheduled_`/`bg_compaction_scheduled_` are decremented. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14474 Test Plan: - listener_test BackgroundJobPressure: 3-phase test verifying no pressure, pressure build-up (speedup, scheduling, proximity), and pressure relief after compaction completes - db_compaction_test CompactRangeBottomPri: verifies num_running_bottom_compactions_ via sync point - db_stress_tool exercises the callback with RandomSleep() Reviewed By: xingbowang Differential Revision: D97423623 Pulled By: hx235 fbshipit-source-id: 07003c8de226ec29d32b8a88e2d86e5de85cd2cc |
||
|
Jay Huh
|
89322fdd9e |
Skip Wal Recovery on SecondaryDB Open if for Remote Compaction (#14462)
Summary: Skip WAL recovery when opening a secondary DB instance in OpenAndCompact() for remote compaction. WAL replay is unnecessary in this flow since only LSM state from MANIFEST is needed. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14462 Test Plan: - make -j db_secondary_test && ./db_secondary_test — 35/35 passed - make -j compaction_service_test && ./compaction_service_test — 43/43 passed (includes new SkipWALRecoveryInOpenAndCompact test) - make -j options_settable_test && ./options_settable_test --gtest_filter="*DBOptionsAllFieldsSettable*" — 1/1 passed - Removed temporary hack in stress test that disables WAL Reviewed By: hx235 Differential Revision: D96788211 Pulled By: jaykorean fbshipit-source-id: f91a2f861f2450ebc83423ed4c6f5b70da7d9e8b |
||
|
Yoshinori Matsunobu
|
b23fc77aca |
Add per-block-type block read byte perf counters (#14473)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14473 Add separate PerfContext byte counters for data, index, filter, compression-dictionary, and metadata block reads while preserving block_read_byte as the aggregate. Wire the new counters through the block fetch and multi-read data block paths, expose them via the C perfcontext API, and extend table tests to verify byte attribution and that the classified counters sum back to block_read_byte. Reviewed By: xingbowang Differential Revision: D97333746 fbshipit-source-id: 3411844d7fa9c76c9ff28af477b3a72a5d6e5d9b |
||
|
Danny Chen
|
b6f498b2c9 |
Add verify_manifest_content_on_close option (#14451)
Summary: Add a new mutable DB option `verify_manifest_content_on_close` (default: false). When enabled, on DB close the MANIFEST file is read back and all records are validated (CRC checksums via log::Reader and logical content via VersionEdit::DecodeFrom). If corruption is detected, a fresh MANIFEST is written from in-memory state using the existing LogAndApply recovery path. This complements the existing size validation in VersionSet::Close() with content validation, reusing the same manifest reading pattern as VersionSet::Recover(). Implementation plan: ## Part 1: New DB Option — verify_manifest_content_on_close - A new mutable bool DB option (default: false) that can be dynamically toggled via SetDBOptions() at runtime, following the pattern of other mutable manifest options like max_manifest_file_size. - Propagation: SetDBOptions() -> DBImpl::mutable_db_options_ -> versions_->UpdatedMutableDbOptions() -> VersionSet::verify_manifest_content_on_close_ ## Part 2: Core Implementation — Content Validation in VersionSet::Close() - Inserted after existing size check, before closed_ = true - Opens manifest as SequentialFileReader, creates log::Reader with checksum=true - Loops ReadRecord with WALRecoveryMode::kAbsoluteConsistency, decodes each record as VersionEdit - On corruption: fires OnIOError listeners, logs error, calls LogAndApply with empty edit to trigger manifest rewrite from in-memory state - If manifest can't be opened for reading: logs warning, doesn't fail close ## Part 3: Unit Tests (in version_set_test.cc) - ManifestContentValidationOnClose_Clean: enable option, normal close, verify no manifest rotation - ManifestContentValidationOnClose_CorruptRecord: enable option, corrupt manifest via SyncPoint, verify rotation occurs and DB reopens cleanly - ManifestContentValidationOnClose_Disabled: default off, verify content validation does not run - ManifestContentValidationOnClose_SizeCheckFails: truncate manifest so size check fails first, verify recovery via size-check path ## What Happens If a Corruption is Detected If corruption was detected, four things happen: 1. **Notify listeners** — Fires `OnIOError` on all registered event listeners (from db_options_->listeners) so monitoring/alerting systems can observe the corruption event. Uses `FileOperationType::kVerify` to categorize it. 2. **Permit unchecked errors** — `PermitUncheckedError()` silences RocksDB's debug-mode assertion that every `IOStatus` must be inspected. These statuses are informational-only here; the real recovery is via `LogAndApply`. 3. **Log the error** — Writes a `ROCKS_LOG_ERROR` message with the filename for operational visibility (grep-able in production logs). 4. **Rewrite the manifest via `LogAndApply`** — This is the actual recovery. `LogAndApply` is called with an empty `VersionEdit` (no changes). Internally, `LogAndApply` detects that the current `descriptor_log_` is null (it was reset at line 5551, or by the previous `LogAndApply` in the size-check path) and creates a brand-new MANIFEST file. It serializes the entire current in-memory LSM state — all column families, all levels, all file metadata, sequence numbers, etc. — into this new file. It then atomically updates the `CURRENT` file pointer to reference the new MANIFEST. This works because the in-memory state was built from the original manifest during `DB::Open()` and has been kept fully up to date through all subsequent operations (flushes, compactions, etc.) during the DB's lifetime. The on-disk manifest is essentially a journal of changes; `LogAndApply` with an empty edit produces a fresh, compacted snapshot of that state. ## Flow Diagram of Manifest Content Validation VersionSet::Close() │ ├─ Close descriptor_log_ and check size │ └─ Size mismatch? → LogAndApply (rewrite manifest) │ ├─ Content validation (if s.ok() && option enabled) │ ├─ Open manifest for sequential reading │ │ └─ Can't open? → WARN log, continue │ │ │ ├─ For each record: │ │ ├─ ReadRecord (CRC32 check, kAbsoluteConsistency) │ │ └─ DecodeFrom (VersionEdit logical check) │ │ │ └─ Corruption detected? │ ├─ Notify OnIOError listeners │ ├─ LOG_ERROR │ └─ LogAndApply (rewrite manifest from in-memory state) │ └─ closed_ = true; return s; ## How This Relates to the Existing Size Check The existing size check (lines 5556-5582) and the new content validation are complementary: | Check | What it catches | How it checks | |----------------|-----------------------------------------|----------------------------| | Size check | Truncation, partial writes, extra bytes | Compare expected vs actual file size | | Content check | Bit-rot, silent corruption, bad records | CRC32 + VersionEdit decode | The size check catches gross corruption (file too short or too long). The content check catches subtle corruption where the file is the right size but individual bytes have been flipped (e.g., storage media bit-rot, buggy filesystem, incomplete block write). Both recovery paths use the same mechanism: `LogAndApply` with an empty `VersionEdit` to rewrite the manifest from in-memory state. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14451 Reviewed By: xingbowang Differential Revision: D96004906 Pulled By: dannyhchen fbshipit-source-id: 0b0ecdada3a74e97d2cadbba2091b8b577f1d684 |
||
|
Xingbo Wang
|
bcaf2794dc |
Fix TSAN data race in InjectedErrorLog by suppressing benign races (#14467)
Summary:
InjectedErrorLog is a lock-free circular ring buffer designed to be safe to call from signal handlers (which cannot use locks). It has an intentional benign data race between Record() (called by worker threads) and PrintAll() (called by the main thread from a signal/termination handler). The code documents this trade-off in comments, but was missing TSAN suppression annotations.
Simply adding TSAN_SUPPRESSION (__attribute__((no_sanitize("thread")))) is insufficient because TSAN still intercepts libc functions like vsnprintf/snprintf -- accesses through these interceptors are still tracked even when the calling function is annotated.
The fix:
1. Add TSAN_SUPPRESSION to both Record() and PrintAll() to suppress direct field reads/writes in the function body.
2. Restructure both functions to use local stack buffers for vsnprintf/snprintf operations instead of operating directly on shared entry data. This avoids passing shared memory through TSAN-intercepted libc functions.
Also adds fault_injection_fs_test with a ConcurrentRecordAndPrintAll test that exercises the concurrent Record() + PrintAll() pattern and verifies no TSAN race is reported.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14467
Test Plan:
- fault_injection_fs_test passes under TSAN (buck2 test fbcode//mode/dbg-tsan)
- Reverted fix, re-ran: Fatal (6 TSAN warnings) -- round-trip confirmed
- fault_injection_fs_test passes under debug mode (no regression)
Reviewed By: mszeszko-meta
Differential Revision: D96948483
Pulled By: xingbowang
fbshipit-source-id: efdd5eafa12a5a82f973e40aa327901cc5f95033
|
||
|
Andrew Chang
|
1cb7594b87 |
Set correct file_type in BackupInfo::file_details (#14464)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14464 SetBackupInfoFromBackupMeta passes file_ptr->filename (which may contain directory components like "private/1/000008.log") directly to ParseFileName. ParseFileName expects a bare filename, so it fails and file_type stays at the default kTempFile for all files in file_details. Fix by extracting the basename before calling ParseFileName. Reviewed By: mszeszko-meta Differential Revision: D96793040 fbshipit-source-id: 7bb6b633eb07bb7ebda06edbc039a96b9b77b410 |
||
|
Dmitry Vinnik
|
166b4a4487 |
Remove Support Ukraine banner from RocksDB website (#14463)
Summary: Remove the Support Ukraine socialBanner div from the RocksDB docs homepage layout. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14463 Reviewed By: hx235 Differential Revision: D96559640 Pulled By: xingbowang fbshipit-source-id: 508c7bd660760e39cc73977c5e5c1bda204efedd |
||
|
anand76
|
03bded03b4 |
Fix finger.prev_[0] assertion failure in MultiGet finger search (#14465)
Summary: The callback loop in InlineSkipList::MultiGet updated finger.prev_[0] as it walked forward through entries (e.g., merge operands). When the MultiGet batch contained duplicate user keys, the next lookup for the same key would find finger.prev_[0] pointing to an entry that sorts AFTER the lookup key in internal key order (because the lookup key has a high sequence number which sorts first), violating the FindSpliceForLevel precondition: before == head_ || KeyIsAfterNode(key, before). Fix: stop updating finger.prev_[0] in the callback loop. Only finger.next_[0] needs advancing to track the walk-forward position. The prev_[0] from FindGreaterOrEqualWithFinger is always a valid lower bound for any subsequent key, whether it uses kMaxSequenceNumber or a snapshot sequence number. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14465 Reviewed By: xingbowang Differential Revision: D96882549 Pulled By: anand1976 fbshipit-source-id: a733fa9d4f23f8b55a027c257a114c4cf35abe2b |
||
|
zaidoon
|
ec22903914 |
Support all operation types in User Defined Index (UDI) interface (#14399)
Summary: Remove the restriction that limited UDI to ingest-only, Puts-only use cases. This enables UDI plugins (including the trie index from https://github.com/facebook/rocksdb/issues/14310) to work with all operation types: Put, Delete, Merge, SingleDelete, PutEntity, etc. Part of https://github.com/facebook/rocksdb/issues/12396 Pull Request resolved: https://github.com/facebook/rocksdb/pull/14399 Reviewed By: pdillinger Differential Revision: D96392636 Pulled By: xingbowang fbshipit-source-id: 0f1e6c38531fa72539a0e2c6a3dffff333392b4c |
||
|
Josh Kang
|
0c6f741422 |
Rename write_prepared_transaction_test_seqno to write_prepared_transaction_seqno_test (#14453)
Summary: write_prepared_transaction_test_seqno keeps showing up in git changes test binaries need to end with _test so they are ignored by .gitignore Pull Request resolved: https://github.com/facebook/rocksdb/pull/14453 Reviewed By: xingbowang Differential Revision: D96202671 Pulled By: joshkang97 fbshipit-source-id: 91642226bedde37ca72c6af03f300990dca8ff3a |
||
|
anand76
|
c66c14258f |
Add memtable MultiGet finger search optimization (#14428)
Summary: Add memtable batch lookup optimization with finger search Optimize memtable MultiGet by using a finger search on the skip list. After finding key[i], the search path (Splice) is retained as a "finger" for key[i+1]. The next search walks up the finger until the forward pointer overshoots, then descends -- costing O(log d) where d is the distance between consecutive sorted keys, rather than O(log N) from the head each time. Controlled by the new `memtable_batch_lookup_optimization` column family option (default: false). ## Changes - Add `FindGreaterOrEqualWithFinger()` and `MultiGet()` to `InlineSkipList` with optional paranoid validation support (key ordering checks and per-key checksum verification via default parameters) - Add virtual `MultiGet()` to `MemTableRep`, override in `SkipListRep` - Add `memtable_batch_lookup_optimization` CF option - Integrate finger search into `MemTable::MultiGet` -- sorts keys, performs batched finger search, then unscrambles results - Add `db_bench`, `db_stress`, and crash test support - Add unit tests for `InlineSkipList::MultiGet` (5 tests) and integration tests at the DB level (25 `BatchLookup` tests), including paranoid validation tests ## Benchmark Results Setup: 2M keys in memtable, release build (`DEBUG_LEVEL=0`). ### Single-threaded `multireadrandom` | Batch Size | Baseline | BatchOpt | vs Base | |:---:|---:|---:|---:| | 2 | 363,593 | 376,562 | **+3.6%** | | 8 | 385,204 | 386,082 | **+0.2%** | | 32 | 360,339 | 375,105 | **+4.1%** | | 64 | 352,696 | 378,497 | **+7.3%** | ### Multithreaded `multireadrandom` (batch_size=64) | Threads | Baseline | BatchOpt | vs Base | |:---:|---:|---:|---:| | 4 | 171,356 | 185,633 | **+8.3%** | | 8 | 161,478 | 171,392 | **+6.1%** | ### `multireadwhilewriting` (batch_size=64) | Threads | Baseline | BatchOpt | vs Base | |:---:|---:|---:|---:| | 1 | 163,938 | 175,068 | **+6.8%** | | 4 | 109,698 | 120,790 | **+10.1%** | | 8 | 116,623 | 123,658 | **+6.0%** | No regression at any batch size or thread count. The finger is stack-allocated per `MultiGet` call, so there is no shared state or cache-line contention between threads. Concurrent writes don't invalidate the finger's bracket since it only reads via acquire-load `Next()` pointers. Small Batch Size Regression Check: memtable_batch_lookup_optimization ===================================================================== All values are ops/sec. Measured with db_bench (release build). 2M keys in memtable, value_size=100, duration=30s per run. Multithreaded multireadrandom — small batch sizes -------------------------------------------------- Threads | Batch Size | Baseline | BatchOpt | Change 4 | 2 | 532,302 | 540,682 | +1.6% 8 | 2 | 1,044,046 | 1,046,920 | +0.3% 4 | 8 | 633,733 | 630,039 | -0.6% 8 | 8 | 1,260,818 | 1,241,792 | -1.5% multireadwhilewriting — small batch sizes ----------------------------------------- Threads | Batch Size | Baseline | BatchOpt | Change 1 | 2 | 107,284 | 105,217 | -1.9% 4 | 2 | 428,508 | 423,747 | -1.1% 8 | 2 | 854,654 | 864,923 | +1.2% 1 | 8 | 114,788 | 118,496 | +3.2% 4 | 8 | 467,995 | 473,437 | +1.2% 8 | 8 | 935,636 | 960,791 | +2.7% No regression at small batch sizes. Variations at batch_size=2 are within noise (~1-2%). At batch_size=8, modest positive trend (+1-3% in read-while-writing). Pull Request resolved: https://github.com/facebook/rocksdb/pull/14428 Test Plan: - `make check` -- all tests pass - `ASSERT_STATUS_CHECKED=1 make check` -- all tests pass - 2-hour `db_crashtest.py blackbox` with `--memtable_batch_lookup_optimization=1` -- passed - 2-hour `db_crashtest.py blackbox` with `--memtable_batch_lookup_optimization=1 --paranoid_memory_checks=1` -- passed Reviewed By: pdillinger Differential Revision: D95813786 Pulled By: anand1976 fbshipit-source-id: b182a023e1026021f1b9682d1cc024c7729326c7 |
||
|
Omkar Gawde
|
e43171d6ca |
Fix memory leak in ExportColumnFamily for empty column families (#14458)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14458 In `CheckpointImpl::ExportColumnFamily()`, the assignment `*metadata = result_metadata` is inside the `for` loop over `db_metadata.levels`. If the column family has no levels, the loop body never executes, so the `new ExportImportFilesMetaData()` is leaked and the caller receives a nullptr despite a success status. Fix: Move `*metadata = result_metadata` outside the loop. Reviewed By: anand1976 Differential Revision: D95303457 fbshipit-source-id: 6a9be47bcca257803969eb3daac7b91e95143ebf |
||
|
Omkar Gawde
|
1e1097d8d3 |
Fix close(-1) on failed open in btrfs rename fsync path (#14443)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14443 In `PosixDirectory::FsyncWithDirOptions()`, when handling btrfs file rename syncing, if `open()` fails and `fd` is -1, the code unconditionally calls `close(fd)`. Calling `close(-1)` is undefined behavior per POSIX (returns EBADF on Linux), and overwrites the original meaningful open error with a misleading "While closing file after fsync" error message. Fix: Guard the `close()` call with `fd >= 0`. Reviewed By: xingbowang Differential Revision: D95303407 fbshipit-source-id: 9b64b45a09e6ba41d87d164a1c094b4d22f7c186 |
||
|
Xingbo Wang
|
e44a99f560 |
Fix false positive corruption in compaction output verification (#14456)
Summary: Fix a bug where `VerifyOutputFiles()` produces false positive "Key-value checksum of compaction output doesn't match what was computed when written" errors when `verify_output_flags` includes `kVerifyIteration` but `paranoid_file_checks` is false. The root cause is a hash enable flag mismatch between writing and verification: - During compaction writing (`OpenCompactionOutputFile`), the `OutputValidator` hash computation was gated solely by `paranoid_file_checks_`. When false, `enable_hash=false` and the hash stays at 0. - During verification (`VerifyOutputFiles`), a new `OutputValidator` is always created with `enable_hash=true`, computing a non-zero hash. - `CompareValidator()` then compares 0 vs non-zero, producing a false positive corruption. This was exposed by the crash test randomization of `verify_output_flags` added in https://github.com/facebook/rocksdb/issues/14433. Before that change, `verify_output_flags` was always 0, so `kVerifyIteration` was only exercised via `paranoid_file_checks=true` (which correctly enabled the hash during writing). The fix ensures hash computation is enabled during writing whenever either `paranoid_file_checks_` is true OR `verify_output_flags` includes `kVerifyIteration`. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14456 Test Plan: - Added `DBCompactionTest.VerifyIterationWithoutParanoidFileChecks` - Added `DBCompactionTest.VerifyAllOutputFlagsWithoutParanoidFileChecks` - Round-trip verified: tests FAIL without fix, PASS with fix Reviewed By: jaykorean Differential Revision: D96371769 Pulled By: xingbowang fbshipit-source-id: 2f7406327496c7b541e4fa2668894df89eb813e8 |
||
|
Jay Huh
|
5494bc1d76 |
Add option to verify file checksum of output files (#14433)
Summary: One of the follow ups from https://github.com/facebook/rocksdb/pull/14103. Users will have the option to verify file checksums for all compaction output files before they are installed. This feature helps prevent corrupted SST files from being added to the LSM tree. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14433 Test Plan: Unit test added Reviewed By: archang19 Differential Revision: D95648000 Pulled By: jaykorean fbshipit-source-id: 512f3c1a7449b96a660865531f3537624c89a9cc |
||
|
Xingbo Wang
|
91f227d9be |
Add injected error log ring buffer for fault injection diagnostics (#14431)
Summary:
Add a circular ring buffer (InjectedErrorLog) to FaultInjectionTestFS that records the last 1000 injected errors. Each entry captures the timestamp, thread ID, FS API name with all arguments (file path, offset, buffer size, first 8 bytes of data in hex), and the injected error status. For example:
Append("/path/035354.sst", size=4096, head=[1a 2b 3c ...]) -> IOError (Retryable): injected write error
RenameFile("/path/tmp.sst", "/path/035355.sst") -> IOError: injected metadata write error
Read(offset=16384, size=4096) -> IOError (Retryable): injected read error
The ring buffer is printed to a file automatically:
- On any fatal signal (SIGABRT, SIGSEGV, SIGTERM, SIGINT, SIGHUP, SIGFPE, SIGBUS, SIGILL, SIGQUIT, SIGXCPU, SIGXFSZ, SIGSYS) via a registered crash callback
- At the end of db_stress main(), for diagnostic visibility even when the test completes normally
This addresses a key debugging gap: when write fault injection causes secondary failures (e.g., the builder error propagation issue in T257612259), the injected errors were previously completely silent with no logging trail. The ring buffer provides the missing diagnostic context to correlate fault injection with downstream failures.
Changes:
- port/stack_trace.h/.cc: Add RegisterCrashCallback() API; extend InstallStackTraceHandler() to catch all catchable termination signals
- utilities/fault_injection_fs.h: Add InjectedErrorLog class with printf-style Record(), HexHead() for data bytes, and signal-safe PrintAll()
- utilities/fault_injection_fs.cc: Record full API arguments and error status at all 31 fault injection call sites
- db_stress_tool/db_stress_tool.cc: Register crash callback and print ring buffer at end of main()
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14431
Reviewed By: hx235
Differential Revision: D95435430
Pulled By: xingbowang
fbshipit-source-id: 6c18e1b072044575d6c8c3f198070127b0f80608
|
||
|
Omkar Gawde
|
cb8bc56d14 |
Fix memory leak on error in C API create_column_family (#14447)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14447 `rocksdb_create_column_family()` and `rocksdb_transactiondb_create_column_family()` allocate a `rocksdb_column_family_handle_t` but always return it even when `CreateColumnFamily()` fails. This leaks the handle and returns an object with an indeterminate `rep` pointer. Other similar functions like `rocksdb_create_column_family_with_import()` correctly delete the handle and return nullptr on error. Fix: Initialize `handle->rep = nullptr`, check `SaveError()` return value, and on error delete the handle and return nullptr. Reviewed By: anand1976 Differential Revision: D95303444 fbshipit-source-id: 5fde7a3ed588794d78429d5cb3d9f621f0fb6388 |
||
|
Maciej Szeszko
|
1ed5052486 |
Prepopulate block cache during compaction (#14445)
Summary:
When RocksDB operates with tiered or remote storage (e.g., Warm Storage, HDFS, S3), reading recently compacted data incurs high-latency remote reads because compaction output files are not present in the block cache. The existing `prepopulate_block_cache = kFlushOnly` avoids this for flush output but leaves compaction output cold until first access.
Add a new `PrepopulateBlockCache::kFlushAndCompaction` enum value that warms all block types (data, index, filter, compression dict) into the block cache during both flush and compaction. Flush-warmed blocks use `LOW` priority (unchanged from kFlushOnly behavior), while compaction-warmed blocks use `BOTTOM` priority — compaction data is less temporally local than freshly flushed data, so it should be the first to be evicted when the cache is full. This gives the remote-read avoidance benefit without risking cache thrashing.
The enum uses `kFlushAndCompaction` rather than separate `kCompactionOnly` + `kFlushAndCompaction` values because there is no practical use case for warming compaction output without also warming flush output. Flush output is by definition the hottest data (just written by the user), so if a workload benefits from warming the colder compaction output, it would always benefit from warming flush output too.
The implementation reuses the existing `InsertBlockInCacheHelper` / `WarmInCache` infrastructure in `BlockBasedTableBuilder`. The only internal change is adding a `warm_cache_priority` field to `Rep` alongside the existing `warm_cache` bool, and plumbing it through to the `WarmInCache` call instead of the previously hardcoded `Cache::Priority::LOW`.
### Key changes
- New `PrepopulateBlockCache::kFlushAndCompaction` enum value in table.h
- `Rep::warm_cache_priority` field in BlockBasedTableBuilder for per-reason priority control
- Serialization support ("kFlushAndCompaction" in string map)
- db_bench support (--prepopulate_block_cache=2)
- Crash test coverage (random choice includes new value)
**NOTE:** Unlike flush output (which is inherently hot — just written by the user), it is hard to distinguish hot from cold blocks in compaction output. Warming all compaction output therefore risks polluting the block cache and evicting genuinely hot entries. The kFlushAndCompaction mode is recommended only for use cases where most or all of the database is expected to reside in cache (e.g., the working set fits in cache). For workloads where only a fraction of the data is hot, kFlushOnly remains the safer choice.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14445
Test Plan:
- New `WarmCacheWithDataBlocksDuringCompaction` test: verifies data blocks from compaction output are present in the block cache and served without misses
- Extended `DynamicOptions` test: verifies dynamic switching through kDisable -> kFlushAndCompaction -> kFlushOnly -> kDisable via SetOptions
- Existing `WarmCacheWithDataBlocksDuringFlush` and parameterized `WarmCacheWithBlocksDuringFlush` tests continue to pass (kFlushOnly behavior unchanged)
- db_block_cache_test: 81/81 passed
- options_test: 74/74 passed
- table_test: 6910/6910 passed
Reviewed By: xingbowang
Differential Revision: D95997952
Pulled By: mszeszko-meta
fbshipit-source-id: 4ad568264992532053947df298e63e343821ddb5
|
||
|
Peter Dillinger
|
db9449a154 |
Fix crash test failures when use_trie_index conflicts with txn options (#14446)
Summary: The trie UDI sanitization in db_crashtest.py disables use_txn because TransactionDB ROLLBACK writes DELETE entries that violate UDI's Put-only restriction. However, it was not clearing use_optimistic_txn or test_multi_ops_txns, which both require use_txn to be true. Since use_trie_index is randomly enabled with 1/8 probability, the optimistic_txn and multiops_txn crash tests would intermittently fail with: - "You cannot set use_optimistic_txn true while use_txn is false" - "-use_txn must be true if -test_multi_ops_txns" Fix by also setting use_optimistic_txn=0 and test_multi_ops_txns=0 in the trie UDI sanitization block alongside the existing use_txn=0. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14446 Test Plan: Watch crash test CI results Reviewed By: xingbowang Differential Revision: D95986370 Pulled By: pdillinger fbshipit-source-id: 0751dcb4e99425ba9eb9aa34c2a99efa8eb3a194 |
||
|
Xingbo Wang
|
26aff32db3 |
Rename IsExpectedTxnLockTimeout to IsExpectedTxnError (#14441)
Summary: The function handles more than just lock timeouts — it also covers TryAgain from optimistic transactions. Rename it and update the comment to reflect its broader scope. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14441 Reviewed By: hx235 Differential Revision: D95877799 Pulled By: xingbowang fbshipit-source-id: 0651ffd39ac9d3a7979750be6dfe5b293eab7a64 |
||
|
Omkar Gawde
|
2afb387917 |
Fix missed condition variable signal in DeleteScheduler (#14442)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14442 In `BackgroundEmptyTrash()`, the bucket counter uses post-decrement (`iter->second--`), which assigns the pre-decrement value to `pending_files_in_bucket`. When a bucket transitions from 1 to 0 files, `pending_files_in_bucket` receives the value 1 (not 0), so the check `pending_files_in_bucket == 0` fails. This means `WaitForEmptyTrashBucket()` is never woken up when a specific bucket empties, unless all global pending files are also zero. Fix: Use pre-decrement (`--iter->second`) so the decremented value correctly triggers the condition variable signal. Reviewed By: xingbowang Differential Revision: D95303385 fbshipit-source-id: 3c5f0978ff33600acaf406b3f839cf13d9983055 |
||
|
Omkar Gawde
|
a066318a31 |
Fix out-of-bounds access in BlobFileReader::MultiGetBlob (#14427)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14427 In `MultiGetBlob()`, the `adjustments` autovector is populated only for blob requests that pass validation. Requests that fail validation are skipped via `continue`, so `adjustments` has fewer entries than `blob_reqs`. When consuming results, `adjustments[i]` uses the `blob_reqs` index instead of the `read_reqs`/`adjustments` index, causing an out-of-bounds read when any request fails validation. Fix: Replace `adjustments[i]` with `adjustments[j - 1]`, since `j` tracks the position in `read_reqs`/`adjustments` and has already been post-incremented. Reviewed By: hx235 Differential Revision: D95303356 fbshipit-source-id: a264ae6481f74ce33f64e40624441d666135bcd0 |
||
|
Josh Kang
|
42eff8b632 |
Add new heauristic 'num_collapsible_entry_reads_sampled' (#14434)
Summary: Add per-file sampling of "collapsible" entry reads (single deletions, merges, and kNotFound results) that may later be used to help inform read-triggered compactions. This is a better metric than `num_reads_sampled` as it is more targeted towards reads that could be avoided via compaction. The existing behavior of `num_reads_sampled` is that reads only gets sampled on iterator creation for a file. It is problematic because next/prev() calls are not sampled, nor are additional seeks(). This PR moves sampling to per-seek/next granularity within `LevelIterator` and adds a new `num_collapsible_entry_reads_sampled` counter that tracks how often a file serves entries that could be eliminated by compaction. Note only L1+ files have iterator seeks/nexts/prevs sampled. Introducing this at L0 would require wrapping table reader iterators, introducing a performance cost. ## Key changes - **New counter `num_collapsible_entry_reads_sampled`** in `FileSampledStats` tracks sampled reads that encounter deletions, single deletions, merges, or kNotFound results in both Get and Iterator paths. - **Moved sampling from file-open to per-operation** in `LevelIterator`: sampling now happens in `SampleRead()` called from `Seek()`, `SeekForPrev()`, `SeekToFirst()`, `SeekToLast()`, `Next()`, `NextAndGetResult()`, and `Prev()`. The `should_sample` parameter was removed from `LevelIterator`'s constructor. - **Differentiated sampling rate for Next() vs Seek()**: `should_sample_file_read_next()` uses a 64x lower sampling rate (`kFileReadSampleRate * 64`) since Next() is cheaper than Seek() and called more frequently. - **Collapsible tracking in Get path**: `Version::Get()` now increments the collapsible counter when `GetContext::State()` is `kNotFound`, `kMerge`, or `kDeleted`. - **Collapsible tracking in MultiGet path**: `MultiGetFromSST` also increments the collapsible counter for the same states. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14434 Test Plan: - Added new DB tests for both num_reads_sampled and num_collapsible_entry_reads_sampled ### Benchmark results (readrandom, readseq) Setup: 1M keys, 16-byte keys, 100-byte values, no compression, fillrandom+compact | Benchmark | Params | ops/s (main) | ops/s (feature) | % change | |------------|--------------------|-------------|--------------------------|----------| | readrandom | seed=1, threads=1 | 387,194 | 389,449 | +0.6% | | readseq | seed=1, threads=1 | 5,598,371 | 5,572,975 | -0.5% | No meaningful performance regression observed — differences are within run-to-run noise. Reviewed By: xingbowang Differential Revision: D95613793 Pulled By: joshkang97 fbshipit-source-id: 9dd09c9b7527b148424bde5686f4157c7a9e1214 |
||
|
Hui Xiao
|
cb43abb1f1 |
Fix incorrect input file expansion in SetupOtherFilesWithRoundRobinExpansion (#14436)
Summary:
### Context/Summary:
**_See below for an example of the bug:_**
L1 has 5 SST files at indices [0, 1, 2, 3, 4] where files at indices 1/2/3
share user key boundaries (e.g., file[1].largest and file[2].smallest have
the same user key).
Round-robin picks file[1] at start_index=1.
Before fix:
1. ExpandInputsToCleanCut expands {file[1]} → {file[1], file[2], file[3]}
2. Loop starts at i=2 (start_index+1), adds file[2] → duplicate!
3. start_level_inputs_ = {file[1], file[2], file[3], file[2]}
4. GetRange uses back()=file[2], returns [file[1].smallest, file[2].largest]
5. ExpandInputsToCleanCut converges on {file[1], file[2]}, missing file[3]
6. AssertCleanCut crashes (debug) or data corruption (release)
After fix:
1. ExpandInputsToCleanCut expands {file[1]} → {file[1], file[2], file[3]}
2. Find last file = file[3] at index 3, loop starts at i=4
3. start_level_inputs_ = {file[1], file[2], file[3], file[4]} (no duplicates)
4. GetRange correctly returns [file[1].smallest, file[4].largest]
_**More details:**_
When round-robin compaction picks a file, PickFileToCompact calls ExpandInputsToCleanCut which may expand start_level_inputs_ from 1 file to N files (when adjacent files share user key boundaries). Then SetupOtherFilesWithRoundRobinExpansion loops from start_index + 1 to add more files, not knowing the expansion already happened. This re-adds files already in start_level_inputs_, creating duplicates.
The duplicates corrupt GetRange, which trusts inputs.back()->largest for non-L0 levels. With a duplicate earlier file at back(), GetRange returns a truncated range. ExpandInputsToCleanCut then converges on an incomplete file set, violating the clean-cut invariant.
In debug builds this crashes at AssertCleanCut. In release builds the compaction proceeds with a non-clean-cut input, causing data corruption (newer data in a later level while older data remains in an earlier level).
The fix finds the position of the last file already in start_level_inputs_ and starts the loop after it, avoiding duplicates. When start_level_inputs_ has only 1 file (no expansion happened), the behavior is unchanged.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14436
Test Plan:
New test RoundRobinCleanCutWithSharedBoundary:
- Without fix: crashes at AssertCleanCut in SetupOtherFilesWithRoundRobinExpansion
```
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from DBCompactionTest
[ RUN ] DBCompactionTest.RoundRobinCleanCutWithSharedBoundary
db_compaction_test: db/compaction/compaction_picker.cc:81: void rocksdb::AssertCleanCut(const rocksdb::InternalKeyComparator*, rocksdb::VersionStorageInfo*, rocksdb::CompactionInputFiles*, int, rocksdb::Logger*): Assertion `false' failed.
Received signal 6 (Aborted)
Invoking GDB for stack trace...
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/usr/local/fbcode/platform010/lib/libthread_db.so.1".
0x00007fd9ba8f0e13 in __GI___wait4 (pid=1004850, stat_loc=0x7fd9b8bfb2ac, options=0, usage=0x0) at ../sysdeps/unix/sysv/linux/wait4.c:30
30 ../sysdeps/unix/sysv/linux/wait4.c: No such file or directory.
https://github.com/facebook/rocksdb/issues/4 __pthread_kill_internal (signo=6, threadid=<optimized out>) at pthread_kill.c:45
45 pthread_kill.c: No such file or directory.
https://github.com/facebook/rocksdb/issues/5 __GI___pthread_kill (threadid=<optimized out>, signo=6) at pthread_kill.c:62
62 in pthread_kill.c
https://github.com/facebook/rocksdb/issues/6 0x00007fd9ba8444ad in __GI_raise (sig=6) at ../sysdeps/posix/raise.c:26
26 ../sysdeps/posix/raise.c: No such file or directory.
https://github.com/facebook/rocksdb/issues/7 0x00007fd9ba82c433 in __GI_abort () at abort.c:79
79 abort.c: No such file or directory.
https://github.com/facebook/rocksdb/issues/8 0x00007fd9ba83bc28 in __assert_fail_base (fmt=0x7fd9ba9e11d8 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x7fd9bcc04819 "false", file=0x7fd9bcc04700 "db/compaction/compaction_picker.cc", line=81, function=<optimized out>) at assert.c:92
92 assert.c: No such file or directory.
https://github.com/facebook/rocksdb/issues/9 0x00007fd9ba83bc93 in __GI___assert_fail (assertion=0x7fd9bcc04819 "false", file=0x7fd9bcc04700 "db/compaction/compaction_picker.cc", line=81, function=0x7fd9bcc04780 "void rocksdb::AssertCleanCut(const rocksdb::InternalKeyComparator*, rocksdb::VersionStorageInfo*, rocksdb::CompactionInputFiles*, int, rocksdb::Logger*)") at assert.c:101
101 in assert.c
https://github.com/facebook/rocksdb/issues/10 0x00007fd9bc2f8181 in rocksdb::AssertCleanCut (icmp=0x7fd9b9a7a840, vstorage=0x7fd9b9b6d040, inputs=0x7fd9b8bfc690, level=1, logger=0x7fd9b9aa2790) at db/compaction/compaction_picker.cc:81
81 assert(false);
https://github.com/facebook/rocksdb/issues/11 0x00007fd9bc2f8fa6 in rocksdb::CompactionPicker::ExpandInputsToCleanCut (this=0x7fd9b9b32900, vstorage=0x7fd9b9b6d040, inputs=0x7fd9b8bfc690, next_smallest=0x0) at db/compaction/compaction_picker.cc:310
310 AssertCleanCut(icmp_, vstorage, inputs, level, ioptions_.logger);
https://github.com/facebook/rocksdb/issues/12 0x00007fd9bc30cbfb in rocksdb::(anonymous namespace)::LevelCompactionBuilder::SetupOtherFilesWithRoundRobinExpansion (this=0x7fd9b8bfc910) at db/compaction/compaction_picker_level.cc:423
```
- With fix: compaction completes, data correctness verified
Reviewed By: joshkang97
Differential Revision: D95718659
Pulled By: hx235
fbshipit-source-id: 6ef125455ef5ae8a07c323835ff25588dbbb3634
|
||
|
Jay Huh
|
e27114b135 |
Skip flush during recovery if read_only (#14440)
Summary:
The whitebox_crash_test crashes with Assertion `checking_set_.count(cfd) == 0` failed in `FlushScheduler::ScheduleWork()` during a read-only DB open (e.g., StressTest::TestBackupRestore).
## Root cause:
Commit
|
||
|
Josh Kang
|
3ad23b2d94 |
Support automated interpolation search (#14383)
Summary: Add automatic per-block interpolation search selection (`kAuto` mode) for index blocks. During SST construction, each index block's key distribution is analyzed using the coefficient of variation (CV) of gaps between restart-point keys. Blocks with uniformly distributed keys are flagged via a new bit in the data block footer, and at read time, `kAuto` resolves to interpolation search for uniform blocks and binary search otherwise. ## Key changes - **New `BlockSearchType::kAuto` enum value**: Resolves per-block at read time to either `kInterpolation` or `kBinary` based on the block's uniformity flag. Falls back to `kBinary` on older versions that don't recognize it. - **Write-path uniformity analysis**: `BlockBuilder::ScanForUniformity()` uses Welford's online algorithm to incrementally compute the CV of key gaps at restart points. The result is stored in a new bit (bit 30) of the data block footer's packed restart count. - **New table option `uniform_cv_threshold`** (default: -1 `disabled`): Controls how strict the uniformity check is. Set to negative to disable. Exposed in C++, Java (JNI), and `db_bench`. - **Code reorganization**: Block entry decode helpers (`DecodeEntry`, `DecodeKey`, `DecodeKeyV4`, `ReadBe64FromKey`) moved from `block.cc` to a new shared header `block_util.h` so they can be reused by `BlockBuilder` on the write path. - **New histogram `BLOCK_KEY_DISTRIBUTION_CV`**: Records the CV (scaled by 10000) of each index block's key distribution for observability. - **Java bindings**: `IndexSearchType.kAuto`, `uniformCvThreshold` getter/setter, JNI portal constructor signature updated, and `HistogramType.BLOCK_KEY_DISTRIBUTION_CV` added. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14383 Test Plan: - `IndexBlockTest.IndexValueEncodingTest` parameterized to include `kAuto` search type alongside `kBinary` and `kInterpolation`, verifying correct seek/iteration behavior across all combinations of key distributions, restart intervals, and key lengths. - Uniformity detection validated: blocks with uniform key distribution correctly set `is_uniform = true`, blocks with clustered/non-uniform keys set `is_uniform = false`. - Stress test coverage - Updated check_format_compatible to also include a "uniform" dataset. By default using uniform_cv_threshold=-1 does not result in an incompatibility issues. When manually changing the threshold (e.g. `uniform_cv_threshold=1000`), I see `bad block contents`, which is expected ## Benchmark readrandom with `fillrandom,compact -seed=1 --statistics`: | Benchmark | Branch | Params | avg ops/s | % change vs main | CV P50 | |-----------|--------|--------|-----------|------------------|--------| | readrandom | main | `binary_search, shortening=1` | 335,791 | baseline | N/A | | readrandom | feature | `binary_search, shortening=1` (default) | 335,749 | -0.0% | 1,500 | | readrandom | feature | `auto_search, shortening=1` (kAuto) | 366,832 | **+9.2%** | 1,500 | | readrandom | feature | `interpolation_search, shortening=1` | 366,598 | **+9.2%** | 1,500 | | readrandom | feature | `auto_search, shortening=2` (kAuto) | 344,631 | **+2.6%** | 1,030,000 | | readrandom | feature | `interpolation_search, shortening=2` | 201,178 | **-40.1%** | 1,030,000 | As seen with shortening=2, a non-uniform distribution produces a high CV, which does not use interpolation search. ## Write benchmark There is a write overhead which scans each restart entry for a block upon Finish. In practice this is very low because currently it is only applied to index blocks. See cpu profile (https://fburl.com/strobelight/io5hwj9h) here of `-benchmarks=fillseq,compact -compression_type=none -disable_wal=1`. Only 0.08% attributed to `ScanForUniformity`. Reviewed By: pdillinger Differential Revision: D94738890 Pulled By: joshkang97 fbshipit-source-id: 9661ac593c5fef89d49f3a8a027f1338a0c96766 |
||
|
Xingbo Wang
|
8a86620653 |
Disable more features in stress test when trie UDI is enabled (#14432)
Summary: Address 2 compatibility issue of UDI: A: Trie UDI (UserDefinedIndex) does not support SeekToFirst/SeekToLast. The crash test already disabled prefix scanning (prefixpercent=0) when use_trie_index=1, but iteration (iterpercent) was still enabled. During iteration, LevelIterator::SkipEmptyFileForward() internally calls file_iter_.SeekToFirst() when Next() crosses SST file boundaries within a level. This propagates to UserDefinedIndexIteratorWrapper::SeekToFirst() which returns NotSupported, causing "Iterator diverged from control iterator" / "VerifyIterator failed" errors across many crash test variants. B: BlobDB is incompatible with trie UDI (user-defined index). When BlobDB is enabled (`enable_blob_files=1`), the flush job stores large values in separate blob files and writes `kTypeBlobIndex` entries in the SST instead of `kTypeValue`. The UDI builder (`UserDefinedIndexBuilderWrapper::OnKeyAdded()`) rejects any entry whose type is not `kTypeValue`, causing the flush to fail with `"user_defined_index_factory only supported with Puts"`. Once the flush fails, the DB enters background error state, and all subsequent `Write()` calls fail with the same error -- printed as the repeated `"multiput error: ..."` messages from `BatchedOpsStressTest::TestPut`. Eventually, `ProcessStatus` encounters this error and triggers `assert(false)`. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14432 Reviewed By: pdillinger Differential Revision: D95457462 Pulled By: xingbowang fbshipit-source-id: bf2bc47bd1ed75926765b2e3ff068f99f89a7793 |
||
|
Maciej Szeszko
|
071b5eff7a |
Fix MemPurge memtable ID ordering assertion (#14424)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14424 ## Context `MemPurge` releases db_mutex_ to do its merge work and re-acquires it before adding the output memtable to the immutable list. During this window, concurrent writers can fill the active memtable and trigger a switch, adding a new immutable memtable with a higher ID. `MemPurge` then assigns its output memtable the stale ID from mems_.back() (the newest memtable in the original flush batch), which is now lower than the front of the immutable list, violating the ordering assertion in `MemTableListVersion::AddMemTable `. ## Fix After re-acquiring db_mutex_, use `std::max(mems_.back()->GetID(), imm()->GetLatestMemTableID())` so the output memtable's ID is never lower than any existing immutable memtable. Two `TEST_SYNC_POINT`s are added to the `MemPurge` success path to enable deterministic repro. Reviewed By: pdillinger Differential Revision: D94756638 fbshipit-source-id: 2e9e15b4285dc6b996c8744795228180dbd73ed3 |
||
|
Jay Huh
|
3aa706c2bf |
Enforce WriteBufferManager during WAL recovery (#14305)
Summary: Add a new immutable DB option `enforce_write_buffer_manager_during_recovery` to control whether WriteBufferManager buffer_size is enforced during WAL recovery. When multiple RocksDB instances share a WriteBufferManager, a recovering instance could exceed the global memory limit by replaying large amounts of WAL data into memtables. This can cause OOM, especially when other instances are actively using the shared memory budget. When this option is enabled (and a WriteBufferManager is configured), RocksDB will check `WriteBufferManager::ShouldFlush()` after each batch insertion during WAL recovery and schedule flushes when needed to keep memory bounded. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14305 Test Plan: Two unit tests added - `WriteBufferManagerLimitDuringWALRecoverySingleDB` and `WriteBufferManagerLimitDuringWALRecoveryMultipleDBs` ``` ./db_write_buffer_manager_test --gtest_filter="*WriteBufferManagerLimitDuringWALRecovery*" ``` Basic stress test added with options toggled ## To follow up Multiple DB scenario in stress test Reviewed By: hx235 Differential Revision: D92533792 Pulled By: jaykorean fbshipit-source-id: 35ca70e2300a8bfd6b81a6646a65ef284fb90a9a |
||
|
Xingbo Wang
|
2dc6d6f765 |
Propagate builder error when flush produces empty output (#14418)
Summary:
Propagate builder error when flush produces empty output
When write fault injection causes the table builder to enter an error
state during flush, all subsequent Add() calls return early (ok() is
false), leaving the builder empty (IsEmpty() == true). Previously,
BuildTable() would call builder->Abandon() but not propagate the
builder's error status to 's', leaving it OK. This caused the downstream
key count validation in flush_job.cc to fire a misleading Corruption
error ('Number of keys in flush output SST files does not match...'),
which the stress test harness couldn't identify as a retryable injected
fault error, leading to SafeTerminate().
This started failing recently because ('Separate keys and
values in data blocks', ) introduced a new SST
block format (separate_key_value_in_data_block) that stores keys and
values in separate sections within data blocks. This format requires
additional write operations during Flush() inside the table builder,
increasing the probability that write fault injection
(--write_fault_one_in=128) hits a data block write and puts the builder
into an error state before any entries are committed. The bug in
BuildTable() existed before, but was rarely triggered because the old
interleaved block format had fewer write points susceptible to fault
injection during the critical Add() path.
Fix: After builder->Abandon(), propagate the builder's error status to
's' when the builder is empty due to an internal error. This ensures the
actual IOError from write fault injection is reported, which the stress
test can properly handle via IsErrorInjectedAndRetryable().
The analysis was based on stack trace. However, it would be great
if we could get direct evidence from fault injection.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14418
Test Plan: Unit test
Reviewed By: hx235
Differential Revision: D95121070
Pulled By: xingbowang
fbshipit-source-id: cfb513bd744ac34ac90cda11c1cbe49a9d0a7c6c
|
||
|
Peter Dillinger
|
ef5b145140 |
Fix ldb dump swallowing errors, but ldb scan for compatibility test (#14422)
Summary: This fixes a longstanding bug in which `ldb dump` swallows iterator errors. This can affect check_format_compatible.sh test results; if lucky, it will misleadingly look like a data mismatch instead of an outright failure. If unlucky, it could cause a test false negative. However the compatibility test uses old versions of ldb, so the best way to improve the test (for the foreseeable future) is to replace `ldb dump` with `ldb scan`. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14422 Test Plan: manual Reviewed By: joshkang97 Differential Revision: D95332577 Pulled By: pdillinger fbshipit-source-id: bef1b427dd8aaa2cabbd23b7ad9f3cad1f67a349 |
||
|
Anand Ananthabhotla
|
3b5cb114e3 |
Refactor MultiScan to use MultiScanIndexIterator (#14401)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14401 Unify the MultiScan and regular iterator codepaths in BlockBasedTableIterator by introducing a MultiScanIndexIterator that implements InternalIteratorBase<IndexValue>. During Prepare(), the original index iterator is swapped out for a MultiScanIndexIterator that wraps the prefetched block handles and scan range metadata. This allows SeekImpl() and FindBlockForward() to use the same code flow for both regular and MultiScan operations, eliminating the need for separate MultiScan-specific methods (SeekMultiScan, FindBlockForwardInMultiScan, MultiScanSeekTargetFromBlock, MultiScanUnexpectedSeekTarget, MultiScanLoadDataBlock, MarkPreparedRangeExhausted). Key changes: - New MultiScanIndexIterator class that manages scan range tracking, block handle iteration, forward-only seek enforcement, and wasted block counting - InitDataBlock() loads blocks from ReadSet when MultiScan is active - FindBlockForward() detects scan range boundaries via IsScanRangeExhausted() after index_iter_->Next() - Disabled reseek optimization for MultiScan so MultiScanIndexIterator::Seek() is always called to update scan range tracking state - Removed MultiScanState struct and all MultiScan-specific methods from BlockBasedTableIterator - No changes to CheckDataBlockWithinUpperBound or CheckOutOfBound — they work as-is through iterate_upper_bound - multi_scan_status_ intentionally not checked in Valid() hot path to avoid performance regression; when status is non-OK, block_iter_points_to_real_block_ is already false - Fixed pre-existing bug in ReadSet::SyncRead() that used the base decompressor without compression dictionary, causing ZSTD data corruption when blocks with dictionary compression needed synchronous fallback reads Reviewed By: xingbowang Differential Revision: D93300655 fbshipit-source-id: 231059208e0cc512bc2ec43ff7055fcb2a2dc72d |
||
|
zaidoon
|
16c81a27e9 |
Extend UDI trie index with seqno side-table for same-user-key block boundaries (#14412)
Summary: When the same user key spans adjacent data blocks with different sequence numbers, the trie index cannot distinguish them because it only stores user keys. This adds a side-table that stores per-leaf sequence numbers and overflow block metadata, enabling correct post-seek correction using seqno comparison. Design: - Trie always stores user-key-only separators (unchanged) - Duplicate separators from same-key boundaries are de-duplicated - Side-table stores leaf_seqnos[], leaf_block_counts[], and overflow arrays (offsets, sizes, seqnos) serialized after the trie data - Post-seek correction: after trie Seek lands on a leaf, compare target_seq vs leaf_seqno to decide whether to advance through overflow blocks or to the next trie leaf - Zero overhead when no same-user-key boundaries exist (no side-table serialized, no seqno checks at seek time) Public API changes (user_defined_index.h): - AddIndexEntry: single pure virtual with IndexEntryContext parameter (replaces old 4-arg version). Context carries last_key_seq and first_key_seq for block boundaries. - SeekAndGetResult: single pure virtual with SeekContext parameter (replaces old 2-arg version). Context carries target_seq. - Fix trailing semicolons on NewBuilder/NewReader default overrides. Wrapper layer (user_defined_index_wrapper.h): - Extract sequence numbers from parsed internal keys and pass via context structs to UDI builder/iterator. - Fix CurrentIndexSizeEstimate() to delegate to internal builder (was returning 0). - Fix ApproximateMemoryUsage() to include UDI reader memory. - Fix typos: lof_err_key -> log_err_key, COuld -> Could, "Bad index name" -> "Bad index name: ". Trie builder (trie_index_factory.cc): - Buffer separator entries during building; at Finish(), detect whether any same-user-key boundary was seen (sticky flag, same strategy as ShortenedIndexBuilder::must_use_separator_with_seq_). - When seqno encoding is needed, re-encode all separators with seqno side-table metadata in the trie. - Default null comparator to BytewiseComparator() to prevent crash. Trie iterator (trie_index_factory.cc): - Post-seek correction: compare target_seq vs leaf_seqno to advance through overflow runs. - Overflow run state tracking: overflow_run_index_, overflow_run_size_, overflow_base_idx_ for O(1) access to overflow block handles. - NextAndGetResult advances within overflow runs before moving to next trie leaf. - Return kOutOfBound instead of kUnknown when Seek/Next finds no blocks. LOUDS trie (louds_trie.h, louds_trie.cc): - Seqno side-table: builder AddKeyWithSeqno/AddOverflowBlock methods, BFS reordering of seqno/block_count arrays, serialization/deserialization of per-leaf seqnos, block counts, overflow handles/seqnos, and overflow_base_ prefix sum for O(1) access. - AppendKeySlot() helper with debug bounds assert on all key-append sites. Dead code removal: - LoudsTrieBuilder::NumKeys() (never called externally) - sparse_leaf_count_ (serialized but never read by the reader) - DenseChildNodeNum(pos), DenseLeafIndex(pos), DenseNodeNum(pos) (superseded by FromRank variants that avoid redundant Rank1 calls) Deserialization hardening (all in InitFromData, cold path only): - Move num_keys_ validation before any dependent arithmetic. - Validate dense bitvector sizes match dense_node_count_ (d_labels must be node_count*256, d_has_child must equal d_labels.NumOnes(), d_is_prefix_key must equal node_count). - Validate sparse bitvector sizes match s_labels_size_ (s_has_child and s_louds must have the same number of bits as the label array). - Validate child position table values within s_labels_size_ bounds. - Validate chain suffix offset+length within suffix data blob. - Validate chain end child indices < num_internal or == UINT32_MAX. - EliasFano: add count_ upper-bound check (<=2^30) to prevent count_*low_bits_ integer overflow. - Bitvector: validate select hint values < num_rank_samples_ to prevent OOB in FindNthOneBit/FindNthZeroBit. - EliasFano: custom move ctor/assignment to re-seat low_words_ after owned_low_data_ move (fixes dangling pointer for SSO strings). https://github.com/facebook/rocksdb/issues/14406 Pull Request resolved: https://github.com/facebook/rocksdb/pull/14412 Reviewed By: anand1976 Differential Revision: D95160933 Pulled By: xingbowang fbshipit-source-id: f2c3681c1059c03d540ce1cb9cc14cc79cd9730c |
||
|
Xingbo Wang
|
3b66de3fac |
Fix out of disk unit test issue (#14425)
Summary:
It contains 3 commits.
Commit 1: Fix /dev/shm exhaustion during make check
Fix /dev/shm exhaustion during make check
1. Add space-heavy tests to NON_PARALLEL_TEST: perf_context_test (1GB
write_buffer_size), obsolete_files_test (~1GB), backup_engine_test
(1GB), prefetch_test (1GB), and db_io_failure_test (256MB).
2. Add per-test cleanup on success: each parallel test script now
removes its test directory after passing. Failed test directories
are preserved for debugging.
3. Add age-based stale directory cleanup: at the start of make check,
remove /dev/shm/rocksdb.* directories older than 3 hours from
previous failed runs, using age-based filtering to avoid disturbing
concurrent runs.
Commit 2: Fix SIGSEGV in prefetch_test due to stale SyncPoint callbacks
Both FilePrefetchBufferTest and FSBufferPrefetchTest fixtures set up
SyncPoint callbacks capturing local variables by reference and enable
processing, but neither fixture's TearDown() clears them. When a
subsequent test runs, the stale callbacks fire with dangling
references, causing memory corruption and SIGSEGV.
Fixed by adding DisableProcessing() and ClearAllCallBacks() to both
fixtures' TearDown() methods.
Commit 3: Fix OpenFilesAsyncTest using excessive disk space
Fix OpenFilesAsyncTest using excessive disk space in /dev/shm
OpenFilesAsyncTest::SetupData() set write_buffer_size to SIZE_MAX to
prevent automatic flushes. This caused each of its ~20 parallel
instances to consume 6-43 GB in /dev/shm (validated: a single
Shutdown/3 instance used 43 GB), totaling ~233 GB and filling the
disk. This was the primary cause of "No space left on device" errors
in make check. The sst flushed is small, but it causes issue in WAL
space reservation, which bloated the disk usage.
The SIZE_MAX is unnecessary — the test writes only 4 tiny key-value
pairs and does explicit Flush() after each. The default 64 MB
write_buffer_size will never auto-flush for such small writes.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14425
Test Plan: Unit Test
Reviewed By: joshkang97
Differential Revision: D95253518
Pulled By: xingbowang
fbshipit-source-id: 03907df59b3d89d90413a0e33996ec205944d48b
|
||
|
Xingbo Wang
|
9a9cc1e2ef |
Fix uninitialized wal_in_db_path_ in read-only and compacted DB open paths (#14419)
Summary: wal_in_db_path_ was declared without an in-class initializer in DBImpl. While DB::Open, OpenAsSecondary, and OpenAsFollower all explicitly set this field, OpenForReadOnly and CompactedDBImpl::Open did not. This was harmless until recently because CloseHelper() only calls PurgeObsoleteFiles when opened_successfully_ is true, and read-only DBs previously did not set opened_successfully_. After https://github.com/facebook/rocksdb/issues/14322 added opened_successfully_ = true to the read-only path, CloseHelper now executes PurgeObsoleteFiles -> DeleteObsoleteFileImpl, which reads wal_in_db_path_ to decide whether WAL files should be deleted in the foreground. Reading an uninitialized bool is undefined behavior, caught by UBSan as "invalid-bool-load" in the secondary_cache_crash_test. Fixed by adding an in-class initializer (= false) to wal_in_db_path_. The default of false means WAL deletions use foreground deletion, which is safe for read-only DBs that don't write WALs. The existing DB::Open path continues to set the correct value explicitly. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14419 Test Plan: - Added regression test ReadOnlyDBWalInDbPathInitialized in db_test2 - Ran test 5 times without failure - Ran all OpenForReadOnly tests (3 tests pass) Task: T257988006 Reviewed By: joshkang97 Differential Revision: D95122268 Pulled By: xingbowang fbshipit-source-id: 7f65fe1f09e7e0b42ba68f44f246615abc0757d4 |
||
|
Xingbo Wang
|
ea2f33c81a |
Disable prefix scanning when use_trie_index is enabled in crash test (#14421)
Summary: The trie-based User Defined Index (UDI) has a bug in its iterator implementation that causes "Not implemented: SeekToFirst not supported" errors during prefix scanning. This causes assertion failures in BatchedOpsStressTest::TestPrefixScan and may cause silent wrong results in other prefix scan tests. Until the trie index is fixed, disable prefix scanning when use_trie_index is enabled by setting prefixpercent=0 and redistributing the percentage to reads. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14421 Test Plan: - Reproduced: db_stress with --use_trie_index=1 --prefixpercent=5 crashes with assertion failure. Instrumentation revealed iterator status "Not implemented: SeekToFirst not supported" on prefix 0x35. - Verified: db_stress with --use_trie_index=1 --prefixpercent=0 completes cleanly. Reviewed By: anand1976 Differential Revision: D95135250 Pulled By: xingbowang fbshipit-source-id: 81540b2426aa1a855a58e6ca9e68a035d53aa2d8 |
||
|
Xingbo Wang
|
15f2f2bf6f |
Relax option sanitization for kv ratio compaction (#14397)
Summary: The kv ratio compaction option sanitization is too strict. Sometimes, it blocks engine start up. Relax the validation and convert it to runtime check. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14397 Test Plan: Unit test Reviewed By: pdillinger Differential Revision: D94709471 Pulled By: xingbowang fbshipit-source-id: cc076c397b3acfa426112063224771a196684798 |