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
|
4c2a977aa5 |
Track newer snapshot versions in RocksDB metadata reads (#15086)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15086 Why: An application built on top of RocksDB could use cache population reads through explicit snapshots and needs to know when the returned value has already been superseded, without a second point read. Previous behavior: Get and MultiGet returned the snapshot-visible value and optional timestamp but could not report a newer point update or covering range deletion. New behavior: experimental C++, C, and Java metadata-read APIs report newer committed puts, deletes, merges, and covering range tombstones; ordinary primary-DB writes completed before the read are guaranteed to be reported, racing writes remain best-effort, and true has no false positives. Implementation: metadata reads widen lookup behind read-callback hooks while keeping metadata-only state off passive common object layouts; per-key state stays in KeyContext/GetContext, ordinary MultiGet row-cache replay semantics are preserved, BlobDB/TTL use shared output helpers, dead single-CF plumbing and an invalid cross-key stress assertion are removed, and the range-tombstone ordering invariant is documented without changing behavior. Safety and unsupported cases: tracking is opt-in and requires an explicit snapshot; persisted-tier reads, custom transactional visibility callbacks, secondary DBs, and unsupported subclasses return NotSupported, while read-only/compacted DBs return false because their in-process views are static. Tests and limitations: 1,265 Buck tests plus focused metadata, 100x coerced-context-switch row-cache, write-committed/write-prepared crash tests, builds, and lint passed. Current-parent release db_bench found no material passive regression: readrandom +0.65%, MultiGet batch 4 -1.40%, and batch 32 +0.48%. Reviewed By: pdillinger Differential Revision: D104182397 fbshipit-source-id: 36f7be1cd50fe2383689adaae31a6bb7d7e462dc |
||
Omkar Gawde
|
88a8fdf849 |
Compare excluded-file lookup against the engine it searched (#15197)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15197 **What**: Compare the excluded-file lookup iterator against the engine it was obtained from, in `BackupEngineImpl::RestoreDBFromBackup()`. **How**: The loop walks `locked_restore_from_dirs` calling `find()` on each engine's `backuped_file_infos_`, but tested the result against `this` engine's `end()`. Test against the same engine's `end()`. **Why**: Comparing iterators obtained from different containers is undefined. It is inert here rather than a live bug: on libstdc++ and libc++ an `unordered_map` end iterator is a null sentinel that is not container-specific, so a miss compares equal and a hit does not. On an implementation with a container-specific end iterator - MSVC, a required CI axis - a miss would read as found and dereference the sentinel node. No test can pin this; see the test plan. Reviewed By: xingbowang Differential Revision: D119585045 fbshipit-source-id: 9ff0e770b426f6a2c4a0fdad261910a927d1bfa7 |
||
Josh Kang
|
a844cbf5b7 |
Add coroutine reads for SstFileReader (#15189)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15189 Add `CoroDB::CoGet` and `CoMultiGet` overloads for standalone `SstFileReader` instances. Reuse a shared sync/coroutine implementation on the private reader `Rep`, configure and validate the read executor at `Open`, and retain synchronous fallback when unavailable. Reviewed By: xingbowang Differential Revision: D119431444 fbshipit-source-id: 1ac1926b6a2ffc11cd3e91a438a8c62cff5d57ec |
||
|
Omkar Gawde
|
3d8e4be2f5 |
Fix SstFileReader heap corruption past MAX_BATCH_SIZE keys (#15196)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15196 **What**: Fix heap corruption and out-of-bounds reads in `SstFileReader::MultiGet()` when the request exceeds `MultiGetContext::MAX_BATCH_SIZE` keys. **How**: - Batch the lookup in `MAX_BATCH_SIZE` slices over the sorted keys, with a fresh `MultiGetContext` and `TableReader::MultiGet()` call per slice. Caller ordering survives because each `KeyContext` already points into the caller's status and value vectors. - `reserve()` the `key_context`, `get_ctx` and `merge_ctx` autovectors, as `DBImpl::MultiGetCommon` does: the setup loop takes addresses of their elements, and past its inline capacity an `autovector` spills into a `std::vector` that reallocates as later keys are appended. - Only the slicing is borrowed from `DBImpl::MultiGetImpl`. Its cross-batch value-size accumulation and per-batch deadline check are deliberately not replicated: value size is accumulated only under `db/` and never by a `TableReader`, and the deadline already reaches every read through the table reader's I/O options. **Why**: `MultiGetContext` caps at `MAX_BATCH_SIZE` keys but only asserts it, so release builds hit a bad `delete[]` from its mismatched `LookupKey` construct/destroy bounds, an out-of-bounds read of its fixed-size `sorted_keys_` array, and an out-of-range shift once the key count exceeds the width of its range mask; `include/rocksdb/sst_file_reader.h` documents no key-count limit, so any caller of this public API could reach it. Reviewed By: xingbowang Differential Revision: D119401930 fbshipit-source-id: 3bbafd72badaa944dbb94f4ab6ecb7f14a11f6d0 |
||
|
Xingbo Wang
|
7d3150ffce |
Report RocksDB benchmark interval latency and perf metrics (#15101)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15101 This diff contains all changes under fbcode/internal_repo_rocksdb/repo for the RocksDB regression benchmark observability work. It adds opt-in per-interval latency percentile reporting to db_bench. When --report_interval_percentiles is enabled with histogram and a positive stats interval, each worker records into pre-created per-operation interval histograms and counters. A background reporter periodically merges and resets those thread-owned stats, skips excluded background-writer stats, performs a final partial-interval flush, and emits IntervalPercentiles lines with sample count, operation count, and P50/P75/P99/P99.9/P99.99 latency. Worker updates and reporter snapshots use a double-buffer handoff, so the reporter never reads or clears a histogram still being updated. The legacy histogram timing path remains unchanged when interval reporting is disabled. The interval implementation uses RocksDB UnorderedMap aliases for the hot lookup maps and UnownedPtr for non-owning reporter references. It also aggregates each worker thread PerfContext and IOStatsContext into the final merged benchmark Stats and emits final PERF_CONTEXT and IOSTATS_CONTEXT blocks, so multi-thread benchmark metrics represent all workers rather than one thread. For options-file runs, explicit benchmark-critical CLI overrides are applied after loading the file. This includes --disable_auto_compactions, fixing an existing precedence bug where disable_auto_compactions=false in the options file silently overrode an explicit CLI true value. The regression harness relies on that CLI setting, so the bug allowed automatic compactions during origin construction and write-heavy benchmarks. The regression test persists false in an options file, passes --disable_auto_compactions=true, reloads the opened DB options, and verifies the effective value is true. One shared WriteBufferManager is created after option loading and reused by all DBs, enforcing the intended aggregate memory cap for multi-DB benchmarks. The compactall benchmark now accepts --compact_all_parallelism. It compacts independent DBs through a bounded worker set, applies --subcompactions to each DB, emits per-DB progress and elapsed time, and propagates the first CompactRange failure after joining all workers. Its default parallelism remains 1 for backward compatibility. The dependent harness diff enables these features, parses their output, and publishes the resulting interval and final metrics. Reviewed By: pdillinger Differential Revision: D116025321 fbshipit-source-id: d44185ba2110aacbe9fadc65a59799bf8c1c54f0 |
||
Jay Huh
|
91b96f41cf |
Retry the OpenAndCompact secondary open when the primary replaces CURRENT (#15179)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15179 A remote compaction worker (`DB::OpenAndCompact()`) opens the *live* primary's DB directory read-only, and that open reads `CURRENT`. The primary publishes each MANIFEST rotation by writing a new `CURRENT` under a temp name and renaming it over the old `CURRENT`. POSIX keeps an already-opened `CURRENT` readable after that replacement, but file systems without read-after-unlink semantics (remote or object stores) do not: the replaced object is reported as gone, surfacing in RocksDB as `IO error: No such file or directory` (`Status::PathNotFound`). The secondary open was not retried, so this transient race failed the whole compaction. There is an equivalent, already-recognized race one step later for the MANIFEST file (`ReactiveVersionSet::MaybeSwitchManifest()` converts it into `Status::TryAgain`), which was fatal here for the same reason. `DB::OpenAndCompact()` now retries the secondary open (up to 3 attempts) when it fails with `TryAgain` / `PathNotFound` / `NotFound` -- exactly the statuses a concurrent `CURRENT`/MANIFEST replacement produces. The retry limit is configurable via `OpenAndCompactOptions::max_secondary_open_retries`. Genuine failures are still returned after the bounded retries, and no safety check is weakened or skipped. Found by a `db_stress` crash test failure. Two non-default options in that run make this reachable and fatal: * `remote_compaction_worker_threads=8` with `remote_compaction_failure_fall_back_to_local=0` -- the latter was only recently randomized to 0; before that a remote compaction failure was always silently retried locally. Now `DbStressCompactionService::Wait()` maps the failure to `kFailure`, which becomes a sticky background error on the primary, so every subsequent `Flush()` returns it and `StressTest::ProcessStatus()` aborts with `assert(false)`. * `max_manifest_file_size=8192` -- the primary rotates MANIFEST (and therefore replaces `CURRENT`) constantly, which is what makes the race window hit in practice. Full chain: primary renames a new `CURRENT` over the old one -> the file system reports the replaced object as gone for the reader that already resolved it -> `GetCurrentManifestPath()` inside the worker's secondary open fails with `IO error: No such file or directory` -> `DB::OpenAndCompact()` returns that error with no retry -> `kFailure` (no local fallback) -> primary background error -> `Flush failed: IO error: No such file or directory: ... /CURRENT ...` -> assertion failure in `db_stress`. Reviewed By: joshkang97 Differential Revision: D118480192 fbshipit-source-id: a2618ee24e582e2e713af04464ee1e4038925ccb |
||
|
Omkar Gawde
|
dfde6a60b0 |
Fix out-of-bounds read in SstFileWriter::DeleteRange
Summary: **What**: Fix an out-of-bounds read and a forged end timestamp in the timestamped `SstFileWriter::DeleteRange()` overload. **How**: - `Rep::DeleteRange()` now builds each boundary through a new `Rep::AppendTimestamp()` helper, which tests one key's adjacency to `timestamp` and either aliases the two or materializes `key + timestamp` into a caller-owned buffer. - Drops the fast path's `assert`. It was never a guard: it performed the same out-of-bounds read it was meant to police, and `DeleteRangeImpl()` already asserts the two boundary timestamps match. - Behavior change for callers whose two keys each carry their own trailing timestamp: they now take one extra copy. Aliasing is still used for whichever key is genuinely adjacent. **Why**: A single `timestamp` buffer can be byte-adjacent to at most one of two distinct keys, but the old branch tested `end_key` using `begin_key`'s length and then widened both slices whenever either test passed - so an asymmetric caller, one key contiguous with the timestamp and the other bare, read past an allocation and persisted a range tombstone carrying foreign heap bytes as its end timestamp. Nothing forbids that layout: the public contract requires only that the keys carry no timestamp. Reviewed By: xingbowang Differential Revision: D119401932 fbshipit-source-id: 4c842d4afbe7bd7c4eabc68c6aee0543cdee2c9c |
||
Peter Dillinger
|
bcda8c4b1a |
Fix Cache::ApplyToAllEntries ignoring average_entries_per_lock (#15186)
Summary: `ShardedCache::ApplyToAllEntries()` clamped `average_entries_per_lock` with `std::min(aepl, 1)`, forcing every positive value (including the default 256) down to 1. The intent was a floor of 1 (`std::max`); `ApplyToSomeEntries` even asserts the value is > 0. As a result a full cache traversal advanced a single hash-table slot per iteration step instead of batching ~256, so the documented 256 default never actually took effect. The bug dates to the original API (https://github.com/facebook/rocksdb/issues/8225) and was carried through the ShardedCache refactor (https://github.com/facebook/rocksdb/issues/10801) unchanged, so the intended default was never exercised: in hindsight the "gather stats" latency in the https://github.com/facebook/rocksdb/issues/8225 test plan is a batch-size-1 measurement. Despite its name, `average_entries_per_lock` is really a batch size for iterating the cache, not something specific to locking: it bounds the work done per uninterrupted step. For a lock-based cache (LRUCache) a shard mutex is held for each batch, so single-entry batches mean many extra lock acquire/release cycles; but lock-free implementations (HyperClockCache) also pay fixed per-batch overhead and benefit from the fix as well. This is a control-path cost (e.g. periodic block cache stats via DumpStats -> CollectCacheEntryStats), not hot-path read/write, so the practical impact is usually minor. The option's doc comment is updated to describe it as a batch size; the field is not renamed given how obscure the option is. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15186 Test Plan: No unit test: asserting on batch/lock-acquisition counts would be intrusive/awkward, and LRUCache is disfavored (though not deprecated). Measured with release-mode cache_bench, comparing the binary built before (`std::min`) vs after (`std::max`); 4 GiB cache, 1 KiB values, ~3.7M entries, default gather_stats_entries_per_lock=256. Average full ApplyToAllEntries traversal latency: | cache_type | before | after | speedup | |-------------------------|---------|---------|---------| | lru_cache | 2.69 s | 0.64 s | ~4.2x | | fixed_hyper_clock_cache | 0.441 s | 0.138 s | ~3.2x | | auto_hyper_clock_cache | 0.264 s | 0.136 s | ~1.9x | All implementations improve; LRUCache most, due to per-batch mutex acquire/release. HCC traversals are already ~6-10x cheaper than LRU in absolute terms, and entries visited are identical before and after. Reviewed By: rban1 Differential Revision: D119256456 Pulled By: pdillinger fbshipit-source-id: e58f7f97d47d8970ebc8c3ab786199132ce6340d |
||
|
Josh Kang
|
37234200b5 |
Handle benign io_uring stderr in crash tests (#15181)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15181 A crash test failed when `use_async_db_api=1` created read-executor workers on a host unable to allocate io_uring rings. Folly logged the setup failure and threw; RocksDB caught it and successfully fell back to synchronous reads, but the harness failed because stderr was non-empty. Representative stderr: ``` WARNING: Logging before InitGoogleLogging() is written to STDERR I0903 14:05:39.470695 286445 IoUringBackend.cpp:527] io_uring_queue_init_params(512,1024) failed errno = 0:"Success" 0x7d2e06295280 retrying with capacity = 512 E0903 14:05:39.470759 286445 IoUringBackend.cpp:537] io_uring_queue_init_params(512,512) failed ret = -12:"Unknown error -12" 0x7d2e06295280 ``` Initialize glog in the Folly-enabled OSS `db_stress` entry point and centralize known stderr sanitization. Only `IoUringBackend` ring-initialization failures are moved to stdout when the async DB API is enabled; existing expected post-SIGTERM errors use the same sanitizer. All unrelated stderr remains fatal. ___ Differential Revision: D118836521 fbshipit-source-id: c9ce05896f417c7723c5cd73d293fbb1cd14ee1e |
||
|
Josh Kang
|
ac0f4ae69f |
Isolate coroutine statistics across async reads (#15142)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15142 Coroutine statistics use TLS, but coroutine reads can interleave on the same executor thread. Previously, when a stats-enabled request suspended, it saved its counters without disabling the executor's TLS configuration. A stats-disabled request running next could inherit those enabled settings and collect statistics unexpectedly. Stats setup also lived in individual DB implementations, so wrapper early returns and future stackable DB implementations could bypass it. Move statistics ownership to the public `CoroDB` and callback-based async read boundaries, before virtual dispatch. Each operation captures its caller's configuration, enabled requests preserve their counters across suspensions, and every suspension or completion leaves executor TLS disabled. Because the scope wraps stackable DB dispatch, current and future `CoroStackableDB` implementations automatically inherit the correct behavior without adding their own stats reset or scope. For consistency between coroutine and callback-based reads, the experimental callback API now uses TLS for both configuring and consuming statistics. `AsyncCallback::OnComplete()` no longer receives context arguments; RocksDB publishes the completed per-operation counters to TLS before invoking it. ``` Before time --------------------------------------------------------------> stats-on A: onSet(enable A) -- work -- onUnset(save A) ...... onSet(A) executor TLS: [A enabled] ------------> [still enabled] ------> [A enabled] stats-off B: run ^ inherits A's enabled TLS After time --------------------------------------------------------------> stats-on A: onSet(enable A) -- work -- onUnset(save A, disable) ... onSet(A) executor TLS: [A enabled] ------------> [disabled] ----------------> [A enabled] stats-off B: run ^ remains stats-disabled ``` Reviewed By: xingbowang Differential Revision: D117017089 fbshipit-source-id: 8a85f7ba43e3b73225995f5d0b79da3892baafb9 |
||
|
Jay Huh
|
bec9dcec7c |
Make db_stress actually tolerate transient remote IO errors (#15170)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15170 `warm_storage_tsan_liveness_crash_test` fails with ``` error : Get() returns IO error: Failed to read 1032 bytes from file ws://ws.zdb.ldc0zdbtest1/.../000093.sst from offset 5461, message [0 code=DEADLINE_EXCEEDED isRetryable=1 ... error=WSE_BLOCK_READ_TIMEOUT classification=TRANSIENT_ERROR ...] for key: 0000...78 (30280). Verification failed :( ``` The Warm Storage error is explicitly `classification=TRANSIENT_ERROR isRetryable=1`, and the DB artifact confirms there is no RocksDB problem: `000093.sst` is intact (37760 bytes, downloaded successfully after the run) and the DB `LOG` reaches `Shutdown complete` with zero IO-error or corruption entries. This is exactly the class of failure `--tolerate_non_injected_io_errors_for_remote_dbs` was added for (D112136438), but that mechanism never actually took effect. Two independent defects: **1. The triggering non-default option: `tolerate_non_injected_io_errors_for_remote_dbs=0` on a remote DB.** `db_crashtest.py` hard-coded the parameter to `0` in `default_params` and only ever *force-disabled* it (for local DBs), leaving each remote job to opt in on its own command line. `rocks/sandcastle/test-driver.sh`'s `warm_storage_*crash*` branch never passed it, so every Warm Storage crash-test job ran with the flag off and a single transient read timeout failed the run. Fix: the parameter now defaults to `1` whenever `--env_uri` / `--fs_uri` names a remote backend, so no remote job has to remember to opt in. A caller can still pass `0` explicitly, and `finalize_and_sanitize()` still force-disables it for local DBs so genuine local IO errors (e.g. io_uring failures) are never masked. **2. `injected_error_count == 0` made the flag a no-op in `no_batched_ops_stress.cc`.** All five read-path guards were `injected_error_count == 0 || !IsErrorInjectedAndRetryable(s)`. `IsErrorInjectedAndRetryable()` covers *non-injected* remote IO errors, which by definition have no injected fault to correlate with — and the `liveness` profile forces every `*_fault_one_in` to `0`, pinning `injected_error_count` to `0` for the whole run. So the left operand short-circuited to true and the error was reported as a verification failure regardless of the flag. Fix: extract `StressTest::IsTolerableNonInjectedRemoteIOError()` and add `StressTest::IsRetryableOperationError(injected_error_count, s)`, which gates the injected-error branch on `injected_error_count > 0` but lets a tolerated non-injected remote IO error through unconditionally. The five call sites now use the helper. With the tolerate flag off (all local runs), the new guard reduces algebraically to the old expression, so local behaviour is unchanged. The genuine consistency-check gate at `no_batched_ops_stress.cc:1146` (`do_consistency_check && injected_error_count == 0`) is intentionally left alone. Fixes T286575055. [Session trajectory link](https://www.internalfb.com/intern/devai/devmate/inspector/?id=9646c050-6791-4ac3-9abb-3ee110dcb54e) Reviewed By: pdillinger Differential Revision: D117886823 fbshipit-source-id: d7591472db8ee6e03c9d9f018480409b6d802838 |
||
|
Jay Huh
|
897162a6bc |
Add session-scoped temporary directory for remote compaction (#15169)
Summary: Remote compaction writes output SSTs into caller-selected job directories. If the primary crashes before installation or falls back to local compaction, those directories can outlive the DB session and accumulate under the DB directory. This change defines `kSessionTmpDirName` as `session_tmp`, the canonical name for a DB-local temporary directory whose contents have the lifetime of a DB session. The immutable boolean option `use_session_tmp_dir_for_remote_compaction` opts remote compaction into this directory. When enabled, an `OpenAndCompact()` directory name is resolved as `<dbname>/session_tmp/<job>`. During DB open, RocksDB creates the session temporary directory, completes recovery, and then synchronously deletes contents left by the previous DB session before scheduling compactions. `DestroyDB()` removes it as part of database deletion, and normal completion, failure, and cancellation paths clean the resolved per-job directory. The option defaults to false. Disabled callers retain the existing `<dbname>/<job>` layout and no session-directory cleanup. For rollout compatibility, legacy callers that already pass a DB-rooted output path are accepted unchanged only while the option is false. Directory creation and invalid-path errors are delegated to the configured filesystem rather than prevalidated by RocksDB. After D118301531 landed, per-job directory creation and resumption cleanup are kept together in `InitializeCompactionWorkspace()`. `OpenAndCompact()` resolves the client-provided name and expects DB open to have initialized the session temporary directory when the feature is enabled. The boolean is wired through C++ and generated C APIs, option serialization and reconstruction, `OpenAndCompact()`, ldb, db_bench, and db_stress. Tests cover path resolution, cross-session deletion, ownership boundaries, disabled behavior, `DestroyDB()`, configured compaction output, resumption fallback, and option round trips. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15169 Reviewed By: pdillinger Differential Revision: D118227112 fbshipit-source-id: 01aa834b0b3f75638102130a86d95d537a2797bd |
||
rocksdb-release-bot
|
1ca38e1442 |
Update version to 11.11.0 for 11.10 release (#15157)
Summary: - Bump version.h from 11.10.0 to 11.11.0 - Add `11.10.fb` to `check_format_compatible.sh` - Sync HISTORY.md `## 11.10.` section from `11.10.fb` - Delete 13 consumed `unreleased_history/` note file(s) from main Part of 11.10 release workflow. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15157 Reviewed By: jaykorean Differential Revision: D118302742 Pulled By: xingbowang fbshipit-source-id: 751fb106cc5a4f5dd502c5838c5653278b5a8529 |
||
|
Josh Kang
|
da3a0961fa |
Add custom read context to ReadOptions (#15173)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15173 Add an opaque, non-owning `ReadOptions::custom_context` pointer that is passed to table implementations so external table readers can consume application-defined read options. Document its lifetime and synchronization contract, and keep the unsupported pointer out of generated C bindings. Reviewed By: xingbowang Differential Revision: D118341098 fbshipit-source-id: e51185f58b5ce80d8fd07efab02a05dbc8c7bd12 |
||
|
Josh Kang
|
96ed085402 |
Blog post for coroutines (#15143)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15143 ## Summary Adds a RocksDB blog post describing the native async/coroutine read APIs introduced in RocksDB 11.10.0. It covers the callback and `folly::coro::Task` interfaces, differences from `ReadOptions::async_io`, executor and I/O-completion handoffs, filesystem integration, request-local statistics across suspension, and API lifetime constraints. The post includes three explanatory diagrams and benchmark results comparing synchronous and coroutine point reads across task counts and `MultiGet` batch sizes. https://www.internalfb.com/intern/px/p/cDGqv/ Reviewed By: xingbowang Differential Revision: D117159178 fbshipit-source-id: 5452e6b07e29abb5594505021995e9b5bac9d198 |
||
|
Peter Dillinger
|
2c1c7ea2f1 |
Surface remote cx crash test failures and fix resumption output-dir cleanup (#15168)
Summary: Follow-up to the "UPDATE" note on D116738823 (PR https://github.com/facebook/rocksdb/issues/15133, "Faster and safer MANIFEST recovery for remote compaction (OpenAndCompact)"). That change observed that the crash test does not really cover remote compaction because `remote_compaction_failure_fall_back_to_local` is pinned to 1 in db_crashtest.py, so any remote compaction failure is silently retried locally instead of failing the test -- masking existing failures. Addressing that was called out as a high-priority follow-up (tracked under T232588967, "Rocksdb Crash Test failed: Remote Compaction"). This does the cited enablement and fixes the first masked failure it surfaces: * Crash test: randomize `remote_compaction_failure_fall_back_to_local` between 0 and 1 (the TODO(jaykorean)), so remote compaction failures actually surface instead of always being hidden by a local fallback. * Fix a real OpenAndCompact bug the enablement immediately exposed. When a compaction is scheduled with `allow_resumption=true` but resumption is internally disabled because it is incompatible with output-hash verification (`paranoid_file_checks=true`, or `verify_output_flags` requesting iteration verification), the worker started a fresh compaction but did NOT clean the output directory. The caller, having requested resumption, does not empty the directory either, so a compaction that was canceled after writing some output and then retried reused the same output file numbers and collided with the leftover files -- surfacing under the crash test's fault-injection FS as `NewWritableFile violates no-reopen-for-write contract`, which became a background error and failed subsequent writes. The fix honors the documented `OpenAndCompactOptions::allow_resumption=true` fallback contract: when resumption is requested but cannot be fulfilled, clean any leftover progress and output files before starting fresh (reusing the existing Scan/Cleanup helpers), so the fresh compaction always begins from a clean directory. When `allow_resumption=false`, the caller still owns emptying the directory (unchanged). No known production exposure: this bug can only bite a caller that requests `allow_resumption=true` on a column family where output-hash verification is enabled (`paranoid_file_checks=true`, or `verify_output_flags` including `kVerifyIteration`), because only that combination disables resumption while the (uncleaned) output directory is left to the caller. Auditing how the option is used across internal deployments found nothing in that state: the deployments that enable resumption for remote compaction do not enable those iteration/paranoid verification modes, and where remote-compaction output verification is enabled at all it is block-checksum-only, which keeps resumption active and never hits the buggy path. So this is effectively a latent-bug fix -- it prevents a failure that would appear if iteration/paranoid output verification were later turned on for a resumption-enabled remote-compaction fleet. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15168 Test Plan: Added compaction_service_test.cc unit test `CleansOutputDirWhenResumptionDisabled`, which drives OpenAndCompact with `allow_resumption=true` and `paranoid_file_checks=true` after seeding a leftover output SST, and asserts the leftover is cleaned and the compaction succeeds. It was confirmed to FAIL without the fix (leftover retained) and pass with it. Local crash test runs (blackbox, `remote_compaction_worker_threads=8`, `remote_compaction_failure_fall_back_to_local=0`): - Without the fix, the collision-trigger config (`allow_resumption_one_in=1 --paranoid_file_checks=1`) fails within the first ~60s iteration on the no-reopen-for-write contract violation. - With the fix, that same config ran ~10 min clean, and a full-random config (everything else randomized) ran ~15 min clean, both with 10-15 kill/reopen cycles plus a final crash-recovery verification. More local crash test runs are planned to shake out any remaining, rarer masked failures now that the fallback no longer hides them. Reviewed By: jaykorean Differential Revision: D118301531 Pulled By: pdillinger fbshipit-source-id: ceb9683ae06ce930ea78bafa3585ad616012243f |
||
Anand Ananthabhotla
|
fe6406f7b2 |
Skip recovery SetNextFile for OPTIONS-only advances (#15163)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15163 Reintroduces the `optimize_manifest_for_recovery` optimization for `OPTIONS-*` and temp files without letting `VersionSet::NewFileNumber()` collide with files already present in the DB directory. Recovery now distinguishes between files that require a persisted `SetNextFile` edit and files that only need the in-memory allocator reserved. If only `OPTIONS-*` or temp files are above the recovered `next_file_number_`, recovery advances `next_file_number_` in memory and still skips the MANIFEST edit. Reviewed By: jaykorean Differential Revision: D117887276 fbshipit-source-id: f211fa90303904befdf331242ba0d97553f84169 |
||
|
Anand Ananthabhotla
|
0df7ed563b |
Pass SST block CRC32C checksums to file writer (#15155)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15155 SST block writing now hands checksum coverage across the table-builder to file-writer boundary when the block checksum algorithm is CRC32C. The block builder passes the raw CRC32C of the block contents to `WritableFileWriter::Append()`, which validates the copied destination-buffer contents and accumulates that checksum into the filesystem handoff checksum instead of recomputing it after the copy. The trailer is still appended separately; table building computes and passes the trailer handoff checksum independently, while preserving the existing persisted block-checksum encoding. Non-CRC32C block checksum algorithms continue to use the existing file-writer checksum path. Destination-buffer validation failures return a stage-specific corruption `Status` that includes the file path. Flush and compaction propagate that status through their existing background error handling, so the same message is logged without adding local duplicate log sites. The SST tests record the builder append stream through sync-point callbacks while writing to a null sink, then replay those exact append calls into a file-backed writer to cover successful handoff, block-content corruption, compression-type corruption, trailer-checksum corruption, non-CRC32C fallback behavior, and a small randomized boundary-injection loop. Reviewed By: xingbowang Differential Revision: D116969191 fbshipit-source-id: f75dcb37936106b7f6f14cf1ed999c81a0879d7b |
||
|
Xingbo Wang
|
068aeda9d8 |
Account partitioned filter reads in PerfContext timing (#15164)
Summary: Instrument partitioned Bloom-filter block retrievals with read_filter_block_nanos so PerfContext includes time spent looking up these blocks in the block cache or reading them from SST files. Root cause: The top-level filter-partition index is read through ReadFilterBlock, which already owns the timing guard. Individual filter partitions bypass that helper and call RetrieveBlock directly. Once the top-level index is cached, workloads can therefore report filter block reads and cache hits while read_filter_block_nanos remains zero. Implementation: Place the timer at the partition retrieval boundary, after the pinned filter-map fast path and immediately around RetrieveBlock. This keeps the metric aligned with actual cache or SST retrieval work without charging accesses to already-pinned partition objects. Testing: Extend the partitioned MultiGet test to enable time-level PerfContext collection and verify that partition reads populate read_filter_block_nanos across whole-key and prefix-filter configurations and table format versions. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15164 Test Plan: - `make check-sources` - `build_tools/rocksptest.sh db_bloom_filter_test` - `ASSERT_STATUS_CHECKED=1 build_tools/rockstest.sh db_bloom_filter_test --gtest_filter=*PartitionedMultiGet*` Reviewed By: rban1 Differential Revision: D118052826 Pulled By: xingbowang fbshipit-source-id: e4467b107ba13e5cfe4826a8a93834b319a131a5 |
||
|
Xingbo Wang
|
a0966fd94e |
Validate remote compaction snapshot ordering (#15160)
Summary: Remote compaction passes snapshot sequence numbers through serialized `CompactionServiceInput`. Downstream compaction logic assumes the sequence numbers are strictly increasing when selecting the earliest snapshot and using `std::lower_bound()` for snapshot visibility. Previously, malformed input could bypass the debug-only ordering assertion in release builds and select an incorrect visibility boundary. - Validate decoded snapshot sequence numbers before remote compaction setup and return `InvalidArgument` for duplicate or descending values. - Document the strict ordering contract while retaining the `CompactionIterator` assertion as defense in depth for internally constructed inputs. - Canonicalize randomized serialization-test snapshots and add coverage for malformed snapshot lists through `DB::OpenAndCompact()`. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15160 Test Plan: - Built `compaction_job_test` and ran `CompactionJobTest.OpenAndCompactRejectsUnorderedSnapshots` in the normal configuration. - Ran the focused test with `ASSERT_STATUS_CHECKED=1`. - Ran the focused test for 100 repetitions. Reviewed By: rban1 Differential Revision: D117888826 Pulled By: xingbowang fbshipit-source-id: 2312205acfe17271c2083808d372c93a5914114c |
||
Ranjan Banerjee
|
fe5123681e |
Fix GetEntity snapshot isolation for WritePrepared transactions (#15136)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15136 `WritePreparedTxnDB` overrides `Get` and `MultiGet` to install a `WritePreparedTxnReadCallback` that performs visibility checking for prepared-but-uncommitted data. However, `GetEntity` (both the single-CF `PinnableWideColumns` variant and the multi-CF `PinnableAttributeGroups` variant) was never overridden, causing it to fall through to `StackableDB::GetEntity` which delegates to the inner `DBImpl` without any WritePrepared visibility callback. This means `GetEntity` could read prepared-but-uncommitted data, violating snapshot isolation. The bug surfaces in the crash test with the combination: `--use_txn=1 --txn_write_policy=1 --use_attribute_group=1 --use_get_entity=1 --unordered_write=1 --two_write_queues=1 --test_batches_snapshots=1` The fix adds `GetEntity` overrides to `WritePreparedTxnDB` that create a `WritePreparedTxnReadCallback` and pass it via `GetImplOptions::callback`, matching the pattern used by the existing `Get` override. Reviewed By: xingbowang Differential Revision: D116324596 fbshipit-source-id: faf636cfbf093f47a645c1a5d5037baa02ff594a |
||
zaidoon
|
f9c117e203 |
Add optional UDI builder protocols (#14960)
Summary: Part 3 of 13 in the UDI split. Stack order: 1. https://github.com/facebook/rocksdb/issues/14954 Add IndexFactory compatibility names 2. https://github.com/facebook/rocksdb/issues/14959 Add UDI index mode API vocabulary 3. https://github.com/facebook/rocksdb/issues/14960 Add optional UDI builder protocols 4. https://github.com/facebook/rocksdb/issues/14961 Add built-in index factory wrappers 5. https://github.com/facebook/rocksdb/issues/14969 Add built-in index factory tests 6. https://github.com/facebook/rocksdb/issues/14962 Promote IndexFactory as UDI SPI 7. https://github.com/facebook/rocksdb/issues/14963 Wire IndexFactory through block-based tables 8. https://github.com/facebook/rocksdb/issues/14964 Add IndexFactory table routing tests 9. https://github.com/facebook/rocksdb/issues/14970 Add IndexFactory parallel routing tests 10. https://github.com/facebook/rocksdb/issues/14965 Account UDI blocks as index blocks 11. https://github.com/facebook/rocksdb/issues/14966 Support parallel AddIndexEntry in trie index 12. https://github.com/facebook/rocksdb/issues/14967 Cover trie index modes 13. https://github.com/facebook/rocksdb/issues/14968 Add IndexFactory stress and benchmark flags Previous: https://github.com/facebook/rocksdb/issues/14959. Next: https://github.com/facebook/rocksdb/issues/14961. Depends on https://github.com/facebook/rocksdb/issues/14959. Until https://github.com/facebook/rocksdb/issues/14954 and https://github.com/facebook/rocksdb/issues/14959 land, GitHub shows the cumulative diff against main. After those land, the intended review diff is: `zaidoon/udi-02-index-mode-api..zaidoon/udi-03-index-builder-protocols` That final review delta is 1 file with 62 insertions. What changed: - Add optional UserDefinedIndexBuilder protocols with default no-op behavior. - Keep the API additive. Runtime wiring comes in later PRs. Validation: - `git diff --check` - `make check-sources` Pull Request resolved: https://github.com/facebook/rocksdb/pull/14960 Reviewed By: jaykorean Differential Revision: D117536541 Pulled By: xingbowang fbshipit-source-id: affe3e49e7185af6cfdf85314bb6f2e002c84c08 |
||
xingbowang
|
82dfd6a510 |
Fix Folly build due to libaio dead download link (#15159)
Summary: Failures in GitHub Actions CI workflows attempting to download assets from releases.pagure.org occur because the Pagure platform has been decommissioned. Maintainers must update their configurations to retrieve packages from active mirrors, alternative repositories. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15159 Reviewed By: rban1 Differential Revision: D117863179 Pulled By: xingbowang fbshipit-source-id: e54e80aa6f1e2e0503dbd7ec16e67b86c4bad7ad |
||
|
Anand Ananthabhotla
|
80a5a6e4b2 |
Revert PR 15106 (Ignore OPTIONS files for manifest recovery advance) (#15158)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15158 Reverts `https://github.com/facebook/rocksdb/pull/15106`, which ignored `OPTIONS-*` and temp files when deciding whether recovery must advance `next_file_number_`. Crash-test failures show backup/checkpoint failing to copy the current `OPTIONS-*` file after options cleanup, because `DeleteObsoleteOptionsFiles()` ignores `next_file_number_` and `options_file_number_` when choosing which files to keep. The failing commands use `optimize_manifest_for_recovery=1`, so restore the previous conservative file-number reservation behavior while we investigate a narrower fix. Reviewed By: xingbowang Differential Revision: D117807052 fbshipit-source-id: 89f8656f045be4ac36d2ec851980573ed83370fe |
||
|
Omkar Gawde
|
d699ffbd5c |
Support partial atomic range replacement (#15154)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15154 **What**: Let `IngestExternalFileArg::atomic_replace_range` replace a half-open `[start, limit)` range that only partially overlaps existing SSTs, by committing a generated range tombstone alongside the replacement files. **How**: - Tombstone: `NumFilesToPrepare` reserves one extra file number and `PrepareAtomicReplaceRangeTombstone` writes the SST only when the pinned `Version` holds a straddling file (`HasPartialOverlap`), so the no-straddle case pays no extra write; run rescans and returns `TryAgain` if a straddler appeared since. On activation it is rotated to the front of `files_to_ingest_` so one `VersionEdit` lands `old < tombstone < data` seqnos, straddlers are retained, contained files metadata-deleted, and `generated_for_ingestion` keeps `move_files` from unlinking it. - Half-open plumbing: `RangesOverlapWithMemtables`, `RangeOverlapWithCompaction`, `OverlapProximalLevelOutputRange`, and `IsRangeOverlapped` take a required limit-exclusivity argument, and `largest_internal_key` now comes from `RangeTombstone::SerializeEndKey()`. `OverlapWithLevelIterator` stays inclusive: over-counting can only push the tombstone one level shallower, which is safe. - Eligibility: `SupportsAtomicReplaceRangeTombstone` requires universal compaction, a delete-range-capable table factory, `allow_global_seqno`, `!allow_db_generated_files`, `!snapshot_consistency`, and no UDT. `fail_if_not_bottommost_level=true` rejects partial overlap because the retained file forces the tombstone and replacements above Lmax. The `atomic_replace_range` doc comment and a new `unreleased_history/public_api_changes/` note state the half-open contract. - `ResetTableReader` passes `kNoFileChecksumFuncName` in `FileOptions` for external files, making "no checksum metadata" explicit rather than empty. - Stress: new `ingest_external_file_atomic_replace_one_in` gflag. `PickAtomicReplaceRange` picks a run of overwriteable keys strictly inside an existing SST so every replacement straddles, writes even-indexed keys and marks the interleaved odd-indexed keys deleted so the tombstone must clear the holes, and rolls back expected values on `TryAgain`. `db_stress_tool.cc` and `db_crashtest.py` gate the flag on the same eligibility set and force `acquire_snapshot_one_in=0`. **Why**: Atomic replacement previously required range bounds to align with SST bounds, and the public doc admitted the upper bound "may be interpreted as inclusive or exclusive". The tombstone overlay removes the alignment restriction and pins the contract to half-open without slowing the Nimble path or the no-straddle case. Reviewed By: pdillinger Differential Revision: D117155307 fbshipit-source-id: 65f45463e8082a7db7a9d44c8dc50debe95e36e7 |
||
|
Peter Dillinger
|
e4a7136d3e |
Preferred-phase scheduling for periodic (time-based) compaction
Summary:
`periodic_compaction_seconds` softly guarantees every SST is rewritten at least
once per interval. Across a fleet of similarly-configured DBs, many events line
those intervals up in lockstep -- a shared enable, a coordinated restart (e.g.
DC power-outage recovery), or just similar write history -- so the periodic
compactions all come due together, a recurring "thundering herd" that is
especially painful with remote compaction (SEV S698635).
The obvious fixes trade one problem for another. Spreading the herd out means
some files get compacted off their natural schedule, which is wasted work; and a
clumsy spread just relocates the herd or spawns a new one. So the real goal is
narrow: **de-herd while keeping "badness" -- extra and/or late compaction --
small, and without inducing fresh herding at any step.**
This change adds an EXPERIMENTAL, opt-in mechanism that gives each `(DB, column
family)` a stable, well-spread *preferred phase* within the interval and steers
periodic compactions toward it. It is randomized and *uncoordinated* (DBs never
talk to each other), which both keeps it simple and, as it turns out, de-herds
far more cheaply than adding jitter to the interval. It is controlled by a single
mutable DBOption and is **off by default**:
- `periodic_compaction_phase_recovery_percent` (default `0` = disabled / kill
switch; recommended `33`; clamped to `[0, 100]`) -- how aggressively each
trigger converges toward the phase, as a percent of the remaining gap, only
ever pulling the trigger *earlier*. `0` reproduces the classic scheduler
exactly; `100` jumps to the phase in one cycle; lower values approach gradually
with much less extra work.
The preferred phase itself needs no tuning: it is derived from the DB ID (stable
across re-open and physical cloning/migration, but distinct across DBs) and each
column family is offset from the DB's base phase, so the CFs of one DB don't fire
together. (An earlier draft exposed a `compaction_schedule_seed` string option
for choosing the phase source; it was over-engineered for the one behavior anyone
needed, and as a mutable string it also broke OPTIONS-file round-trip
verification in crash testing. It has been removed in favor of always using the
DB ID.)
## The two kinds of badness
"Badness" here is the fraction of compaction work that is ideally unnecessary:
over-compaction (rewriting a file earlier than the interval required) plus a
penalty for lateness (going overdue past the interval). Steady state is 0; both
"never compact" and "compact twice as often as needed" approach 1. De-herding
inherently spends a little badness to spread load out; every design choice below
exists to bound one of those two costs *without* re-clustering the fleet.
The scenario names cited in the design bullets below (`upgrade_dynamic_r33`,
`naive_upgrade_r25`, `burst_r*`, `turndown_r33`, `turnup_r33`, ...) all come from
the out-of-tree simulator; see its results page (the "Simulator and
visualization" section below, with an interactive plot per scenario) for the
evidence behind each claim.
## Design
- **A stable per-(DB, CF) preferred phase = one fixed attractor per DB.** Each DB
gets a base phase from its DB ID; each CF is offset from it. Because the target
is fixed (not re-rolled each cycle) and unique per DB, the fleet converges to a
uniform spread and then compacts *on time* -- badness decays toward ~0. This is
the crucial contrast with naive per-cycle jitter, which de-herds but never stops
jittering and so pays a permanent over-compaction cost (see
`naive_upgrade_r25`: flat ~12-14%, only diffusive spreading) vs.
`upgrade_dynamic_r33`, whose badness decays to ~0 for the same anti-herding.
- **Converge gradually, and only ever earlier.** Each trigger closes a
`recovery_percent` *fraction* of the gap to the phase, reaching it over a few
cycles rather than in one burst, and never later than
`periodic_compaction_seconds` in steady state. A lower rate does less extra
work, which matters when the phase keeps moving (write-driven / bursty
workloads), where a high rate wastefully "chases" it -- the burst sweep shows
`burst_r25 < burst_r33 < burst_r50 < burst_r100` in sustained badness. `33` is
the default balance: quick spreading, low extra work.
- **Fractional recovery (not "snap") is latency-tolerant and doesn't re-herd.**
Closing a fraction of the gap gives a single stable convergence point, so
real-world scheduling/queueing latency -- the delay between a file becoming due
and its compaction actually running -- merely shifts the whole fleet uniformly
instead of re-clustering it. A tried "snap onto the phase when in range" variant
converged a bit faster with no latency but re-herded around the snap
threshold under latency; it was rejected. Graceful degradation beat peak speed.
- **Spread a suddenly-due cohort instead of bursting it (bounded lateness).**
Enabling the feature, turning `periodic_compaction_seconds` down, or catching up
after downtime can leave a whole cohort past due at the same instant. Firing it
all at once would just be the herd again, so such a cohort is spread over the
first quarter-interval (~N/4) after the phasing anchor on an *absolute*,
epoch-aligned per-(DB, CF) grid. This trades a bounded lateness (<= ~N/4 past
the deadline, so the interval is a *soft* upper bound during these transients)
for anti-herding, and the absolute grid keeps a rapidly restarting DB from being
perpetually re-deferred and starved. `turndown_r33` shows the past-due cohort
smeared over N/4 rather than bursting; `turnup_r33` confirms a turn-up creates
no past-due cohort (deadlines only grow) and so no burst. For immediate
enforcement at such a transition, trigger a manual compaction.
- **Off by default, with a kill switch.** Inert unless
`periodic_compaction_phase_recovery_percent > 0`; at `0` behavior is identical
to the classic scheduler.
## Implementation notes
- Phase math is integer-only (`Hash64` + `FastRange64`); floating point is avoided
for its pitfalls. The per-CF offset is a golden-ratio (Weyl/Kronecker) additive
sequence -- low-discrepancy for any number of CFs.
- Logic lives in `db/periodic_compaction_phaser.{h,cc}`
(`PeriodicCompactionPhaser`), which owns the recovery percent and the phasing
anchor and exposes the stateless trigger-time math; `VersionSet` holds one and
caches the per-CF result on each `Version`. The anchor is (re)set to now on DB
open, on a recovery-percent change, and on any `periodic_compaction_seconds`
change -- the three events that can create a past-due cohort.
- Plumbed through the C API, `db_bench`, `db_stress`, and `db_crashtest`.
## Simulator and visualization (out-of-tree, not part of this change)
A standalone simulator drives 40 real RocksDB instances through a real
periodic-task scheduler, records every compaction (periodic vs write-driven), and
scores the badness metric above across ~17 scenarios: dynamic/restart enable,
interval turn-down/turn-up via `SetOptions`, on-cadence writes, random write
bursts with a recovery-rate sweep, and the naive-jitter baseline. It is the basis
for the numbers cited above. Interactive HTML (**NOTE** fb-internal URL sanitization might require you to copy-paste the URL yourself):
|
||
|
Peter Dillinger
|
f7b791b213 |
Fix bottommost-files infinite compaction loop with preserve/preclude (#15146)
Summary: Leveled compaction combined with preserve_internal_time_seconds or preclude_last_level_data_seconds is a known-broken combination, but it is easy to enable by accident -- for example a fleet-wide config rollout that turns on the preserve time for CFs that happen to be leveled. Rather than degrading gracefully, it triggers an infinite kBottommostFiles compaction loop: a bottommost file gets marked, rewritten with no progress, and immediately re-marked, continuously rewriting the file and growing the MANIFEST so the file set never quiesces. This change makes such a misconfiguration a harmless no-op instead. Before this change, the marking logic (ComputeBottommostFilesMarkedForCompaction) marked a bottommost file whenever its largest seqno was below the oldest snapshot, without checking whether a compaction could actually zero that seqno. Recently written seqnos stay within the seqno->time "preserve" window and cannot be zeroed, so the rewrite never made progress. The same futile marking also occurred with user-defined timestamps when full_history_ts_low is unset. The underlying issue is that the "can this bottommost seqno be zeroed?" question was answered independently by the marking logic and by the compaction output path (CompactionIterator::PrepareOutput), and the two had drifted apart. With this change, both paths answer that question through one shared predicate (BottommostSeqnoCanBeZeroed), and the preserve-window boundary that marking needs is plumbed into VersionStorageInfo from DBImpl, mirroring the existing oldest-snapshot plumbing. Snapshot visibility stays a per-site check. On CFs without preserve/preclude the predicate is a no-op. Confirmed that caller invariants ensure that relocating the `ikey_.sequence <= preserve_seqno_after_` check in CompactionIterator::PrepareOutput() will not trip the ROCKSDB_LOG_FATAL. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15146 Test Plan: - compaction_picker_test: new CompactionPickerTest BottommostFileNotMarkedWithinPreserveWindow / BottommostFileMarkedBelowPreserveWindow cover the preserve-window boundary; new CompactionPickerU64TsTest BottommostNotMarkedWithEmptyFullHistoryTsLowAndUnknownMaxTs covers the UDT edge case. - tiered_compaction_test: new PrecludeBottommostLoopTest LeveledPreserveTimeNoBottommostLoop is an end-to-end regression for the loop and the DBImpl plumbing: a bottommost file with a nonzero largest seqno inside the preserve window, made a marking candidate via a snapshot release, must not be marked. It reproduces the loop (hangs) when the plumbing is neutralized and passes with the fix. Reviewed By: xingbowang Differential Revision: D117299979 Pulled By: pdillinger fbshipit-source-id: 51e8e2d4e2116984d16d3c7ef54a9635d5aded74 |
||
|
Josh Kang
|
6dbb6f30e6 |
Avoid restoring coroutine stats on foreign threads (#15145)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15145 ## Summary RocksDB coroutine statistics are stored in `RequestContext`, but their `RequestData` is thread-affine. A filesystem implementation may propagate the context while completing `SubmitReadAsync` on another thread. Installing that context invokes `CoroutineStatsRequestData::onSet()`, whose `AssertSameThread()` detects the thread hop and aborts. ``` Before: Thread A Thread B RequestContext ├── caller tracing/attribution data └── RocksDB coroutine stats ── propagated ──> setContext() │ ▼ CoroutineStatsRequestData::onSet() │ ▼ AssertSameThread(A != B) │ ▼ abort ``` Keep propagating the complete `RequestContext`, including the coroutine-stats token, so filesystem callbacks retain caller tracing and attribution. `CoroutineStatsRequestData` now records its owner using `Env::GetThreadID()` and ignores `onSet()` and `onUnset()` on foreign threads. When `ViaIfAsync` schedules the continuation back onto the RocksDB read EventBase, it restores the saved context on the owner thread before the coroutine continues, so the stats are safely loaded back into RocksDB TLS. ``` After: Thread A Thread B caller RequestContext ├── caller data ───────── propagated ───────> caller data └── RocksDB coroutine stats ─ propagated ───> onSet()/onUnset() │ ▼ owner thread != current thread │ ▼ no-op continuation queued back onto Thread A │ ▼ saved RequestContext restored on owner thread │ ▼ RocksDB coroutine stats reinstalled in TLS ``` Reviewed By: xingbowang Differential Revision: D117274942 fbshipit-source-id: 9995cc3053a0a1d3a63d0cb2f4dd9320ab553005 |
||
Kunal Pathak
|
bd16531142 |
Hoist top-level filter-partition index iterator out of MultiGet per-key loop (#15137)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15137 Reviewed By: pdillinger Differential Revision: D115489931 fbshipit-source-id: 9d5b607c834a6e8754d55619309ec3705094cc76 |
||
|
Xingbo Wang
|
e6a2ee0bd2 |
Fix subset checkpoints with incomplete MANIFEST tails (#15123)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15123 Subset checkpoint post-processing appends column-family drop records to a copied MANIFEST. Recovery can tolerate an incomplete atomic group at the tail while retaining the reader high-water size, so appending at that size can produce a checkpoint that fails recovery. Track the valid logical append boundary together with its MANIFEST file number, refresh it after recovery and successful writes, reset it across reactive MANIFEST switches, and trim only subset-checkpoint copies before appending drop records. Whole-DB checkpoints and the source MANIFEST remain unchanged. Reviewed By: anand1976 Differential Revision: D116449394 fbshipit-source-id: a162dd6a2558f0f9ec40d878ee83956a89df698b |
||
Andrei Preda
|
ad81b654a4 |
Prune resolved 2PC recovery state on secondaries (#15121)
Summary: Previously, during WAL replay, a secondary stored each 2PC prepare in `recovered_transactions_`. Replaying the matching commit or rollback marker deletes it. If the primary purges that marker's WAL before the secondary reads it, the prepared `WriteBatch` and its tracker entry remain until the secondary closes. This retention case was flagged by xingbowang in https://github.com/facebook/rocksdb/issues/15066. Even when the marker is replayed, the secondary records completion but previously did not prune the completed prefix from `logs_with_prep_tracker_`. This change deletes recovered transactions whose prepare WALs are all below `min_log_number_to_keep` after opening every discovered WAL reader but before replaying records. The primary keeps that watermark at or below every unresolved prepare and every prepare with unflushed committed data, so an entry below it is committed and flushed, or rolled back. Deleting before replay lets a reused transaction name start a new generation instead of merging into stale state. If a reader cannot be opened, no recovered transaction is deleted. After a round successfully replays every discovered WAL, the secondary also prunes the completed prefix from `logs_with_prep_tracker_`. This covers completions from normal commit or rollback replay and from the watermark-based cleanup. The oldest outstanding prepare stops the pruning scan. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15121 Test Plan: Added coverage for missed-marker cleanup, transaction-name reuse before replay, the exact watermark boundary, a catch-up round that reports success after replaying no WALs, and tracker pruning after normal commit replay. The missed-marker case verifies cleanup of both retained structures. Reviewed By: pdillinger Differential Revision: D116620460 Pulled By: xingbowang fbshipit-source-id: baca7faf53f414507957f2afdcc4ca67133c5669 |
||
|
Peter Dillinger
|
8f492b2f47 |
Store common user-key prefix once per block (format_version 8) (#15117)
Summary:
Building on D116105558, this change finishes and publishes `format_version = 8` as
production-ready.
The headline feature is a tweaked block encoding that stores the common user-key
prefix shared by all keys in a block **once**, in the block's leading bytes
`[0, restarts[0])`, instead of repeating it at every restart point. This shrinks
prefix-heavy blocks (and their block-cache footprint) and, for bytewise-ordered
keys, speeds up `Seek` by comparing prefix-stripped suffixes. It is applied to
**data blocks** and **every index tier** (leaf/partition index, the
partitioned-index top-level, and the partitioned-filter top-level).
Motivation: MyRocks/ZippyDB/timeseries-style keys carry a long shared prefix
(table/index id, partition, etc.). Repeating it at each restart wastes space and
cache, and every Seek re-compares those bytes. Profiles show index-block seek CPU
is comparable to data-block seek CPU (partitioned index does two index seeks per
data seek), so index blocks are a first-class target, not just data blocks.
### Format (self-describing, no new footer bit)
- The common prefix occupies the block's leading bytes; a non-zero `restarts[0]`
(the offset of the first entry, i.e. the prefix length) self-signals its
presence. `restarts[0]` is 0 in every block that does not use the feature (all
versions, all block types), so a single fv8 reader transparently handles any
mix of stripped and unstripped blocks -- no footer bit is consumed.
- Previous versions would misinterpret this encoding by quietly ignoring the
common prefix, which (along with the lack of extensibility in data-block
metadata) is why it is gated behind format_version 8.
- Only restart-point entries change on disk; non-restart entries are byte
identical to fv7, so the writer produces the exact same bytes regardless of
implementation.
### Public option
New `BlockBasedTableOptions::optimize_key_common_prefix`
(`enum class OptimizeKeyCommonPrefix`), default `kIfFastSeek`:
- `kDisabled` -- never use it.
- `kIfFastSeek` (default) -- use it only where it also enables the Seek speedup:
format_version >= 8 with a built-in (reverse-)bytewise comparator.
- `kEnabled` -- use it for all comparators at fv8; (reverse-)bytewise get space +
fast seek, other comparators get the space savings only (the reader
reconstructs full keys and compares with the real comparator).
Expressed as a criteria so it need not be toggled per CF based on the comparator
(which can vary by CF). It additionally requires `use_delta_encoding` (the
feature builds on delta encoding) and `ts_sz == 0`; when `use_delta_encoding` is
false -- the setting that also preserves zero-copy key pinning -- it is a no-op.
Default format_version is unchanged (still 7); the feature is opt-in via fv8.
### Scope: every index tier
The reader handles all index-like blocks uniformly (a non-zero `restarts[0]`
self-signals a stripped block regardless of block type), so the writer strips the
common prefix from every tier:
- **Data blocks** and **leaf / partition index blocks**: enabled.
- **Partitioned-index top-level block**: enabled. Under the default
`index_shortening = kShortenSeparatorsAndSuccessor` the file's last separator
would otherwise be a short successor sharing no leading bytes, collapsing the
single-block top-level's common prefix to 0. When common-prefix is on the
builder suppresses that one successor (keeps the full last key -- i.e.
`kShortenSeparators` behavior for that one entry), so the top-level strips
uniformly under every shortening mode; the full last key is also a tighter file
upper bound, so it is not worse.
- **Partitioned-filter top-level index**: enabled (same uniform treatment). Its
space/CPU effect is small at default partitioning (the top-level filter index
is tiny) but grows with many small partitions -- a filter miss short-circuits
at, and is dominated by, the top-level filter-index seek (see Performance).
Included for a uniform on-disk surface across tiers.
Bloom filter *contents* still hash full user keys, so prefix/whole-key filtering
is unaffected.
### Implementation
- Writer (`block_builder.{h,cc}`): keys are encoded incrementally in the fv7
layout while the running common prefix is tracked cheaply (reusing the delta
`shared`); at `Finish()` only the restart-point entries are rewritten in place
with the prefix stripped, and the prefix is written once at the block start.
The rewrite is generalized to the value-delta (fv4) index layout used by index
blocks (no `value_size` header; bulk-copy the inline BlockHandle and
non-restart entries verbatim).
- Reader (`block.{h,cc}`, `dbformat.h`): `Block` decodes the prefix from
`restarts[0]`; `BlockIter::ParseNextKey` prepends it on a restart point
(`shared == 0`). The suffix-seek helpers (strip target once, compare shortened
suffixes) live in the templated `BlockIter` base and are shared by data and
index iterators (binary search, templated on `DecodeKey`/`DecodeKeyV4`); for
non-bytewise comparators (data) and the hash `PrefixSeek` refine step, the
reader reconstructs the probed restart key and compares full keys.
- Gating (`block_based_table_builder.cc`, `index_builder.{h,cc}`,
`partitioned_filter_block.{h,cc}`): the criteria is threaded to the data-block
builder and to every index tier -- leaf/partition index, the partitioned-index
top-level (`use_common_prefix_top`), and the partitioned-filter top-level --
together with the last-separator successor suppression
(`ShortenedIndexBuilder::GetSeparatorWithSeq`) that lets the top-level strip
under the default shortening mode. The index base gate also requires
`super_block_alignment_size == 0` (block alignment can trigger
`skip_delta_encoding`, producing non-restart `shared == 0` entries the reader
could not distinguish from stripped restart keys).
- Memory: the per-block/per-iterator prefix is stored as a `uint32_t` length
(the prefix is always at offset 0) rather than a full `Slice`, and `Block` /
`BlockBuilder` members were repacked so the feature adds little or no struct
size.
- Java: a new `OptimizeKeyCommonPrefix` enum and a `BlockBasedTableConfig`
getter/setter, plumbed through JNI in both directions (the
`newTableFactoryHandle` writer and the options-reconstruction constructor).
### Whole-file (table-level) prefix: investigated and rejected
A natural extension is to store the whole-file common prefix once per *file*
(when an `SstPartitioner` guarantees every key in the file shares a fixed prefix)
instead of once per *block* -- e.g. the MyRocks index/table id, or a future
128-bit global id. It was prototyped end-to-end (partitioner-provided prefix
captured at file open, stored in a `user_key_common_prefix` table property, and
stripped from every data/index block; the reader re-prepends it) and then
dropped. The `user_key_common_prefix` table property reserved in the
previous diff is dropped in this one. The rationale, in case it is revisited:
- Marginal benefit over per-block. Per-block already strips the full shared
prefix from every key; the only thing whole-file removes on top is the single
per-block copy of that prefix, i.e. `~= prefix_len / block_size` of the
uncompressed (block-cache) footprint, with no meaningful read-CPU change
(per-block already delivers strip-once + suffix compares). Measured at
prefix_len=12, 4 KB blocks this was ~-0.3% incremental space; at the 8-16 KB
data blocks typical of the space-constrained target (MyRocks/UDB) it is
~0.1-0.2% even with a 16-byte global id -- a fraction of a percent, seen only
by partitioner users. Small block-size workloads are becoming more
niche, and even where they exist, they are also CPU-sensitive (see next
paragraph).
- Real, if small, CPU tax on the *per-block* path. Reconstructing a key from two
non-contiguous parts (file prefix + block residual) cost ~0.5-1.0% additional
retired instructions on the per-block read path (deterministic across gcc and
clang), wall-clock-neutral in cache-resident tests but present even when no
whole-file prefix is configured. Attempts to remove it did not pan out: a
compile-time specialization of the reconstruction *increased* instructions
(+2.9%) by defeating inlining; a per-block combined-prefix buffer trades the
CPU cost for a memory cost on every (default) block; and a per-iterator buffer
was the original larger regression. Only invasive iterator-level
monomorphization would truly zero it, which is not justified on this hot path.
- Decision. A fraction-of-a-percent, partitioner-only space win does not justify
a permanent per-block-read-path CPU tax plus additional on-disk format surface
(a table property that changes whole-file interpretation). Dropped. The generic
`SstPartitioner::ShouldPartitionByPrefix` hook is kept (cheap, and useful
independently), and the reserved `user_key_common_prefix` table property is
removed since no writer will emit it.
### Performance (db_bench, release/native, cache-warm, best-of-3, pinned)
Three arms: (A) fv7 on the base/main branch, (B) fv7 with this change (should be
neutral / byte-identical writing), (C) fv8 with this change and
`optimize_key_common_prefix=enabled` (the optimization is active). Partitioned
index + filters; `compression_type=none`, so SST bytes are the uncompressed
block-cache footprint. `int` = db_bench default keys (variation-early, near worst
case); `pfxN` = an N-byte shared prefix + varying suffix (MyRocks-like).
Setup: `num=3,000,000`, `reads=1,500,000`, single thread,
`metadata_block_size=4096`, cache holds the working set, pinned to one core.
| shape (key/val) | arm | SST bytes | readrandom us/op | seekrandom us/op |
|---|---|---:|---:|---:|
| int (32/8) | A fv7 main | 138,983,658 | 3.484 | 4.995 |
| | B fv7 ours | 138,983,146 | 3.518 | 5.024 |
| | C fv8 ours | 137,681,420 | 3.490 | 4.986 |
| pfx24 (32/8) | A fv7 main | 71,697,531 | 3.417 | 5.561 |
| | B fv7 ours | 71,698,043 | 3.448 | 5.579 |
| | C fv8 ours | 65,578,236 | 3.284 | 5.386 |
| pfx56 (64/16) | A fv7 main | 102,942,266 | 3.624 | 5.830 |
| | B fv7 ours | 102,942,266 | 3.678 | 5.835 |
| | C fv8 ours | 90,554,642 | 3.444 | 5.626 |
fv8 vs fv7-main (C vs A) -- space / readrandom / seekrandom:
- int (32/8): **-0.9%** / ~neutral / ~neutral
- pfx24 (32/8): **-8.5%** / **-3.9%** / **-3.1%**
- pfx56 (64/16): **-12.0%** / **-5.0%** / **-3.5%**
fv7-ours vs fv7-main (B vs A): SST bytes byte-identical (within 512 B; the fv8
paths are gated off), read/seek us/op within run-to-run noise (~1%, best-of-3 on
a shared host). Net: fv8 is a space **and** read/seek-CPU win on prefix-heavy
data and neutral on variation-early (`int`) data, so `kIfFastSeek` is safe on by
default at fv8.
### Space by tier (compression=none, num=3M, mdb=4096; fv7 -> fv8)
Where the win comes from -- the index tiers strip the most:
| shape | data | leaf/partition index | top-level index | filter |
|---|---:|---:|---:|---:|
| int | -0.8% | -29% | -45% | -0.2% |
| pfx24 | -8.3% | -69% | -89% | -0.8% |
| pfx56 | -11.1% | -83% | -96% | -1.6% |
Data bytes dominate the absolute total, but every index tier is stripped:
leaf/partition index -29% to -83% and the partitioned-index top-level -45% to
-96%. Filter *blocks* barely move (-0.2% to -1.6%) because filter bits hash full
user keys; only the small top-level filter index is stripped.
### Point-miss lookups (readmissing)
A partitioned-filter miss short-circuits at the filter (no data-block read), so
it is dominated by the top-level filter-index seek plus the partition bloom. At a
typical `metadata_block_size=4096` (few partitions, tiny top-level filter index)
fv8 is neutral on misses (int +0.9%, pfx24 -1.0%, pfx56 -0.7% us/op -- all
noise). The top-level-filter stripping pays only with many small partitions: at
`metadata_block_size=256` it is pfx24 1.166->1.138 (**-2.4%**) and pfx56
1.273->1.188 (**-6.7%**) us/op.
### Backward / forward compatibility
- Default `format_version` remains 7; the feature is inert unless a CF opts into
fv8. Older RocksDB rejects fv8 files at footer decode (fail-safe).
- Within fv8, the stripped-block set a writer can emit is fixed and every fv8
reader can read it (the `restarts[0]` signal plus uniform reader handling), so
the on-disk surface is self-consistent across builds/downgrade.
### Follow-ups (not in this change)
- Suffix-aware interpolation / hash index seek (currently correct via the
reconstruction fallback, not perf-tuned).
Pull Request resolved: https://github.com/facebook/rocksdb/pull/15117
Test Plan:
- New focused unit tests (`table_test`), run at the full `format_version` matrix
including the now-supported fv8:
`GeneralTableTest.OptimizeKeyCommonPrefix` (data-block space+read+Seek
correctness: existing and missing keys straddling the prefix, before/after-all,
SeekForPrev), `OptimizeKeyCommonPrefixNonBytewise` (reconstruction path), and
`IndexBlockCommonPrefixEquivalence` across `{kBinarySearch,
kBinarySearchWithFirstKey, kHashSearch, kTwoLevelIndexSearch}` x
`{partitioned filters}` x `{bytewise, reverse-bytewise}` (so both top-level
tiers -- partitioned-index and partitioned-filter -- are exercised), asserting
identical reads and a shrunken fv8 index.
- The randomized `ParameterizedHarnessTest` now runs at fv8 (reverse-bytewise,
restart intervals `{1,16,1024}`, special/empty keys fuzzed against the feature).
- `db_crashtest.py` randomizes `format_version` including 8 and
`optimize_key_common_prefix` in {0,1,2}.
- Removed the reserved-property carve-out: dropped the `ReservedUserKeyCommonPrefix`
reader test and the C API getter for the property; C API regenerated via
`tools/c_api_gen/` (`make regen-c-api`), and the intentional public-C-API
removal is recorded in `tools/c_api_gen/api_compatibility_allowlist.json`.
- Java: `BlockBasedTableConfigTest.optimizeKeyCommonPrefix` (default
`kIfFastSeek`, plus set/get of every value).
- `table_test` `FooterTests` updated for the now-published fv8 footer, whose
metaindex-gap encoding stores the metaindex block size in 16 bits (the test
caps that size for fv >= 8).
- Manual verification: the db_bench runs in Performance above (three-arm
space + read/seek CPU, per-tier byte breakdown via `sst_dump`, and
`readmissing`); and `readrandom`/`seekrandom` with
`--block_protection_bytes_per_key=8` find 100% of keys at both fv7 and fv8
(reconstructed keys pass per-key checksum).
Reviewed By: xingbowang
Differential Revision: D116365016
Pulled By: pdillinger
fbshipit-source-id: 66f4b5c60ea4fe0ff48d34c98d191ba8ebdf0665
|
||
|
Peter Dillinger
|
e8554f1b11 |
Faster and safer MANIFEST recovery for remote compaction (OpenAndCompact) (#15133)
Summary: Faster and safer MANIFEST recovery for remote compaction (OpenAndCompact) `DB::OpenAndCompact` (remote/distributed compaction) opens the target DB as a secondary and reconstructs the LSM state from the MANIFEST before running the requested compaction. Previously, point-in-time recovery would stat/open the SST (and blob) files referenced by the MANIFEST to classify them found/missing and materialize candidate versions, and eagerly load their table handlers. That is (a) a lot of unnecessary file operations and open latency, especially on remote/distributed storage, and (b) fragile: when a file referenced by the MANIFEST but not an input to the compaction was momentarily unavailable to the worker -- e.g. an obsolete file the primary had already deleted -- recovery could fail the job (surfacing as an incomplete "no valid result") or roll back to an earlier version, even though the compaction only needs its own input files, which the primary keeps referenced for the job's duration. This was seen as a flaky failure of CompactionServiceTest.BasicCompactions on aarch64, and reproduces on x86 under COERCE_CONTEXT_SWITCH. Efficiency + reliability: recovery for the OpenAndCompact path now trusts the MANIFEST. It reconstructs the current version from FileMetaData without stat-ing/opening files to classify them and without eagerly opening candidate-version table handlers, so it does far fewer file operations and never fails or rolls back over a file the compaction does not need. The compaction's input files are opened (and unique-id verified) on demand as it reads them. Correctness hardening: trusting the MANIFEST means the worker's own (possibly eventually-consistent) view of CURRENT/MANIFEST drives the reconstructed LSM shape, which feeds correctness-critical decisions (Compaction::IsBottommostLevel / KeyNotExistsBeyondOutputLevel). A worker that read a stale CURRENT (older MANIFEST), a truncated MANIFEST, or a MANIFEST with a corrupt/torn tail could reconstruct against an older LSM shape and silently drop keys that should be kept. To prevent this, the primary records the MANIFEST position (file number and size) it scheduled the compaction from and passes it in CompactionServiceInput as a floor. After recovery the worker compares the size of the maximal *valid* prefix it reconstructed -- which stops at any corrupt or torn record and at a partial atomic group at EOF, so tolerated tail garbage cannot inflate it -- against that floor, and refuses to compact against an older view (an older file number, or the same file number whose valid prefix is shorter), returning Status::Incomplete so the CompactionService can fall back to a local compaction. Accept-equal-or-later is safe by monotonicity: while the inputs stay locked (being_compacted), a later view can only have moved data further down, never resurrecting a shadowed value, so only an older view is unsafe. Single kill switch: both behaviors are bundled behind one signal and one option. Presence of the floor on the request (min_manifest_file_number != 0) couples on both trust-manifest recovery and the floor check in the worker; absence reverts the worker to the original point-in-time recovery with no floor check. A single mutable DBOption, remote_compaction_manifest_floor (default on), gates whether the primary populates the floor, so it is the one escape hatch that disables the entire new recovery path on a running DB via SetDBOptions if it ever misbehaves. The new CompactionServiceInput fields are wire-compatible in both directions (0 = not provided), so an old worker or an old primary simply behaves as if the switch were off. Follow-up: recovery is not yet strictly "open only the compaction's inputs". A limited number of non-input files are still opened by the bounded file-property sampling in Version::PrepareAppend -> UpdateAccumulatedStats (tolerating any that are missing), which today populates input FileMetaData::num_entries for compaction input-record-count verification. A natural follow-up is to record the needed per-file info (e.g. num_entries) in the MANIFEST so the worker can skip these auxiliary opens entirely; alternatively, source input num_entries from the worker's own read of its input table properties. Relatedly, the floor comparand is the recovered "maximal valid prefix" size, which today equals the MANIFEST file size but is defined to stay correct if a future MANIFEST format writes trailing padding/footer past the last record. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15133 Test Plan: Added tests in compaction_service_test.cc: - RemoteCompactionMissingLowerFileTrustsManifest: a non-input file referenced by the MANIFEST is unavailable during a remote compaction; the job completes with correct results (fails without trust-manifest recovery). The underlying race also reproduces under COERCE_CONTEXT_SWITCH. - RemoteCompactionManifestFloorRejectsStaleView: inflate the floor so the worker's recovered MANIFEST is below it; assert the worker rejects, the job falls back to local, and a below-output deleted key stays deleted (no silent resurrection). - RemoteCompactionManifestFloorSendsPosition: a non-zero floor is attached and the worker accepts its equal-or-later view. - RemoteCompactionManifestFloorKillSwitchOff: with the option off, no floor is sent, the worker uses the original recovery, and a transiently-missing non-input file makes the job fail/roll back (the escape hatch reverts behavior). - RemoteCompactionManifestFloorMutable: SetDBOptions flips the option; the floor stops being attached and the worker skips the check. options_settable_test covers the new mutable DBOption. Crash test: remote compaction is already exercised (db_crashtest randomly enables remote_compaction_worker_threads, driving DB::OpenAndCompact), so with remote_compaction_manifest_floor defaulting on, the new recovery path is covered by the crash test by default -- no new crash-test plumbing is needed. The kill switch is intentionally left un-randomized (always on): turning it off re-enables the original, occasionally-broken recovery, which would only add noisy failures for no benefit. UPDATE: crash test is not well covered because of `"remote_compaction_failure_fall_back_to_local": 1` which is masking other existing failures. Addressing this is a high-priority follow-up item. (Not practical to fix in this diff.) Reviewed By: jaykorean Differential Revision: D116738823 Pulled By: pdillinger fbshipit-source-id: c02e0a437d62fc526dbbfcf70edf43434dfc7c1f |
||
|
rocksdb-release-bot
|
330ded5129 |
Update version to 11.10.0 for 11.9 release (#15135)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15135 - Bump version.h from 11.9.0 to 11.10.0 - Add `11.9.fb` to `check_format_compatible.sh` - Sync HISTORY.md `## 11.9.` section from `11.9.fb` - Delete 17 consumed `unreleased_history/` note file(s) from main Part of 11.9 release workflow. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15096 Reviewed By: anand1976 Differential Revision: D116833037 Pulled By: joshkang97 fbshipit-source-id: 04ebda8db5b2c4fdae71a2cf107784b6abdc0f3c |
||
|
Anand Ananthabhotla
|
4b41ccd2a7 |
Fix full_history_ts_low race for long-running snapshots in db_stress (#15129)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15129 When `persist_user_defined_timestamps=false` and `long_running_snapshots=1`, `AssertSame` in `MaybeReleaseSnapshots` reuses the timestamp saved at snapshot acquisition time. That timestamp can become stale because concurrent flushes advance `full_history_ts_low`, and long-running snapshots can be held much longer than the 10-second headroom added by `GetReadTimestamp()`. The snapshot validation is intended to check that a held snapshot still exposes the same sequence-number view. It should not require the acquisition-time read timestamp to remain valid after history below `full_history_ts_low` has been collapsed. The fix keeps the timestamped read API contract intact. Column families with user-defined timestamps still require `ReadOptions.timestamp`, even when timestamps are memtable-only. For `persist_user_defined_timestamps=false`, `AssertSame` now uses a fresh validation timestamp so the read remains above `full_history_ts_low` while the snapshot sequence number continues to pin data visibility. For persisted timestamps, it continues reusing the original timestamp to validate the exact timestamped snapshot view. Reviewed By: joshkang97 Differential Revision: D116110124 fbshipit-source-id: 9cf6cae6e7123f8453b7065565e9e2e0eb34321a |
||
|
Ranjan Banerjee
|
2846163d9e |
Fix remote compaction input record count mismatch due to uninitialized FileMetaData stats (#15130)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15130 ## Summary Fix a corruption error in remote compaction where `FilterInputsForCompactionIterator` produces different results on the primary host vs. the remote worker. **Root Cause:** When a standalone range tombstone file completely shadows another input file, `FilterInputsForCompactionIterator()` removes the shadowed file from `input_levels_`. This filtering relies on `FileIsStandAloneRangeTombstone()`, which checks `num_entries == num_range_deletions == 1`. On the primary host, these `FileMetaData` fields are always populated (set during flush/compaction when the file was created). On the remote worker, however, these fields are NOT persisted in the MANIFEST and are only lazily loaded from table properties via `UpdateAccumulatedStats()` during DB open, which is capped at 20 files (`kMaxInitCount`). If the standalone range tombstone file is not among those first 20 initialized files, its `num_entries` remains 0 on the remote worker. This causes `FileIsStandAloneRangeTombstone()` to return false, so the remote worker does NOT filter the shadowed file and processes all its keys. Meanwhile the primary's expected record count is computed from its (filtered) view, leading to: ``` Corruption: Compaction number of input keys does not match number of keys processed. Expected 0 but processed 12. ``` **Fix:** Before constructing the `Compaction` object on the remote worker, explicitly initialize `FileMetaData` stats from table properties for all input files. This ensures `FilterInputsForCompactionIterator` produces consistent filtering results on both sides. **Introduced by:** D83375779 (2025-09-29), which enabled `FilterInputsForCompactionIterator` on the remote worker by passing `earliest_snapshot` to `PickCompactionForCompactFiles`. Reviewed By: xingbowang Differential Revision: D116252973 fbshipit-source-id: 2cb2d61dae1f3982edeaf41f9a0f39d5aef1e789 |
||
|
Josh Kang
|
03b8d1afa0 |
Prevent normal flushes from starving error recovery (#15132)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15132 This fixes a liveness bug in background-error recovery. Retryable soft errors allow foreground writes to continue, but recovery must wait for every scheduled background flush worker to exit before it can submit the recovery flush. While recovery was active, normal flush requests could still enter `flush_queue_`. Atomic flush requests bypass per-column-family deduplication, so writes could accumulate a backlog of normal requests. For each request, the background worker: 1. Pops and drops the normal request because BG work is stopped 2. Sleeps for the one-second background-error backoff 3. Decrements `bg_flush_scheduled_`: 1 -> 0 4. Calls `MaybeScheduleFlushOrCompaction()` 5. Schedules the next queued request: 0 -> 1 6. Only then signals the recovery thread The rescheduling happens under the DB mutex before recovery is signaled: Queued requests: R1, R2, R3, ... Flush worker Recovery thread ------------ --------------- drop R1 sleep 1 second waiting for scheduled == 0 scheduled: 1 -> 0 schedule R2: 0 -> 1 signal -------------------------> wake, observe scheduled == 1 wait again drop R2 sleep 1 second scheduled: 1 -> 0 schedule R3: 0 -> 1 signal -------------------------> wake, observe scheduled == 1 wait again The dropped request is not requeued. Instead, the worker exit path guarantees that the next queued request is scheduled while `unscheduled_flushes_ > 0`. A finite backlog therefore delays recovery by roughly one second per request. If writes keep producing requests as quickly as workers discard them, the queue may never drain and recovery is starved indefinitely. Recovery can complete after writes become quiescent, so this is a liveness failure rather than a lock deadlock or data-corruption bug. There is also a queue-accounting edge. Scheduling consumes `unscheduled_flushes_`, but a worker does not own its `FlushRequest` until it pops `flush_queue_`. A worker can exit on the background error before popping, leaving: flush_queue_.size() > 0 unscheduled_flushes_ == 0 Clearing only unscheduled requests therefore misses scheduled-but-unclaimed requests. The fix: - Reject new non-recovery requests while background work is stopped. - Allow error-recovery flush reasons to bypass that guard. - Clear the entire flush queue before recovery waits for scheduled workers. - Retain the worker-side reason check for requests queued before the error. - Rebuild recovery flushes and schedule a catch-up flush for writes made during recovery. Reviewed By: pdillinger Differential Revision: D116708546 fbshipit-source-id: 6e29fd3653ef72d047c64ca89d311a7bee51ab84 |
||
|
Xingbo Wang
|
42fca1bb39 |
Enable core dumps for sanitizer crash tests (#15131)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15131 ASAN and other sanitizer runtimes can disable coredumps or terminate without a core after the test driver sets its shell core limit. This leaves sanitizer stress-test failures with an LLDB stack but no core for the DB artifact. Configure ASAN_OPTIONS, TSAN_OPTIONS, and UBSAN_OPTIONS during sanitizer crash-test steps so reports terminate with a core-generating abort and core dumping remains enabled. Preserve caller-provided options and the existing Make fallback semantics. Keep the behavior scoped to test steps and crash targets. Enabling sanitizer cores raises a concern that the generated core-dump artifacts can be large because they include the process working set. Reduce RocksDB's cache and memtable contribution to the core size without replacing the normal randomized test configuration. Add a db_crashtest cache/write-buffer size multiplier that is applied after each iteration chooses its values, and pass 0.5 from ASAN, TSAN, and UBSAN crash lanes. For the simple profile, cache_size remains randomized with 8/32 MiB becoming 4/16 MiB, while the 32 MiB write buffer becomes 16 MiB. Preserve RocksDB's 64 KiB minimum write-buffer size and leave non-sanitizer lanes unchanged. Reviewed By: rban1 Differential Revision: D116666171 fbshipit-source-id: aa157cfa255bb637d4a68e54ea2039db779fdc53 |
||
|
Peter Dillinger
|
d3a82f28ce |
Optimize SstPartitioner overhead in compaction via ShouldPartitionByPrefix() (#15118)
Summary: When a column family configures an SstPartitioner, compaction paid a per-key cost for every key-value written to an output SST: a full user-key copy (into `last_key_for_partitioner_`) plus a virtual `SstPartitioner::ShouldPartition()` call (which also builds a PartitionerRequest and truncates/compares two Slices). This is motivated by an internal user whose custom SstPartitioner parses a variable-length prefix (a prefix code) out of each key to decide partition boundaries; today that prefix parsing runs on every key written during compaction, on top of the copy and virtual call above. This wires up the `SstPartitioner::ShouldPartitionByPrefix()` API (added, but unused, alongside the format_version=8 groundwork) so compaction can skip that per-key work. When a partitioner can express its next boundary as a key prefix, `CompactionOutputs` caches that prefix once per output file (re-derived from the file's first key) and replaces the per-key copy + virtual call with an inlined `Slice::starts_with()` check. That also lets a partitioner like the one above do its prefix parsing once per output file instead of once per key. When the partitioner returns no prefix (ambiguous, e.g. `SstPartitionerFixedPrefix` for keys shorter than its length), compaction falls back to the previous per-key `ShouldPartition()` path. Output file boundaries are byte-for-byte identical to before. `SstPartitionerFixedPrefix` implements `ShouldPartitionByPrefix()`; the base-class default returns "no prefix", so existing custom partitioners are unaffected until they opt in. Also: - db_bench: new `--sst_partitioner_fixed_prefix_len` to configure a fixed-prefix partitioner (enables benchmarking; no default behavior change). - `ShouldPartitionByPrefix()` API doc updated now that it is used and recommended for compaction CPU efficiency (any prefix code works, not just fixed-size). No new option and no behavior or format change. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15118 Test Plan: Added db_compaction_test cases covering the new fast path and its fallbacks: many keys sharing a prefix (one output file per prefix, no spurious cuts), keys shorter than the fixed prefix length (fallback to ShouldPartition), the zero-length-prefix "never partition" case, and short<->long key transitions across output-file boundaries. Existing SstPartitioner tests and the range tombstone + partitioner tests (db_range_del_test) provide regression coverage that file boundaries are unchanged, including the fast path with range tombstones. The crash test already exercises the partitioner via `enable_sst_partitioner_factory`. Benchmark (db_bench `compact` of a pre-built 50M-key DB; 16B keys / 8B values, 8-byte shared key prefixes with FixedPrefix length 8, no compression, single subcompaction). A/B compares this change against the same binary with only the compaction hot-path change reverted. Means of 6 trials, optimized faster on every trial and every metric; both variants produced identical output (50 files, i.e. identical partition boundaries): | Metric | Baseline | Optimized | Delta | |---------------------------------|----------|-----------|--------| | Instructions (whole process) | 92.46 B | 90.09 B | -2.6% | | CPU cycles | 46.20 B | 45.28 B | -2.0% | | CPU time (task-clock) | 18.74 s | 18.39 s | -1.9% | | `compact` wall time | 20.58 s | 20.08 s | -2.4% | DB-internal compaction stats corroborate: CompMergeCPU 15.84s -> 15.42s (-2.6%), compaction write throughput 38.9 -> 39.9 MB/s. The relative gain shrinks with larger values or compression enabled, where the partitioner is a smaller fraction of total compaction work; it is a pure win whenever a prefix partitioner is configured. Reviewed By: rban1 Differential Revision: D116372363 Pulled By: pdillinger fbshipit-source-id: 63de190329112aa2ce5b48da9fc77ef386494558 |
||
|
Xingbo Wang
|
d8536d3274 |
Enable continuous verification for secondary stress tests (#15114)
Summary: PR https://github.com/facebook/rocksdb/issues/15066 fixed the bug that has caused this combination fails in the past. Re-enable this combination in the stress test. db_crashtest.py currently disables continuous_verification_interval whenever test_secondary=1. This change removes that quarantine and enables continuous verification for secondary DB stress tests. The disable predates auto_refresh_iterator_with_snapshot. When auto-refresh was later added, its stress-test integration mechanically added snapshot setup to several iterator paths, including the secondary- only NonBatchedOpsStressTest::ContinuouslyVerifyDb() path: snapshot = std::make_unique<ManagedSnapshot>(db_); read_opts.snapshot = snapshot->snapshot(); This creates a snapshot from the primary db_ and passes it to an iterator created from secondary_db_. That was never supported: a ReadOptions snapshot must belong to the DB being read, and secondary DB iterators explicitly reject non-null snapshots. The incompatibility went unnoticed because the existing sanitizer made this combination unreachable in recurring crash tests. It was therefore a stress-test integration bug, not evidence that snapshot- preserving auto-refresh was supported by secondary DBs. This change: - Stops forcing continuous_verification_interval=0 for secondary tests. - Avoids attaching a primary-owned snapshot to secondary iterators. - Continues exercising auto_refresh_iterator_with_snapshot through primary iterator paths. Secondary iterators still support ordinary Iterator::Refresh(), which advances them to the latest caught-up secondary state. Snapshot-preserving automatic refresh remains unsupported. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15114 Test Plan: - python3 tools/db_crashtest_test.py - make check-sources - AUTO_CLEAN=1 make -j192 db_stress - Targeted blackbox crash test with: - test_secondary=1 - continuous_verification_interval=1000 - auto_refresh_iterator_with_snapshot=1 Reviewed By: jaykorean Differential Revision: D116381630 Pulled By: xingbowang fbshipit-source-id: a162ab357a537c2e5047c1eda85d0c2138b56ed1 |
||
|
zaidoon
|
136d0d1fa1 |
Add UDI index mode API vocabulary (#14959)
Summary: Part 2 of 13 in the UDI split. Stack order: 1. https://github.com/facebook/rocksdb/issues/14954 Add IndexFactory compatibility names 2. https://github.com/facebook/rocksdb/issues/14959 Add UDI index mode API vocabulary 3. https://github.com/facebook/rocksdb/issues/14960 Add optional UDI builder protocols 4. https://github.com/facebook/rocksdb/issues/14961 Add built-in index factory wrappers 5. https://github.com/facebook/rocksdb/issues/14969 Add built-in index factory tests 6. https://github.com/facebook/rocksdb/issues/14962 Promote IndexFactory as UDI SPI 7. https://github.com/facebook/rocksdb/issues/14963 Wire IndexFactory through block-based tables 8. https://github.com/facebook/rocksdb/issues/14964 Add IndexFactory table routing tests 9. https://github.com/facebook/rocksdb/issues/14970 Add IndexFactory parallel routing tests 10. https://github.com/facebook/rocksdb/issues/14965 Account UDI blocks as index blocks 11. https://github.com/facebook/rocksdb/issues/14966 Support parallel AddIndexEntry in trie index 12. https://github.com/facebook/rocksdb/issues/14967 Cover trie index modes 13. https://github.com/facebook/rocksdb/issues/14968 Add IndexFactory stress and benchmark flags Previous: https://github.com/facebook/rocksdb/issues/14954. Next: https://github.com/facebook/rocksdb/issues/14960. Depends on https://github.com/facebook/rocksdb/issues/14954. Until https://github.com/facebook/rocksdb/issues/14954 lands, GitHub shows the cumulative diff against main. After that lands, the intended review diff is: `zaidoon/udi-01-index-factory-compat-shim..zaidoon/udi-02-index-mode-api` That final review delta is 4 files with 52 insertions. What changed: - Add inert IndexMode and ReadIndex vocabulary to the public options headers. - Block those experimental enum fields from automatic C API generation until the explicit C API mapping lands later in the stack. - Keep behavior unchanged. Runtime wiring comes in later PRs. Validation: - `git diff --check` - `make check-sources` - `python3 tools/c_api_gen/verify_generated_up_to_date.py` - `AUTO_CLEAN=1 make check-c-api-gen` - `./c_test` Pull Request resolved: https://github.com/facebook/rocksdb/pull/14959 Reviewed By: jaykorean Differential Revision: D116211178 Pulled By: xingbowang fbshipit-source-id: 42edd3f0991d90b0b5c90232e5023de6e3740dac |
||
|
Anand Ananthabhotla
|
49faa51e11 |
Fix crash test Restore() failure with empty trace when replay_write_ops == 0 (#14840)
Summary: `ExpectedState::Restore()` always attempted to open and replay the post-`saved_seqno_` trace. In the crash-test failure, recovery had already returned the DB exactly to `saved_seqno_`, so `replay_write_ops == 0` and the trace was legitimately empty. `FileTraceReader` reported `Status::Incomplete()` on that empty trace, which incorrectly failed restore. Skip the trace replay work when `replay_write_ops == 0`, but still run the normal state promotion, cleanup, and `saved_seqno_` reset path so later saves continue to work. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14840 Reviewed By: xingbowang Differential Revision: D107968797 fbshipit-source-id: b4024e1ff329aa3bd47f0307e67d9a4f224b66d4 |
||
|
Anand Ananthabhotla
|
7d18ffe316 |
Ignore OPTIONS files for manifest recovery advance (#15106)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15106 When `optimize_manifest_for_recovery` is enabled, `MaybeUpdateNextFileNumber` uses on-disk files to decide whether recovery must append a `SetNextFile` edit. `OPTIONS-*` and temp options files are written as sidecar option snapshots during open, after recovery, so their presence alone should not force MANIFEST growth on a later clean reopen. Exclude `kOptionsFile` and `kTempFile` from the trigger that requires a MANIFEST file-number advance. If any real file requires an advance, the code still advances past the largest observed file number, including OPTIONS files. Also tighten db_stress strict MANIFEST verification so fatal zero-growth checks only run when all fault injection is disabled. Open write/sync fault injection can legitimately make `reuse_manifest_on_open` fall back to a fresh MANIFEST. Reviewed By: xingbowang Differential Revision: D116092705 fbshipit-source-id: 65b57b0b7f57b2cef42efc4df353fb2218688d34 |
||
|
Xingbo Wang
|
db1adf47bb |
Add bootstrap configuration for RocksDB external tables
Summary: Add an opaque external_table_config option to the external table adapter. RocksDB persists the string in OPTIONS files and passes it to ExternalTableFactory::Configure during bootstrap, allowing implementations such as Nimble to parse implementation-specific settings without adding RocksDB-specific fields. The option is immutable after DB open, and attempted runtime updates fail safely for non-clonable table factories. A reopen test verifies that callers can supply a different valid configuration between DB opens. Initialize the adapter option metadata lazily as const to avoid namespace-scope dynamic initialization and mutable-global lint issues. PR https://github.com/facebook/rocksdb/pull/15119 Reviewed By: anand1976 Differential Revision: D116362444 fbshipit-source-id: 225a4aedc455f320b5c90ac2f0862917da477ccb |
||
|
Xingbo Wang
|
bbdcd2825f |
Add remote storage liveness crash test signal (#15102)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15102 Add a dedicated Skycastle signal that invokes liveness through the existing remote-storage test-driver path. For remote DB runs, double default wrapper/no-progress timeouts while preserving explicit overrides. Default remote liveness `expected_values_dir` to local storage so `UniqueIdVerifier` never treats a remote DB path as a local path; local liveness behavior remains unchanged. Normalize both equals and spaced `--env_uri`/`--fs_uri` forms once so remote detection, workload execution, and cleanup share the same arguments. Track the crash harness and internal release scripts as CI source triggers, scoped so release-script changes select only the remote-storage signal. Add command-level regression coverage and failure-safe expected-values cleanup. Use generic remote DB/storage terminology throughout public RocksDB code and comments. Reviewed By: pdillinger Differential Revision: D115936164 fbshipit-source-id: e4997bf19eec18e4914626d479690b78c6cd3ad2 |
||
|
Peter Dillinger
|
495c21dc5c |
Unify lazy wide-column blob read verify policy; tool coverage (#15115)
Summary:
Follow-ups to the lazy wide-column read API (DB::GetEntityLazy / MultiGetEntityLazy). Three related, low-risk changes under one theme; no public API change.
1. Internal read-API cleanup (retires TODO(lazy-blob-resolution-cleanup)). The lazy byte-range work had left a whole/range/force-verify method fan-out on the blob read path: VersionBlobFetcherBase::FetchBlobRange plus a separate FetchBlobForceVerify, and SameFileBlobReader::GetSameFileBlob paired with GetSameFileBlobRange, with the (verify_checksums, force_verify) decision duplicated across branches. This folds them into a single 3-valued BlobVerifyPolicy (kVerifyIfPresent / kVerifyIfNoAmplification / kSkip, added to db/blob/blob_constants.h) derived once at the resolver boundary and threaded down. FetchBlobRange and GetSameFileBlob now take a range descriptor (range_length == kWholeBlobLength means the whole value) plus the policy; FetchBlobForceVerify and the *Range twins are removed. The public LazyColumnReadRequest::force_verify bool is unchanged. The widely-called lower-layer whole-value primitives (BlobFileReader / BlobSource / Version GetBlob) are intentionally left as delegated primitives to bound blast radius; folding those is a later cleanup.
2. Embedded block-cache-tier quirk fix (retires TODO(lazy-blob-resolution-quirks)). ValidateEmbeddedBlobIndex rejected read_tier == kBlockCacheTier before the blob-cache probe, so a same-file (embedded) column already in the blob cache returned Incomplete under kBlockCacheTier -- unlike the separate-file path, which probes the cache first. The up-front rejection moves to the two no-cache paths (ResolveEmbeddedBlob / ResolveEmbeddedBlobPinned); the cache-probing paths now serve a cached record under kBlockCacheTier, matching separate-file behavior. This is a shared lower-layer change: the non-lazy Get / GetEntity and iterator embedded read paths (get_context / MaybeResolveEmbeddedValue -> ResolveEmbeddedBlobCached) get the same corrected behavior, not just the lazy resolver.
3. Coverage. db_bench gains a readrandomentitylazy benchmark (GetEntityLazy + MultiResolve of --lazy_entity_read_length bytes per column) for measuring byte-range (partial) blob read savings; it requires open_files == -1. The crash test now also fuzzes LazyColumnReadRequest::force_verify in the lazy-vs-eager differential (force_verify affects only verification, not the returned bytes, so the comparison is unchanged).
This also sets up the next planned step -- cross-key coalescing for MultiGetEntityLazy / LazyWideColumnsBatch::MultiResolve. The internal BlobReadRequest already carries a {offset, len} range descriptor, and with the verify decision now a single BlobVerifyPolicy rather than a bool plus a separate force-verify method, a batched resolve can group a vector of range+policy requests per (CF, Version, blob file) and coalesce them onto the existing MultiGetBlob machinery -- without re-introducing whole/range/force-verify method twins in the multi-read path.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/15115
Test Plan:
- New unit test DBLazyEntityTest.EmbeddedBlockCacheTierServedFromCachedValue: a cached embedded column serves whole- and sub-range reads under kBlockCacheTier instead of returning Incomplete. Existing force_verify / BlockCacheTier tests are unchanged (public behavior preserved).
- New unit test DBBlobIndexTest.EmbeddedBlobWideColumnGetEntityBlockCacheTierServesCached: covers the non-lazy GetEntity path -- a cached embedded column is served under kBlockCacheTier; the existing ...GetEntityBlockCacheTier test still asserts a genuine miss yields Incomplete.
- Crash test: extended the existing lazy differential to fuzz force_verify (reuses --lazy_entity_read_one_in).
- db_bench: readrandomentitylazy over 2000 entities x 2 uncompressed embedded blob columns (4000 B each), reading 100 B per column:
| metric | value |
|---|---|
| rocksdb.blobdb.lazy.partial.read.count | 4000 |
| rocksdb.blobdb.lazy.read.bytes | 400000 |
| rocksdb.blobdb.lazy.partial.bytes.saved | 15600000 |
i.e. 100 B read per column instead of the 4000 B whole-column value.
Reviewed By: xingbowang
Differential Revision: D116334969
Pulled By: pdillinger
fbshipit-source-id: cebe90456b23be0f5be60cdc4bff85c8a37b5442
|
||
|
Anand Ananthabhotla
|
575b6df1b8 |
Force immediate creation of a new MANIFEST if the current one cannot be reused (#15103)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15103 The crash artifact showed `MANIFEST-000005` was physically well-formed but contained an incomplete atomic group that had been tolerated at EOF. `reuse_manifest_on_open` then appended ordinary records after that tail, turning the tolerated crash tail into mid-file `corrupted atomic group` corruption seen by OpenAndCompact and backup restore. Record whether MANIFEST recovery ended with a pending atomic group. When that marker is set, skip `ReopenManifestForAppend()` so the next manifest write creates a fresh MANIFEST instead of appending after the partial group. The existing physical-size guard remains as a separate torn-tail defense. This replaces the earlier keep-size preallocation hypothesis with the artifact-backed root cause. Reviewed By: xingbowang Differential Revision: D116062618 fbshipit-source-id: fb55ce2891a6fd50fa2c670dc720e91bad27e94d |
||
|
Peter Dillinger
|
f0c1e3d7d9 |
Fix cleanup-on-failure and flaky liveness check in point_lock_manager_stress_test (#15116)
Summary: The point_lock_manager_stress_test (via PointLockValidationTestRunner) was seen flaky under USE_CLANG=1 COMPILE_WITH_ASAN=1 COMPILE_WITH_UBSAN=1, manifesting as a heap-use-after-free plus "terminate called without an active exception" / SIGABRT. There were two distinct problems. Cleanup on failure: run() uses gtest ASSERT_* macros, which return early from run() on failure. That early return skipped the shutdown flag set and the join of the worker threads, so the worker threads kept running and accessed members of the (now destroyed) test runner object (heap-use-after-free), and destroying the still-joinable threads_ vector called std::terminate. Fixed by adding a destructor plus a StopAndJoinThreads() helper that always signals shutdown and joins the worker threads before any member is destroyed, no matter how run() exits. Copy/move are deleted since the object owns worker threads. Underlying flaky failure: the per-second "No locks were acquired in the last 1 second" liveness assertion was too strict. Under sanitizer builds combined with very short lock timeout/expiration and post-acquisition sleeps, all threads can occasionally fail to acquire a single lock within a given one-second window without the lock manager actually being stuck. The per-thread variant of this check was already disabled for this reason; the aggregate one is now relaxed to tolerate a few consecutive no-progress seconds, which still reliably detects a genuinely stuck (dead/livelocked) lock manager. Also fixed the end-of-run check from `>= 0` (always true) to `> 0` so it actually catches a fully stuck manager. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15116 Test Plan: Verified the cleanup path by temporarily forcing the liveness assertion to fail while worker threads were active: the test now reports a normal gtest failure with clean teardown, with no heap-use-after-free and no std::terminate/SIGABRT. I don't have a consistent reproducer for the failure but ran * 3x `USE_CLANG=1 COMPILE_WITH_ASAN=1 COMPILE_WITH_UBSAN=1 make check` * `USE_CLANG=1 COMPILE_WITH_ASAN=1 COMPILE_WITH_UBSAN=1 rocksptest point_lock_manager_stress_test -r100` Reviewed By: xingbowang Differential Revision: D116349420 Pulled By: pdillinger fbshipit-source-id: 104784c5c59770414224ac4751eabf69f65e0958 |
||
|
Andrei Preda
|
b616b203b8 |
Drop flushed WAL data from the secondary's active memtable (#15066)
Summary: Fix https://github.com/facebook/rocksdb/issues/15051: a secondary serves stale reads because entries replayed from a WAL the primary has since flushed are never evicted from the active memtable. During TryCatchUpWithPrimary(), seal the active memtable once the installed Version provably covers everything it holds, so the following RemoveOldMemTables() call drops it. Every memtable-dropping decision takes that same watermark, so a file set the secondary cannot read never turns the stale read into a vanished key. ## Why and How A secondary replays the primary's WALs into its active memtable and stops replaying a WAL once the primary flushes it. Entries already replayed stayed behind, and since reads consult the memtable first, they shadowed the newer flushed values indefinitely. Iterators go stale too once bottommost compaction rewrites the flushed entry's sequence number to 0; until then Get() and an iterator disagree. Seal the active memtable during catch-up, gated on the installed Version: each point-in-time Version now carries the column family's log number it reflects, exposed via the new ReactiveVersionSet::GetInstalledVersionLogNumber(). Sealing requires cf_id_to_current_log_[cfd->GetID()] < that number, which guarantees the primary flushed everything the memtable holds (unflushed WAL data is kept) and that the flushed data is readable through the installed Version. The gate asks the Version directly because proxies fail: the log number alone advances even when no Version was installed, and "no missing files" misses atomic_flush groups parked by a sibling's missing file and corrupt-but-present files with verify_sst_unique_id_in_manifest=false. In those cases the memtable holds the only readable copy, so dropping it would turn a stale read into a vanished key; the gate fails closed and keeps it. Reconcile every initialized column family, not just cfds_changed: a round that failed after advancing the log number leaves a stale memtable that later rounds see no new MANIFEST record for. It can also leave a collectible immutable memtable, so the loop consults the new O(1) MemTableList::HasOldMemTablesToRemove() before skipping a column family. The predicate keeps the skip precise: reconciling any column family with a non-empty immutable list would be correct but wasteful, installing a fresh SuperVersion every catch-up call for memtables legitimately waiting on a primary flush and costing readers a mutex acquisition each time. With it, no extra super versions are installed in the steady state. That predicate and the RemoveOldMemTables() cutoff take the installed Version's log number as well, not cfd->GetLogNumber(): that advances as soon as the flush record is read, so an immutable memtable sealed on a WAL switch could otherwise be freed while the flushed file superseding it is unreadable. A sealed memtable is stamped with cf_id_to_current_log_[cfd->GetID()] + 1, the log number following its contents. cf_id_to_current_log_ is keyed by column family id rather than ColumnFamilyData*, because ids are never reused while a dropped column family's pointer can be recycled, and it is recorded before WriteBatchInternal::InsertInto() rather than after: a batch stops at its first failure with earlier entries already inserted, so recording afterwards could leave the map naming an older WAL than the memtable holds. While the watermark lags and uncollectible memtables accumulate, the secondary warns, since it has no write path to enforce max_write_buffer_number. The seal sequence, shared with RecoverLogFiles() and the follower's TryCatchUpWithLeader(), is extracted into DBImplSecondary::SealActiveMemtable(). The follower is refactored onto it with no behaviour change: it keeps cfd->GetLogNumber() as its cutoff because that catch-up path never replays WALs, so its memtables hold nothing that only a not-yet-installed Version could make readable. ## Testing New tests in db_secondary_test cover: the stale read via both Get() and an iterator, before and after bottommost compaction; unflushed WAL-only data surviving sealing, including two column families sharing a WAL; no data loss with a missing, atomic_flush-sibling-missing, or corrupt flushed file; an immutable memtable sealed on a WAL switch surviving an unreadable flushed file; a batch rejected partway through replay leaving the recorded WAL no older than the memtable's contents; and reconciliation after a failed round (injected via FaultInjectionTestFS at a sync point). Each mechanism was individually neutralized and confirmed to fail its test. Verified with build_tools/rocksptest.sh over db_secondary_test, db_follower_test, memtable_list_test, version_set_test and version_edit_test (all pass, also under ASSERT_STATUS_CHECKED=1), plus db_basic_test, corruption_test, repair_test and db_flush_test; the secondary tests also ran 20x under COERCE_CONTEXT_SWITCH=1 with no flakes. make format-auto and make check-sources are clean. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15066 Reviewed By: jaykorean, anand1976 Differential Revision: D115317523 Pulled By: xingbowang fbshipit-source-id: a1d4615eb951d75e69ed242731336485c0dfa137 |
||
|
Anand Ananthabhotla
|
92a6308300 |
Verify compressed block bytes after checksum computation (#15098)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15098 Compute the base block checksum in `CompressAndVerifyBlock()` before `verify_compression` decompresses the compressed output. Parallel compression stores that base checksum on the block rep so worker threads can keep the CPU-heavy checksum and verification work off the ordered write path; the writer only applies the offset context modifier before writing the trailer. Update the regression test to cover both serial and parallel compression paths. Reviewed By: pdillinger Differential Revision: D115950785 fbshipit-source-id: b5e47f7d7c41680e7e7907443fc805bc0d043d6e |