mirror of
https://github.com/facebook/rocksdb.git
synced 2026-10-23 08:09:02 +03:00
main
992 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
cm-sally-mate
|
94c7f76da0 |
Keep read-path tombstone conversion from deleting ingested files an older iterator cannot see (#15461)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15461 **What:** With `min_tombstones_for_range_conversion` set, an iterator whose view predates an ingestion can delete the ingested keys for good. **Why it matters:** Silent, permanent data loss. `Get` returns the ingested key right after ingestion. After one scan from the older iterator, `Get` returns NotFound, also after flush and reopen. Every MyRocks bulk load with the option set takes this path: MyRocks ingests with `allow_global_seqno=false` and `snapshot_consistency=false` (seqno 0). **Root cause:** - The older iterator does not see the ingested file. It sees the deleted keys on both sides of the file as one run, and inserts a range tombstone at its read sequence number. The tombstone covers the ingested keys. - `ingest_seqno_barrier_` (D102044512) refuses a conversion only when the iterator's sequence number is below the ingested seqno. That misses two cases: 1. The file gets no new seqno: no overlap (seqno 0), `ingest_behind`, or `allow_db_generated_files`. Ingestion does not move the barrier at all (`assigned > 0`). 2. `NewIterator()` references the SuperVersion before it reads the sequence number. If an ingestion and a write happen in between, the iterator reads at a sequence number above the ingested file, with a SuperVersion that does not have it. This also loses files ingested with a new seqno. - Ingestions that flush the memtable are safe: the older iterator's memtable becomes immutable. **Fix:** Barrier on the Version, not only on the sequence number. - After `LogAndApply()`, ingestion records the new Version number on the active memtable (`ingest_version_barrier_`), while it still holds the per-CF `ingest_sst_lock`. - `DBIter` passes the number of the Version it reads. `AddLogicallyRedundantRangeTombstone()` refuses the conversion if that number is below the barrier. - Version numbers only go up, and every later Version of the column family has the ingested files. So the check is true exactly when the iterator cannot see an ingestion into this memtable. Iterators created after the ingestion convert as before. - The seqno barrier stays: it still covers old explicit snapshots that read a newer Version. **Related:** D126796014 (`Disable tombstone conversion for kMemtableTier iterators`) fixes a different trigger of the same feature. Reviewed By: joshkang97, archang19 Differential Revision: D126875720 fbshipit-source-id: 5767b866c6174d791f9ddae78311816300ac49f3 |
||
Yoshinori Matsunobu
|
d6a7847d37 |
Add a DB option to log long-held global mutexes and release sites (#15436)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15436 ## Why Long-held global mutexes can stall foreground and background work. Existing mutex statistics measure acquisition waits, not continuous ownership. This adds opt-in logging to identify the mutex, owning thread, operation and release site. ## Enable for an investigation Provide an INFO logger and shared `Statistics` object when opening the DB, then: - Set the shared statistics level to `StatsLevel::kAll`. - Set `mutex_hold_log_threshold_us` through `DB::SetDBOptions`; `100000` means 100 ms. - Restore threshold 0 and the previous statistics level afterward. The threshold defaults to **0 (disabled)**. `kAll` alone does not enable these hold probes; threshold 0 leaves existing statistics timers unchanged. ## Output Illustrative records, not production observations: ``` SLOW_GLOBAL_MUTEX mutex=DBImpl::mutex_ mutex_addr=0x12340000 owner_thread_id=12345 thread_operation=Compaction hold_us=200000 threshold_us=100000 observed_end_us=1792205400000000 buffer_dropped_records=0 unlock=db_impl_compaction_flush.cc:4660 BackgroundCallCompaction() SLOW_GLOBAL_MUTEX mutex=DBImpl::wal_write_mutex_ mutex_addr=0x12340100 owner_thread_id=12346 thread_operation= hold_us=35000 threshold_us=25000 observed_end_us=1792205401000000 buffer_dropped_records=0 scope_exit=db_impl_write.cc:2250 PreprocessWrite() ``` The first proves a completed 200 ms DB-mutex ownership interval on a thread tagged `Compaction`. The second uses a 25 ms threshold; an empty operation tag means none was available. Neither record reports acquisition wait or proves the cause of a stall. Release attribution uses compiler-embedded strings available in optimized, stripped builds, with a GCC/Clang fallback where standard source locations are unavailable. Direct unlock and condition wait identify their call sites; scope exit and temporary unlock identify guard declarations, not closing braces. No stack unwinding or symbol lookup. ## Scope and safety - Track six selected mutexes: DB, WAL, point-lock wait graph, two prepare-log bookkeeping mutexes and transaction name map. This is not comprehensive synchronization tracing. - Acquisition waits and condition-variable release gaps are excluded from hold duration. Configuration is pre-first-use; repeated setup retains the first valid context. - Sample at most once per mutex per second, with eight deferred records per thread. `buffer_dropped_records` counts capacity overflow only; rate limiting, pause and emission contention can omit samples. - Emit after covered outer lock scopes release. Emission is nonblocking best-effort; shutdown drains/pauses emission and stops diagnostics before the owned logger closes. Closing from that logger's own callback is unsupported. - No data-file format change. The mutable option is persisted in OPTIONS and wired into bench/stress tools. Typed C binding is deferred; string DB-option APIs are available. Disabled bookkeeping still has a cost. Release A/B measurements found about 3 ns added per selected mutex pair at level 1, and about 70 ns for hold timing over `kAll`/0. Get and reused-iterator Seek showed no resolved small probe effect; fresh iterator+Seek at 16 threads showed +8.7% CPU/query and -9.4% QPS. No logs were emitted in those benchmarks; logging cost and broader workload qualification remain open. Reviewed By: xingbowang Differential Revision: D125790955 fbshipit-source-id: 1096163f3bd25ffee585dc6ec8ebfd7f1759851a |
||
Peter Dillinger
|
539532377d |
Remove Solaris support (#15449)
Summary: Solaris support was contributed by outside users starting with |
||
generatedunixname3846135475516776
|
61831d47be |
Rocksdb Crash Test failed: stderr: Failure: latest seqno changed from <N> to <N> with W (#15433)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15433 Reviewed By: jaykorean, xingbowang Differential Revision: D125887282 fbshipit-source-id: 611491c1ece2c6e3bc74f22545bd5d93467cf9b9 |
||
Anand Ananthabhotla
|
a3b0f187ff |
Add FileSystem::IsPreallocationSupported() to skip WAL truncation on open (#15395)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15395 **Why the change is required:** During `DB::Open`, RocksDB reopens the last WAL file and truncates it to its logical size. This is done even when every column family's log number has already advanced past that WAL, so the file is obsolete and will be deleted later in the same open. The only purpose of the truncation is to release space preallocated beyond the logical end of file (e.g. by `fallocate` with `FALLOC_FL_KEEP_SIZE`); `GetFileSize()` reports the apparent size, so truncating to it can only free preallocated blocks. On a FileSystem that never preallocates, this work is pure overhead: a `GetFileSize`, a `ReopenWritableFile`, a no-op `Truncate` (the default `FSWritableFile::Truncate` returns OK without doing anything), and a `Close` on every open. On remote or append-oriented FileSystems the reopen can be expensive. **Previous behavior:** - If recovery flushed all data, or the WALs contained no data (including when all WALs are older than the min log number to keep), `MaybeFlushFinalMemtableOrRestoreActiveLogFiles()` always called `GetLogSizeAndMaybeTruncate()` on the last WAL. In read-only mode it still issued a `GetFileSize()` whose result was discarded. - With `avoid_flush_during_recovery`, `RestoreAliveLogFiles()` always reopened and truncated the last live WAL. **New behavior:** A FileSystem can report that it never preallocates space by returning false from the new `FileSystem::IsPreallocationSupported()`. When it does, DB open skips reopening and truncating the WAL. In the flushed / no-data path it also skips the unused `GetFileSize()`, so no I/O is issued against the old WAL. The read-only no-data path no longer issues the discarded `GetFileSize()` either. **Implementation:** - `include/rocksdb/file_system.h`: new `virtual bool IsPreallocationSupported()`, defaulting to `true`. `FileSystemWrapper` forwards it to the target. - The policy lives in the two callers in `db_impl_open.cc`; `GetLogSizeAndMaybeTruncate()` is unchanged. `RestoreAliveLogFiles()` still calls `GetFileSize()` because the size is needed for `alive_wal_files_` and `wals_total_size_`. **Safety, compatibility, and limitations:** - The default is `true`, so every existing FileSystem, including ones that override `SupportedOps()` without calling the base class, keeps the current behavior. This is why it is a separate virtual function instead of an opt-in `FSSupportedOps` bit, which would silently disable truncation for those FileSystems. - No built-in FileSystem overrides the default: Posix, Windows, `MockFileSystem`, and `LegacyFileSystemWrapper` all report `true`, so their WAL truncation behavior is unchanged. Posix deliberately reports `true` even in builds without `ROCKSDB_FALLOCATE_PRESENT`: WALs may have been preallocated by a different build running on the same machine, and a build-dependent answer would leave that space unreclaimed. - The function describes what the FileSystem can do, not per-file options such as `allow_fallocate` or build configuration. A WAL may have been preallocated by an earlier process with different options, so gating on the current options could leak space. - A FileSystem that wrongly returns false can only leak preallocated space, in archived WALs (`WAL_ttl_seconds` / `WAL_size_limit_MB`) or across a crash loop before obsolete WALs are deleted. Data is unaffected. Reviewed By: joshkang97 Differential Revision: D125049741 fbshipit-source-id: ac9795f2045c46911cd86b8faacef5aa49043496 |
||
Josh Kang
|
014e1c43c1 |
Apply readability fixes (isolate decls, braces) (#15394)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15394 ## Summary RocksDB's `.clang-tidy` enables several readability checks that existing code still violates: `readability-isolate-declaration` (476 findings), `readability-braces-around-statements` (49), `readability-redundant-string-init` (25), `modernize-use-using` (10), `misc-definitions-in-headers` (3) and `readability-duplicate-include` (1). They keep getting flagged whenever nearby lines change. This diff fixes 553 of the 564; upstream has since fixed the other 11. What changed: - Multi-variable declarations such as `std::string a, b;` are split into one declaration per variable. Pointer and reference declarators are kept per variable. - Braces are added around single-statement `if`/`else`/`for`/`while` bodies, and empty spin loops get an empty `{}` body. - `std::string s = ""` / `member("")` initializers are removed. - Remaining `typedef`s in the touched C++ files become `using`. C headers and `extern "C"` code are untouched. - `inline` is added to non-template function definitions in test headers. - A duplicate `#include "util/udt_util.h"` in `db_impl.cc` is removed. These are clang-tidy's own fix-its, applied mechanically and then clang-formatted on the changed lines. Three kinds of broken fix-it output were repaired by hand. Removing a `member("")` initializer left a dangling comma in constructor initializer lists (`ExternalSstFileInfo`, `ColumnFamilyMetaData` and others). The function-type typedefs in `block_test.cc` came out malformed. Empty `while (cond);` spin loops got a `{;` body. Splitting declarations also exposed two vectors in `db_etc2_test.cc` to `performance-inefficient-vector-operation`, so a `reserve()` was added for each. The lint bot reports every finding in a file a diff touches. This diff splits declarations in `options/configurable_test.cc`, so it also fixes that file's 14 older `cert-err58-cpp` and `cppcoreguidelines-avoid-non-const-global-variables` findings, which come from seven mutable file-level maps. Six are now `const` and built where they are used (function-local statics, or locals in their only user). The unused `imm_struct_option_info` is deleted. Compatibility: no behavior change. In public headers, the only edits are removed redundant `""` initializers, split declarations and added braces. Reviewed By: rban1 Differential Revision: D123186310 fbshipit-source-id: f9c8e88b00e997db0196f8725c7c8d880cf798da |
||
|
Josh Kang
|
4b983da055 |
Spell out special member functions (rule of five) (#15387)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15387 ## Summary RocksDB's `.clang-tidy` enables `cppcoreguidelines-special-member-functions`, `cppcoreguidelines-virtual-class-destructor` and the noexcept-move checks, and clang warns `-Wdeprecated-copy-with-user-provided-dtor`. A full scan reports 476 findings. In most of them, a class declares a destructor (or copy operations) but leaves the rest of copy/move implicit. That has two costs. Readers can't tell whether a class may be copied or moved. And resource-owning classes keep an implicit copy that would double-free if anyone ever used it. This diff resolves 453 of the 476. Approach, picked per class from least to most invasive: 1. An empty user-provided destructor becomes `= default`. This covers most interface classes, such as `virtual ~X() {}` -> `= default`. If the class is still copied, that would only rename the deprecated-copy warning, so the destructor is removed instead when nothing needs it (`Customizable`, `TableFactory` and `InternalKeyComparator` inherit a virtual one; `IteratorWrapperBase` isn't polymorphic), and `MemTableRep::KeyComparator` gets defaulted copy and move. 2. Classes whose copy is already deleted get explicitly deleted move operations too. Move was already unavailable, so this only documents it. 3. Resource-owning classes that are never copied get deleted copy and move operations. Examples: the C API wrapper structs, which run a `destructor_` callback (edited in `tools/c_api_gen/c_base.cc` and regenerated into `db/c.cc`); `ShardedCache`, which frees its shards; iterator wrappers; and RAII handles. 4. Value types that are copied or moved get the missing operations as `= default`, which keeps their current behavior. 5. Exception specifications of existing functions are unchanged, except for `WriteBatch`'s move assignment (below): no other `noexcept` is added or removed. Only the move operations this diff newly declares (`Comparator`, `FileSampledStats` and the thread-local `Entry`) are `noexcept`; they copy only scalars or atomics. `WriteBatch`'s copy and move assignment destroyed `*this` and placement-new'ed a new `WriteBatch` over it. That is undefined behavior when `*this` is a subclass, and if the copy constructor threw, `*this` was left destroyed and was later destroyed again. Move assignment now assigns member-wise and ends in the same state as the move constructor, which leaves `has_key_with_ts_`, the timestamp-size tracking and the other fields it doesn't copy at their defaults. `rep_` is swapped through a temporary, because a plain string move-assignment can keep the old buffer (e.g. a large batch reset with `batch = WriteBatch()` would keep its memory). Nothing in it can throw, so it is now `noexcept`. Copy assignment copies into a temporary and moves it in, so a throwing copy leaves `*this` unchanged. Bugs this exposed and fixed: - `RestoreAfterCopyOrCreateWorkItem` (backup engine): the hand-written move constructor and assignment moved only some fields, so `from_file` and `to_file` were empty after `push_back(std::move(...))`. A restore that hit a checksum mismatch then reported "While restoring -> : ..." without file names. It now uses the compiler-generated moves, which are still `noexcept`. - `ExternalSstFileIngestionJob` lived in a `std::vector`, and its destructor frees raw-owned pointers, so the implicit copy used on vector reallocation would double-free. Its file batches also point into its own inline file storage, so neither a copy nor a move is safe after `Prepare()`. Copy and move are now deleted, and `DBImpl` holds jobs through `std::unique_ptr`. Compatibility: no public class loses an operation users can call. Public interfaces mostly only get `virtual ~X() = default;`. `WriteBatch`'s move assignment becomes `noexcept`, which is not part of the mangled name, so the ABI doesn't change. `SubcompactionJobInfo` and `CompactionJobInfo` intentionally stay C++20 aggregates, so users' aggregate and designated initializers keep compiling. `Configurable`, `CompareInterface`, `WriteBatchBase`, `ObjectLibrary::Entry` and `MemTableRep::KeyComparator` declare defaulted copy and move, and `Customizable` no longer declares a destructor. As a result, `Configurable` and `Customizable` subclasses that declare no destructor or copy operations of their own now move their registered-options list instead of copying it. In-tree, these are the mutable-options adapters, the table factories, the memory allocators and test classes; all are held through smart pointers or used in place, and `Clone()` copies, so none is moved by value. Not changed (23): `IODebugContext` (custom deep copy plus a `std::shared_mutex`), `DBWithColumnFamilies` in db_bench (stored in a `std::vector` with non-movable members), `Snapshot`'s protected virtual destructor (intended), `ManagedSnapshot`, the `SubcompactionJobInfo` and `CompactionJobInfo` aggregates, and the 16 noexcept-move-constructor and noexcept-swap findings on existing functions, including `autovector`'s move assignment (adding `noexcept` would change their exception specifications). Copying `ManagedSnapshot` already double-releases the snapshot. Fixing that needs a real move implementation and is a behavior change, so it is left for a separate diff. Reviewed By: rban1 Differential Revision: D123186309 fbshipit-source-id: 6f8e57e233e12199a007407130bb9a1f6d52e8d1 |
||
Xingbo Wang
|
baa4d373a6 |
Commit authoritative OPTIONS file numbers in MANIFEST (#15305)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15305 Why the change is required: Column-family topology is durable in MANIFEST, while its operational configuration snapshot is stored in a separate OPTIONS file. CreateColumnFamily and DropColumnFamily previously committed MANIFEST first and wrote OPTIONS afterward. An OPTIONS I/O failure could therefore make the API report failure after the topology change was already durable. Persisted-options readers also selected the highest file number and could consume an orphan left by a failed publication. Previous behavior: RocksDB had no durable correspondence between a MANIFEST state and the OPTIONS snapshot describing it. LoadLatestOptions and GetLatestOptionsFileName treated the highest-numbered OPTIONS file as authoritative. Column-family creation and deletion returned the later OPTIONS persistence error even when their MANIFEST edit had already succeeded. Multi-column-family operations wrote a complete OPTIONS snapshot after every successful member, producing quadratic OPTIONS bytes. Ordinary DB::Open consumes caller-supplied DB and column-family options rather than loading OPTIONS. This change preserves that contract. New behavior: Add the default-disabled experimental DB option track_options_file_number_in_manifest. Its value is serialized into each OPTIONS file so the selection rule is self-describing and participates in the normal DB-options compatibility checks. When enabled, RocksDB first durably creates and renames OPTIONS-N and then commits N with one safely ignorable MANIFEST field. A normal OPTIONS rotation uses one MANIFEST commit. Column-family creation and deletion prewrite the proposed snapshot and commit its number together with a topology edit in the same VersionEdit. A failure before the OPTIONS file is durable changes no topology. A failure after the file is durable but before MANIFEST commit leaves an orphan that binding-aware readers ignore, while the previous committed snapshot remains authoritative. Each multi-CF create or drop writes one proposed snapshot, independent of the batch size. Create commits the new number with the first successful add; drop commits it with the last successful drop. Existing partial-success semantics can therefore leave a committed snapshot with extra CFs, but never one missing a live MANIFEST CF. Persisted-options APIs filter this safe superset against recovered MANIFEST membership. A snapshot missing a live CF is corruption. DB::Open with create_missing_column_families uses the same batched path. If the newest OPTIONS file has tracking disabled, it retains the legacy highest-numbered-file behavior, including after an old or disabled writer has run. If it has tracking enabled, readers use the recovered MANIFEST commit and reject newer tracked orphans. A missing usable commit falls back to the highest tracked snapshot for RepairDB and legacy-rewrite recovery. DB open logs that fallback through its configured logger; logger-less utility APIs remain silent. Writable open removes rejected tracked orphans when local obsolete-file deletion is enabled. Read-only open never mutates the DB directory. A DB with no recovered commit and tracking disabled takes the legacy fast path without parsing the newest OPTIONS file. When inspection is required, RocksDB reads only the tracking marker. Marker-read failures used for selection do not fail DB open; a recovered commit remains authoritative and ambiguous newer candidates are conservatively retained. The first topology change after enabling tracking first binds the current topology, so failure of the proposed change cannot make its orphan authoritative. MANIFEST failure during an ordinary tracked OPTIONS rotation also enters the existing manifest-write background-error path. Implementation: - Add the safely ignorable CommittedOptionsFileNumber VersionEdit field and carry it through recovery, MANIFEST snapshots, rollover, and subset checkpoints. - Persist the tracking marker through C++, C, and Java options, generated round-trip coverage, compatibility checking, and db_stress configuration. - Centralize binding-aware selection for DB open and public persisted-options APIs. Reuse RecoveryContext's directory listing, use a narrow marker scan, and recover live CF names while replaying MANIFEST. - Filter manifest-selected safe-superset snapshots to live MANIFEST CFs in LoadLatestOptions and compatibility verification without discarding their option maps. - Prevalidate and sanitize CF batches outside the DB mutex, write one complete snapshot while options_mutex_ serializes topology changes, and hold the DB mutex only for the existing individual MANIFEST commits. Batch missing-CF creation during DB open follows the same locking boundary. - Keep the committed snapshot live in cleanup and live-file APIs, and make subset checkpoints write an OPTIONS snapshot and filtered MANIFEST with identical membership in tracked and legacy modes. - Preserve the first partial multi-CF drop error while consuming legacy OPTIONS persistence Status, and sync the checkpoint MANIFEST directory after replacement. - Pass checkpoint helper callbacks by reference and group VersionEdit storage by alignment so the new paths add neither callback copies nor avoidable object padding. Safety boundaries and unsupported cases: - Tracking defaults to false. Disabled writers and never-tracked databases retain legacy write ordering and highest-numbered selection. - Older binaries safely ignore the new MANIFEST tag and continue choosing the highest OPTIONS number. Downgrade is readable but not crash-atomic: an old binary can consume a tracked orphan. That is the same class of OPTIONS/MANIFEST inconsistency as the previous behavior. - A rollback binary must know the serialized option name before the feature is enabled. Exact compatibility checks intentionally report a mismatch when the caller's tracking setting differs from the persisted snapshot. - A newer untracked OPTIONS file deliberately supersedes an older committed number, allowing an old or disabled writer to become authoritative without rewriting MANIFEST. - Reactive secondary and follower instances do not enforce or repair the binding. Ordinary DB::Open still uses caller-supplied options. - Writable open does not delete rejected tracked or ambiguous files when a compaction service owns OPTIONS-file retention. Read-only open performs no repair writes. - Direct consumers that scan OPTIONS filenames themselves are unchanged; the binding is enforced by RocksDB's persisted-options APIs. - If the committed snapshot is missing and only tracked candidates remain, selection intentionally falls back to the highest candidate so RepairDB output remains usable. Validation coverage and known limitations: Focused coverage exercises encoding compatibility, tracked and legacy selection, the never-tracked open fast path, marker-read failures, exact tracking-marker compatibility, activation and disable/re-enable transitions, batched create/drop and create-missing behavior, partial-batch safe supersets, OPTIONS/MANIFEST failure injection, background-error propagation, orphan cleanup and retention, read-only non-mutation, subset-checkpoint behavior, stale OPTIONS recovery, RepairDB option sanitization, and transient read-corruption reconstruction. The assertion-enabled full executable suite passes. The repository check-format and check-sources wrappers cannot run because they invoke Git inside this Sapling checkout; fbsource lint is used instead. Cross-platform CI is not part of local validation. Reviewed By: pdillinger Differential Revision: D123104845 fbshipit-source-id: 77d94c989243c3f3f9fedfa81fa364c166250303 |
||
|
Josh Kang
|
da82001d81 |
Mark overriding methods and destructors override (#15366)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15366 ## Summary RocksDB's `.clang-tidy` enables `modernize-use-override`, and clang reports `-Winconsistent-missing-destructor-override` when a class marks its other overrides `override` but not its destructor. A full scan finds 190 `modernize-use-override`, 100 `inconsistent-missing-destructor-override` and 2 `suggest-override` findings in existing code. Each one comes back whenever a nearby line is edited. They also leave classes where it isn't obvious which methods are overrides and which introduce new virtuals. This diff marks every overriding member function and destructor `override` and drops the redundant `virtual` on them. It also changes `final override` to plain `final`, following the check's default. Methods that introduce a new virtual keep `virtual`. Most edits are clang-tidy's own fix-its, applied mechanically and then clang-formatted. Compatibility: no signatures change. In public headers the only effect is `override` on existing overrides (mostly destructors, plus `Cache::Name()` and `Cache::GetPrintableOptions()`, whose base declarations are unconditional), so user code that subclasses these types is unaffected. Every touched class now marks all of its overrides consistently. This matters because a partially-marked class would make clang emit `-Winconsistent-missing-override` in user builds. Reviewed By: rban1 Differential Revision: D123186308 fbshipit-source-id: 1f489ea32d938e3ec0faf9abc4f7ff49e042d37a |
||
|
Josh Kang
|
928527b869 |
Fix bugs and false positives from clang-analyzer/bugprone (#15325)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15325 ## Summary RocksDB's `.clang-tidy` enables the `clang-analyzer-*` and `bugprone-*` checks. A full scan of the repo still reports 78 findings from these checks in existing code. Each time a nearby line is edited, they come back as noise, and a few of them are real defects. This diff resolves 73 of the 78; upstream has since fixed one more, and the four left are listed under "Intentionally not changed". Some fixes repair real bugs. The rest make the code's intent explicit to the analyzer without changing behavior. Real bugs fixed: - `port/mmap.cc`: `MemMapping::operator=(MemMapping&&)` ran the destructor on `*this`, `memcpy`'d over a non-trivially-copyable object, then placement-new'd the source. That is undefined behavior, and `*this` can be the base subobject of a `TypedMemMapping`. It now moves the members one by one, and the previous mapping is released through a local `MemMapping`. - `port/mmap.h`: `TypedMemMapping::operator=(MemMapping&&)` had no `return *this;`, so calling it was undefined behavior. - `cache/typed_cache.h`: `if constexpr (sizeof(TCreateContext) > 0)` is always true, so the branch meant for empty create contexts (for example `BlobContentsCreator`) could never run. The check is now `!std::is_empty_v<TCreateContext>`. Existing instantiations still call the same `Create` function. - `cassandra_row_merge_test.cc`: the test reused a vector after `std::move`-ing it into `RowValue::Merge(std::vector<RowValue>&&)`. That only worked because `Merge` happens not to consume its input. The test now rebuilds the input. Behavior-preserving changes: - Virtual calls in constructors and destructors are now explicitly qualified (for example `DBImpl::CloseImpl()` in `~DBImpl`). During construction and destruction, dispatch already resolved to these functions. - Stale `/*name=*/` argument comments now match the parameter names. - `unique_ptr::release()` after a successful cache `Insert` (where the cache takes ownership) uses `[[maybe_unused]] T* released = x.release();`, the pattern already used in `table_cache.cc`. - Removed `class Mutex;` and `class StaticMeta;` forward declarations that sat in the wrong namespace. - Added default member initializers for the `TablePropertiesCollectorFactory::Context` fields that its default constructor left uninitialized. - Smaller cleanups: a redundant `cond ? 3 : 3` in a test, `sizeof(std::string)` in a memory estimate, and an explicit `Cleanable` base move via `up_cast`. Tests that intentionally use moved-from objects now carry `NOLINT(bugprone-use-after-move)`, matching the existing convention. Intentionally not changed: enum casts in tests that check invalid-value handling; three `clang-analyzer-core.NullDereference` reports the analyzer can't prove safe (the C API timestamp out-parameters in `db/c.cc`, which are only written when the caller asked for a timestamp, and the waiter-list walk in `util/async_file_reader.cc`); bit-flag enums in public headers (`FileOpenContract`, `OptionTypeFlags`), where OR-combined values are valid; ownership transfers the analyzer can't see across translation units (`partitioned_index_reader.cc`, `fs_posix.cc`); and the intentional two-separator check in `file/filename.cc`. Reviewed By: rban1 Differential Revision: D123186307 fbshipit-source-id: 623e871286b1c73263b1309cb194692194199e0f |
||
Ranjan Banerjee
|
a38cd3a439 |
Bypass stalled cross-DB WBM flush candidates (#15331)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15331 ## Motivation The parent diff already implements cross-DB flushable-memory accounting and a background sorter that ranks DBs outside the write path. This change does not alter how the best candidate is selected. The remaining liveness problem occurs after the sorter selects a DB and hands it off for execution. Previously, the coordinator tracked one remote handoff as pending and waited for that DB to report completion before advancing. A selected DB could therefore monopolize coordinator progress if its job waited in Env::LOW, blocked on the DB mutex, waited behind an existing write leader or write stall, or otherwise failed to complete promptly. Other ranked DBs could be ready to reclaim memory but would not be attempted while the first handoff remained pending. ## Solution Make cross-DB flush handoff execution bounded and bypassable. Cross-DB attempts now run on a small four-worker executor owned by the WriteBufferManager instead of consuming Env::LOW compaction capacity. The executor is created only for kFlushLargestAcrossDBs and its workers are retired when that policy is disabled. The coordinator distinguishes queued and executing handoffs. A candidate lease begins when an executor worker starts the callback, rather than while the callback is merely waiting in the executor queue. If an executing handoff exceeds its lease, the coordinator releases ownership of that attempt and advances to the next ranked candidate. The original callback is not cancelled and may still complete, so the executor and outstanding-handoff count provide a strict bound on concurrent stuck work. Each registration has a per-DB outstanding-job reservation, preventing the sorter from dispatching the same DB again while an earlier callback remains alive. The coordinator also records the identity of the active candidate, so late completion from an expired attempt cannot clear or corrupt a newer candidate lease. DB mutex acquisition, write-thread joining, memtable switching, and flush dispatch continue to happen outside registry and coordinator locks. The executor invokes the existing BackgroundCallWBMFlush path directly; actual immutable-memtable flushing still uses RocksDB normal flush infrastructure. ## Compatibility This behavior is limited to kFlushLargestAcrossDBs. Existing policies retain their previous local flush behavior, and no WBM executor threads are started for them. ## Validation Unit coverage exercises lease expiry, advancement to the next candidate, late completion, duplicate suppression, bounded worker capacity, policy changes with blocked callbacks, shutdown, and reranking after progress. The end-to-end stack was also compared against the original local policy in one matched memory-pressure run with 1,000 DBs and 16 writers. The complete largest-across-DBs policy delivered 61.0% higher throughput, 55.5% fewer WBM flushes, 37.9% lower mean latency, 47.9% lower P99 latency, 5.17x larger average flushes, and 62.8% lower peak RSS. These measurements validate the full policy stack rather than isolating the performance contribution of this liveness follow-up. Reviewed By: xingbowang Differential Revision: D123461329 fbshipit-source-id: 4ad86102c2a43beb469f7672d722674dfbda9871 |
||
|
Ranjan Banerjee
|
0561153b73 |
Add configurable largest-memtable flush policies to WriteBufferManager (#15047)
Summary: Motivation A WriteBufferManager can be shared by many RocksDB instances and column families. Under the existing policy, the DB whose writer first observes memory pressure flushes its own oldest mutable memtable. That DB might own only a small fraction of the reclaimable memory, so workloads with many DBs or CFs can generate many small flushes while the largest memory consumer remains untouched. Scanning every registered DB while holding process-wide or DB mutexes would improve selection accuracy but would serialize unrelated write paths and is not viable for deployments with thousands of DBs. Previous behavior kFlushOldest was the only behavior: the write-group leader that noticed WBM pressure synchronously selected the oldest eligible mutable memtable in its own DB. The WBM had no per-DB view of reclaimable memory and could not direct reclamation to another DB sharing the same budget. New behavior This change adds configurable WriteBufferFlushPolicy modes: - kFlushOldest preserves the existing behavior and remains the default. - kFlushLargest selects the largest eligible flushable column family in the writing DB. - kFlushLargestAcrossDBs selects the DB advertising the most reclaimable memory across all DBs sharing the WBM, then flushes its largest eligible CF. Atomic-flush DBs advertise and flush the aggregate eligible memory across their CFs. High-level implementation Each writable DB registers a lightweight FlushInitiator with the shared WBM. Per-DB accounting publishes two approximate values using atomics: total mutable memory and the largest flushable CF contribution, including eligible immutable memtables waiting for flush. Allocator reporting is batched, so the allocation hot path avoids registry scans and DB mutexes. A dedicated WBM coordinator thread maintains an approximate ranked snapshot of registered DBs. Refreshes are driven by accounting changes, threshold crossings, and periodic pressure checks. Ranking and O(number of DBs) work happen on that coordinator thread; writers only update counters and signal the coordinator. Registration lifetime is protected independently so DB close can safely drain an in-flight handoff. Threading and flush handoff The coordinator reserves and pins one ranked candidate at a time, without holding a registry lock while invoking the DB. The target DB uses TryLock on its DB mutex and rejects the handoff if it is closing, paused, stopped, stalled, already handling a WBM job, or no longer has flushable memory. Rejected candidates do not block consideration of the next DB. An accepted handoff queues one LOW-priority WBM job. The LOW worker joins the target DB write thread and revalidates eligibility before switching the memtable. The resulting ordinary flush is then scheduled through RocksDB's existing flush machinery on the HIGH-priority flush pool. Keeping the write-thread join off the requesting writer avoids making one DB's write path wait on another DB's mutex or write queue. Only one manager-wide remote handoff is active at a time. Completion or cancellation clears the handoff and requests a rerank. If memory reaches the hard limit while remote coordination is delayed, a rate-limited local fallback lets a writer seal a local memtable to preserve forward progress. Write stalls, shutdown, WaitForCompact, policy changes, and DB deregistration explicitly drain or reject outstanding handoffs. Compatibility The legacy constructor and kFlushOldest behavior remain unchanged unless a new policy is explicitly selected. There are no on-disk format changes. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15047 Reviewed By: xingbowang Differential Revision: D112396106 fbshipit-source-id: 3049adc7034dd4e00d74d7d86e1d19004292d299 |
||
|
Peter Dillinger
|
1c490a4a4d |
Fix "ancester" spelling; deprecate TablePropertiesNames (#15335)
Summary: Follow-up to https://github.com/facebook/rocksdb/issues/15304, which renamed `TableProperties::creation_time` to `oldest_ancestor_time` but left the long-standing "ancester" misspelling everywhere else, so the same concept was spelled two ways (e.g. `TableProperties::oldest_ancestor_time` vs. `FileMetaData::oldest_ancester_time` and `SstFileMetaData::oldest_ancester_time`). This change makes the spelling consistent and addresses remaining review feedback on https://github.com/facebook/rocksdb/issues/15304. Change: * All internal identifiers and comments now use "ancestor": `FileMetaData::oldest_ancestor_time`, `CompactionServiceOutputFile::oldest_ancestor_time`, `kUnknownOldestAncestorTime`, `TryGetOldestAncestorTime()`, `Compaction::MinInputFileOldestAncestorTime()`, the `kOldestAncestorTime` manifest tag enum, and related locals and test sync point names. * Public API: `SstFileMetaData::oldest_ancestor_time` is the new name; `oldest_ancester_time` remains as a deprecated alias sharing storage via an anonymous union (same approach as `TableProperties::creation_time`, so size, layout, and copy semantics are unchanged). * Public API: `TablePropertiesNames::kOldestAncestorTime` is the new name; `kCreationTime` remains as a deprecated `static const std::string&` alias. A static reference member has no per-object cost and binds at constant initialization, so it adds no static-init-order exposure. * `TablePropertiesNames` as a whole is marked DEPRECATED for public use. An fbsource-wide BigGrep found no references outside RocksDB and its vendored copies, and it is only meaningful to table format internals; users should read the corresponding `TableProperties` fields. Fix: * The `table_file_creation` event log entry had the `oldest_key_time` and `newest_key_time` values swapped since https://github.com/facebook/rocksdb/issues/13083 (v10.0). Now each label is paired with its own value. Also: tools/c_api_gen/auto_simple_bindings.py resets the pending anonymous union field list after use and skips unnamed members inside the union (review suggestion); generated C API output is unchanged. Compatibility: names that are persisted or externally parsed intentionally keep the old spelling: the on-disk property name `rocksdb.creation.time`, the `"oldest_ancester_time"` key in remote compaction serialization, and the `oldest_ancester_time` / `OldestAncesterTime` labels in `VersionEdit` DebugString/DebugJSON (ldb manifest_dump output). The manifest tag value is unchanged. HISTORY.md is left as historical record. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15335 Test Plan: DBTablePropertiesTest.CreationTimeAliasesOldestAncestorTime now also verifies `TablePropertiesNames::kCreationTime` and `kOldestAncestorTime` refer to the same object, and new DBTablePropertiesTest.SstFileMetaDataOldestAncesterTimeAlias verifies the two `SstFileMetaData` names share an address and that writes through the old name are visible through the new one. Remote compaction serialization through the unchanged `"oldest_ancester_time"` key is covered by compaction_service_test; manifest encode/decode through the renamed field by version_edit_test. `make check-c-api-gen` confirms the generated C API is up to date and backward compatible. Reviewed By: jaykorean Differential Revision: D123559299 Pulled By: pdillinger fbshipit-source-id: 480106e8d539e654f31db210ef6e1de02325d604 |
||
Jay Huh
|
f16157cfc5 |
Fix ClipColumnFamily dropping the file that ends at begin_key (#15320)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15320 Why the change is required: `DB::ClipColumnFamily(cf, begin_key, end_key)` keeps `[begin_key, end_key)`. It first drops whole non-L0 files outside the range with `DeleteFilesInRanges` on `(-inf, begin_key)` and `[end_key, +inf)`, but called it with the default `include_end = true`. The first range was therefore treated as `(-inf, begin_key]`, so a non-L0 file whose largest key is `begin_key` was deleted even though that key is inside the kept range. Clipping `[b, d)` on L1 files `[a, b]` and `[c, d]` lost `b`. This showed up while smoke-testing `SplitDB` (D120757697): single-key files trivially moved to the bottom level made the split destination miss its first key. New behavior: - Pass `include_end = false`, so `DeleteFilesInRanges` skips files whose largest key equals `begin_key`. The second range has no end, so it is unaffected. The remaining keys below `begin_key` are still removed by the existing `DeleteRange` and compaction steps. Reviewed By: xingbowang Differential Revision: D123306376 fbshipit-source-id: 65cb36ae730a55750ea558a6cdcd48d8fc17f0e9 |
||
|
Josh Kang
|
c0512c4c34 |
Remove deprecated write_global_seqno ingestion support (#15287)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15287 `IngestExternalFileOptions::write_global_seqno` has been deprecated since RocksDB 8.0 and will soon be removed from the API. It requires ingestion to mutate SST files, sync them, regenerate checksums, and expose file-offset plumbing solely for that mutation. Remove the option and its C, Java, and `ldb` APIs. Assigned sequence numbers now live only in the MANIFEST, leaving newly ingested SST bytes and whole-file checksums unchanged. ## Backward Compatibility New readers continue to support existing SSTs containing a nonzero global sequence number written by legacy ingestion. Because that old write did not update the properties-block checksum, checksum verification retries against a temporary copy with only the global-seqno value restored to its original zero placeholder. The parsed sequence number and file contents remain unchanged. This fallback only accepts mismatches explained by the legacy rewrite. Other checksum failures continue through normal corruption handling, and `verify_checksums=false` continues to bypass verification. Reviewed By: xingbowang Differential Revision: D122405856 fbshipit-source-id: 934e9bc755d58ea8d84e1c085e9769015bd8dfd7 |
||
|
Yoshinori Matsunobu
|
8585299552 |
Rate-limit DestroyDB cleanup of existing trash (#15285)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15285 Why the change is required: MyRocks bulk-load v2 creates a temporary RocksDB and calls DestroyDB when the load is committed, rolled back, abandoned, or recovered. Successfully ingested SSTs are moved into the main DB, so leftover SST cleanup is primarily an error, abort, or recovery case; WALs, obsolete files, and compaction-created trash can still remain. All physical deletion performed by DestroyDB should honor the configured SstFileManager rate, including rocksdb_sst_mgr_rate_bytes_per_sec in MyRocks. Previous behavior: DestroyDB sent recognized table, blob, and WAL files through SstFileManager, but ignored filenames ending in .trash because ParseFileName rejects that suffix. The final best-effort DeleteDir then silently left the directory behind. DB::Open handled the same files through CleanupDirectory, forcing MyRocks cleanup code to discover and register trash itself. A global WaitForEmptyTrash workaround would also drain unrelated compaction, DROP TABLE, and DROP INDEX deletion work from every DB sharing the manager. New behavior: DestroyDB waits only for pending deletion work associated with each DB, data, WAL, and archive directory. It then schedules any remaining .trash file through the existing rate-limited scheduler and waits only for the private bucket created for this DestroyDB call. Directory keys are normalized for bookkeeping, so equivalent path spellings identify the same pending work while filesystem operations retain the original path. Accounting and failure handling: A failed accounted trash deletion retains its original file path and remaining trash-byte charge. If DB open or DestroyDB later discovers the file, the retry preserves the accounted classification, avoids charging the trash bytes twice, and clears SstFileManager tracked-size and trash-size accounting after successful deletion. Scope and safety: There is no new sysvar, public RocksDB API, or persisted-format change. Existing rate settings, hard-link handling, partial deletion, and separate-WAL foreground deletion remain unchanged. The bookkeeping is confined to file enqueue/completion and DestroyDB cleanup. A shared FIFO can still delay this DB behind unrelated work already ahead of its files, but unrelated work queued behind this DB no longer gates DestroyDB. Reviewed By: xingbowang Differential Revision: D122221503 fbshipit-source-id: ec106f57efa70e24827234b4173366e5dbcbda5c |
||
|
Xingbo Wang
|
866e8e9f06 |
Make full error-recovery flush atomic (#15293)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15293 Why the change is required: Full background-error recovery flushes all column families because WAL durability can be uncertain. A later recovery-flush failure or process crash must not persist different WAL replay boundaries for different column families. Otherwise reopen can recover only part of a write batch spanning those column families. The risk includes non-file-scoped WAL write or sync errors. A failed write can have reached the WAL without reaching the memtables. An earlier unsynced write can instead remain in the memtables while its WAL bytes are not durable; retrying the fsync after resetting the writer error does not by itself prove those bytes survived. Sequential recovery can make either case visible in only some column families after reopen. Previous behavior: File-scoped WAL-error recovery already set FlushOptions::force_atomic_flush. Other full kErrorRecovery paths flushed column families sequentially when DBOptions::atomic_flush was false. Each successful per-column-family flush independently advanced its MANIFEST log number, so a later flush failure or process crash could leave mismatched replay boundaries. New behavior: Every full kErrorRecovery atomically commits the flush results and WAL replay boundaries for all participating column families. A successful recovery commits every column family; a failed recovery commits none. If an atomic recovery attempt fails, all of its flush output is discarded and recreated by a later attempt instead of retaining output from column families that finished first. Implementation: Set FlushOptions::force_atomic_flush for every full error-recovery FlushAllColumnFamilies call. Remove the now-unreachable recovery-only behavior from FlushMemTable and assert that recovery does not enter the non-atomic path. Keep the two-write-queue sequence fence at the atomic memtable-switch point. Add an end-to-end fault-injection test for a retryable, non-file-scoped WAL sync error after a multi-column-family record is appended. It covers successful and failed recovery flushes, including a column family with no other memtable data, and verifies all-or-none behavior after reopen. Update the manual_wal_flush comment and add a release note. Safety, compatibility, and limitations: Only full kErrorRecovery changes. Normal, manual, shutdown, and kErrorRecoveryRetryFlush behavior is unchanged. File-scoped WAL-error recovery was already atomic. There are no API, ABI, option-default, or file-format changes. The additional work is limited to the cold recovery path. A failed atomic recovery can redo more flush work than the previous sequential path, but this avoids partial multi-column-family recovery. Automatic recovery with manual_wal_flush remains disabled as fatal pending separate analysis and testing; this change does not remove that workaround. Validation covers local Linux builds and focused recovery tests; platform CI for this version is pending. Reviewed By: pdillinger, rban1 Differential Revision: D122469648 fbshipit-source-id: f72845903d9b6bff70f5d881e02a4cb2e920ed72 |
||
|
Xingbo Wang
|
19b6f85266 |
Support async file open for SecondaryDB (#15294)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15294 Why the change is required: `open_files_async` reduces startup latency by moving SST validation and table-reader opening to background work, but `DB::OpenAsSecondary()` still performed that work synchronously. Previous behavior: A secondary using `open_files_async=true` replayed its MANIFEST through `ReactiveVersionSet`, synchronously verified and opened every current SST, and only then returned. The option therefore provided no startup benefit for secondary instances. New behavior: `DB::OpenAsSecondary()` can now return after installing its initial read view and start initial SST validation and table-reader pinning through the existing asynchronous opener. With `max_open_files=-1`, the opener processes all current SSTs in the background; a finite table cache can stop the eager pass at capacity and validate remaining files on demand. Later `TryCatchUpWithPrimary()` rounds remain synchronous. Async-open failures notify listeners and are latched according to the existing severity and `paranoid_checks` policy. If an error is not latched, a later read retries the file access and reports a persistent failure. A latched error requires fixing the underlying problem and reopening the secondary. Read-only instances do not start write-oriented recovery. Implementation: Initial reactive MANIFEST replay can defer SST verification and handler loading. The deferral is immutable constructor state on `ReactiveVersionSet`, is cleared before catch-up mode, and is restricted to normal secondary recovery. `VersionBuilder::GetAndClearIntermediateFiles()` now moves out and clears its own historical add-then-delete bookkeeping so callers cannot accidentally retain stale state. Secondary open schedules the background opener after publishing the read view. Its context pins the captured Versions and column families, including retained secondary handles whose column family was subsequently dropped by the primary. Existing table-reader opening performs SST size and unique-ID validation, avoiding a separate serial metadata pass. Closing the DB or canceling background work safely cancels a queued opener or waits for a running opener. Read-only async-open errors use an ErrorHandler path that notifies listeners without automatic recovery. Secondary async open explicitly rejects FIFO compaction. Safety, compatibility, and limitations: The default `open_files_async=false` path and existing primary async-open behavior are unchanged. WAL and blob recovery, as well as later secondary catch-up, remain synchronous. Follower DB and `OpenAndCompact()` do not use this deferral. There are no file-format, ABI, or public API signature changes. Secondary async open requires `skip_stats_update_on_db_open=true` and does not support FIFO compaction. Applications must keep files accessible by coordinating deletion with the primary or by using an Env/FileSystem that delays physical deletion until every instance releases a file. With asynchronous opening, `max_open_files=-1` cannot guarantee every SST is pinned before `OpenAsSecondary()` returns; the table-files-only `max_open_files=-1` workaround applies only when asynchronous opening is disabled. No completion API is added. Validation covers initial deferral and pinning, synchronous catch-up, table-reader detection of physical-size mismatch, FIFO rejection, listener-adjusted errors and synchronous read retry, retained handles after primary column-family deletion, close and explicit cancellation with no HIGH-priority worker, stopping new table opens during shutdown, primary async open, follower isolation, and remote compaction. Linux Buck builds and focused tests passed. Cross-platform CI is pending, and `make check-sources` cannot run successfully in this Sapling checkout because its script requires Git. Reviewed By: pdillinger Differential Revision: D122417547 fbshipit-source-id: 91d408469c35c4ea21165c5e7db75068e65c2389 |
||
|
Xingbo Wang
|
31675ca6d0 |
Recover from poisoned WAL handles on remote file systems (#15193)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15193 Keep `DB::Write`'s existing immediate `IOError` contract while allowing RocksDB to recover safely when a remote file system reports that a WAL handle is permanently unusable. The public API documentation makes the persisted-state boundary, reopen behavior, and safe post-resumption retry point explicit. Normal operation remains unchanged; the additional protocol is entered only after a supported file-scoped WAL error. ### Motivation A WAL write crosses an acknowledgement boundary: storage can accept the complete record and then fail before the caller receives confirmation. The resulting `IOError` therefore cannot tell RocksDB or the application whether the record is absent, partial, or complete. At the same time, the remote file handle can become permanently poisoned, so retrying that same handle can never succeed. The previous recovery path retried the poisoned handle. Simply replacing it is not sufficient: RocksDB must also prevent reuse of an ambiguous sequence range, keep every column family on one durable boundary, and prevent WAL tailers from publishing a physically complete record that successful live resumption deliberately abandoned. The new protocol quarantines the affected WALs, reserves the ambiguous sequence range, creates a replacement WAL, atomically persists acknowledged state, retires the quarantined handles, and only then resumes writes. ### Behavior changes #### 1. Recovering from a poisoned remote WAL handle - **Why required:** A remote file system can permanently poison a WAL handle after an I/O failure. Retrying that handle cannot succeed and leaves the database write-stopped. - **Previous behavior:** RocksDB retried the same WAL handle during recovery. - **New behavior:** The remote file system marks the failure as file-scoped. RocksDB creates a replacement WAL, atomically persists all state that could depend on the failed WAL, and then removes the quarantined WAL handles from active use. Physical file deletion remains part of normal cleanup. #### 2. Preserving an indeterminate WAL write outcome - **Why required:** A WAL record can reach storage before the append returns `IOError`; neither RocksDB nor the application can know the outcome from that error alone. - **Previous behavior:** Recovery could reuse a file-scoped failed write group's sequence numbers, allowing a later acknowledged write to overlap a record that might already be in the WAL. The public write API did not explain the acknowledgement ambiguity or distinguish live resumption from close/reopen recovery. - **New behavior:** `DB::Write` still returns the original `IOError` immediately and does not retry the logical batch. Normal DB reads can continue, but the failed write is not visible in that DB session. The persisted DB remains indeterminate with respect to the failed write until supported file-scoped live resumption succeeds without closing or reopening the DB. On reopen, or for any other WAL error without the durable-abandonment guarantee, applications must resolve consistency within RocksDB's documented guarantees, including `WALRecoveryMode`, through idempotency, deduplication, reconciliation, or an equivalent mechanism. After successful supported file-scoped live resumption, RocksDB has durably abandoned the failed batch and reserved its sequence range; retrying the same logical write is then recommended to preserve one linear logical history. #### 3. Keeping TransactionLogIterator consistent with live resumption - **Why required:** A physically complete error-returned record can remain readable from its WAL even though supported file-scoped recovery excludes that batch from the database. Returning it from `GetUpdatesSince()` would make replication or change-data-capture consumers diverge from the recovered DB. - **Previous behavior:** An iterator opened before recovery could return the abandoned batch. A new iterator could also return it while the physical WAL remained visible. - **New behavior:** For file-scoped failures, RocksDB records the exact sequence range and WAL number of each indeterminate write. For the lifetime of that open DB, existing and new WAL iterators stop with `Status::NotFound` before the range, whether the physical record is complete or absent. A caller can begin a new iterator after the reserved range. Iterator creation snapshots the recovery-gap generation together with its WAL-file view so a failure on a later, unseen WAL returns `TryAgain` for the old iterator instead of skipping its valid prefix. #### 4. Keeping all column families on one recovery boundary - **Why required:** A write batch can update multiple column families. Retiring its WAL after independently persisting only some column families can expose a partially applied batch after reopen. - **Previous behavior:** With `atomic_flush=false`, recovery could persist column families independently before abandoning the WAL. - **New behavior:** File-scoped WAL recovery force-flushes every live column family atomically and verifies that each one advanced beyond the failed WAL before writers are retired or writes resume. #### 5. Merging concurrent WAL failures - **Why required:** Another WAL write can fail while an earlier recovery attempt temporarily releases the DB mutex. - **Previous behavior:** A later failed WAL cutoff or ambiguous sequence could be overwritten or cleared by the earlier recovery attempt. - **New behavior:** Each error episode monotonically merges the largest failed WAL cutoff and the sequence range of the latest ambiguous write group. Recovery clears the quarantine cutoff only if no concurrent failure extended it; otherwise the newer failure remains write-stopping for the next attempt. Iterator fencing tracks the actual ambiguous write range separately, so an older failed `SyncWAL()` and a newer ambiguous write are not incorrectly paired. #### 6. Retaining file scope through `NoSpace` recovery - **Why required:** `NoSpace` can still identify one permanently unusable WAL handle. - **Previous behavior:** The `SstFileManager` recovery path entered generic recovery without carrying the failed WAL context. - **New behavior:** File-scoped `NoSpace` carries the same cutoff and sequence evidence through the `SstFileManager` path as other file-scoped WAL errors. #### 7. Scoping recovery context to one error episode - **Why required:** A completed recovery must not affect a later unrelated background error. - **Previous behavior:** WAL recovery context could remain latched after `ClearBGError` succeeded. - **New behavior:** Successful background-error clearing resets the context. A failed recovery retains it so a later retry has the same safety information. The separate iterator gap record remains for the lifetime of the open DB because later WAL readers must continue to reject the abandoned physical record. #### 8. Preserving successful `SyncWAL` metadata - **Why required:** A healthy WAL can finish syncing while a separate recovery error already exists. - **Previous behavior:** `SyncWAL` could return the existing background error before applying the successful WAL's `VersionEdit`, dropping its MANIFEST update. - **New behavior:** `SyncWAL` skips quarantined writers, applies metadata for successfully synced healthy WALs, and only then returns any existing recovery error. #### 9. Classifying only the intended remote-file-system failure - **Why required:** File-scoped recovery is valid only for an actual WAL `IOError` from a handle whose ownership is still valid. - **Previous behavior:** Poison state could be queried before ownership fencing was resolved, and file scope could be stamped onto a non-I/O status. - **New behavior:** Ownership fencing is resolved first, poison state is queried afterward, and only an actual `IOError` can be classified as file-scoped. `IOFenced` remains fatal. #### 10. Preserving `paranoid_checks=false` retry behavior - **Why required:** Non-paranoid mode historically cleared a writer's sticky error so a later operation could retry after storage recovered. - **Previous behavior:** Avoiding reclassification of a cached synthetic writer error also bypassed `WALIOStatusCheck`, so later `SyncWAL` calls kept returning the stale error. - **New behavior:** A cached error still fails the current `SyncWAL` call, but the non-paranoid path resets the writer for a later retry without setting a background error or entering file-scoped recovery. #### 11. Preserving async WAL precreation across recovery - **Why required:** A precreated WAL is an unpublished replacement and is not the active WAL. Recovery can also consume the prepared slot while background work is stopped. - **Previous behavior:** A file-scoped failure while creating or starting the unpublished replacement was attributed to the active WAL, unnecessarily quarantining it. After successful recovery consumed a prepared WAL, the next slot was not replenished, so the following rotation created its WAL synchronously. - **New behavior:** Only an active WAL `WriteBuffer()` failure attaches the active-WAL cutoff. A failed unpublished candidate is discarded while the current WAL remains active, and successful recovery schedules the next async precreation after `ClearBGError()` re-enables background work. ### Implementation - Carry `DBRecoverContext` through generic, `NoSpace`, and `SstFileManager` recovery, merging WAL cutoffs and failed write-group sequence ranges independently of error severity. Generic errors retain the range only so a later retry that establishes file scope can activate the stronger protocol; they do not persist a reservation or install an iterator gap on their own. - Document the WAL acknowledgement ambiguity, persisted-state boundary, reopen behavior, application consistency requirements, and post-resumption retry guidance on the public `DB::Write` API. - Publish a lock-free quarantine cutoff used by write and sync paths; replace even an empty failed current WAL; persist file-scoped ambiguous sequence reservations; force and verify an atomic all-column-family flush; and detach quarantined writers only after that durable boundary. - Record each file-scoped ambiguous write group's exact WAL number and sequence range in a DB-lifetime tracker shared by `WalManager` and all `WalIteratorImpl` instances. Stop an iterator before a batch that contains or crosses the range, and use a generation snapshot to preserve the existing `TryAgain` behavior when an iterator's fixed WAL-file view predates the failure. - Keep successful healthy-WAL tracking edits in `SyncWAL`, reset completed recovery context, and preserve legacy non-paranoid writer reset behavior. - Distinguish failures on an unpublished replacement WAL from failures flushing the active WAL. Log the failed replacement identity, keep the active WAL out of quarantine, and replenish the async-precreation slot after successful recovery. - Classify poisoned remote-file-system handles after ownership fencing and only for `IOError`. - Add lifecycle logging and a state-machine comment covering quarantine, sequence reservation, iterator fencing, replacement, atomic persistence, retirement, and concurrent-error handoff. - Add an opt-in `db_stress` mode that injects file-scoped faults only into WAL writes, disables WAL tracking in that mode, validates unsupported direct invocations, and sanitizes crash-test parameters to the supported recovery envelope. A small fraction of ordinary non-specialized crash-test runs select this mode automatically; existing stress behavior is unchanged when the flag is off. - Make the concurrent-error regression test hold the initial background flush until `SyncWAL` records the intended older-WAL error, preventing scheduler-dependent retirement of the test WAL before fault injection. ### Safety boundaries and unsupported cases - Automatic file-scoped WAL recovery fails closed with `NotSupported` for manual WAL flush, 2PC/transactions, two write queues, pipelined or unordered writes, WAL recycling, WAL TTL or size retention, either WAL-tracking mode, or blob direct writes. - Unsupported recovery leaves the DB write-stopped. `DB::Resume()` returns `NotSupported`; after correcting the I/O condition, operational recovery requires close and reopen. - The failed logical batch is never retried. Only supported file-scoped recovery durably reserves its sequence range and fences `GetUpdatesSince()`. Generic WAL errors retain the existing indeterminate outcome and can replay after close/reopen; applications must use idempotency or reconciliation unless successful file-scoped live resumption has provided the stronger guarantee. - Failed recovery attempts retain their context. Only a successful `ClearBGError` resets it. - Physical WAL deletion is not part of the recovery correctness boundary; normal obsolete-file cleanup removes files later. - The TransactionLogIterator ambiguity fence is kept for the lifetime of the DB instance that observed the file-scoped error and is not persisted as new MANIFEST metadata. After a later close/reopen, normal WAL recovery and obsolete-WAL retention/deletion semantics apply. - The in-memory iterator-gap tracker can grow with the number of recovered file-scoped failures during one DB lifetime. Safe pruning must account for retained WALs and existing iterator snapshots; it is left for follow-up. - Async WAL precreation remains disabled with WAL recycling. A prepared WAL is unpublished until its start records succeed; failure before installation does not make the active WAL file-scoped. - The new stress mode is test-only and defaults off, so normal `db_stress` and database operation without an error are unchanged. - Automatic stress-profile selection is limited to fully parsed, supported configurations. Explicit targeted runs reject options files and other unsupported combinations. ### Tests and known limitations - The affected recovery, WAL-manager, WAL-iterator, fault-injection, async-WAL-precreation, and secondary-close binaries build successfully. - Checked-status suites pass for all 70 `error_handler_fs_test` cases, all 7 `wal_manager_test` cases, all 11 `db_log_iter_test` cases, all 8 `fault_injection_fs_test` cases, and all 5 `DBWALTest.AsyncWalPrecreate*` cases. `DBSecondaryTest.SecondaryCloseFiles` passes 20 consecutive repetitions. - Focused regressions cover file-scoped gap fencing after an iterator snapshot, generic errors not installing a file-scoped sequence gap, and later recovery discovering that a generic error is file-scoped. - Earlier validation on this diff also passed crash-test unit coverage, remote-file-system adapter tests, repeated concurrency tests, two bounded multi-column-family stress runs, and bounded black-box crash/reopen dry runs. - The exact internal RocksDB format check and `arc f --take-formatters --enforce-lint-clean` report no formatting or lint issues. - The full crash/stress matrix and TSAN were not run. Reviewed By: pdillinger Differential Revision: D119324616 fbshipit-source-id: 05779d2bff7018f0ae69c4cbbe5b6455a45675d6 |
||
|
Xingbo Wang
|
eed2f96676 |
Wait for in-flight secondary read-view reclamation (#15289)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15289 Why the change is required: Secondary catch-up must release reader-free retired read views before scanning for obsolete files. T290043099 and duplicate T290043100 exposed a race where that ordering was lost, leaving two obsolete table files open and failing DBSecondaryTest.SecondaryCloseFiles. The assertion failure also exposed unsafe test cleanup because the secondary DB could outlive the test-local Env. Previous behavior: Retiring a read view always woke the background reclaimer. The worker could remove a reader-free view from the retirement queue and pause before destroying its SuperVersions. Synchronous cleanup then saw no queued work, ignored the in-flight reclamation, and continued to obsolete-file discovery while the old files were still referenced. The test also used a static lambda that captured the first fixture across repeated runs and closed the secondary only on the success path. New behavior: Catch-up synchronously reclaims reader-free views, or waits when a competing background worker has already claimed them. Views still held by active readers remain non-blocking and are reclaimed asynchronously after the last reader releases them. The test now closes the secondary before its local Env on every exit path and creates its verification lambda per test invocation. Implementation: Publishing a replacement view no longer wakes the background reclaimer; the immediately following synchronous cleanup owns that work. Reader release still wakes the worker. Cleanup waits on the existing in-flight counter when a worker won an earlier race. The test uses a Defer guard for lifetime ordering and removes the repeat-unsafe static lambda. Safety, compatibility, and limitations: This changes no public API, ABI, file format, recovery behavior, or read consistency. Cleanup does not wait for views retained by active readers, so the non-blocking multi-column-family iterator behavior is preserved. It waits only for reclamation already in progress. Validation covered the internal Linux ASAN/UBSAN target; the external GitHub Actions matrix was not run. Repository make-based format/source checks require a Git worktree and were unavailable in this Sapling checkout. Reviewed By: jaykorean Differential Revision: D121816425 fbshipit-source-id: a86cfff43a0cedd10b90e526a56e862f18d2a58d |
||
Omkar Gawde
|
7510773057 |
Fix ClipColumnFamily key lifetime and atomic-flush wait alignment (#15273)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15273 **What**: `ClipColumnFamily` keeps boundary keys valid across unlocked writes, and atomic-flush waits pair every column family with the correct memtable bound. **How**: `DBImpl::ClipColumnFamily` copies boundary keys from the current `Version` while holding the DB mutex, with a sync point that lets the regression test replace that version before deletion. `DBImpl::AtomicFlushMemTables` and `DBImpl::RetryFlushesForErrorRecovery` construct wait bounds in `cfds` order and supply no-bound fallbacks when a column family is absent from the flush-request map. Both wait sites gain a debug invariant asserting each id matches the map entry for the cfd at the same index. **Why**: Background version replacement could leave `ClipColumnFamily` reading freed metadata, while unordered or missing atomic-flush map entries could cause early completion or out-of-bounds access for idle or duplicate column families. Reviewed By: xingbowang Differential Revision: D121840843 fbshipit-source-id: 8e49b5c953a1aa29a0244fc0aae2ca3ab84be74f |
||
|
Josh Kang
|
dbc2db5228 |
Make external table APIs more consistent with native tables (#15241)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15241 Native `TableFactory` implementations receive the RocksDB `WritableFileWriter` in `NewTableBuilder()` and `RandomAccessFileReader` in `NewTableReader()`. These wrappers provide buffering, checksums, rate limiting, statistics, listeners, tracing, and direct-I/O handling. External table implementations previously received a limited writable-file adapter and reopened readers from a path, bypassing parts of this wrapper logic. This change makes `WritableFileWriter` and `RandomAccessFileReader` implement `FSWritableFile` and `FSRandomAccessFile`, respectively, so the same RocksDB wrappers can be passed to external tables. `ExternalTableFactory::NewTableReader()` now receives the already-opened reader and file size. The filesystem interfaces expose standard append, flush, sync, fsync, file metadata, synchronous reads, asynchronous reads, and batched reads. `RandomAccessFileReader::SubmitReadAsync()` schedules its wrapped coroutine read on the configured filesystem read executor, and the DB table-cache and `SstFileReader` construction paths pass that filesystem through. `SstFileReader` also supplies its configured statistics, listeners, rate limiter, and file temperature, removing the need for a separate external-table metrics filesystem wrapper. For direct I/O, `MultiRead()` checks whether requests are already sorted. When alignment and merging are needed, it constructs a sorted index only for unsorted input, preserving caller request order and result mapping. Fully aligned requests using aligned caller buffers bypass this path and may be supplied in any order. Reviewed By: xingbowang Differential Revision: D121035539 fbshipit-source-id: 27cb897726c31cb1a4dc811c65c8d79e690d82b1 |
||
|
Jay Huh
|
ffd4ac31af |
Make SecondaryDB multi-CF iterator reads non-blocking (#15259)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15259 Publish immutable SecondaryDB read views after open and successful catch-up so `NewIterators()` can acquire one cross-CF sequence and SuperVersion set without waiting for `DBImpl::mutex_`. Keep failed catch-ups on the previous view, reject refresh for batch iterators, and exercise concurrent catch-up plus batch scans in the CF-consistency stress profile. Reviewed By: xingbowang Differential Revision: D121318547 fbshipit-source-id: 3198f4766c90ab0bb39400963df3c329b4ecf6bf |
||
|
Anand Ananthabhotla
|
dd54c5abc9 |
Log DB session id for file deletion and manifest creation (#15229)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15229 Add `db_session_id` to obsolete file deletion logs and structured table/blob deletion event logs. Also include `db_session_id` when logging new MANIFEST creation so split-brain DBImpl sessions can be correlated without relying on process-local pointers. Reviewed By: joshkang97 Differential Revision: D120413139 fbshipit-source-id: c175b70d9b1f93e3242ee9e3191c274dc51ef3bf |
||
|
Jay Huh
|
bcf8711165 |
Add per-column-family ReadOptions to NewIterators (#15230)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15230 Add a raw C++ `DB::NewIterators()` overload accepting parallel vectors of `ReadOptions` and column family handles. Each iterator receives its own options while all iterators share one consistent sequence snapshot. Require identical snapshot pointers and tailing modes across the options. Implement the overload for primary, secondary, read-only, stackable, and write-prepared DBs. Building on D120048299, both secondary overloads share one helper that captures all SuperVersions and the sequence number under the DB mutex. Preserve read-only `table_filter` support when range-tombstone conversion is configured, and reject that unsafe combination in write-prepared read-write iterators before allocating any iterator. Add validation, per-CF prefix-seek coverage, a secondary catch-up regression test, table-filter contract tests, and a public API release note. Reviewed By: xingbowang Differential Revision: D120296578 fbshipit-source-id: 3775884537ea29a0d32bb3fe10330cf62ac5a406 |
||
|
Peter Dillinger
|
34d5074bfa |
Cross-key coalescing for lazy wide-column blob reads (#15178)
Summary: Follow-up (Phase 3) to the experimental lazy wide-column read API (DB::GetEntityLazy / DB::MultiGetEntityLazy). A MultiGetEntityLazy batch now resolves blob references across keys with coalesced I/O instead of key-by-key. - MultiGetEntityLazy acquires one SuperVersion and one consistent (implicit) sequence number for the whole batch via MultiCFSnapshot (as multi-CF iterators do) and transfers a single shared per-column-family pin into the LazyWideColumnsBatch, replacing the former per-key SuperVersion + explicit-snapshot loop. Single column family for now; a cross-CF ColumnFamily** overload remains a follow-up (the batch already models a per-CF pin map). - LazyWideColumnsBatch::MultiResolve groups the batch's reads by (Version, blob file) for separate-file references and by SST for embedded references, and issues one coalesced read per group -- for whole-column and byte-range reads alike -- caching whole values in each entity's resolver and pinning partial reads directly. - Adds the coalesced blob-read primitives this needs (none existed before): range-aware BlobFileReader::MultiGetBlobRange / BlobSource::MultiGetBlobRange / Version::MultiGetBlobLazy for separate-file references, and an embedded (SimpleGen2) batch read path (blob_gen2_format batch readers + BlobSource::MultiGetSimpleGen2Blob[Range] + BlockBasedTable::MultiGetSameFileBlob). Whole-value separate-file coalescing reuses the existing MultiGetBlob. The batch readers sort their requests by file offset before issuing the underlying MultiRead (which requires ascending offsets), and gen2 checksum verification is factored into one helper shared by the scalar and batch readers. - The hot non-lazy Get / MultiGet / iterator / compaction paths are untouched: the new multis are lazy-only, and the GetImpl change is gated on a new opt-in GetImplOptions field (an injected shared SuperVersion + sequence number) used only by the batched lazy read. - Blob read failures on these paths (short reads, invalid offsets, out-of-range requests, compression mismatches) now carry file/offset/size and expected-vs-actual context, mirroring the block-checksum diagnostics. - db_bench gains a multireadrandomentitylazy benchmark for measuring the coalesced batch path. No public API change, so this is a performance follow-up to the still-recent lazy API rather than a new feature. EXCEPT that `LazyColumnReadRequest::status == nullptr is now explicitly disallowed`. Left for later follow-up: - When a whole fetch is planned for an (entity, column), bind ordinary range requests for that column to the same fetch and slice them after adoption instead of issuing separate range I/O. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15178 Test Plan: New unit tests in db/wide/db_lazy_entity_test.cc assert cross-key coalescing via a blob-file / SST MultiRead counter added to the test FileSystem wrapper: - N whole-column separate-file reads across keys in one blob file collapse to a single coalesced MultiRead (and the fetched values are cached, so a repeat read does no further blob I/O); - N byte-range separate-file reads collapse to one MultiRead, save the un-read bytes (rocksdb.blobdb.lazy.partial.bytes.saved), and never fill the blob cache; - embedded (same-file) reads across keys in one SST collapse to one MultiRead over that SST; - an out-of-order embedded batch (reads issued in descending record offset) still resolves correctly in one coalesced read, covering the batch readers' internal offset sort; - a mixed batch (inline + whole + byte-range across keys) returns the correct bytes for each read; - a batched MultiGetEntityLazy + one MultiResolve matches resolving each key via a separate GetEntityLazy. The existing db_stress lazy-vs-eager differential (MaybeTestMultiGetEntityLazy) exercises the new batched read and cross-key MultiResolve unchanged (same public API). Reviewed By: xingbowang Differential Revision: D118660682 Pulled By: pdillinger fbshipit-source-id: 6a832175e730917eddeedab8e0b9cb2b8d6ae7b4 |
||
|
Jay Huh
|
79b58b0dd6 |
Keep secondary multi-CF iterators consistent (#15211)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15211 Capture the secondary DB sequence number and all requested column-family SuperVersions while holding the DB mutex, then construct the iterators after releasing it. This keeps every child on one state even when `TryCatchUpWithPrimary()` concurrently installs file-backed SuperVersions. Add a file-only catch-up regression test and release note. Reviewed By: xingbowang Differential Revision: D120048299 fbshipit-source-id: 89f4de533c4fbc1c6e86656845bda4bbb9da4d2c |
||
|
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 |
||
|
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 |
||
|
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
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
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
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
Josh Kang
|
9475798205 |
Refresh recovery plan after error escalation (#15111)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15111 T284514290 exposed an online-recovery race between the automatic recovery coordinator and a foreground synchronous write. A retryable no-WAL flush error selected lightweight retry-flush recovery. While that recovery attempt waited for the failed background flush, `Delete(sync=true)` appended and synced sequence N to the active WAL, then failed while persisting WAL-tracking metadata to the MANIFEST. Because memtable insertion and sequence publication happen after that MANIFEST step, the record was durable but logically unpublished. OLD: the concurrent error triggered another attempt, but the attempt reused a stale recovery plan. That preserved both ingredients needed for a duplicate: the first `Delete(key, N)` remained in the active WAL, while the published sequence remained `N-1`. ``` Recovery coordinator Foreground sync-write thread ------------------------------------ -------------------------------- [db_mutex held] plan = RetryFlush (copied once before the retry loop) attempt 1: ResumeImpl(plan) WaitForBackgroundWork() bg_cv_.Wait() [atomically releases db_mutex] ---------> assign sequence N append + sync Delete(key, N) [db_mutex held] MANIFEST WAL edit fails no memtable insert no LastSequence publication write returns I/O error recovery_error_ = Hard (requests another attempt) bg_error_ = Hard recover_context_ eventually becomes FullRecovery [db_mutex released] [db_mutex reacquired] <--------- ClearBGError() sees recovery_error_ attempt 1 cannot clear the error attempt 2: recovery_error_ = OK ResumeImpl(plan) ^ +-- still the cached RetryFlush plan RetryFlushesForErrorRecovery() retries the failed immutable flush only does not call SwitchMemtable() does not switch the active WAL ClearBGError() succeeds caller retries Delete(key) LastSequence is still N-1 => assigns sequence N again appends Delete(key, N) to same WAL memtable insert succeeds publishes LastSequence = N Reopen / WAL replay: first (key, N, deletion) -> memtable insert succeeds second (key, N, deletion) -> TryAgain("key+seq exists") normal WAL recovery has seq_per_batch_ == false assert(seq_per_batch_) fails ``` The cached argument to `ResumeImpl()` is therefore the direct bridge to the assertion: lightweight retry-flush recovery can report success without retiring the WAL that contains the unpublished record. Since it also does not publish sequence N, the successful caller retry writes the identical internal key into that same WAL. NEW: the same recovery coordinator consumes the concurrent error through a mutex-ordered handoff. ``` Recovery coordinator Foreground sync-write thread ------------------------------------ -------------------------------- [db_mutex held] attempt 1: plan = recover_context_ // RetryFlush ResumeImpl(plan) WaitForBackgroundWork() bg_cv_.Wait() [atomically releases db_mutex] ---------> append + sync Delete(key, seq=N) [db_mutex held] MANIFEST WAL edit fails CheckAndSet...(Hard, FullRecovery): recovery_error_ = Hard bg_error_ = Hard recover_context_ = FullRecovery [db_mutex released] [db_mutex reacquired] <--------- ClearBGError() sees recovery_error_ current attempt cannot clear the error next retry, with db_mutex held: recovery_error_ = OK plan = recover_context_ // FullRecovery ResumeImpl(plan) SwitchMemtable() switch to a new WAL clear the background error ``` The foreground unlock of `db_mutex_` happens before the recovery coordinator reacquires it and snapshots the next plan. Publishing `bg_error_` and `recover_context_` together under that mutex prevents the next attempt from observing a new severity with an old recovery requirement. `recovery_error_` is the per-attempt latch: a concurrent I/O error cannot be cleared as part of the in-flight attempt; it either stops recovery or forces another attempt to consume the updated context. If another error arrives while `ResumeImpl()` releases the mutex again, the same handoff repeats. No second recovery coordinator is needed. Manual `Resume()` now uses the retained recovery context instead of reconstructing one from the background-work scheduling flag, and the soft-error clear-only shortcut is allowed only when that flag shows there is no pending recovery work. The write ordering and `assert(seq_per_batch_)` detector are unchanged. Reviewed By: anand1976, xingbowang Differential Revision: D116106177 fbshipit-source-id: a78450c0924a326b5302ae6709e367f07bd3d5a6 |
||
|
Peter Dillinger
|
7217bf34a7 |
Add SST build context: is_remote_compaction, db_name, kSstFileWriter (#15085)
Summary: Custom filter and compression strategies decide what to build per SST from a FilterBuildingContext (shared by FilterPolicy::GetBuilderWithContext and CompressionManager::GetCompressorForSST). This adds more context so those strategies can adapt to where and how a file is being produced. FilterBuildingContext gains two fields: * is_remote_compaction: true only when the file is produced by an offloaded CompactionService worker. Detected with a new virtual CompactionJob::IsRemoteCompaction() (default false) overridden by CompactionServiceCompactionJob, mirroring the existing GetTableFileName hook. Local/fallback compactions and flushes are false. * db_name: a non-owning Slice naming the DB, populated for flush, compaction, recovery, and repair, and empty only where there is no DB, as with SstFileWriter. Under remote compaction it is the DB as opened by the worker, which can differ from the primary's. Both are threaded through TableBuilderOptions and populated in BlockBasedTableBuilder::Rep, whose "is this file in the LSM" test becomes a switch with no default case, so that a future TableFileCreationReason has to be classified there. Separately, TableFileCreationReason is now backed by uint8_t and gains a kSstFileWriter value, which SstFileWriter reports instead of the catch-all kMisc. It is appended after kMisc so existing enumerator values are unchanged; those values are public API, surfaced to EventListener and mirrored by the Java binding (updated here). Note that SstFileWriter neither notifies listeners nor consults CompactionFilterFactory, so for now the new value is only observable through FilterBuildingContext. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15085 Test Plan: * New CompactionServiceTest.IsRemoteCompactionInCompressorContext installs a custom CompressionManager and asserts GetCompressorForSST sees is_remote_compaction == true for the offloaded worker's output files and false for the primary's flushes in the same run, so the flag is shown to be per-file rather than global. IsRemoteCompactionFalseForLocalCompaction covers the same manager with no compaction_service configured. * DBBloomFilterTest.ContextCustomFilterPolicy now reports db_name as a token (expected / none / UNEXPECTED:<value>), so the value rather than just its presence is checked for flush and compaction, and expects kSstFileWriter instead of kMisc for the external SstFileWriter file. * bloom_test's "Like SST file writer" case uses kSstFileWriter so that it keeps modelling SstFileWriter. * Java EventListenerTest references TableFileCreationReason.MISC.getValue() instead of a hardcoded enum byte, so it stays correct across enum changes. Reviewed By: jaykorean Differential Revision: D115603616 Pulled By: pdillinger fbshipit-source-id: ad73fbc391a5377ace406725238ae6eede46c8d9 |
||
Michael C Huang
|
276495843d |
Add blob file writer buffer size option (#15061)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15061 Adds mutable ColumnFamilyOptions field `blob_file_writable_file_max_buffer_size` to cap blob-file WritableFileWriter buffers independently from DBOptions::writable_file_max_buffer_size. The default value is 0, which preserves existing behavior by inheriting the DB-wide writer buffer cap. Non-zero values apply to both flush/compaction blob writers and direct-write blob writers. Direct-write updates are propagated through DBImpl::SetOptions to newly opened blob files without requiring a DB restart. Also wires option parsing, dumping, db_bench, db_stress, crashtest, and release-note coverage. Reviewed By: xingbowang Differential Revision: D114979265 fbshipit-source-id: 3f764b797628eee0ce379110c1a1f8a16beb3fa2 |
||
Andrew Chang
|
9381544cd9 |
Add num-running-remote-compactions DB property (#15074)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15074 Why: We want to be able to monitor how many remote compaction jobs are running on a given server. Currently we know the total number of running compactions but not the local vs remote breakdown. What: Add a new `rocksdb.num-running-remote-compaction-jobs` statistic. This measures in-progress remote compaction jobs. Note that remote compaction work is scheduled per subcompaction, so one RocksDB compaction may contribute more than one running remote compaction job. Reviewed By: jaykorean Differential Revision: D115324149 fbshipit-source-id: 9397e39acd696c88ce7499ecb0da68de215bf1f3 |