mirror of
https://github.com/facebook/rocksdb.git
synced 2026-10-23 08:09:02 +03:00
main
14217 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Xingbo Wang
|
6f5587ae2d |
Fix GetCorruptionStatus() crash with separated KV blocks (#15006)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15006 Block::GetCorruptionStatus() uses restart_offset_ in error state to decide whether it can re-decode the data block footer for a more specific corruption status: nonzero means the footer decode failed and restart_offset_ stores the original block size; zero means the corruption was detected after the footer decoded. For separated-KV data blocks, Block::Block() first sets restart_offset_ to the restart-array offset, then validates footer.values_section_offset. If that offset is past the restart array, the block is marked corrupt with size = 0, but restart_offset_ was left nonzero. NewDataIterator() then called GetCorruptionStatus(), which treated the stale restart-array offset as an original block size, re-decoded a syntactically valid footer, and hit DEBUG_FAIL("ok status on presumed bad block contents"). Reset restart_offset_ to 0 on this semantic-corruption path so GetCorruptionStatus() reports generic block corruption instead of trying to re-decode a footer that already decoded successfully. The regression test builds a separated-KV hash-index data block, corrupts only the encoded values_section_offset to point past the block, and verifies NewDataIterator() returns "Corruption: bad block contents". Removing the fix line makes the test reproduce the original assertion. This update also applies the clang-format layout required by check-format for the new regression test. Reviewed By: joshkang97 Differential Revision: D113561303 fbshipit-source-id: 8d989a38761063c62694837976aad98f1868f884 |
||
Peter Dillinger
|
c73841572a |
Simplify ReadOptions plumbing for read-path blob fetchers (#15003)
Summary: Follow-up cleanup to the embedded (same-file) blob wide-column work (PR https://github.com/facebook/rocksdb/issues/14990): the "ReadOptions usage cleanup/optimization" item. Split the Version-backed blob fetcher into two variants sharing a common VersionBlobFetcherBase (which implements FetchBlob via a virtual read_options()): - VersionBlobFetcher now holds a `const ReadOptions&` for transient/stack fetchers (the common case: point Get/MultiGet, DBIter, version_set), so the growing ReadOptions is no longer copied per fetcher on the hot path. A deleted rvalue-ref constructor rejects binding to a temporary. - OwningVersionBlobFetcher takes `ReadOptions&&` and owns a copy, for fetchers whose lifetime is independent of the caller (the read-path lazy resolver and compaction), making ownership transfer explicit. DBIter::BlobReader now holds a single blob fetcher instead of decomposed scalar fields (read_tier/verify_checksums/fill_cache/io_activity). This removes the decompose-then-recompose that the old CreateBlobFetcher and RetrieveAndSetBlobValue performed, and closes the field-loss gap: all ReadOptions fields now flow through to blob reads rather than just those four. CreateBlobFetcher is gone; the merge path uses the reader's owned fetcher directly. A CanResolve() predicate on the fetcher preserves the prior "unexpected blob index" Corruption instead of dereferencing a null Version, and the now-redundant allow_blob_write_path_fallback_ member/ parameter (equal to the fetcher's own flag) was dropped. ReadPathBlobResolver keeps owning its fetcher (now OwningVersionBlobFetcher) so it stays self-contained for the planned lazy entity read path, where the result outlives the read call. A TODO in db_iter.h records the remaining duplication (BlobReader and ReadPathBlobResolver each own a fetcher with the same {version, read_options, blob_file_cache, allow_write_path_fallback}), deferred to that work. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15003 Test Plan: Pure refactoring with no intended behavior change; Version::GetBlob's Slice overload just decodes and calls the BlobIndex overload, so routing whole-value blob reads through the fetcher is equivalent, and the direct-write fallback flag/read options are unchanged. Covered by existing tests: - db/blob/db_blob_index_test.cc: embedded/entity blob GetEntity/MultiGetEntity zero-copy, plain Get of an embedded default column, merge over an embedded-blob entity, read_tier == kBlockCacheTier Incomplete, corrupt record Corruption. - db/blob/db_blob_basic_test.cc and db/wide/db_wide_blob_direct_write_test.cc: separate-file and direct-write blob resolution (the fallback branch). - db/db_iterator_test.cc (blob) and db/compaction/compaction_iterator_test.cc: DBIter blob/merge paths and the compaction (owning) fetcher. Reviewed By: xingbowang Differential Revision: D113582247 Pulled By: pdillinger fbshipit-source-id: 231c9364e266b826d12a8479cbb7f026a0d26bf6 |
||
|
Xingbo Wang
|
14a7646fb5 |
Fix PinnableSlice move construction from pinned slices (#14998)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14998 T280925646 reported an ASAN double-free in db_stress' batched prefix scan. The visible free was in VerifyWideColumns(), but WideColumn values are non-owning Slice views, so that destructor should only free the vector element storage. The stack points to prior ownership corruption rather than an intentional verifier free. The concrete bug is in PinnableSlice's move constructor: it delegates to move assignment without first initializing the target object. When moving from a pinned slice, move assignment does not set buf_ because the target is still pinned. If that moved-to slice is later Reset() and reused with PinSelf(), it writes through an uninitialized buf_ pointer. This became reachable after D112212266 changed PinnableWideColumns to keep backing buffers in std::forward_list<PinnableSlice> and emplace moved PinnableSlice values into backing nodes. Fix the move constructor by delegating to the default constructor first, so buf_ is initialized before move assignment. Add a regression that poisons placement-new storage, move-constructs from a pinned slice, resets it, and reuses it with PinSelf(); this crashed before the fix. Reviewed By: pdillinger Differential Revision: D113480016 fbshipit-source-id: 38ce9030f392ff9f0438d38d53d19e356e2d2fbf |
||
|
Xingbo Wang
|
03c3c090fb |
Measure IODispatcher async read latency (#15001)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15001 Add IODispatcher-specific histograms for async read observed completion latency, `FileSystem::Poll` wait time, and submit-to-consumer-poll prefetch lead time. Record submit timestamps on async read state, report completion latency from the callback, and report poll wait/prefetch lead when the block consumer polls the handle. Keep Java histogram byte mappings stable by assigning the new histogram IDs after the existing range. Add a controlled async filesystem test that verifies one deferred async read records all three histograms. Address Infer feedback by iterating `async_state->block_indices` directly when removing async IO map entries. Keep `PollAndProcessAsyncIO()` holding an owning `shared_ptr<AsyncIOState>` while erasing those entries so the coalesced request state cannot be destroyed mid-loop. Format `util/io_dispatcher_imp.cc` and `util/io_dispatcher_test.cc` with clang-format 21.1.2 to fix the format signal. Reviewed By: pdillinger, anand1976 Differential Revision: D113549401 fbshipit-source-id: dafc272c9252e1baccf404f681d33f76890ce0b2 |
||
Jay Huh
|
71d3bd1773 |
Fix heap-buffer-overflow(read) parsing a corrupted data-block hash index (#14993)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14993 A corrupted RocksDB SST data block whose footer sets the hash-index bit (`kDataBlockBinaryAndHash`) could drive an out-of-bounds heap read while the block is being parsed. `DataBlockHashIndex::Initialize` guarded its `num_buckets` / `map_offset` arithmetic only with `assert()`, which is compiled out in release and ASAN builds. When the encoded `num_buckets` exceeds the block, the `uint16_t` `map_offset` computation underflows; that value then flows into `Block`'s ctor at `input.remove_suffix(input.size() - map_offset)`, inflating `restart_offset_` far past the real allocation. If `size` is later zeroed as an error marker (e.g. a corrupted separated-KV footer), `Block::GetCorruptionStatus` builds a `Slice(data(), restart_offset_)` and `DataBlockFooter::DecodeFrom` reads `data() + restart_offset_ - 4`, over-reading the block's heap buffer (reported by agentic fuzzing, T279933950). Fix: - `DataBlockHashIndex::Initialize` now returns `bool` and rejects a `num_buckets` of 0 or one whose bucket array plus the `NUM_BUCKETS` field does not fit within `size`, instead of relying on debug-only asserts. - `Block`'s ctor checks that return value and also rejects blocks too large for the 64KiB `uint16_t`-offset hash-index format, marking the block corrupt (`size = 0`) rather than under-flowing `map_offset` and over-reading. Reviewed By: pdillinger Differential Revision: D113429299 fbshipit-source-id: 3167106e793252c09fd6a558e3ac96de7a2169c0 |
||
|
Peter Dillinger
|
bb77d17072 |
Make Flush() optionally wait for OnFlushCompleted (FlushOptions::listener_wait) (#14992)
Summary: DB::Flush() with wait=true only waits until the flush result is committed (memtables removed from the immutable list). The EventListener::OnFlushCompleted callbacks run afterward, on the background flush thread, with the DB mutex released, and the waiter is only woken by a shared condition variable that many unrelated events also signal. As a result a Flush(wait=true) caller can return before -- or in the middle of -- its OnFlushCompleted callbacks. This surfaced as a flaky failure in the c_test table_filter_listener_metadata case, which asserts listener_state.flush_count == 3 immediately after Flush() returns (https://github.com/facebook/rocksdb/actions/runs/29976431449). It is also long-standing behavior that other tests have worked around with TEST_WaitForBackgroundWork(). This adds an opt-in FlushOptions::listener_wait (default false). When set together with wait=true, Flush() does not return until the OnFlushCompleted callbacks for the flushed memtables have finished. Default behavior is unchanged. Implementation: MemTableList gains a per-column-family num_pending_flush_notifications_ counter (guarded by the DB mutex). The background flush thread reserves a pending notification at flush-commit time -- before the mutex is released to run the callbacks -- and releases it after they return, in both the single-CF (FlushMemTableToOutputFile) and atomic (AtomicFlushMemTablesToOutputFiles) paths. WaitForFlushMemTables treats a CF as finished only when its normal predicate holds and the counter is zero. Because the memtables are removed and the counter incremented under one continuous mutex hold (and, for atomic flush, all CFs are reserved before the per-CF notify loop starts releasing the mutex), a listener_wait waiter can never observe the flush as finished before the callbacks complete. The counter is per-CF rather than per-flush, so a listener_wait waiter can wait somewhat longer than strictly necessary: under rapid flushing it may block until an unrelated, later flush's OnFlushCompleted on the same CF also drains (e.g. a waiter satisfied by GetEarliestMemTableID() advancing still waits for the whole CF's pending notifications). This can only over-wait, never return early, and is tolerated in favor of implementation simplicity; a per-memtable-id watermark could tighten it if it ever matters. To keep the counter balanced against future edits, the release uses an RAII Defer (runs on every exit path) and ~MemTableList asserts the counter has drained to zero; SubPendingFlushNotifications asserts against underflow. A reservation is taken only when a callback will actually fire (committed flush info non-empty, not a mempurge, CF not dropped, listeners registered). Also adds the C API rocksdb_flushoptions_set/get_listener_wait (regenerated via tools/c_api_gen) and a DBImpl::TEST_FlushMemTableWithListenerWait() helper. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14992 Test Plan: New db_flush_test cases ListenerWaitWaitsForOnFlushCompleted and ListenerWaitAtomicFlushWaitsForAllOnFlushCompleted use a listener that blocks inside OnFlushCompleted plus spurious bg_cv_ wakeups to deterministically verify that Flush(wait=true, listener_wait=true) does not return until the callback (all CFs, for atomic flush) has finished; the atomic case guards the per-CF mutex-release gap in the notify loop. Converted existing racy/worked-around cases to the new option: c_test table_filter_listener_metadata, listener_test OnSingleDBFlushTest and MultiDBMultiListeners, and db_flush_test FireOnFlushCompletedAfterCommittedResult (the last previously relied on TEST_WaitForBackgroundWork()). The auto-generated C API roundtrip test covers the new flushoptions field. db_stress StressTest::TestFlush now sets listener_wait on a hard-coded 1 in 4 of its manual flushes (both the atomic and non-atomic branches), so the crash test exercises the new path with the DbStressListener registered. Reviewed By: jaykorean Differential Revision: D113425407 Pulled By: pdillinger fbshipit-source-id: 6fd0f5bc93fabaccd8010c2fead1d93c4c44ccfb |
||
|
Peter Dillinger
|
1eb4a6e3e1 |
Rocksdb Secondary Cache Test failed: assertion failed - corruption: corrupted atomic group The file (#15000)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15000 Reviewed By: xingbowang Differential Revision: D113225538 Pulled By: pdillinger fbshipit-source-id: fea9a671e9291ed030c5396264734195b2fb78bf |
||
Ranjan Banerjee
|
19482988e3 |
Add parallel CheckpointEngine [4/4] (#14948)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14948 Adds a reusable `CheckpointEngine` (opened once, works for any DB) that creates checkpoints via the shared `CopyEngine` pool, copying and hard-linking data files across `max_background_operations` threads. `CheckpointImpl::CreateCheckpoint` is refactored into a shared `CreateCheckpointImpl`, with the serial/parallel split behind a `CheckpointFileMover`; files the FileSystem cannot hard-link fall back to copies. The legacy `Checkpoint` API stays serial and unchanged. Reviewed By: xingbowang Differential Revision: D111387993 fbshipit-source-id: 036d375da9c9b8f051cfc3da0551b035e29d80c4 |
||
|
Ranjan Banerjee
|
0c0bfbe8be |
Add hard-link support to CopyEngine [3/4] (#14947)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14947 Teaches the shared `CopyEngine` to hard-link files through its background pool: adds `WorkItemType::Link` and `CopyEngine::LinkFile(src, dst, dst_env)`, which returns the FileSystem's `NotSupported` when links are unavailable so callers can fall back to copying. This is what CheckpointEngine (next diff) needs for warm storage, where linking is supported but benefits from parallelism. No existing caller links through the engine yet (BackupEngine never hard-links), so no behavior change; the path is exercised by a new `copy_engine_test.cc` (parallel copy, parallel link, single-thread copy, per-item error reporting). Reviewed By: xingbowang Differential Revision: D111387972 fbshipit-source-id: ec0b0e9b80749a3f51e62066ceef2df21a1811d2 |
||
|
Ranjan Banerjee
|
fac47c3072 |
Extract shared CopyEngine [2/4] (#14946) (#14946)
Summary:
Extracts BackupEngine's parallel file-copy/checksum thread pool into a standalone `CopyEngine` (`utilities/copy_engine/copy_engine.{h,cc}`) so `CheckpointEngine` (later in the stack) can reuse it. Behavior-preserving; all `TEST_SYNC_POINT` names are unchanged.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14946
Reviewed By: xingbowang
Differential Revision: D111387950
Pulled By: rban1
fbshipit-source-id: b5046adb98735a606fd1ba471731fd316ce6f036
|
||
Anusha Mukka
|
f56112abb7 |
Plumb Env::IOActivity into DBImpl::ResumeImpl (#14952)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14952 `DBImpl::ResumeImpl()` constructed its local `ReadOptions`/`WriteOptions` with defaults and carried a `// TODO: plumb Env::IOActivity, Env::IOPriority` comment. Those options flow into `VersionSet::LogAndApply()` (the MANIFEST write during error recovery), so the rate limiter could not attribute or prioritize that I/O correctly. This resolves the TODO for `ResumeImpl` (1 of 91 identical TODOs across the codebase) by accepting an `Env::IOActivity` parameter and constructing the options with it, following the pattern already established in `Get()`, `MultiGet()`, and `NewIterator()`, and in the `kDBOpen` paths in `db_impl_open.cc`. Error recovery is driven by flush operations (as the existing comment in `ErrorHandler::RecoverFromRetryableBGIOError` notes), so both production callers and the `TEST_ResumeImpl` hook pass `Env::IOActivity::kFlush`. Setting `io_activity` on these options is safe: `LogAndApply` does not assert `io_activity == kUnknown`. Reviewed By: pdillinger Differential Revision: D112537885 fbshipit-source-id: 59045fd2f0a6e5bbacef6e0381e03ff95cd28c51 |
||
|
Peter Dillinger
|
744f120b12 |
Zero-copy reads of embedded (same-file) blob wide-column entities (#14990)
Summary: This is a follow-up to D112212266 / https://github.com/facebook/rocksdb/pull/14943 (multi-buffer PinnableWideColumns), which added the zero-copy read path for separate-blob-file references and explicitly left extending it to SST embedded-blob references as a follow-up. Point GetEntity/MultiGetEntity of a wide-column entity whose columns are stored as embedded (same-file) blob records previously deserialized the entity, copied every column name/value, fetched each embedded blob (copying it), and re-serialized the whole entity before GetContext ever saw it (BlockBasedTable::MaybeResolveEmbeddedValue's wide-column branch). This extends the existing multi-buffer PinnableWideColumns zero-copy machinery (previously only for separate blob files / direct-write) to embedded blob columns. Changes: - New SameFileBlobReader interface (db/blob/same_file_blob_reader.h), a narrow seam for resolving a same-file BlobIndex against the current SST. BlockBasedTable implements it (GetSameFileBlob), delegating to the existing blob-cache-backed / pinned embedded reads. - BlobFetcher is now an abstract interface; the Version-backed implementation moved to VersionBlobFetcher. Added an EmbeddedAwareBlobFetcher decorator that routes same-file references to a SameFileBlobReader and delegates all others to a base fetcher (EffectiveFetcher() selects the decorator only when a same-file reader is present). - The point-Get and MultiGet loops now hand the raw entity to GetContext with the table as SameFileBlobReader, instead of pre-resolving/re-serializing. This is gated on not building a row-cache replay log, so a row-cache entry still captures a fully resolved entity (a later row-cache hit never reopens the SST to resolve same-file references). - GetContext composes the decorator and resolves same-file and separate-file blob columns zero-copy across the columns / default-value / merge consumers. - The iterator (DBIter) path is unchanged (still re-serializes via MaybeResolveEmbeddedValue). - Also fixes latent method hiding in the test-only MockBlobFetcher. Intended follow-ups: - Lazy/deferred embedded resolution: return a LazyWideColumns result whose blob columns stay unresolved and are resolved on demand (whole or by byte range). The SameFileBlobReader seam and the polymorphic BlobFetcher introduced here are the mechanism for embedded support there. - ReadOptions usage cleanup/optimization: have DBIter/BlobReader hold a single ReadOptions instead of decomposed scalar fields (removing the decompose-then-recompose in CreateBlobFetcher and closing the field-loss gap), and split VersionBlobFetcher into a ReadOptions-referencing variant for transient/stack fetchers plus an owning variant for lifetime-independent ones (the lazy resolver and compaction), to avoid copying the (growing) ReadOptions on the hot path. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14990 Test Plan: Added embedded wide-column tests to db/blob/db_blob_index_test.cc: - GetEntity/MultiGetEntity of an embedded-blob wide-column entity return the correct columns and resolve them zero-copy (blob columns served from the blob cache: identical data() across reads and in separate backing buffers; pointer-stable across a PinnableWideColumns move). - Plain Get of an entity whose default column is an embedded blob. - Merge base and GetMergeOperands over an embedded-blob entity. - read_tier == kBlockCacheTier yields Incomplete for an unresolved same-file reference. - A corrupt embedded record surfaces Corruption, not a raw BlobIndex. # Performance validation with db_bench Measured with db_bench benchmarks added in https://github.com/facebook/rocksdb/issues/14995 / D113455538, temporarily stacked on this diff for A/B testing. ## Setup - **Baseline**: this diff's parent + db_bench benchmark commit. - **Treatment**: this diff, incl. its test/lint fixups + the same db_bench benchmark commit. - Both are release builds (`DEBUG_LEVEL=0 make db_bench`). The diff touches only the read path, so both binaries produce byte-identical SSTs; the db_bench commit is additive (`tools/db_bench_tool.cc` only) and identical in both. - Host: AMD EPYC-Genoa, 166 vCPU, 224 GiB RAM. Single reader thread. ## Method - Datasets are created **once** and read by **both** binaries via `--use_existing_db`, so the A/B compares identical on-disk data. - Each read runs twice in-process (warmup + measured); the blob cache (1 GiB) and block cache (512 MiB) hold the whole ~400 MiB dataset, so the reported (2nd) run is fully warm — isolating the column-resolution cost the diff changes rather than I/O. - 100k keys, `value_size=1024`, `embedded_blob_min_size=256`, `compression_type=none`, `format_version=7`, `--duration=10` per measured run. - Datasets: - **A** — wide-column entity, `num_short_wide_columns=1` + `num_wide_columns=4` (small inline default column + 4 embedded-blob columns). Read via `readrandomentity` (GetEntity) and `multireadentity` (MultiGetEntity, batch 32). - **B** — wide-column entity, `num_short_wide_columns=0` + `num_wide_columns=4` (the default column itself is an embedded blob). Read via plain `readrandom` (Get returns the resolved default column). - **C** — whole-value embedded blobs (`fillembeddedblob`). Read via plain `readrandom`. This path is **not** changed by the diff — a regression check. Example commands: ``` # create (once) db_bench --benchmarks=fillembeddedentity --db=/tmp/perfA --use_existing_db=0 \ --num=100000 --value_size=1024 --embedded_blob_min_size=256 --format_version=7 \ --num_wide_columns=4 --num_short_wide_columns=1 \ --use_blob_cache=true --blob_cache_size=1073741824 --cache_size=536870912 \ --compression_type=none # read (each binary; warmup + measured) db_bench --benchmarks=readrandomentity,readrandomentity --db=/tmp/perfA \ --use_existing_db=1 --threads=1 --duration=10 <same options> ``` ## Results (warm, 2 runs each) | Benchmark (read path) | Baseline µs/op | Treatment µs/op | Latency | Baseline Kops/s | Treatment Kops/s | Throughput | |---|---|---|---|---|---|---| | `readrandomentity` — GetEntity, inline default + 4 embedded cols | 7.16 | 5.00 | **-30%** | 140 | 200 | **1.43×** | | `multireadentity` — MultiGetEntity (batch 32) | 6.46 | 4.54 | **-30%** | 155 | 220 | **1.42×** | | `readrandom` — Get of embedded default column (dataset B) | 6.55 | 2.12 | **-68%** | 153 | 471 | **3.09×** | | `readrandom` — whole-value embedded blob (dataset C, unchanged path) | 1.92 | 1.91 | ~0% | 521 | 524 | neutral | Both runs were within ~1–3% of each other (raw numbers below). ## Interpretation - **GetEntity / MultiGetEntity of embedded-blob wide-column entities improve ~30% in latency (~1.4× throughput)** — the expected win from resolving embedded blob columns zero-copy into the `PinnableWideColumns` backing buffers instead of the old deserialize + per-column copy + re-serialize round trip. - **Plain `Get` of an entity whose default column is an embedded blob improves ~3×** — the largest win, because the old path re-serialized the entire entity even when only the default column is returned, whereas the new path resolves just that column. - **The whole-value embedded blob `Get` path is unchanged (~0% delta), confirming no regression** on the pre-existing zero-copy path. No unexpected regressions observed; all deltas match the diff's intended scope. ## Raw measured numbers (both runs) ``` readrandomentity BASELINE 7.183 / 7.137 µs/op 139225 / 140118 ops/s readrandomentity TREATMENT 4.992 / 5.008 µs/op 200333 / 199681 ops/s multireadentity BASELINE 6.458 / 6.458 µs/op 154837 / 154843 ops/s multireadentity TREATMENT 4.509 / 4.577 µs/op 221798 / 218463 ops/s readrandom (Bdef) BASELINE 6.497 / 6.609 µs/op 153920 / 151307 ops/s readrandom (Bdef) TREATMENT 2.126 / 2.120 µs/op 470317 / 471805 ops/s readrandom (Cblob) BASELINE 1.891 / 1.947 µs/op 528760 / 513554 ops/s readrandom (Cblob) TREATMENT 1.895 / 1.920 µs/op 527767 / 520807 ops/s ``` Reviewed By: xingbowang Differential Revision: D113347622 Pulled By: pdillinger fbshipit-source-id: c51bc187085695d559411d9c0555bb52c8e0a61a |
||
|
Peter Dillinger
|
0347b4df61 |
Add db_bench benchmarks for embedded-blob and wide-column entity reads (#14995)
Summary: db_bench had no way to exercise SST embedded (same-file) blob reads or wide-column entity reads (GetEntity/MultiGetEntity), which are needed to measure the zero-copy embedded-blob wide-column entity read path. This adds: - Two setup benchmarks, fillembeddedentity and fillembeddedblob, that build an SST via SstFileWriter::OpenWithEmbeddedBlobs and ingest it (embedded blobs can only be produced this way, not by fillrandom/flush/compaction). Entity mode writes wide-column entities whose large columns are embedded blobs; blob mode writes whole-value embedded blobs. Both require format_version >= 7. - readrandomentity (GetEntity) and multireadentity (MultiGetEntity) read benchmarks; plain readrandom is reused for the whole-value embedded blob baseline and for entity default-column Get. - Flags embedded_blob_min_size, num_wide_columns, and num_short_wide_columns to control the embedded threshold and the inline/embedded column mix per entity. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14995 Test Plan: Manual db_bench runs of the new benchmarks, used to validate D113347622. Reviewed By: xingbowang Differential Revision: D113455538 Pulled By: pdillinger fbshipit-source-id: c6b62f5a2d6a61ae0b7cc32ac1b445b566ea8842 |
||
|
Peter Dillinger
|
755c104bca |
Unify clang detection for the C API codegen and add make regen-c-api (#14994)
Summary: `USE_CLANG=1 make check-c-api-gen` failed with "clang++ not found" because the staleness check and the Python generator detected clang inconsistently: the Makefile inspected the `$(CXX)` make variable (the fbcode clang path) while the generator read the `CXX` environment variable, which make never exports. More broadly, clang++ was detected in several ad hoc places with fragile heuristics (grepping the compiler path, duplicated fallback lists in shell and Python). Centralize clang++ detection in build_detect_platform as a single `CLANG_CXX`: - `CLANG_CXX` honors `$CXX` when it is itself clang (so CI's `CXX=clang++-21` is used), else takes the first clang++ on PATH (bare, versioned, or the Meta-internal clang++.par). `CXX` (when building with clang) and `CLANG_ANALYZER` now derive from `CLANG_CXX` instead of re-detecting. - Infer `USE_CLANG=1` when the compiler is clang, detected via the compiler's own `__clang__` predefined macro rather than matching the binary path (which false-positives, e.g. a home dir containing "clang"), and emit it so clang-specific Makefile logic is consistent. - Emit `CLANG_CXX` to make_config.mk and pass it explicitly to the codegen, so `make check-c-api-gen` works without requiring `USE_CLANG=1` and the generator never guesses. Add a first-class `make regen-c-api` target -- now the preferred regenerate command referenced by the generated banners and user-facing messages. The Python generators no longer detect clang themselves; `--clang` defaults to `clang++` for manual runs and is overridden by the Makefile with `CLANG_CXX`. Regeneration verifies the clang runs before writing any file, so a missing/broken toolchain aborts cleanly instead of leaving a partially regenerated tree. Make field discovery clang-version-independent: older clang (e.g. 15) spells field types fully qualified (`rocksdb::ReadTier`, `rocksdb::BlockBasedTableOptions::IndexType`) while the fbcode/CI clang (21) uses the bare `ReadTier` / `IndexType` the family tables expect. Strip the ROCKSDB namespace and enclosing-struct qualifiers so both agree; this is a no-op on newer clang, so checked-in output and CI are unaffected. Also escape the literal "generated" in the generator .py sources (they emit the marker but are not themselves generated) so tooling stops treating them as read-only generated files. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14994 Test Plan: Manual verification (not covered by CI): - `make check-c-api-gen` now runs the staleness sub-check (instead of skipping) and passes in a default gcc build as well as with `USE_CLANG=1` on an fbcode host, since `CLANG_CXX` is always populated. - `ROCKSDB_NO_FBCODE=1 make regen-c-api` using a system clang 15 (clang++.par) produces output byte-identical to the checked-in files (generated with clang 21), and `check-c-api-gen` reports "up to date" -- confirming the namespace/struct stripping is version-robust and a no-op on clang 21. - `regen_all.py --clang /nonexistent` exits non-zero with a clear message and leaves the generated files untouched. - `make regen-c-api` is idempotent (no diff on a clean tree). - Confirmed the escaped `\x40generated` still emits `// generated` byte-for-byte in the regenerated c.h/c.cc/.inc. - Reviewed .github/workflows/pr-jobs.yml: the build-linux-clang-21-no_test_run job's `CXX=clang++-21 make check-c-api-gen` resolves `CLANG_CXX` to clang++-21 (honoring `$CXX`) rather than the container's stray bare clang++. Reviewed By: xingbowang Differential Revision: D113447462 Pulled By: pdillinger fbshipit-source-id: 91ca92d21a71a8b92f9ff57500a8cec91a884078 |
||
|
Ranjan Banerjee
|
0a2ba2a164 |
Add shared backup/checkpoint option bases [1/4] (#14945)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14945 First diff in a stack that lets `Checkpoint` and `BackupEngine` share one parallel copy/link infrastructure. Pure options refactor, no behavior change. Adds two shared option bases in `rocksdb/utilities/checkpoint.h`: `CheckpointOrBackupEngineOptions` (engine-level config) and `CreateCheckpointOrBackupOptions` (per-operation options). `CheckpointEngineOptions` / `CreateCheckpointOptions` derive from them for checkpoint; `BackupEngineOptions` / `CreateBackupOptions` derive from them for backup and keep their backup-only fields. Reviewed By: xingbowang Differential Revision: D111387916 fbshipit-source-id: d7efa528ac91fb00090130c9d2a671b13e934a82 |
||
|
Jay Huh
|
dcb5c2bf53 |
Restore MultiScan prefetch stats dropped in the IODispatcher refactor (#14991)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14991 D91705195 ("Replace Prefetch Logic in BlockBasedTableIterator with IODispatcher") moved MultiScan block prefetching out of BlockBasedTableIterator and into the IODispatcher. The IO-level RecordTick/RecordInHistogram calls lived in the deleted prefetch code, and the IODispatcher was not given equivalent instrumentation, so these tickers were still defined but never populated (they published as absent): - MULTISCAN_BLOCKS_PREFETCHED - MULTISCAN_BLOCKS_FROM_CACHE - MULTISCAN_PREFETCH_BYTES - MULTISCAN_IO_REQUESTS - MULTISCAN_IO_COALESCED_NONADJACENT - MULTISCAN_BLOCKS_PER_PREPARE (histogram) The PREPARE_*, SEEK_ERRORS, and PREFETCH_BLOCKS_WASTED tickers were unaffected (recorded on the prepare/teardown path, which was not replaced). Fix: - ReadSet now exposes generic, caller-agnostic prefetch counters (GetNumBlocksPrefetched, GetNumIORequests, GetPrefetchBytes, GetNumCoalescedNonadjacent), incremented once per block in DispatchPrefetch -- the single choke point for both immediate and memory-deferred dispatch. Counting at dispatch time (not completion) keeps async and deferred prefetches accounted for without double counting the async->sync fallback. - PrepareIORequests now outputs prefetch_bytes (sum of request len) and num_coalesced_nonadjacent (within-threshold gap count) while building coalesced requests, per review feedback to avoid the extra pass over read_reqs/coalesced_block_indices at former line 836. DispatchPrefetch now gets stats directly from PrepareIORequests. - The MULTISCAN_* ticker names stay at the caller: MultiScanIndexIterator's destructor records the aggregate counters at end-of-scan (alongside the existing PREFETCH_BLOCKS_WASTED), so async/deferred reads that complete during iteration are included. MULTISCAN_BLOCKS_PER_PREPARE is recorded in BlockBasedTableIterator::Prepare. This keeps the IODispatcher a generic mechanism rather than leaking a caller's metric names into it. - Also plumb the same metrics into PerfContext for per-operation visibility. Reviewed By: xingbowang Differential Revision: D113356436 fbshipit-source-id: 660e9755d6b1b9aecccd112df94eb6838061b298 |
||
|
Jay Huh
|
7c385f4441 |
Add license headers to generated C API fragments (#14985)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14985 The Open Source Automated Checkup (T280493780) flagged the 27 generated `c_api_gen/c_generated_*.inc` fragments for missing the required copyright header ("Meta Platforms, Inc. and affiliates"). The fragments are emitted by the C API code generators, none of which wrote a header, so every regeneration reproduced the violation. Root-cause fix in the generators so regenerated output is compliant: - `tools/c_api_gen/generate_c_api.py` (`HEADER_BANNER`) - `tools/c_api_gen/auto_simple_bindings.py` (`auto_header_banner`) - `tools/c_api_gen/gen_roundtrip_tests.py` Each now emits the standard Meta dual-license block (matching `c_base.h` / `include/rocksdb/c.h`) ahead of the `// generated` marker, and the 27 checked-in `.inc` fragments are regenerated accordingly. The assembled `include/rocksdb/c.h` and `db/c.cc` already carry a top-level header, so `regen_all.py`'s inliner now strips each fragment's license block when inlining to avoid repeating it once per fragment; those two files are therefore unchanged. Also adds the header to `tools/gtest_parallel_repro.py`, the one non-`.inc` source file flagged by the checkup. Reviewed By: pdillinger Differential Revision: D113242908 fbshipit-source-id: 4d81d1355c8d8a35cbc4377dd9b21df901ad3cb7 |
||
Josh Kang
|
2c77c32ac9 |
Add Get and MultiGet sync/async plumbing (#14934)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14934 First diff in the async read stack: pure plumbing, no behavior change. Both the point `Get` path and the batched `MultiGet` path gain synchronous/coroutine twins generated by `DEFINE_SYNC_AND_ASYNC`; `DB::GetAsync` and `DB::MultiGetAsync` become the public callback-based async entry points with synchronous default implementations. For `Get`, the existing `DBImpl::Get` / `GetImpl` bodies are relocated into the sync/async wrapper. For `MultiGet`, the same wiring continues through the DBImpl, version, and table-cache layers. The generated coroutine variants still run the existing synchronous logic. This keeps the refactor mechanical while preserving dispatch semantics: the coroutine side remains virtual at the same `GetImpl` boundary as the synchronous path, read-only and secondary DBs use generated sync/coroutine overrides instead of falling back to base `DBImpl`, and `StackableDB` leaves async `Get` / `MultiGet` to the base wrappers so wrapper-specific `Get` / `MultiGet` behavior is preserved. For ABI compatibility and avoiding MACRO blocks in public headers, the asynchronous APIs are callback based instead of coroutine based. Internally it will be developed using coroutines, and applications can also develop their own coroutine wrappers. This is meant to make the later changes to actually enable coroutine-backed async queries much easier to review. Benchmarks: Compared `db_bench` with and without this diff using: `db_bench --benchmarks=fillseq,readrandom --num=1000000 --reads=5000000 --key_size=16 --value_size=100 --seed=12345 --db=<tmp>` | Benchmark | Without diff avg | With diff avg | Delta | | `fillseq` | 4.076 us/op | 4.165 us/op | +2.2% | | `readrandom` | 3.939 us/op | 3.947 us/op | +0.2% | No meaningful local `db_bench` regression was observed. Reviewed By: xingbowang Differential Revision: D109346179 fbshipit-source-id: 0dee1239f49099b67c11c63426204466e963f903 |
||
Andrew Chang
|
2b628ccfe5 |
Preserve fast_sst_open metadata across MANIFEST rotation (#14987)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14987 The `fast_sst_open` optimization speeds up DB open times by persisting file metadata information in the MANIFEST. When MANIFESTs are rotated however, this metadata is currently getting dropped, since it is not fed through `VersionEdit::AddFile`. There were also a few additional missing `AddFile` call sites. Fix: Add an optional `file_open_metadata` to `AddFile`. Feed this through wherever we call `AddFile`. Reviewed By: anand1976 Differential Revision: D113265655 fbshipit-source-id: c8371d66f6555bb9c372eea095bfa30a5f56a88a |
||
|
Andrew Chang
|
0745290ca6 |
Add OpenAndCompact DB open latency histogram (#14986)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14986 Why: We want to measure the DB open time during offloaded compactions. What: Add a histogram for the DB open time in `DB::OpenAndCompact()`. Reviewed By: anand1976 Differential Revision: D113136652 fbshipit-source-id: f8b2d74bcda3506f7b9ac80fa3e50a172deafc5e |
||
|
Peter Dillinger
|
ffa95117b9 |
Bug fix and clean up error/status handling for wide-column + blob support (#14980)
Summary: An audit of the wide-column blob-reference read paths (from the preceding "multiple backing buffers" change, D112212266) surfaced several inconsistencies in how Status is used: control-flow-via-Status, a status classification that diverged from the rest of the codebase, the same "no blob fetcher" Corruption repeated at many call sites, a swallowed non-OK status (bug), and read statistics advanced on failed reads. This change cleans those up. It is internal only -- no public API or on-disk format change -- and stacks on that diff. Concretely, this resolves two of the intended follow-ups called out in D112212266: * "Resolve FIXME about pre-existing swallowed non-OK status in db_impl.cc" * the error-status/API cleanup (classify an unrecognized wide-column version as Corruption, not NotSupported, at all entry points; replace the NotSupported sentinel from GetValueOfDefaultColumn with an is_blob_reference out-param, removing GetValueOfDefaultColumnResolvingBlobs; and centralize the "blob-backed value without a blob fetcher" Corruption into the resolvers, resolving the ResolveEntityForMerge FIXME, rather than repeating it at each caller). The other two follow-ups from that diff -- the value_size_soft_limit TODO in db_impl.cc and extending the zero-copy optimization to SST embedded-blob references -- are deferred to separate diffs. Unrecognized version -> Corruption. A wide-column version newer than kVersion2 now returns Corruption instead of NotSupported, consistent with how the rest of the codebase classifies an unrecognized serialized version/type (BlobIndex, the blob log header, the table footer). The guard is applied at every entry point (Deserialize, GetValueOfDefaultColumn, HasBlobColumns, ForEachBlobFileNumber); the latter two previously lacked it and would misparse a future/corrupt version. Reject trailing data after an empty entity. Deserialize() previously returned OK early for a zero-column entity, silently ignoring any trailing bytes; it now dispatches zero-column entities to the V1/V2 parser like the non-empty case, so the "exactly one serialized entity" trailing-data check the preceding diff added for non-empty entities now covers the empty case too. Default-column extraction no longer signals via Status. now returns (value, is_blob_reference): for a blob-referenced default column it returns OK with the raw serialized BlobIndex bytes and the flag set, instead of a NotSupported sentinel that every caller had to detect and pair with a second, redundant full deserialization. GetValueOfDefaultColumnResolvingBlobs and the memtable GetDefaultColumnBlobIndexSlice helper are removed; a single ResolveDefaultColumnBlobReference() resolves the raw BlobIndex (handling an inlined index with no fetch). This also fixes a case where a non-default column 0 that happened to be a blob reference was misreported as a blob-backed default. The one remaining NotSupported is the honest, explicit base-DB case: reading a blob-index default column when is_blob_index was not requested. Null-blob-fetcher handling centralized. "A blob-backed value with no fetcher to read it" (reachable via a reader without blob support such as SstFileReader, or corrupt/legacy data) was checked -- and reported as Corruption -- at ~6 call sites with three slightly different messages, forced by the resolvers asserting the fetcher was non-null. The asserts are gone; each resolver now owns the check in one commented place: ResolveEntityBlobColumnsMultiBuffer (whole entity), ResolveDefaultColumnBlobReference (default column), and the MaybeResolveMemtableBlobValue plain-value path (which open-codes its fetch). Callers just propagate the status. A side benefit: an entity/column whose only blob references are inlined now resolves without a fetcher instead of being wrongly rejected. Return Status from the memtable resolve helpers, fixing a swallowed error. MaybeResolveMemtableBlobValue (was bool + Status*) and PostprocessMemtableValue- Read (was void + Status*) now return Status, matching codebase convention; MaybeResolveMemtableBlobValue reports "there was a reference to resolve" via a bool* did_resolve out-param (true means it has finalized value/columns). This resolves a pre-existing bug: on the blob-resolve path a non-OK status was returned early without the output cleanup the plain path performed, leaving a half-populated result behind an error. PostprocessMemtableValueRead now has a single success/error finalization block reached by every path (self-pin on success, clear outputs on any error), and returns the finalized status. Also switched the memtable "no blob fetcher" case from NotSupported to Corruption (an internal helper whose sole caller treats all non-OK statuses identically). Read-only/secondary read stats mirror the primary path on error. DBImpl::GetImpl gates byte/throughput accounting (BYTES_READ, BYTES_PER_READ, get_read_bytes) and merge-operand output on s.ok(); the read-only and secondary Get paths did so unconditionally, logging spurious samples (e.g. a 0-byte BYTES_PER_READ sample) on NotFound/error. They now gate on s.ok() too, while keeping NUMBER_KEYS_READ, MEMTABLE_HIT, and super-version cleanup unconditional. Header docs updated to match (Deserialize/DeserializeSimple return Corruption for an unexpected blob reference; unknown per-column value types are rejected as Corruption). Pull Request resolved: https://github.com/facebook/rocksdb/pull/14980 Test Plan: Extended db/wide/wide_column_serialization_test.cc: DeserializeUnsupportedVersion now expects Corruption; new FutureVersionRejectedConsistently verifies that Deserialize, GetValueOfDefaultColumn, HasBlobColumns, and ForEachBlobFileNumber all reject a future version as Corruption; V2GetValueOfDefaultColumnBlobRef now checks the is_blob_reference flag and raw-BlobIndex bytes and that resolving without a fetcher is Corruption; DeserializeRejectsTrailingData now also covers empty (zero-column) V1 and V2 entities. db/blob/db_blob_index_test.cc: MaybeResolveMemtableBlobValueWithoutFetcherFails- Closed updated to the new signature (asserts did_resolve + Corruption); new PostprocessMemtableValueReadClearsOutputsOnError pins the swallowed-status fix, covering both an already-non-OK incoming status and (the actual FIXME scenario) an OK incoming status whose blob-resolve path fails -- both clear outputs and return the error. The StackableDB-style DBBlobIndexTest.Get contract is unchanged (its NotSupported comes from the base-DB blob-index read path, not these helpers). Reviewed By: xingbowang Differential Revision: D113099114 Pulled By: pdillinger fbshipit-source-id: 4bc374dd74f8ba83bdf49bd8753c1a8d88529cde |
||
|
Peter Dillinger
|
30ba4908ba |
Enforce value_size_soft_limit for direct-write blob values in MultiGet (#14983)
Summary: Resolves the value_size_soft_limit TODO left in db_impl.cc by the PinnableWideColumns multi-buffer change (D112212266 / PR https://github.com/facebook/rocksdb/issues/14943). ReadOptions::value_size_soft_limit is meant to bound how much a MultiGet buffers, but it was enforced in the batch loop on pre-resolution sizes. Pre-flush blob direct write references (a plain value encoded as a BlobIndex, or a wide-column entity with unresolved blob-valued columns) are only resolved later, in MultiGetImpl's post-processing loop, so their (possibly large) blob values were fetched with no limit applied -- read unbounded. Separate-file blob columns do not have this gap because they are resolved eagerly in GetContext and thus already counted. Now the post-processing loop enforces the limit on direct-write resolution using the "always make progress" soft-limit semantics: a key is aborted only once the bytes already returned by this MultiGet exceed the limit, so at least one key is always read even if its value alone exceeds the limit. Once the returned total is over the limit, subsequent keys that need fetching get Status::Aborted. This bounds the reads while ensuring a caller retrying the aborted keys cannot loop forever on a single oversized value. Pieces that benefited from human input and refinement: * Enforcement semantics: settled on "always make progress" -- checking the bytes already returned (rather than a projected pre-fetch size, which in foreseeable user integrations could starve a lone key that by itself exceeds the limit) -- matching the existing separate-file/batch soft-limit behavior. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14983 Test Plan: Added to db/wide/db_wide_blob_direct_write_test.cc (memtable / pre-flush direct-write reads, using the ResolveDirectWriteWideColumns:BlobFetched sync point to confirm which blobs are fetched): * MultiGetEntitySoftLimitBoundsDirectWriteBlobResolution -- two over-limit entities: first read (progress), second aborted; one blob fetch. * MultiGetEntitySoftLimitAlwaysReadsOversizedKey -- a single entity whose value alone far exceeds the limit is returned OK (no starvation). * MultiGetSoftLimitBoundsDirectWritePlainValue -- same for plain values stored as direct-write blob references: first read, second aborted. Reviewed By: xingbowang Differential Revision: D113159249 Pulled By: pdillinger fbshipit-source-id: 5af443b86bdfc3ecd0112d3c5af541a77f0d3d67 |
||
|
Jay Huh
|
144deb248a |
Harden MaybeOpenReadOnlyOnPrimary stress test: fix TSAN lock-limit crash + drop false-positive read-only Get (#14979)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14979 `MaybeOpenReadOnlyOnPrimary` (added in D111995911) opens a read-only DB on the primary's live directory while the primary keeps mutating it. Two separate stress-test false positives were observed there; this diff fixes both and consolidates the earlier one-off fixes into a single diff. 1) TSAN deadlock-detector crash in the CF-name snapshot (`fbcode_tsan_blackbox_crash_test`): ThreadSanitizer: CHECK failed: sanitizer_deadlock_detector.h:67 "((n_all_locks_)) < (...)" (0x80, 0x80) `n_all_locks_` is the number of mutexes a thread holds *simultaneously*; TSAN caps it at 128 (0x80). The snapshot loop called `LockColumnFamily(cf)` to read `column_family_names_[cf]`, and that helper `.Lock()`s ALL per-key mutexes for the CF at once (`ceil(max_key / 2^log2_keys_per_lock)` -- e.g. 25,000 in blackbox with max_key=100000, log2_keys_per_lock=2) without releasing any, so a single call blows past 128. The writer (`MaybeClearOneColumnFamily`) holds all key locks while renaming a CF, so a single key lock is enough to synchronize the read. Fix: take one `MutexLock` on key-lock index 0 per CF instead of `LockColumnFamily`, keeping the simultaneously-held count at 1. (`LockColumnFamily` is a latent hazard for any caller; the pre-existing caller `MaybeClearOneColumnFamily` never runs in the crash-test config -- `clear_column_family_one_in=0` -- so only this new, ungated caller exposed it.) 2) False-positive read-only Get (folded in from D112987468). After flushing the primary, the test performed a `Get(Key(0))` on each CF of the read-only instance. That Get races against concurrent primary compaction, which can delete SST files the read-only's frozen MANIFEST still references, surfacing as IOError/PathNotFound/Corruption ("truncated block read"). The Get adds no unique coverage -- the scenario under test (the read-only close wrongly deleting primary live files) is detected on the primary's own verification paths -- so remove the Get block entirely instead of widening a tolerated-status list. Note on D112936376: that diff tried to fix crash (1) by setting `table_cache_numshardbits=0` on the read-only instance, on the theory that the primary's and reader's table caches contribute "128 unique lock objects." That is a misdiagnosis: the failing CHECK counts locks held *simultaneously*, and sharded-cache shard mutexes are held transiently (one at a time; read-path nesting is a constant depth of 2), so shard count does not affect `n_all_locks_`. The real cause is `LockColumnFamily`, fixed in (1), so `table_cache_numshardbits=0` is unnecessary and is not included here. This diff subsumes and replaces D112936376 and D112987468. Reviewed By: joshkang97 Differential Revision: D112980148 fbshipit-source-id: 012ff852109223f3d19d8dbbc065fde9340f9c66 |
||
|
Jay Huh
|
990827e916 |
db_bench: tolerate remote IO errors by skipping ops and continuing (#14971)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14971 Follow-up to https://github.com/facebook/rocksdb/pull/14942. That PR made db_bench read ops end the run with an INVALID result (`SetFatal` -> exit 1) on a tolerable remote IO error and left write ops on unconditional `SetFatal`. In practice that still failed the remote benchmark TW jobs (exit 1) on transient WS infra errors, and did not cover write ops or `DB::Open`. Change the toleration to skip-and-continue so a transient remote IO error no longer fails the run: - `HandleBenchmarkIOError` now just logs and returns on a tolerable error (was: `SetFatal`); the caller skips the failed op and keeps going. Non-tolerable, data-loss, and local-DB errors still `abort()` (core dump for debugging). - Applied uniformly to all worker op error paths: `Get` / `MultiGet`, put / merge, delete / delete-range, `DoDelete`, `RandomWithVerify` put/delete-many, and the `BGScan` iterator (stops the scan and continues). - `DB::Open`: on a tolerable remote IO error, exit cleanly (0) -- the benchmark cannot run, but it is not a db_bench / RocksDB failure. Gated by `--tolerate_io_errors_for_remote_dbs` plus a remote `--env_uri` / `--fs_uri` backend, so local runs and non-IO / data-loss errors are unaffected. The result is best-effort (skipped ops are not counted); the goal is to keep remote benchmark jobs green through transient WS infrastructure errors instead of failing or crashing. db_stress is intentionally NOT changed: it is a correctness (crash-recovery) test and must keep failing on real errors. Differential Revision: D112749712 fbshipit-source-id: a8ccd46ef66e6c3c97a1baffd35d27830879d428 |
||
Hui Xiao
|
7c4bf11044 |
CPU corruption injector: robust seed handling, per-run command logging, and doc fixes (#14940)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14940 A few improvements to the CPU corruption injector: - Seed handling: `--seed` is now validated against db_stress's accepted range and no longer silently wraps, so an out-of-range seed fails fast instead of producing misleading NO_INJECTION runs. Omit `--seed` for a fresh random base seed. - Per-run reproducibility: each run's full db_stress command is recorded at the top of its `gdb.log`, so any run can be reproduced by copy-pasting one line. - README: clarified the `--seed` semantics and examples. **Test plan:** All db_stress runs use a debug build: `make DEBUG_LEVEL=0 EXTRA_CXXFLAGS="-g -fno-inline" db_stress`. ## Seed handling: out-of-range seeds fail fast Command: ``` python3 tools/cpu_corruption_injector/runner.py --op write --runs 2 --stress_cmd ./db_stress --report_dir /tmp/tp_badseed --seed 4294967295; echo "exit=$?" python3 tools/cpu_corruption_injector/runner.py --op write --runs 1 --stress_cmd ./db_stress --report_dir /tmp/tp_badseed0 --seed 0; echo "exit=$?" ``` Output: ``` 2026-07-15 01:26:14,408 ERROR --seed range [4294967295, 4294967296] is outside db_stress's accepted [1, 4294967295]; pass a smaller --seed exit=2 2026-07-15 01:26:14,558 ERROR --seed range [0, 0] is outside db_stress's accepted [1, 4294967295]; pass a smaller --seed exit=2 ``` Both are rejected before any run starts (previously the seed silently wrapped and every run came back a misleading NO_INJECTION). ## Per-run command logged (full db_stress command in gdb.log) Command: ``` python3 tools/cpu_corruption_injector/runner.py --op write --runs 2 --stress_cmd ./db_stress --report_dir /tmp/tp_seed --seed 424242 head -6 /tmp/tp_seed/run_00000/gdb.log ``` Output (the first line is the full command; the run's gdb/db_stress output follows it): ``` # /usr/bin/gdb --batch --nx -iex 'py import sys; sys.argv=["injector.py", "--op", "write", "--op_index", "259", "--entry_fn", "rocksdb::MemTable::Add", "--target_fn", "rocksdb::MemTable::Add", "--seed", "424242", "--dir", "/tmp/tp_seed/run_00000"]' -x /data/users/huixiao/fbsource/fbcode/internal_repo_rocksdb/repo/tools/cpu_corruption_injector/injector.py --args /data/users/huixiao/fbsource/fbcode/internal_repo_rocksdb/repo/db_stress --db=/tmp/tp_seed/run_00000/db --expected_values_dir=/tmp/tp_seed/run_00000/exp --seed=424242 --verify_cpu_corruption_dir=/tmp/tp_seed/run_00000 --ops_per_thread=1000 --max_key=1000 --value_size_mult=8 --column_families=1 --nooverwritepercent=0 --writepercent=50 --delpercent=40 --delrangepercent=10 --readpercent=0 --iterpercent=0 --prefixpercent=0 --flush_one_in=20 --compact_range_one_in=10 --compact_files_one_in=10 --target_file_size_base=65536 --reopen=0 --destroy_db_initially=1 --clear_column_family_one_in=0 --disable_wal=0 --threads=1 --num_dbs=1 --disable_auto_compactions=1 --write_buffer_size=1073741824 --remote_compaction_worker_threads=0 --subcompactions=1 --batch_protection_bytes_per_key=8 --memtable_protection_bytes_per_key=8 --block_protection_bytes_per_key=8 --paranoid_file_checks=1 --paranoid_memory_checks=1 --memtable_verify_per_key_checksum_on_seek=1 --detect_filter_construct_corruption=true --verify_checksum=1 --compression_checksum=1 --file_checksum_impl=crc32c --verify_checksum_one_in=1 --verify_file_checksums_one_in=1000 --read_fault_one_in=0 --write_fault_one_in=0 --metadata_read_fault_one_in=0 --metadata_write_fault_one_in=0 --open_read_fault_one_in=0 --open_write_fault_one_in=0 --open_metadata_read_fault_one_in=0 --open_metadata_write_fault_one_in=0 --secondary_cache_fault_one_in=0 --sync_fault_injection=0 This GDB supports auto-downloading debuginfo from the following URLs: <https://debuginfod.centos.org/> Debuginfod has been disabled. ``` The first line of `gdb.log` is the complete `gdb` + `db_stress` command, including `--seed=424242` and `--max_key=1000`; the run's own output follows below it. ## The logged command reflects real flag values Temporarily set `max_key` to `12345` in `runner_build.py`, then: Command: ``` python3 tools/cpu_corruption_injector/runner.py --op write --runs 1 --stress_cmd ./db_stress --report_dir /tmp/tp_maxkey --seed 424242 head -1 /tmp/tp_maxkey/run_00000/gdb.log | tr ' ' ' ' | grep -- --max_key ``` Output: ``` --max_key=12345 ``` The logged command tracked the changed preset value. Reverted the change afterward. Reviewed By: archang19 Differential Revision: D112075800 fbshipit-source-id: 0d2be1741ede806a84a1a0f9ca6b2a32147c4e9c |
||
|
Peter Dillinger
|
81ea677072 |
PinnableWideColumns support for >1 backing buffer to avoid re-serialization (#14943)
Summary:
Wide-column read paths that materialize blob-referenced columns used to re-serialize the entire entity into a fresh contiguous buffer, copying the (potentially large) resolved column values byte-for-byte. This was forced by PinnableWideColumns holding a single backing buffer with all column Slices pointing into it.
PinnableWideColumns now holds its columns as zero-copy Slices into one or more stable backing buffers, kept in a std::forward_list<PinnableSlice> whose nodes never relocate on move, move-assignment, or splice. Resolved blob columns can be spliced in as additional backing buffers instead of re-serializing the entity.
Using address-stable nodes also cleans up the old Slice-invalidation handling. The previous single-buffer representation had to cope with a self-pinned PinnableSlice whose small (SSO) std::string relocates when the PinnableSlice is moved: on every move it detected the relocated backing pointer and re-derived the column Slices by re-parsing. With forward_list nodes that never move, the column Slices stay valid across move / move-assignment / splice, so that handling is gone and the move operations are now the defaulted member-wise moves.
The multi-buffer plumbing is internal only: a new PinnableWideColumnsHelper (a friend) performs the splice and the little bit of private-member access the read paths need, so there is no new public API surface and the previous `friend class DBImpl` is gone. The public view is unchanged: columns() still returns a single, in-order set.
Read paths converted to resolve without re-serializing:
- GetContext::SaveWideColumnEntityToColumns (eager GetEntity SST reads, separate-blob-file references)
- DBImpl::ResolveDirectWriteWideColumns (direct-write blob references)
- DBImpl::MaybeResolveMemtableBlobValue (blob-backed memtable reads)
Merge output still serializes (the MergeOperator API consumes/produces a serialized entity). Same-file ("embedded") blob columns are resolved -- and still re-serialized -- earlier in the block-based table reader (MaybeResolveEmbeddedValue / ResolveEmbeddedValueForGet) before the entity reaches GetContext, so the GetEntity path above only avoids re-serialization for separate-blob-file references. Compaction's separate entity machinery is unchanged.
API/accounting: deprecated PinnableWideColumns::serialized_size() (comment-only, per repo convention) in favor of the new payload_size() (sum of column name + value sizes; equals the value size for a plain value). Read statistics for entities (BYTES_READ, BYTES_PER_READ, per-key GetEntity/MultiGetEntity byte counts) and the ReadOptions::value_size_soft_limit threshold now measure entities by payload_size() instead of the serialized size. This drops framing bytes (entity measurements are slightly smaller), thus making plain-value and wide-column accounting consistent and reducing unnecessary exposure to implementation details.
Serialization cleanup enabled by the above:
- Consolidated Deserialize/DeserializeV2 into a single Deserialize(const Slice&, WideColumns&, blob_columns*) plus a DeserializeSimple alias for the no-blob-references case; both take const Slice&, require the input to be exactly one serialized entity, and reject any trailing data uniformly (V1 previously ignored trailing bytes). An unexpected blob reference where a fully-resolved entity is required now returns Corruption.
- ResolveEntityBlobColumns (serialized output, used only by merge) is now a thin wrapper over the new multi-buffer resolver plus Serialize, removing the duplicated fetch loop and the SerializeResolvedEntity helper.
- Consistently use `const Slice& input` rather than `Slice& input` for wide column functions expecting an input slice with no trailing data.
Design/polish from human review:
* The initial plan called for more complex handling around a vector<PinnableSlice> backing store (unique_ptr indirection and re-deriving the column Slices on move to cope with reallocation and SSO relocation); switching the backing store to a std::forward_list of address-stable nodes was a human suggestion that removed that complexity and the move-time Slice re-derivation entirely.
* Keep resolve plumbing off the public API via PinnableWideColumnsHelper and removing the DBImpl friend
* Various pieces of cleanup and polish (more left to do)
* The eager-GetEntity optimization
* The memtable inline-copy NOTE/TODO.
Intended follow-up:
* Resolve FIXME about pre-existing swallowed non-OK status in db_impl.cc
* Resolve TODO about value_size_soft_limit in db_impl.cc
* Extend this optimization to SST embedded-blob references
* Error-status/API cleanup: classify an unrecognized wide-column version as Corruption (not NotSupported) at all entry points; replace the NotSupported sentinel from GetValueOfDefaultColumn with an explicit is_blob_reference out-param (removing GetValueOfDefaultColumnResolvingBlobs); and centralize the "blob-backed value without a blob fetcher" Corruption into the resolvers (resolving the ResolveEntityForMerge FIXME) rather than repeating it at each caller
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14943
Test Plan:
Extended db/wide/wide_column_serialization_test.cc with PinnableWideColumns unit tests: set/payload_size for plain, V1, and resolved entities; move and move-assign of a multi-buffer result preserve column data pointers (the node-stability guarantee); Reset; ResolveColumns splices blob buffers in zero-copy (resolved values' data() point into the spliced-in buffers, not a re-serialized entity); serialized_size() == SerializedSizeV1(columns()); and Deserialize rejects trailing data after a serialized entity for both V1 and V2 layouts.
Built and ran (debug): wide_column_serialization_test, db_wide_basic_test, db_wide_blob_direct_write_test, db_blob_basic_test, compaction_iterator_test, db_merge_operator_test, db_iterator_test, udt_util_test, write_batch_with_index_test -- all pass. librocksdb and db_stress build. make format-auto and build_tools/check-sources.sh are clean.
Reviewed By: xingbowang
Differential Revision: D112212266
Pulled By: pdillinger
fbshipit-source-id: 13b89e2bb7d8371f4c1ff18b234a549bea9c9e77
|
||
|
Jay Huh
|
04cad97e7e |
Stress test concurrent primary and read-only DB on the same directory (#14938)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14938 Adds db_stress coverage for the topology where a read-only DB (`DB::OpenForReadOnly`) is opened on the same directory as a live read-write primary. This interaction was previously untested: `FLAGS_read_only` runs a DB by itself in read-only mode (no live writer), and `FLAGS_test_secondary` co-locates a secondary that tails the MANIFEST and never owns/deletes files, so neither exercises a read-only instance holding a stale live-file snapshot while the primary keeps creating files. That gap allowed a bug where a read-only DB's close ran obsolete-file cleanup against its stale snapshot and deleted the primary's live SST files. New flag `open_read_only_one_in=N`: on thread 0, with probability 1/N per operation, open a read-only instance on the primary's live directory, flush the primary (so new SSTs exist that are absent from the reader's frozen snapshot), do a few reads, then close the reader. If the read-only close wrongly purges obsolete files, the primary's live files are deleted and surface through db_stress's existing missing-file/corruption detection (deterministically on the next reopen). The read-only reader is opened with the primary's options (so it inherits the table factory, e.g. UDI) but with `compaction_service` and `listeners` cleared, since a read-only reader never compacts and must not share the primary's background-coordination services -- otherwise the reader's open/close would abort the primary's in-flight remote compactions. `db_crashtest.py` enables the mode randomly and excludes configurations it does not support (transactions, BlobDB, TTL, and the blob-direct-write profile). Startup validation in `db_stress_tool.cc` rejects those same combinations when the flag is set directly. Reviewed By: joshkang97 Differential Revision: D111995911 fbshipit-source-id: 0f7a2ef93e63cfa6677b6fa32c1bd34eb818c978 |
||
anand1976
|
865568398a |
Update version to 11.8.0 for 11.7 release (#14950)
Summary: - Bump version.h from 11.7.0 to 11.8.0 - Add `11.7.fb` to `check_format_compatible.sh` Part of 11.7 release workflow. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14950 Reviewed By: rban1 Differential Revision: D112627463 Pulled By: anand1976 fbshipit-source-id: 0685a7a7a45c157c2a7e392420e3e16d5a364727 |
||
|
Jay Huh
|
2809ce9d0f |
Tolerate remote IO errors in db_stress crash test and db_bench (#14942)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14942 Remote storage reached over `--env_uri` / `--fs_uri` (e.g. Warm Storage) can surface transient infrastructure IO errors that are unrelated to any RocksDB bug. Previously such an error would take down the test/benchmark process, making remote runs unusable. This diff lets both `db_stress` and `db_bench` tolerate non-data-loss IO errors, behind default-off flags that only take effect against a remote backend, so local behavior is unchanged and local IO errors are never masked. **db_stress:** Add a default-off flag `--tolerate_non_injected_io_errors_for_remote_dbs` that treats non-injected, non-data-loss IO errors as retryable, so remote infrastructure errors do not fail the stress harness through `SharedState::SafeTerminate()`. `IsErrorInjectedAndRetryable` enforces at the binary level that this only applies when a remote backend (`--env_uri` / `--fs_uri`) is in use; `db_crashtest.py` additionally force-disables it for local DBs. Remote crash-test jobs opt in by passing the flag on the command line (see the companion Tupperware config change). **db_bench:** Add a default-off flag `--tolerate_io_errors_for_remote_dbs`. When set and a remote backend (`--env_uri` / `--fs_uri`) is in use, a non-data-loss IO error surfaced by a benchmark operation ends the run gracefully with an INVALID result through the existing cooperative shutdown path (`SharedState::SetFatal`) instead of calling `abort()` (which previously produced a `Received signal 6 (Aborted)` process crash). All read `Get` / `MultiGet` abort sites and the `BGScan` iterator-error site are routed through a shared `HandleBenchmarkIOError` helper; IO errors on local DBs and data-loss errors are never tolerated. With the flag off (default), the original `abort()` behavior is preserved so local runs still produce a core dump for debugging. Reviewed By: anand1976 Differential Revision: D112136438 fbshipit-source-id: 510dbc644da78c4315a476ec766e26b2711dc060 |
||
|
Josh Kang
|
223b5ee13d |
Fix data race in VerifyPkSkFast between virtual call and TransactionDB destruction (#14930)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14930 Fix a TSAN-detected data race on the vptr between `MultiOpsTxnsStressTest::VerifyPkSkFast` making virtual calls through `db_` and `WritePreparedTxnDB::~WritePreparedTxnDB` updating the vptr during destruction. The race occurs because `VerifyPkSkFast` (called from a compaction notification callback on a background thread) accesses `db_` which is a non-atomic raw pointer. Meanwhile, the main thread destroys the TransactionDB wrapper via `db_owner_.reset()` in `StressTest::CleanUp()`. The previous fix (commit 768838ddcd99, `OnDBShutdownBegin`) reduced the race window by adding a `shutting_down_` check in the listener, but did not fully eliminate it because a background thread can pass the check before shutdown begins and then proceed to use `db_` while the destructor runs. This fix addresses the root cause in two ways: 1. In `VerifyPkSkFast`: Use the atomically loaded `db` local variable (from `db_aptr_.load(memory_order_acquire)`) for ALL virtual calls instead of the non-atomic `db_` member. This ensures the pointer used for virtual dispatch was safely acquired before any shutdown signaling. 2. In `StressTest::CleanUp()`: Store nullptr to `db_aptr_` before calling `Close()`. This prevents any NEW entries into `VerifyPkSkFast` from background threads that have not yet loaded the pointer. `Close()` then waits for all in-flight background work (including any `VerifyPkSkFast` that already loaded a non-null pointer) to complete before proceeding to destroy the DB. Together, these changes ensure that no background thread performs a virtual call through the TransactionDB pointer while the destructor is modifying the vptr. Reviewed By: xingbowang Differential Revision: D111480308 fbshipit-source-id: 0c998888d07712236c28eee60d04c6f169949a80 |
||
|
Jay Huh
|
6d8370ec56 |
cmake: discover gtest tests at ctest time to fix macOS CMake CI build (#14951)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14951 The `build-macos-cmake` CI job intermittently but persistently fails during `make` with: GoogleTestAddTests.cmake:... (parse_tests_from_json) * Line 1, Column 1 Syntax error: value, object or array expected. make: *** [all] Error 2 This is `gtest_discover_tests` running in its default POST_BUILD mode: it executes each freshly-built test binary during the parallel `-j` build to enumerate test cases as JSON. On the resource-constrained arm64 macOS runners, that discovery run competes with the in-flight compile jobs and can produce empty output, which CMake's JSON parser then rejects, aborting the whole build. The failing targets (e.g. `cache_reservation_manager_test`, `lru_cache_test`) are unrelated to whatever change is under test -- other test binaries in the same run discover fine -- confirming it is a build-phase discovery race, not a code defect. Switch discovery to `DISCOVERY_MODE PRE_TEST` so enumeration happens at ctest time (serially, after the build) instead of during the parallel build. This keeps the build green while preserving full test discovery. `PRE_TEST` requires CMake >= 3.18, so it is guarded by a version check; on older CMake (the project's declared minimum is 3.12) behavior is unchanged. Reviewed By: xingbowang Differential Revision: D112478854 fbshipit-source-id: 0d5edea416610f9cc1e9ae806d7d521165abe656 |
||
Laurynas Biveinis
|
d461606f0f |
Make range-lock waits interruptible with SetIsKilledCallback (#14927)
Summary: Add RangeLockManagerHandle::SetIsKilledCallback, a std::function<bool()> predicate mirroring SetEscalationBarrierFunc. When installed, RangeTreeLockManager wakes the toku lock_request::wait loop every 100ms to poll it; if it fires, the wait completes with a new DB_LOCK_INTERRUPTED result mapped to Status::Aborted(), so a blocked GetRangeLock() returns promptly instead of waiting out the lock timeout. Range-lock users that install no callback keep the original wait-until-grant-or-timeout behavior (no extra wakeups). Also remove the unused toku kill_waiter abort path (lock_request and locktree_manager), which had no caller. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14927 Test Plan: `build_tools/rocksptest.sh range_locking_test` passes, including the two new tests. KilledCallbackAbortsWait completes in ~90ms despite a 100s lock timeout, confirming the kill callback aborts the wait promptly; KilledCallbackFalseStillTimesOut confirms an installed-but-false callback still times out. Tested end-to-end together with the MyRocks side in D110890436: with rocksdb_use_range_locking=ON, a KILL QUERY parked on a conflicting range-lock wait is interrupted promptly (rocksdb.range_locking MTR suite) instead of waiting out rocksdb_lock_wait_timeout. Reviewed By: pdillinger Differential Revision: D111663405 Pulled By: laurynas-biveinis fbshipit-source-id: 4819e09bde66f4e932aef74f6adfd14789bb1425 |
||
|
Peter Dillinger
|
c9536aaef7 |
Fix quadratic point-in-time MANIFEST recovery in VersionBuilder (#14929)
Summary: Point-in-time MANIFEST replay -- best-efforts recovery (VersionSet::TryRecoverFromOneManifest) and secondary/follower catch-up (ManifestTailer) -- was quadratic in the number of MANIFEST records. |
||
|
Jay Huh
|
cf22ee8f5b |
Harden RocksDB PR comment artifact downloads (#14941)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14941 Download GitHub Actions artifacts into `_artifact` before reading comment result files so untrusted artifacts cannot overwrite checked-out workflow helper scripts. Keep `post-pr-comment.js` loaded only from `.github/scripts` and remove unnecessary inherited secrets from the review-comment wrapper workflows. Reviewed By: pdillinger Differential Revision: D112129548 fbshipit-source-id: c807eab2197018b0672a5e1d9710e4c6bfcfd0ac |
||
|
Peter Dillinger
|
c7f7b1b2ae |
Skip redundant DeleteWalsBefore during recovery when markers already advanced (#14918)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14918 When `optimize_manifest_for_recovery=true` and `track_and_verify_wals_in_manifest=true`, the close-time code (`MaybeWriteWalMarkersToManifestOnClose`) already writes `DeleteWalsBefore(max_wal+1)` to the MANIFEST. On the subsequent clean reopen, recovery was unconditionally emitting the same `DeleteWalsBefore` edit, causing 43 bytes of unnecessary MANIFEST growth. This violated the zero-growth guarantee that the stress test STRICT mode verifies, triggering `exit(1)` which then caused a secondary `property_info != nullptr` assertion in `DumpStats()` (from the Timer thread running during static destructor teardown). The fix adds the same monotonicity guard (`new_min_log > GetMinWalNumberToKeep()`) that is already used at all four other `DeleteWalsBefore` call sites (db_impl.cc:748, db_impl_files.cc:906, db_impl_write.cc:3331, memtable_list.cc:1015). This skips the redundant write when the WalSet min was already advanced at close time, while still correctly emitting it after a crash where the close-time markers were never written. Reviewed By: xingbowang Differential Revision: D110713771 fbshipit-source-id: 3592d11b20a9e660f1fea58a7a72565b2030c6d8 |
||
|
Josh Kang
|
dc72a64a5b |
Flush no-WAL live-file data with LockWAL (#14931)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14931 The failing crash-test scenario combines `disable_wal=1`, `LockWAL()`, and backup/checkpoint live-file-copy verification. A WAL-disabled write can leave a key only in the memtable. The old live-file path skipped `FlushForGetLiveFiles()` whenever `LockWAL()` was active, so the copied SST/WAL file set could omit that memtable-only key even though the original DB still served it. Restore verification could then report the key as present in the original DB but missing from the restored backup/checkpoint. Fix live-file capture to flush unpersisted WAL-disabled data even while `LockWAL()` is active. If all data is already persisted, the path still skips the flush while WAL is locked. If unpersisted data exists, backup/checkpoint can now block behind `LockWAL()` until another thread calls `UnlockWAL()` and the flush can safely proceed. To avoid self-deadlock, track `LockWAL()` ownership by thread with recursive counts. `UnlockWAL()` now has to be called by the same thread that successfully called `LockWAL()`, and wait=true flushes called by that same owner return `Aborted("Likely deadlock as the same thread called LockWAL()")` instead of waiting forever. Cross-thread flushes remain allowed and wait for all `LockWAL()` owners to unlock. Also audited MyRocks and can confirm they already conform to the Lock/Unlock on same thread semantics. Reviewed By: pdillinger Differential Revision: D111793089 fbshipit-source-id: 1bbb03b25d00d47440126cc6bb4ce7410302e25b |
||
|
Peter Dillinger
|
74aba599ab |
Fix WriteUnprepared transaction rollback after commit write failure (#14920)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14920 When a WriteUnprepared transaction's commit write fails while two_write_queues is enabled (e.g. due to an I/O error), CommitInternal() unconditionally cleared unprep_seqs_ and the flushed/unflushed save points before returning the error. That left the transaction with no record of its prepared sequence numbers, so a subsequent Rollback() -- the natural way for a caller to recover from a failed commit -- constructed a WriteUnpreparedCommitEntryPreReleaseCallback over an empty unprep_seqs_ and tripped assert(unprep_seqs.size() > 0) in its constructor. In release builds this would instead corrupt commit-cache accounting rather than aborting. Fix: only clear unprep_seqs_ and the save points on the commit success path. On the two-write-queues commit-write failure path we still remove the prepared entry that AddPreparedCallback may have added for commit_batch_seq (that seq is never stored in unprep_seqs_, so a later Rollback cannot clean it up), but unprep_seqs_ is otherwise preserved so the transaction can still be rolled back once the caller resumes the DB from the write error. Originally authored by Devmate trigger Reviewed By: xingbowang Differential Revision: D110271443 fbshipit-source-id: 7d991e01ddc74ccd170cbc25f6af160c5f3d5663 |
||
xingbowang
|
329d9e7adf |
Add blog post on Blob Direct Write and partitioned blob files (#14873)
Summary: - Add a RocksDB blog post explaining Blob Direct Write and partitioned blob files. - Cover the write-path transformation to `BlobIndex`, partition selection, blob-file lifecycle, read fallback for in-flight direct-write files, and the current v1 scope. - Describe policy-driven grouping use cases such as TTL bucketing with Universal Compaction and wide-column metadata. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14873 Test Plan: {F1991646296} Reviewed By: joshkang97 Differential Revision: D109564817 Pulled By: xingbowang fbshipit-source-id: 712fc0b6a8ffe68e2068af756dceb421c65a9af8 |
||
|
Josh Kang
|
5b1c3b37ac |
Disable codex reviews on PRs (#14933)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14933 Codex reviews currently have some issues with the API Key. Will disable them from reviews to prevent noise until it is properly fixed. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14932 Differential Revision: D111798073 Pulled By: joshkang97 fbshipit-source-id: 9340b89371185979e34465325c20d02a494990e5 |
||
|
Jay Huh
|
eca1a114e0 |
Count unreleased MultiScan prefetch blocks as wasted (#14926)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14926 This fixes a gap in the MultiScan prefetch waste accounting introduced by https://github.com/facebook/rocksdb/pull/14876. That change added reverse MultiScan support and introduced `unreleased_prefetch_blocks_` to track prefetched blocks still owned by the `ReadSet`, because reverse scans can prefetch a suffix of the collected block list and release blocks in non-prefix order. However, those unreleased prefetched blocks were only released during `MultiScanIndexIterator` destruction; they were not included in `MULTISCAN_PREFETCH_BLOCKS_WASTED`. As a result, scans that stopped before consuming all prefetched blocks underreported wasted prefetch blocks. This change counts unreleased prefetched blocks as wasted before destructor cleanup, while avoiding counting the currently positioned block as wasted when it has been reached. Reviewed By: xingbowang Differential Revision: D111531409 fbshipit-source-id: 5e605ce686037256bf6847ab7ca363c3f7701401 |
||
|
Hui Xiao
|
7c80a5ac85 |
Size-tolerant deserialization for compaction service response's counts array (#14807)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14807 The "counts" array in CompactionServiceResult (one compaction count per CompactionReason) is non-critical information. During cross-process remote compaction the primary and the remote worker can run different RocksDB versions and therefore disagree on kNumOfReasons, so the serialized counts array can have a different length than the reader expects. The plain OptionTypeInfo::Array parser rejects any element-count mismatch, which fails deserialization of the entire result over this non-critical field. This makes deserialization of "counts" size-tolerant via SizeTolerantDeserializeArray, which overrides only the parse callback: extra trailing elements are dropped and missing trailing elements are zero-filled, so a length mismatch no longer fails deserialization. Serialization and comparison are inherited from Array<T, kSize> unchanged. The serialized width is intentionally kept at kNumOfReasons - 1 here, so this change does not alter what is written -- only the read path becomes tolerant. Widening the serialized width to the full kNumOfReasons is done as a separate follow-up change, which must not land until this tolerant read has been deployed to every primary (including rollback targets), so that no reader still rejects a wider result. == Test plan == **Legend** CompactionReason is an enum whose last entry is kNumOfReasons (currently 21). CompactionStats::counts is an int[kNumOfReasons] holding one count per reason; it is serialized into the remote compaction result and read back by the primary. - "narrow" = kNumOfReasons - 1 counts (omits the newest reason); what current code writes. - "wide" = kNumOfReasons counts (includes the newest reason). - strict reader: accepts only its exact expected element count; any other count fails deserialization (the plain OptionTypeInfo::Array behavior). - tolerant reader: accepts any count -- more elements than expected are dropped, fewer are zero-filled; never fails on a count mismatch (this change, SizeTolerantDeserializeArray). **Unit test** CompactionServiceTest.CompatCheckCountsWidthTolerance -- verifies that deserializing "counts" tolerates a serialized element count equal to, one greater than, or one less than the reader's expected width (extras dropped, missing zero-filled), and that the binary serializes exactly the expected number of elements. **Manual cross-version verification (ldb remote_compaction)** Test 1: baseline (unchanged behavior) -- expect-narrow strict-read primary reads narrow counts written by a worker ``` $ strict_narrow remote_compaction_worker --db=$DB --job_dir=$JOB & $ strict_narrow remote_compaction_primary --db=$DB --job_dir=$JOB remote_compaction_worker: OK (4913 bytes result) remote_compaction_primary: OK $ grep 'remote compaction result' $DB/LOG [default] [JOB 3] Received remote compaction result, output path: /tmp/cs_run/output, files: 000022.sst ``` => PASS: the primary parsed and installed the remote result. Test 2: expect-narrow tolerant-read primary reads the narrow counts a worker writes ``` $ strict_narrow remote_compaction_worker --db=$DB --job_dir=$JOB & $ tolerant_narrow remote_compaction_primary --db=$DB --job_dir=$JOB remote_compaction_worker: OK (4912 bytes result) remote_compaction_primary: OK $ grep 'remote compaction result' $DB/LOG [default] [JOB 3] Received remote compaction result, output path: /tmp/cs_run/output, files: 000022.sst ``` => PASS: the primary parsed and installed the remote result. Test 3: expect-narrow tolerant-read primary reads the wide counts a worker writes (extra reason dropped) ``` $ tolerant_wide remote_compaction_worker --db=$DB --job_dir=$JOB & $ tolerant_narrow remote_compaction_primary --db=$DB --job_dir=$JOB remote_compaction_worker: OK (4915 bytes result) remote_compaction_primary: OK $ grep 'remote compaction result' $DB/LOG [default] [JOB 3] Received remote compaction result, output path: /tmp/cs_run/output, files: 000022.sst ``` => PASS: the primary parsed and installed the remote result. Test 4: expect-wide, tolerant primary reads the narrow counts a worker writes (missing reason zero-filled) ``` $ strict_narrow remote_compaction_worker --db=$DB --job_dir=$JOB & $ tolerant_wide remote_compaction_primary --db=$DB --job_dir=$JOB remote_compaction_worker: OK (4911 bytes result) remote_compaction_primary: OK $ grep 'remote compaction result' $DB/LOG [default] [JOB 3] Received remote compaction result, output path: /tmp/cs_run/output, files: 000022.sst ``` => PASS: the primary parsed and installed the remote result. Test 5: expect-wide, tolerant primary reads a wide counts a worker write ``` $ tolerant_wide remote_compaction_worker --db=$DB --job_dir=$JOB & $ tolerant_wide remote_compaction_primary --db=$DB --job_dir=$JOB remote_compaction_worker: OK (4917 bytes result) remote_compaction_primary: OK $ grep 'remote compaction result' $DB/LOG [default] [JOB 3] Received remote compaction result, output path: /tmp/cs_run/output, files: 000022.sst ``` => PASS: the primary parsed and installed the remote result. Reviewed By: jaykorean Differential Revision: D106321320 fbshipit-source-id: fc1bd66a4d11ac6bf38dff6aa214ebafab75f3d0 |
||
|
Josh Kang
|
053a9de468 |
Fix broken github codex review (#14923)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14923 Fix the GitHub Actions Codex review job by running generated review prompts through generic `codex exec` instead of the specialized `codex exec review` subcommand. The workflow already embeds PR metadata, reviewer instructions, and the diff in `/tmp/prompt.txt`, while `codex exec review --base` rejects a positional/stdin prompt. This removes the invalid `--base`/stdin combination from both automatic and manual Codex review paths while leaving `/codex-query` behavior unchanged. Reviewed By: xingbowang Differential Revision: D111135454 fbshipit-source-id: 16e86adda41d3e784c59466658cb769278c909c7 |
||
|
Jay Huh
|
f32f747125 |
Address reverse MultiScan follow-up comments (#14922)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14922 - Preserve source compatibility for `Scan::Reset` by restoring the default `reverse` argument. - Make `HasMoreScanRanges()` defensive for reverse prepared scans instead of asserting. - Restore broader forward coverage in `MultiScanAsyncIOTest` while keeping reverse coverage on the representative async IO matrix. - Let `--multiscan_reverse` drive reverse MultiScan stress coverage independently of `--test_backward_scan`. - Add the reverse MultiScan public API release note. Reviewed By: xingbowang Differential Revision: D111127403 fbshipit-source-id: 1a17d0a36aaf66e8cbc51f445d0f85e145fd653c |
||
generatedunixname1395027625275998
|
c3f6e4d625 |
Cache GetKeysEndOffset in BlockIter to speed up Next inner loop (#14917)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14917 Reviewed By: kunalspathak Differential Revision: D110209012 fbshipit-source-id: 8ce3b94ea9617977d956f3856d92d50f2b547ddc |
||
|
Jay Huh
|
e1ea04d9ef |
Add reverse order support to MultiScan (#14876)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14876 Add reverse scan support to `MultiScanArgs`/`MultiScan` so callers can iterate bounded scan ranges in descending order. Plumb reverse scan state through `DBIter`, `BlockBasedTableIterator`, and `MultiScanIndexIterator`, including reverse prefetch windowing and async I/O support. Update `db_bench` `multiscanrandom` to honor `--reverse_iterator` and report rows scanned so reverse MultiScan and iterator baselines can be compared on row throughput. Add `db_stress --multiscan_reverse` coverage so the prepared MultiScan path can be stressed with `SeekForPrev()`/`Prev()` against a control iterator. Also fix the table child iterator path to accept reverse `SeekForPrev()` targets above a file's prepared ranges. `MergingIterator` and `LevelIterator` can legally seek every child to the current range limit; a child file may only have lower prepared ranges, so treating that as invalid caused intermittent `SeekForPrev target is outside prepared ranges` exceptions in the benchmark. Reviewed By: xingbowang Differential Revision: D109357730 fbshipit-source-id: eccece5ca7d8f5d6760d51ebe76801a332a49061 |
||
|
anand1976
|
27b48646cd |
Update folly hash for 11.6 release (#14912)
Summary: - Update `folly.mk` FOLLY_COMMIT_HASH to 2a68075b77d9 This is folly's `main` at release-cut time. Review and merge to take the bump, or close to skip folly for this release. Part of 11.6 release workflow. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14912 Reviewed By: jaykorean Differential Revision: D110524765 Pulled By: pdillinger fbshipit-source-id: 3361fdc6277b5f02828f23942b9301237b1ed272 |
||
|
generatedunixname1395027625275998
|
66bccb467d |
Store BySmallestKey by value to avoid shared_ptr atomic copy in Rep (#14919)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14919 Reviewed By: DenisYaroshevskiy Differential Revision: D110842287 fbshipit-source-id: df84b4b319aefd9b21abdb7db20989ba1efd7eba |
||
|
generatedunixname1395027625275998
|
0f15da1c2d |
Reserve key_context capacity in MultiGetCommon to avoid autovector reallocs (#14909)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14909 Reviewed By: kunalspathak Differential Revision: D110380492 fbshipit-source-id: d1d3214c52a65326de237adcbfe6ea5d23d94960 |
||
|
anand1976
|
13d55eba73 |
Update version to 11.7.0 for 11.6 release (#14913)
Summary: - Bump version.h from 11.6.0 to 11.7.0 - Add `11.6.fb` to `check_format_compatible.sh` - Sync HISTORY.md `## 11.6.` section from `11.6.fb` - Delete 5 consumed `unreleased_history/` note file(s) from main Part of 11.6 release workflow. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14913 Reviewed By: anand1976 Differential Revision: D110520723 Pulled By: pdillinger fbshipit-source-id: 20f2f9b4be03c08d78d6f2c4e6b70812a408d9f0 |
||
|
Peter Dillinger
|
1192897aad |
Fix table cache entry leak in VersionBuilder::UnrefFile on failed LogAndApply (#14914)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14914 Pull Request resolved: https://github.com/facebook/rocksdb/pull/14720 ### Context `VersionSet::LogAndApply` loads table handlers before the `MANIFEST` update is durable. If that `MANIFEST` update later fails, newly added files can be discarded by `VersionBuilder` without ever being installed in a `Version`. `VersionBuilder::UnrefFile` released the `FileMetaData` `pinned_reader` handle, but releasing that handle only dropped the reference; it did not erase the cache key, so an orphaned table-cache entry could survive for a file that is neither live nor quarantined. When metadata read fault injection prevents the obsolete-file scan from cleaning up the orphan, DB close can trip `TEST_VerifyNoObsoleteFilesCached` with File N is not live nor quarantined. ### Fix After releasing a pinned_reader for a `FileMetaData` whose refcount reaches zero, explicitly evict that file number from the table cache. The new `VersionBuilderDBTest.FailedLogAndApplyEvictsTableCacheEntry` regression test exercises the realistic production path: it creates a real SST, calls `LogAndApply` through the DB's own `VersionSet`, injects a MANIFEST sync failure via the `AfterSyncManifest` sync point, and asserts that the table cache no longer contains the file after `ProcessManifestWrites` destroys the `VersionBuilder`. With the eviction removed, the test fails deterministically with `leaked_cache_entry=true`. Reviewed By: hx235 Differential Revision: D99759696 fbshipit-source-id: 0ebb32b1544ad1982d23a6c06569984be97a0a88 |