mirror of
https://github.com/facebook/rocksdb.git
synced 2026-10-23 08:09:02 +03:00
main
544 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Anand Ananthabhotla
|
df90749560 |
Wire compaction to retry TableBuilder file cuts (#15344)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15344 Add the internal TableBuilder::TryAdd() contract so RocksDB-owned table builders can distinguish accepted rows, real builder errors, and clean requests to cut the current output table and retry the same row. Add diagnostic reason codes for retryable table boundaries; existing internal table builders inherit a default TryAdd() implementation that forwards to Add(). Translate the public ExternalTableBuilderBase::TryAdd() result in ExternalTableBuilderAdapter so external table implementations can request a file cut while RocksDB compaction uses the internal TableBuilder contract. Teach compaction output writing to honor kRequiresNewTable by closing the current output file, opening a new one through the existing callbacks, and retrying the same key without advancing the input iterator. Split OutputValidator validation into CanAdd() and AddValidated() so validator state is not updated until the key is actually accepted by a table builder. Add a DB compaction test with a custom TableFactory that wraps block-based tables and requests one mid-compaction file cut. The test verifies the cut produces two readable output files and preserves all key/value results. Reviewed By: jaykorean, xingbowang Differential Revision: D123932406 fbshipit-source-id: 9ed82c3782787de192bca15e40bfbf014ef28c15 |
||
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 |
||
|
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 |
||
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 |
||
|
Peter Dillinger
|
c83a84e98c |
Rename TableProperties::creation_time to oldest_ancestor_time (#15304)
Summary: The `creation_time` table property has long stored the file's oldest ancestor time (the oldest key time among the file's ancestors, falling back to flush/compaction time), not the file's creation time, which is `file_creation_time`. A TODO in table_properties.h acknowledged the misleading name. This change starts the API migration: `TableProperties::oldest_ancestor_time` is the new name, and `creation_time` remains as a deprecated alias sharing storage via an anonymous union, so existing external reads and writes continue to compile and behave identically. An anonymous union is used rather than a `uint64_t&` alias member. A reference member would delete TableProperties copy/move assignment, make the implicit copy constructor bind the copy's alias to the source object (dangling), permit writes through a const TableProperties, add 8 bytes, and break the uint64_t-range walk in TEST_SetRandomTableProperties. The union leaves size, layout, copying, constness, and offsetof unchanged. Reading the other union member is formally UB in ISO C++, but both members are uint64_t and this is supported in practice by GCC, Clang, and MSVC. All in-repo uses of the old name now use `oldest_ancestor_time`. Names that are externally observable or persisted are intentionally unchanged: * On-disk property name `rocksdb.creation.time` (`TablePropertiesNames::kCreationTime`) * The `"creation_time"` key in TableProperties serialization (used by remote compaction) * The event-log JSON key and `TableProperties::ToString()` label * Java `TableProperties.getCreationTime()` and C `rocksdb_table_properties_creation_time()` C API: the auto-binding generator (tools/c_api_gen/auto_simple_bindings.py) only considered direct FieldDecls and failed on the anonymous union's implicit unnamed field. It now treats anonymous union members as direct fields. Regenerating adds a new C getter `rocksdb_table_properties_oldest_ancestor_time()` alongside the existing `rocksdb_table_properties_creation_time()`. Follow-up: remove the deprecated `creation_time` alias in a future release. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15304 Test Plan: New unit test DBTablePropertiesTest.CreationTimeAliasesOldestAncestorTime verifies the two names share an address and that a write through the old name is visible through the new one. options_settable_test and compaction_service_test cover TableProperties serialization round trips through the unchanged "creation_time" key, now mapped to the renamed field. Reviewed By: jaykorean Differential Revision: D123079083 Pulled By: pdillinger fbshipit-source-id: 18b07d91f303a8f6de58da0c730f80d8c371505c |
||
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 |
||
Jay Huh
|
9d358e63d6 |
Widen compaction service counts to full kNumOfReasons (#14924)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14924 Widen the serialized "counts" array in CompactionServiceResult from kNumOfReasons - 1 back to the full kNumOfReasons, so the count for the newest CompactionReason is included in the remote compaction result. The width was previously reduced by one to match readers that expected the narrower array; restoring it changes what is written. This is safe only because deserialization of "counts" is now size-tolerant (a reader expecting fewer elements drops the extra, one expecting more zero-fills the missing). **It must not land until that size-tolerant read D106321320 has been deployed to every primary, including rollback targets, so should be >= 2 releases ahead; otherwise a primary can still or fallback to running the strict reader would reject the wider result and fail deserialization of this field.** == Test plan == **Legend** CompactionReason is an enum whose last entry is kNumOfReasons (currently 21). CompactionStats::counts is an int[kNumOfReasons] holding one count per reason; it is serialized into the remote compaction result and read back by the primary. - "narrow" = kNumOfReasons - 1 counts (omits the newest reason); what current code writes. - "wide" = kNumOfReasons counts (includes the newest reason). - strict reader: accepts only its exact expected element count; any other count fails deserialization (the plain OptionTypeInfo::Array behavior). - tolerant reader: accepts any count -- more elements than expected are dropped, fewer are zero-filled; never fails on a count mismatch (this change, SizeTolerantDeserializeArray). **Unit test** CompactionServiceTest.CompatCheckCountsWidthTolerance -- verifies that deserializing "counts" tolerates a serialized element count equal to, one greater than, or one less than the reader's expected width (extras dropped, missing zero-filled), and that the binary serializes exactly the expected number of elements. **Manual cross-version verification (ldb remote_compaction)** Test 1: expect-narrow tolerant-read primary reads wide counts a worker write (extra reason dropped) ``` $ tolerant_wide remote_compaction_worker --db=$DB --job_dir=$JOB & $ tolerant_narrow remote_compaction_primary --db=$DB --job_dir=$JOB remote_compaction_worker: OK (4915 bytes result) remote_compaction_primary: OK $ grep 'remote compaction result' $DB/LOG [default] [JOB 3] Received remote compaction result, output path: /tmp/cs_run/output, files: 000022.sst ``` => PASS: the primary parsed and installed the remote result. Test 2: expect-wide tolerant-read primary reads narrow counts a worker writes (missing reason zero-filled) ``` $ strict_narrow remote_compaction_worker --db=$DB --job_dir=$JOB & $ tolerant_wide remote_compaction_primary --db=$DB --job_dir=$JOB remote_compaction_worker: OK (4911 bytes result) remote_compaction_primary: OK $ grep 'remote compaction result' $DB/LOG [default] [JOB 3] Received remote compaction result, output path: /tmp/cs_run/output, files: 000022.sst ``` => PASS: the primary parsed and installed the remote result. Test 3: expect-wide tolerant-read primary reads wide counts a worker writes ``` $ tolerant_wide remote_compaction_worker --db=$DB --job_dir=$JOB & $ tolerant_wide remote_compaction_primary --db=$DB --job_dir=$JOB remote_compaction_worker: OK (4917 bytes result) remote_compaction_primary: OK $ grep 'remote compaction result' $DB/LOG [default] [JOB 3] Received remote compaction result, output path: /tmp/cs_run/output, files: 000022.sst ``` => PASS: the primary parsed and installed the remote result. Reviewed By: hx235 Differential Revision: D111068809 fbshipit-source-id: d78dd4b7501cb74e60bcb27dccd869b73052ac6d |
||
|
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
|
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 |
||
|
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
|
0df7ed563b |
Pass SST block CRC32C checksums to file writer (#15155)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15155 SST block writing now hands checksum coverage across the table-builder to file-writer boundary when the block checksum algorithm is CRC32C. The block builder passes the raw CRC32C of the block contents to `WritableFileWriter::Append()`, which validates the copied destination-buffer contents and accumulates that checksum into the filesystem handoff checksum instead of recomputing it after the copy. The trailer is still appended separately; table building computes and passes the trailer handoff checksum independently, while preserving the existing persisted block-checksum encoding. Non-CRC32C block checksum algorithms continue to use the existing file-writer checksum path. Destination-buffer validation failures return a stage-specific corruption `Status` that includes the file path. Flush and compaction propagate that status through their existing background error handling, so the same message is logged without adding local duplicate log sites. The SST tests record the builder append stream through sync-point callbacks while writing to a null sink, then replay those exact append calls into a file-backed writer to cover successful handoff, block-content corruption, compression-type corruption, trailer-checksum corruption, non-CRC32C fallback behavior, and a small randomized boundary-injection loop. Reviewed By: xingbowang Differential Revision: D116969191 fbshipit-source-id: f75dcb37936106b7f6f14cf1ed999c81a0879d7b |
||
|
Xingbo Wang
|
a0966fd94e |
Validate remote compaction snapshot ordering (#15160)
Summary: Remote compaction passes snapshot sequence numbers through serialized `CompactionServiceInput`. Downstream compaction logic assumes the sequence numbers are strictly increasing when selecting the earliest snapshot and using `std::lower_bound()` for snapshot visibility. Previously, malformed input could bypass the debug-only ordering assertion in release builds and select an incorrect visibility boundary. - Validate decoded snapshot sequence numbers before remote compaction setup and return `InvalidArgument` for duplicate or descending values. - Document the strict ordering contract while retaining the `CompactionIterator` assertion as defense in depth for internally constructed inputs. - Canonicalize randomized serialization-test snapshots and add coverage for malformed snapshot lists through `DB::OpenAndCompact()`. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15160 Test Plan: - Built `compaction_job_test` and ran `CompactionJobTest.OpenAndCompactRejectsUnorderedSnapshots` in the normal configuration. - Ran the focused test with `ASSERT_STATUS_CHECKED=1`. - Ran the focused test for 100 repetitions. Reviewed By: rban1 Differential Revision: D117888826 Pulled By: xingbowang fbshipit-source-id: 2312205acfe17271c2083808d372c93a5914114c |
||
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 |
||
|
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 |
||
|
Peter Dillinger
|
d3a82f28ce |
Optimize SstPartitioner overhead in compaction via ShouldPartitionByPrefix() (#15118)
Summary: When a column family configures an SstPartitioner, compaction paid a per-key cost for every key-value written to an output SST: a full user-key copy (into `last_key_for_partitioner_`) plus a virtual `SstPartitioner::ShouldPartition()` call (which also builds a PartitionerRequest and truncates/compares two Slices). This is motivated by an internal user whose custom SstPartitioner parses a variable-length prefix (a prefix code) out of each key to decide partition boundaries; today that prefix parsing runs on every key written during compaction, on top of the copy and virtual call above. This wires up the `SstPartitioner::ShouldPartitionByPrefix()` API (added, but unused, alongside the format_version=8 groundwork) so compaction can skip that per-key work. When a partitioner can express its next boundary as a key prefix, `CompactionOutputs` caches that prefix once per output file (re-derived from the file's first key) and replaces the per-key copy + virtual call with an inlined `Slice::starts_with()` check. That also lets a partitioner like the one above do its prefix parsing once per output file instead of once per key. When the partitioner returns no prefix (ambiguous, e.g. `SstPartitionerFixedPrefix` for keys shorter than its length), compaction falls back to the previous per-key `ShouldPartition()` path. Output file boundaries are byte-for-byte identical to before. `SstPartitionerFixedPrefix` implements `ShouldPartitionByPrefix()`; the base-class default returns "no prefix", so existing custom partitioners are unaffected until they opt in. Also: - db_bench: new `--sst_partitioner_fixed_prefix_len` to configure a fixed-prefix partitioner (enables benchmarking; no default behavior change). - `ShouldPartitionByPrefix()` API doc updated now that it is used and recommended for compaction CPU efficiency (any prefix code works, not just fixed-size). No new option and no behavior or format change. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15118 Test Plan: Added db_compaction_test cases covering the new fast path and its fallbacks: many keys sharing a prefix (one output file per prefix, no spurious cuts), keys shorter than the fixed prefix length (fallback to ShouldPartition), the zero-length-prefix "never partition" case, and short<->long key transitions across output-file boundaries. Existing SstPartitioner tests and the range tombstone + partitioner tests (db_range_del_test) provide regression coverage that file boundaries are unchanged, including the fast path with range tombstones. The crash test already exercises the partitioner via `enable_sst_partitioner_factory`. Benchmark (db_bench `compact` of a pre-built 50M-key DB; 16B keys / 8B values, 8-byte shared key prefixes with FixedPrefix length 8, no compression, single subcompaction). A/B compares this change against the same binary with only the compaction hot-path change reverted. Means of 6 trials, optimized faster on every trial and every metric; both variants produced identical output (50 files, i.e. identical partition boundaries): | Metric | Baseline | Optimized | Delta | |---------------------------------|----------|-----------|--------| | Instructions (whole process) | 92.46 B | 90.09 B | -2.6% | | CPU cycles | 46.20 B | 45.28 B | -2.0% | | CPU time (task-clock) | 18.74 s | 18.39 s | -1.9% | | `compact` wall time | 20.58 s | 20.08 s | -2.4% | DB-internal compaction stats corroborate: CompMergeCPU 15.84s -> 15.42s (-2.6%), compaction write throughput 38.9 -> 39.9 MB/s. The relative gain shrinks with larger values or compression enabled, where the partitioner is a smaller fraction of total compaction work; it is a pure win whenever a prefix partitioner is configured. Reviewed By: rban1 Differential Revision: D116372363 Pulled By: pdillinger fbshipit-source-id: 63de190329112aa2ce5b48da9fc77ef386494558 |
||
|
Peter Dillinger
|
021374c62c |
Start format_version=8 development toward new features (#15104)
Summary: format_version background: SST files are shared across RocksDB versions. format_version is a critical tool in ensuring prior (and later) versioned readers do not misinterpret a file created by some version. Opt-in features can sometimes rely on previously-rejected corners of the schema space, but not always. This change: lands much of the format_version=8 (fv8) format surface as an internal-only draft -- readable and testable but not user-writable -- so upcoming features can build on a stable base. fv8 is NOT published: kLatestBbtFormatVersion stays 7, and reading or writing fv8 is gated behind the existing TEST-only TEST_AllowUnsupportedFormatVersion() hook. Publishing fv8 (raising the ceiling) is deferred to a later change. The pieces of fv8 added here, and what each unlocks: - Index value-delta escape. Existing behavior: index blocks (format_version >= 4) store each block handle as a delta against the previous entry's handle, and signal "this value is a full handle instead" by reusing the key sentinel `shared == 0`. Because the two are coupled, an index entry whose handle is not contiguous with the previous one must be forced to a restart, costing a full key as well as a full value. fv8 decouples them with an in-value escape codepoint: varint 0 means "a full BlockHandle follows," and genuine size deltas are remapped to zigzag(delta)+1 so that 0 stays free. A non-contiguous entry can then remain an ordinary key-delta-encoded, on-cadence entry. The escape applies to every index-shaped block, which includes the top-level index of index partitions and the index of filter partitions, not just the index over data blocks. Unlocks: the upcoming optimize_key_common_prefix index feature (removes its need to special-case super_block_alignment) and value-delta encoding for embedded-blob SSTs (previously disabled outright because interleaved blob records break handle contiguity). Guarded strictly on fv8; format_version <= 7 encoding and decoding are byte-for-byte unchanged. Note that restart points must not delta-encode, so they still have full values without a sentinel. - Data block footer bit 30 is reserved as an "extended metadata present" escape. Decode rejects a set bit as unsupported/corrupt at all versions. Unlocks: adding block-level metadata in the future through the one shared footer decoder, without another format_version bump or plumbing format_version into every block parser. (Older reader versions relied on this bit being zero for some integrity/consistency checking; format_version bump now makes it safe to use.) - Footer metaindex gap. For fv8, the top 16 bits of the footer's 4-byte metaindex-size field record the gap (in bytes) between the end of the metaindex block and the start of the footer; the low 16 bits hold the metaindex block size. Unlocks: a space very near the end of the file to save full file checksum state, for a "files carry their own checksums" feature that doesn't require SST parsing to extract the checksum states (to run them to completion on the trivial file tail). - Reserved "user_key_common_prefix" table property. The reader returns NotSupported when it is present and non-empty. Unlocks: a future whole-file optimization that stores the common prefix of all user keys once instead of in every restart key; until implemented, fv8 readers refuse such a file rather than mis-read it. - Because of the problem of inferring such a prefix at the start of constructing an SST file, we expect it will rely on SstPartitioner. Here we've added SstPartitioner::ShouldPartitionByPrefix() (not yet used) that will enable this optimization, and avoid the overhead of a virtual callback to SstPartitioner on every key written to an SST file. Supporting cleanups: the Footer's "unset" format_version sentinel is named (kInvalidFormatVersion) and the FormatVersionUses*() predicates assert against it, as each is a `version >= N` test that an unpopulated Footer would silently satisfy. Index block reader entry points (Block::NewIndexIterator() and friends) drop their defaulted trailing arguments so each caller states which codec it expects, and BlockBuilder::Add() takes delta_value by const reference rather than a pointer whose null case no caller used. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15104 Test Plan: New unit tests, all exercising fv8 through TEST_AllowUnsupportedFormatVersion(): - block_test: value-delta escape codec round-trip (delta 0, negative, large, and the escape; asserts format_version <= 7 bytes match the legacy encoding and a legacy 0x00 still decodes as delta 0), and rejection of footer bit 30 at all versions. - db_flush_test: fv7-vs-fv8 read equivalence for super_block_alignment, including the separate_key_value_in_data_block intersection, confirming padding events (hence escape entries) occur and fv8 index values are delta encoded. - sst_file_reader_test: fv7-vs-fv8 read equivalence for embedded-blob SSTs, confirming fv8 (unlike fv7) delta-encodes their index values and actually emits escape entries (counted through a sync point). - block_based_table_reader_test: a builder+reader test that injects a footer metaindex gap via a sync point (verifies the gap bytes are written and the reader locates the metaindex via the recorded gap, and that too large a gap surfaces as a builder error), and that a non-empty reserved user_key_common_prefix property makes the reader return NotSupported. - table_test: two-level index with partitioned filters and index_block_restart_interval > 1, the configuration in which both top-level indexes delta-encode their values; verifies scan and point lookup round trips at each tested format_version including fv8. - (Not strictly fv8) Basic SstPartitionerFixedPrefix tests in slice_test (ShouldPartition, ShouldPartitionByPrefix, their mutual consistency, and CanDoTrivialMove), which had no non-DB coverage before. fv8 is also added to the table_test harness (GenerateArgList) and the shared footer version set (test::kFooterFormatVersionsToTest), so the existing table-level and DB-level property/checksum suites now also run at fv8. And added to crash test (minor revisions to db_stress). ## Performance details Release db_bench (PORTABLE=0, -march=native) A/B/C on the read/write fast path, comparing (a) parent, (b) this commit @ fv7, (c) this commit @ fv8. All DBs on tmpfs, 20M keys, block_size=2048, compression=none. Note index_block_restart_ interval=16 (not the default 1) is required to actually exercise the fv7<->fv8 index value-delta codec -- with restart interval 1 every index entry is a full- handle restart and the changed path never runs (confirmed delta-value=1 via sst_dump). Reads single-thread + core-pinned, warm cache, interleaved/rotated rounds, paired deltas. Reads (readrandom + seekrandom, non-partitioned single ~1.9MB index block and partitioned index+filters): fv7->fv8 in the *same* binary is within noise on all four workloads (largest -1.0%, CIs cross zero). before->after (fv7 path) is also within noise; perf shows a deterministic +0.4-0.6% instructions/op from the added branches (the format_version>=8 value_delta_escape guard + refactored data-block footer decode), which is below the throughput noise floor. Writes (fillseq + compact total CPU): fv7->fv8 null. compact (which rewrites all index blocks) is null before->after. fillseq showed +3.5% CPU before->after, but perf attributes it entirely to cycles/IPC with *identical* instructions/op (+0.01-0.04%) -- a code-layout effect on the untouched memtable-insert loop, not this change. Sanity: any single throughput/CPU delta <~1% is within the build-to-build noise floor, measured directly by comparing two byte-identical binaries (swings up to ~0.9% reads; the pre-native/PORTABLE=1 build showed a spurious -1.85% that vanished under -march=native). Net: no measurable regression from fv8 draft; fv7 encoding/decoding byte-for-byte unchanged, only the noted sub-1% instruction bump on the shared read path. Reviewed By: xingbowang Differential Revision: D116105558 Pulled By: pdillinger fbshipit-source-id: ead776cdd396d497d47872c0956ea8c651c0d13f |
||
|
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 |
||
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 |
||
|
Xingbo Wang
|
44e0294c1d |
Fix remote compaction filter test flake (#15075)
Summary: - Disable automatic compactions in `CompactionServiceTest.CompactionFilter`. - Keep the test focused on the explicit `CompactRange()` path that verifies remote compaction filter behavior. - Avoid a flaky race where background remote compaction can obsolete an input SST before the manual compaction runs. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15075 Test Plan: - `make format-auto` - `AUTO_CLEAN=1 COERCE_CONTEXT_SWITCH=1 make -j32 compaction_service_test` - `tools/gtest_parallel_repro.py --binary ./compaction_service_test --gtest_filter=CompactionServiceTest.CompactionFilter --processes-per-iteration 32 --iteration-count 30 --cpu-count 4 --timeout 120 --stop-on-failure --out /tmp/ws28_compaction_service_flake_fixed_20260809_1` - `git diff --check` - `AUTO_CLEAN=1 make check-sources` Reviewed By: jaykorean Differential Revision: D115357593 Pulled By: xingbowang fbshipit-source-id: 9c640b807b8d0c204a23fb805e511b39db69acbc |
||
|
Andrew Chang
|
3949208acf |
Make max_open_files configurable via options_map in OpenAndCompact (#15077)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15077 __Why__: Allow offload compaction workers to configure `max_open_files` via `options_map`. __Problem__: `OpenAndCompact` hard-codes `max_open_files` to -1. This overwrites any value set via the `options_map` and prevents configuration in the remote compaction workers. __Fix__: Remove the hard-coding assignment. The value now comes from: 1. OPTIONS file via `LoadOptionsFromFile` 2. `override_options.options_map` via `GetDBOptionsFromMap` The default value of `max_open_files` was already -1, so existing behavior is preserved. Reviewed By: jaykorean Differential Revision: D115373592 fbshipit-source-id: fa742397246092544ab6f9004678be7ae938d051 |
||
|
Peter Dillinger
|
af0eae4de3 |
Rename TransactionLogIterator to WalIterator and document its behavior (#15056)
Summary:
`TransactionLogIterator` iterates the write-ahead log. It does not return transactions, does not require `TransactionDB`, and in fact returns `Status::NotSupported()` for two of the three `TransactionDB` write policies (WritePrepared and WriteUnprepared). The name predates `TransactionDB` and comes from "transaction log" being an old synonym for WAL. Everything else in this header was already renamed away from that terminology -- `LogFile` -> `WalFile`, `VectorLogPtr` -> `VectorWalPtr` -- and the C API has long used `rocksdb_wal_iterator_t` and `rocksdb_wal_iter_*`. This finishes the job.
The public class is now `WalIterator` in `include/rocksdb/wal_iterator.h`. `include/rocksdb/transaction_log.h` remains as a forwarding header and `TransactionLogIterator` remains as an alias, both marked deprecated by comment only (not `[[deprecated]]`) so existing users compiling with -Werror are unaffected, matching how `LogFile` and `VectorLogPtr` were retired. `DB::GetUpdatesSince()` keeps its name.
More importantly, the documentation was incomplete in ways that could cost a user data, so this states plainly what the API has always done:
- It only sees the WAL. Writes made with `WriteOptions::disableWAL`, and sequence numbers consumed by `IngestExternalFile()`, advance the DB sequence number without producing a WAL record. They leave permanent holes that end iteration. This was not documented anywhere, so it was unreasonable to expect a user to know those features silently break WAL tailing.
- The three iterator states, in particular that `!Valid()` with an OK `status()` means caught up rather than finished, and that calling `Next()` again is the intended way to tail a DB. The old comment on `Next()` said "REQUIRES: Valid() to be true", which forbids the primary use case.
- `Status::TryAgain`, which a caller must handle but which appeared nowhere in the public headers.
- That `GetUpdatesSince(seq)` silently starts later when `seq` is unavailable. Combined with the above this is the sharp edge: recovering from a spent iterator means calling `GetUpdatesSince()` again, so the recovery step is exactly where data can be skipped unnoticed. A consumer applying these batches to a copy of the DB diverges permanently and undetectably if it misses one, so the header now says such consumers must verify continuity themselves using `BatchResult::sequence` and `WriteBatch::Count()`.
Almost all of this diff is mechanical: renaming the type and its files, updating `#include` paths, rewriting comments, and updating build lists (`src.mk`, `CMakeLists.txt`, `BUCK`, `java/Makefile`, `java/CMakeLists.txt`). `db/c.cc` is regenerated from `tools/c_api_gen/c_base.cc`. Reviewers should focus on the four places that are not:
1. `WalIteratorImpl::UpdateCurrentWriteBatch()` in `db/wal_iterator_impl.cc` (renamed from `db/transaction_log_impl.cc`) drops dead code. On a sequence discontinuity it set `current_status_` to `NotFound` and then called `SeekToStartSequence()`, which returns immediately when `current_status_` is not OK. That guard and the early return in `Next()` were both added by
|
||
|
Omkar Gawde
|
dbf9328c71 |
Add per-column-family compaction abort and resume (#14982)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14982 **What**: Add reference-counted `DB::AbortCompactions(cf)` / `DB::ResumeCompactions(cf)` to abort and resume compactions for a single column family without stopping unrelated column families. They compose with the DB-wide `AbortAllCompactions()` / `ResumeAllCompactions()` and are no-ops once DB shutdown has begun. **How**: - **Per-CF abort flag.** `ColumnFamilyData` gains `compaction_aborted_`, wrapped in `CacheAlignedWrapper` so the lock-free reads done by compaction workers do not false-share with the DB-mutex-written fields packed around it. A new `IsCompactionAborted(cfd)` helper checks it alongside the DB-wide counter at every entry point (`CompactRange()`, `RunManualCompaction()`, `CompactFilesImpl()`), and `CompactionJob::ProcessKeyValue()` checks both on entry and in its periodic cron check. - **Parking.** Automatic work for an aborted CF is preserved rather than dropped. `PickCompactionFromQueue()` now returns `std::variant<CompactionQueuePicked, CompactionQueueThrottled, CompactionQueueParked>` and moves an aborted CF into `parked_compaction_cfds_` only once it reaches the front of the queue, so CFs behind it keep their place and the scheduler credit is not restored (which would cause a hot re-pick loop). `EnqueuePendingCompaction()` parks newly needed work for an aborted CF and now early-returns for dropped CFs. `RestoreParkedCompaction()` re-enqueues on the final resume; a CF dropped while parked is handled in `DropColumnFamily()`, and one dropped before its aborted queue entry is reached is handled by a fast path in `PickCompactionFromQueue()`. - **Knowing when it is safe to return.** `ColumnFamilyData::num_in_flight_compactions_` counts actually-picked work, and `Defer` balances `BeginInFlightCompaction()` / `EndInFlightCompaction()` on every `BackgroundCompaction()` and `CompactFiles()` exit path. That covers the `NotifyOnCompactionCompleted()` window that `CompactionPicker::compactions_in_progress_` does not, since the picker set is cleared during `Install`'s manifest write. `MustWaitForCompaction()` combines the counter with the picker set (filtered to skip `kExternalSstIngestion` / `kRefitLevel` reservations, which are not real jobs) because only the picker set covers the LOW-to-BOTTOM priority handoff, where the counter briefly dips to 0. - **Waking the waiter.** `AbortCompactions()` cancels matching `manual_compaction_dequeue_` entries and then waits on `bg_cv_`; `per_cf_compaction_abort_waiters_` forces the compaction completion paths to signal, because the picker's `UnregisterCompaction()` does not. `CloseHelper()` drains `parked_compaction_cfds_` and releases their references. - **API surface.** The two methods are new pure virtuals on `DB`, so `StackableDB` forwards them and the `db_test.cc` DB wrapper stubs them. `db.h` documents how the per-CF API composes with DB-wide abort, `DisableManualCompaction()`, `PauseBackgroundWork()`, shutdown, ingestion reservations, level refit and remote compaction, plus the parked-work caveat on `WaitForCompact()`. Comments in `listener.h`, `statistics.h` and `status.h` now name both abort APIs. - **Coverage.** 10 new cases in `db_compaction_abort_test.cc` (CF scoping, counter composition, parked-work restore, CF drop both while parked and before parking, trivial-move rejection, waiting for the completion callback, post-close no-op). `db_stress` gains `abort_and_resume_cf_compactions_one_in` and `TestAbortAndResumeCfCompactions()`, rejected in combination with `clear_column_family_one_in` because that would drop the column family out from under an active abort. Also a `db_crashtest.py` knob and one `new_features` release note. - **Known gap.** A `TODO` records the pre-existing post-pick stale compaction-score problem: aborting after `PickCompaction()` releases the inputs without recomputing the score, so the resume can observe a stale sub-trigger score and re-enqueue nothing. `ResumeAllCompactions()` has the same gap and carries a pointer to the same TODO; fixing it is left to a separate change covering both APIs. **Why**: Atomic-replace bulk loads only conflict with their target column family, but the DB-wide abort suppresses compaction for every column family colocated in the same RocksDB instance. Scoping the abort keeps unrelated column families compacting for the duration of a bulk load. Reviewed By: pdillinger Differential Revision: D113103899 fbshipit-source-id: 9bef924132676ed118a37aee86da6ee8bcefef09 |
||
|
Peter Dillinger
|
c73841572a |
Simplify ReadOptions plumbing for read-path blob fetchers (#15003)
Summary: Follow-up cleanup to the embedded (same-file) blob wide-column work (PR https://github.com/facebook/rocksdb/issues/14990): the "ReadOptions usage cleanup/optimization" item. Split the Version-backed blob fetcher into two variants sharing a common VersionBlobFetcherBase (which implements FetchBlob via a virtual read_options()): - VersionBlobFetcher now holds a `const ReadOptions&` for transient/stack fetchers (the common case: point Get/MultiGet, DBIter, version_set), so the growing ReadOptions is no longer copied per fetcher on the hot path. A deleted rvalue-ref constructor rejects binding to a temporary. - OwningVersionBlobFetcher takes `ReadOptions&&` and owns a copy, for fetchers whose lifetime is independent of the caller (the read-path lazy resolver and compaction), making ownership transfer explicit. DBIter::BlobReader now holds a single blob fetcher instead of decomposed scalar fields (read_tier/verify_checksums/fill_cache/io_activity). This removes the decompose-then-recompose that the old CreateBlobFetcher and RetrieveAndSetBlobValue performed, and closes the field-loss gap: all ReadOptions fields now flow through to blob reads rather than just those four. CreateBlobFetcher is gone; the merge path uses the reader's owned fetcher directly. A CanResolve() predicate on the fetcher preserves the prior "unexpected blob index" Corruption instead of dereferencing a null Version, and the now-redundant allow_blob_write_path_fallback_ member/ parameter (equal to the fetcher's own flag) was dropped. ReadPathBlobResolver keeps owning its fetcher (now OwningVersionBlobFetcher) so it stays self-contained for the planned lazy entity read path, where the result outlives the read call. A TODO in db_iter.h records the remaining duplication (BlobReader and ReadPathBlobResolver each own a fetcher with the same {version, read_options, blob_file_cache, allow_write_path_fallback}), deferred to that work. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15003 Test Plan: Pure refactoring with no intended behavior change; Version::GetBlob's Slice overload just decodes and calls the BlobIndex overload, so routing whole-value blob reads through the fetcher is equivalent, and the direct-write fallback flag/read options are unchanged. Covered by existing tests: - db/blob/db_blob_index_test.cc: embedded/entity blob GetEntity/MultiGetEntity zero-copy, plain Get of an embedded default column, merge over an embedded-blob entity, read_tier == kBlockCacheTier Incomplete, corrupt record Corruption. - db/blob/db_blob_basic_test.cc and db/wide/db_wide_blob_direct_write_test.cc: separate-file and direct-write blob resolution (the fallback branch). - db/db_iterator_test.cc (blob) and db/compaction/compaction_iterator_test.cc: DBIter blob/merge paths and the compaction (owning) fetcher. Reviewed By: xingbowang Differential Revision: D113582247 Pulled By: pdillinger fbshipit-source-id: 231c9364e266b826d12a8479cbb7f026a0d26bf6 |
||
|
Peter Dillinger
|
744f120b12 |
Zero-copy reads of embedded (same-file) blob wide-column entities (#14990)
Summary: This is a follow-up to D112212266 / https://github.com/facebook/rocksdb/pull/14943 (multi-buffer PinnableWideColumns), which added the zero-copy read path for separate-blob-file references and explicitly left extending it to SST embedded-blob references as a follow-up. Point GetEntity/MultiGetEntity of a wide-column entity whose columns are stored as embedded (same-file) blob records previously deserialized the entity, copied every column name/value, fetched each embedded blob (copying it), and re-serialized the whole entity before GetContext ever saw it (BlockBasedTable::MaybeResolveEmbeddedValue's wide-column branch). This extends the existing multi-buffer PinnableWideColumns zero-copy machinery (previously only for separate blob files / direct-write) to embedded blob columns. Changes: - New SameFileBlobReader interface (db/blob/same_file_blob_reader.h), a narrow seam for resolving a same-file BlobIndex against the current SST. BlockBasedTable implements it (GetSameFileBlob), delegating to the existing blob-cache-backed / pinned embedded reads. - BlobFetcher is now an abstract interface; the Version-backed implementation moved to VersionBlobFetcher. Added an EmbeddedAwareBlobFetcher decorator that routes same-file references to a SameFileBlobReader and delegates all others to a base fetcher (EffectiveFetcher() selects the decorator only when a same-file reader is present). - The point-Get and MultiGet loops now hand the raw entity to GetContext with the table as SameFileBlobReader, instead of pre-resolving/re-serializing. This is gated on not building a row-cache replay log, so a row-cache entry still captures a fully resolved entity (a later row-cache hit never reopens the SST to resolve same-file references). - GetContext composes the decorator and resolves same-file and separate-file blob columns zero-copy across the columns / default-value / merge consumers. - The iterator (DBIter) path is unchanged (still re-serializes via MaybeResolveEmbeddedValue). - Also fixes latent method hiding in the test-only MockBlobFetcher. Intended follow-ups: - Lazy/deferred embedded resolution: return a LazyWideColumns result whose blob columns stay unresolved and are resolved on demand (whole or by byte range). The SameFileBlobReader seam and the polymorphic BlobFetcher introduced here are the mechanism for embedded support there. - ReadOptions usage cleanup/optimization: have DBIter/BlobReader hold a single ReadOptions instead of decomposed scalar fields (removing the decompose-then-recompose in CreateBlobFetcher and closing the field-loss gap), and split VersionBlobFetcher into a ReadOptions-referencing variant for transient/stack fetchers plus an owning variant for lifetime-independent ones (the lazy resolver and compaction), to avoid copying the (growing) ReadOptions on the hot path. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14990 Test Plan: Added embedded wide-column tests to db/blob/db_blob_index_test.cc: - GetEntity/MultiGetEntity of an embedded-blob wide-column entity return the correct columns and resolve them zero-copy (blob columns served from the blob cache: identical data() across reads and in separate backing buffers; pointer-stable across a PinnableWideColumns move). - Plain Get of an entity whose default column is an embedded blob. - Merge base and GetMergeOperands over an embedded-blob entity. - read_tier == kBlockCacheTier yields Incomplete for an unresolved same-file reference. - A corrupt embedded record surfaces Corruption, not a raw BlobIndex. # Performance validation with db_bench Measured with db_bench benchmarks added in https://github.com/facebook/rocksdb/issues/14995 / D113455538, temporarily stacked on this diff for A/B testing. ## Setup - **Baseline**: this diff's parent + db_bench benchmark commit. - **Treatment**: this diff, incl. its test/lint fixups + the same db_bench benchmark commit. - Both are release builds (`DEBUG_LEVEL=0 make db_bench`). The diff touches only the read path, so both binaries produce byte-identical SSTs; the db_bench commit is additive (`tools/db_bench_tool.cc` only) and identical in both. - Host: AMD EPYC-Genoa, 166 vCPU, 224 GiB RAM. Single reader thread. ## Method - Datasets are created **once** and read by **both** binaries via `--use_existing_db`, so the A/B compares identical on-disk data. - Each read runs twice in-process (warmup + measured); the blob cache (1 GiB) and block cache (512 MiB) hold the whole ~400 MiB dataset, so the reported (2nd) run is fully warm — isolating the column-resolution cost the diff changes rather than I/O. - 100k keys, `value_size=1024`, `embedded_blob_min_size=256`, `compression_type=none`, `format_version=7`, `--duration=10` per measured run. - Datasets: - **A** — wide-column entity, `num_short_wide_columns=1` + `num_wide_columns=4` (small inline default column + 4 embedded-blob columns). Read via `readrandomentity` (GetEntity) and `multireadentity` (MultiGetEntity, batch 32). - **B** — wide-column entity, `num_short_wide_columns=0` + `num_wide_columns=4` (the default column itself is an embedded blob). Read via plain `readrandom` (Get returns the resolved default column). - **C** — whole-value embedded blobs (`fillembeddedblob`). Read via plain `readrandom`. This path is **not** changed by the diff — a regression check. Example commands: ``` # create (once) db_bench --benchmarks=fillembeddedentity --db=/tmp/perfA --use_existing_db=0 \ --num=100000 --value_size=1024 --embedded_blob_min_size=256 --format_version=7 \ --num_wide_columns=4 --num_short_wide_columns=1 \ --use_blob_cache=true --blob_cache_size=1073741824 --cache_size=536870912 \ --compression_type=none # read (each binary; warmup + measured) db_bench --benchmarks=readrandomentity,readrandomentity --db=/tmp/perfA \ --use_existing_db=1 --threads=1 --duration=10 <same options> ``` ## Results (warm, 2 runs each) | Benchmark (read path) | Baseline µs/op | Treatment µs/op | Latency | Baseline Kops/s | Treatment Kops/s | Throughput | |---|---|---|---|---|---|---| | `readrandomentity` — GetEntity, inline default + 4 embedded cols | 7.16 | 5.00 | **-30%** | 140 | 200 | **1.43×** | | `multireadentity` — MultiGetEntity (batch 32) | 6.46 | 4.54 | **-30%** | 155 | 220 | **1.42×** | | `readrandom` — Get of embedded default column (dataset B) | 6.55 | 2.12 | **-68%** | 153 | 471 | **3.09×** | | `readrandom` — whole-value embedded blob (dataset C, unchanged path) | 1.92 | 1.91 | ~0% | 521 | 524 | neutral | Both runs were within ~1–3% of each other (raw numbers below). ## Interpretation - **GetEntity / MultiGetEntity of embedded-blob wide-column entities improve ~30% in latency (~1.4× throughput)** — the expected win from resolving embedded blob columns zero-copy into the `PinnableWideColumns` backing buffers instead of the old deserialize + per-column copy + re-serialize round trip. - **Plain `Get` of an entity whose default column is an embedded blob improves ~3×** — the largest win, because the old path re-serialized the entire entity even when only the default column is returned, whereas the new path resolves just that column. - **The whole-value embedded blob `Get` path is unchanged (~0% delta), confirming no regression** on the pre-existing zero-copy path. No unexpected regressions observed; all deltas match the diff's intended scope. ## Raw measured numbers (both runs) ``` readrandomentity BASELINE 7.183 / 7.137 µs/op 139225 / 140118 ops/s readrandomentity TREATMENT 4.992 / 5.008 µs/op 200333 / 199681 ops/s multireadentity BASELINE 6.458 / 6.458 µs/op 154837 / 154843 ops/s multireadentity TREATMENT 4.509 / 4.577 µs/op 221798 / 218463 ops/s readrandom (Bdef) BASELINE 6.497 / 6.609 µs/op 153920 / 151307 ops/s readrandom (Bdef) TREATMENT 2.126 / 2.120 µs/op 470317 / 471805 ops/s readrandom (Cblob) BASELINE 1.891 / 1.947 µs/op 528760 / 513554 ops/s readrandom (Cblob) TREATMENT 1.895 / 1.920 µs/op 527767 / 520807 ops/s ``` Reviewed By: xingbowang Differential Revision: D113347622 Pulled By: pdillinger fbshipit-source-id: c51bc187085695d559411d9c0555bb52c8e0a61a |
||
|
Peter Dillinger
|
81ea677072 |
PinnableWideColumns support for >1 backing buffer to avoid re-serialization (#14943)
Summary:
Wide-column read paths that materialize blob-referenced columns used to re-serialize the entire entity into a fresh contiguous buffer, copying the (potentially large) resolved column values byte-for-byte. This was forced by PinnableWideColumns holding a single backing buffer with all column Slices pointing into it.
PinnableWideColumns now holds its columns as zero-copy Slices into one or more stable backing buffers, kept in a std::forward_list<PinnableSlice> whose nodes never relocate on move, move-assignment, or splice. Resolved blob columns can be spliced in as additional backing buffers instead of re-serializing the entity.
Using address-stable nodes also cleans up the old Slice-invalidation handling. The previous single-buffer representation had to cope with a self-pinned PinnableSlice whose small (SSO) std::string relocates when the PinnableSlice is moved: on every move it detected the relocated backing pointer and re-derived the column Slices by re-parsing. With forward_list nodes that never move, the column Slices stay valid across move / move-assignment / splice, so that handling is gone and the move operations are now the defaulted member-wise moves.
The multi-buffer plumbing is internal only: a new PinnableWideColumnsHelper (a friend) performs the splice and the little bit of private-member access the read paths need, so there is no new public API surface and the previous `friend class DBImpl` is gone. The public view is unchanged: columns() still returns a single, in-order set.
Read paths converted to resolve without re-serializing:
- GetContext::SaveWideColumnEntityToColumns (eager GetEntity SST reads, separate-blob-file references)
- DBImpl::ResolveDirectWriteWideColumns (direct-write blob references)
- DBImpl::MaybeResolveMemtableBlobValue (blob-backed memtable reads)
Merge output still serializes (the MergeOperator API consumes/produces a serialized entity). Same-file ("embedded") blob columns are resolved -- and still re-serialized -- earlier in the block-based table reader (MaybeResolveEmbeddedValue / ResolveEmbeddedValueForGet) before the entity reaches GetContext, so the GetEntity path above only avoids re-serialization for separate-blob-file references. Compaction's separate entity machinery is unchanged.
API/accounting: deprecated PinnableWideColumns::serialized_size() (comment-only, per repo convention) in favor of the new payload_size() (sum of column name + value sizes; equals the value size for a plain value). Read statistics for entities (BYTES_READ, BYTES_PER_READ, per-key GetEntity/MultiGetEntity byte counts) and the ReadOptions::value_size_soft_limit threshold now measure entities by payload_size() instead of the serialized size. This drops framing bytes (entity measurements are slightly smaller), thus making plain-value and wide-column accounting consistent and reducing unnecessary exposure to implementation details.
Serialization cleanup enabled by the above:
- Consolidated Deserialize/DeserializeV2 into a single Deserialize(const Slice&, WideColumns&, blob_columns*) plus a DeserializeSimple alias for the no-blob-references case; both take const Slice&, require the input to be exactly one serialized entity, and reject any trailing data uniformly (V1 previously ignored trailing bytes). An unexpected blob reference where a fully-resolved entity is required now returns Corruption.
- ResolveEntityBlobColumns (serialized output, used only by merge) is now a thin wrapper over the new multi-buffer resolver plus Serialize, removing the duplicated fetch loop and the SerializeResolvedEntity helper.
- Consistently use `const Slice& input` rather than `Slice& input` for wide column functions expecting an input slice with no trailing data.
Design/polish from human review:
* The initial plan called for more complex handling around a vector<PinnableSlice> backing store (unique_ptr indirection and re-deriving the column Slices on move to cope with reallocation and SSO relocation); switching the backing store to a std::forward_list of address-stable nodes was a human suggestion that removed that complexity and the move-time Slice re-derivation entirely.
* Keep resolve plumbing off the public API via PinnableWideColumnsHelper and removing the DBImpl friend
* Various pieces of cleanup and polish (more left to do)
* The eager-GetEntity optimization
* The memtable inline-copy NOTE/TODO.
Intended follow-up:
* Resolve FIXME about pre-existing swallowed non-OK status in db_impl.cc
* Resolve TODO about value_size_soft_limit in db_impl.cc
* Extend this optimization to SST embedded-blob references
* Error-status/API cleanup: classify an unrecognized wide-column version as Corruption (not NotSupported) at all entry points; replace the NotSupported sentinel from GetValueOfDefaultColumn with an explicit is_blob_reference out-param (removing GetValueOfDefaultColumnResolvingBlobs); and centralize the "blob-backed value without a blob fetcher" Corruption into the resolvers (resolving the ResolveEntityForMerge FIXME) rather than repeating it at each caller
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14943
Test Plan:
Extended db/wide/wide_column_serialization_test.cc with PinnableWideColumns unit tests: set/payload_size for plain, V1, and resolved entities; move and move-assign of a multi-buffer result preserve column data pointers (the node-stability guarantee); Reset; ResolveColumns splices blob buffers in zero-copy (resolved values' data() point into the spliced-in buffers, not a re-serialized entity); serialized_size() == SerializedSizeV1(columns()); and Deserialize rejects trailing data after a serialized entity for both V1 and V2 layouts.
Built and ran (debug): wide_column_serialization_test, db_wide_basic_test, db_wide_blob_direct_write_test, db_blob_basic_test, compaction_iterator_test, db_merge_operator_test, db_iterator_test, udt_util_test, write_batch_with_index_test -- all pass. librocksdb and db_stress build. make format-auto and build_tools/check-sources.sh are clean.
Reviewed By: xingbowang
Differential Revision: D112212266
Pulled By: pdillinger
fbshipit-source-id: 13b89e2bb7d8371f4c1ff18b234a549bea9c9e77
|
||
Hui Xiao
|
7c80a5ac85 |
Size-tolerant deserialization for compaction service response's counts array (#14807)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14807 The "counts" array in CompactionServiceResult (one compaction count per CompactionReason) is non-critical information. During cross-process remote compaction the primary and the remote worker can run different RocksDB versions and therefore disagree on kNumOfReasons, so the serialized counts array can have a different length than the reader expects. The plain OptionTypeInfo::Array parser rejects any element-count mismatch, which fails deserialization of the entire result over this non-critical field. This makes deserialization of "counts" size-tolerant via SizeTolerantDeserializeArray, which overrides only the parse callback: extra trailing elements are dropped and missing trailing elements are zero-filled, so a length mismatch no longer fails deserialization. Serialization and comparison are inherited from Array<T, kSize> unchanged. The serialized width is intentionally kept at kNumOfReasons - 1 here, so this change does not alter what is written -- only the read path becomes tolerant. Widening the serialized width to the full kNumOfReasons is done as a separate follow-up change, which must not land until this tolerant read has been deployed to every primary (including rollback targets), so that no reader still rejects a wider result. == Test plan == **Legend** CompactionReason is an enum whose last entry is kNumOfReasons (currently 21). CompactionStats::counts is an int[kNumOfReasons] holding one count per reason; it is serialized into the remote compaction result and read back by the primary. - "narrow" = kNumOfReasons - 1 counts (omits the newest reason); what current code writes. - "wide" = kNumOfReasons counts (includes the newest reason). - strict reader: accepts only its exact expected element count; any other count fails deserialization (the plain OptionTypeInfo::Array behavior). - tolerant reader: accepts any count -- more elements than expected are dropped, fewer are zero-filled; never fails on a count mismatch (this change, SizeTolerantDeserializeArray). **Unit test** CompactionServiceTest.CompatCheckCountsWidthTolerance -- verifies that deserializing "counts" tolerates a serialized element count equal to, one greater than, or one less than the reader's expected width (extras dropped, missing zero-filled), and that the binary serializes exactly the expected number of elements. **Manual cross-version verification (ldb remote_compaction)** Test 1: baseline (unchanged behavior) -- expect-narrow strict-read primary reads narrow counts written by a worker ``` $ strict_narrow remote_compaction_worker --db=$DB --job_dir=$JOB & $ strict_narrow remote_compaction_primary --db=$DB --job_dir=$JOB remote_compaction_worker: OK (4913 bytes result) remote_compaction_primary: OK $ grep 'remote compaction result' $DB/LOG [default] [JOB 3] Received remote compaction result, output path: /tmp/cs_run/output, files: 000022.sst ``` => PASS: the primary parsed and installed the remote result. Test 2: expect-narrow tolerant-read primary reads the narrow counts a worker writes ``` $ strict_narrow remote_compaction_worker --db=$DB --job_dir=$JOB & $ tolerant_narrow remote_compaction_primary --db=$DB --job_dir=$JOB remote_compaction_worker: OK (4912 bytes result) remote_compaction_primary: OK $ grep 'remote compaction result' $DB/LOG [default] [JOB 3] Received remote compaction result, output path: /tmp/cs_run/output, files: 000022.sst ``` => PASS: the primary parsed and installed the remote result. Test 3: expect-narrow tolerant-read primary reads the wide counts a worker writes (extra reason dropped) ``` $ tolerant_wide remote_compaction_worker --db=$DB --job_dir=$JOB & $ tolerant_narrow remote_compaction_primary --db=$DB --job_dir=$JOB remote_compaction_worker: OK (4915 bytes result) remote_compaction_primary: OK $ grep 'remote compaction result' $DB/LOG [default] [JOB 3] Received remote compaction result, output path: /tmp/cs_run/output, files: 000022.sst ``` => PASS: the primary parsed and installed the remote result. Test 4: expect-wide, tolerant primary reads the narrow counts a worker writes (missing reason zero-filled) ``` $ strict_narrow remote_compaction_worker --db=$DB --job_dir=$JOB & $ tolerant_wide remote_compaction_primary --db=$DB --job_dir=$JOB remote_compaction_worker: OK (4911 bytes result) remote_compaction_primary: OK $ grep 'remote compaction result' $DB/LOG [default] [JOB 3] Received remote compaction result, output path: /tmp/cs_run/output, files: 000022.sst ``` => PASS: the primary parsed and installed the remote result. Test 5: expect-wide, tolerant primary reads a wide counts a worker write ``` $ tolerant_wide remote_compaction_worker --db=$DB --job_dir=$JOB & $ tolerant_wide remote_compaction_primary --db=$DB --job_dir=$JOB remote_compaction_worker: OK (4917 bytes result) remote_compaction_primary: OK $ grep 'remote compaction result' $DB/LOG [default] [JOB 3] Received remote compaction result, output path: /tmp/cs_run/output, files: 000022.sst ``` => PASS: the primary parsed and installed the remote result. Reviewed By: jaykorean Differential Revision: D106321320 fbshipit-source-id: fc1bd66a4d11ac6bf38dff6aa214ebafab75f3d0 |
||
zaidoon
|
7affaee1c4 |
Add use_direct_io_for_compaction_reads option (#14743)
Summary: Adds a new `DBOption use_direct_io_for_compaction_reads` (default false). When on, compaction-input SST files are opened with `O_DIRECT` so the sequential read-once data from compaction doesn't pollute the OS page cache and evict the hot user-read working set. User reads keep going through the buffered fast path. This protects user-read tail latency on write-heavy workloads without forcing user reads onto the existing global `use_direct_reads` knob (which pays in throughput and P50 — see the bench below). The interesting bit is that just flipping the FileOptions returned by `FileSystem::OptimizeForCompactionTableRead` doesn't actually trigger `O_DIRECT` at the kernel level. The TableCache (and `FileMetaData::pinned_reader`) is already holding buffered handles opened at flush time or at `DB::Open` via `LoadTableHandlers`. When compaction asks for an iterator, it gets back the cached buffered handle and the kernel never sees the `O_DIRECT` flag. So this PR also adds a small bypass path: - `TableCache::FindTable` / `NewIterator` learn a `open_ephemeral_table_reader` mode. When set, the pinned-reader fast path and the shared cache are skipped, `GetTableReader` is called directly with the caller's FileOptions, and ownership of the freshly opened TableReader is handed back via a `unique_ptr`. The iterator takes ownership via `RegisterCleanup` and frees the reader on destruction. - `VersionSet::MakeInputIterator` and `LevelIterator` plumb the flag through both L0 and L1+ compaction-input paths. - `CompactionJob::ProcessKeyValueCompaction` turns the bypass on when `use_direct_io_for_compaction_reads` is set, the global `use_direct_reads` is off, and `OptimizeForCompactionTableRead` produced `use_direct_reads=true` in the compaction-read FileOptions. The option is opt-in: when off, nothing changes for existing users. When on, only the compaction-input opens take the bypass path; user reads keep hitting the TableCache and the buffered fast path normally. There's also a small db_bench helper in the same PR: a new `--bgwriter_num` flag that lets the writer thread in `readwhilewriting` (and the other "while writing" variants) spread its puts across `[0, bgwriter_num)` instead of `[0, num)`. Without this the readers and writer share a key range and you can't have both a hot read subset and meaningful compaction work — this lets you have both. ### Benchmark Setup: Ubuntu 24.04 (kernel 7.0.5, OrbStack Linux VM on Apple Silicon), 14 vCPUs, virtio-blk disk, btrfs. MGLRU disabled (`echo 0 > /sys/kernel/mm/lru_gen/enabled`) so the kernel uses the classic active/inactive LRU. 14 GB DB (3.5M keys × 4 KB values), no compression. Each measurement run is pinned to a 1 GB cgroup via `systemd-run --scope -p MemoryMax=1G -p MemorySwapMax=0`. Page cache is dropped between configs. db_bench is Release build. Workload: `readwhilewriting` for 120s. 4 reader threads doing random reads over a hot key subset, plus 1 writer thread spreading overwrites across the full 3.5M-key keyspace (via `--bgwriter_num=3500000`) throttled at 200 MB/s, so there's continuous compaction running while the readers go. The size of the hot reader subset relative to available page cache controls how visible the optimization is. The Cassandra blog ([Lightfoot 2026](https://lightfoot.dev/direct-i-o-for-cassandra-compaction-cutting-p99-read-latency-by-5x/)) documented the same thing: biggest wins when the hot set is big enough to actually compete for cache, smaller wins when the hot set trivially fits, neutral when the hot set is way bigger than cache. So I ran two hot-set sizes. #### Small hot set: ~30 MB (~3% of the 1 GB cgroup) — N=5 iterations, mean (CV) `--num=7500`. The hot set is small enough that the page cache holds it without much trouble even under compaction, so the wins here are real but on the modest side. | Config | Throughput (ops/s) | Read P50 (µs) | Read P99 (µs) | Read P99.9 (µs) | Read P99.99 (µs) | |---|---|---|---|---|---| | buffered (default) | 233,477 (8.2%) | 16.09 | 82.24 | 721.0 | 2,102.5 | | direct_compaction_writes_only (existing knob alone) | 287,405 (2.8%) — **+23.1%** | 13.00 (−19.2%) | **66.77 (−18.8%)** | 553.9 (−23.2%) | 1,787.6 (−15.0%) | | direct_compaction_read_only (new knob alone) | 250,669 (2.4%) — +7.4% | 14.16 (−12.0%) | 102.99 (+25.2%) | 689.8 (−4.3%) | 1,801.3 (−14.3%) | | direct_compaction_read_write (new + existing, recommended) | 277,920 (3.3%) — **+19.0%** | **12.99 (−19.3%)** | 84.23 (+2.4%) | 613.4 (−14.9%) | **1,738.2 (−17.3%)** | | use_direct_reads=true (existing global) + write-side | 249,014 (2.5%) — +6.7% | 15.95 (−0.9%) | 68.78 (−16.4%) | **450.8 (−37.5%)** | 1,814.5 (−13.7%) | CV is 2.4–3.3% on the optimized configs (8.2% on buffered), so the deltas are real. With a hot set this small, the existing `use_direct_io_for_flush_and_compaction` knob is already doing most of the work — the new flag's main extra contribution here is P99.99 (combined wins it by ~2 points vs writes-only-alone). Worth noting: the new flag *alone* (without the existing write-side flag) improves P99.99 but regresses P99 by 25% on this small-hot-set workload, because direct compaction reads lose kernel readahead and compaction-output writes are still hitting the page cache. That regression goes away once you combine with the existing write-side flag, or once the hot set is bigger (see next table). So if you're using just one knob, use the existing one. If you're using this PR's flag, pair it with `use_direct_io_for_flush_and_compaction=true`. #### Larger hot set: ~400 MB (~40% of cache) — N=5 iterations, mean (CV) `--num=100000`. This is the case the Cassandra blog calls out — hot set big enough to actually fight compaction for cache. Their analogous setup (1M hot partitions, ~33% hot/cache) reported 1.93× p99 improvement. Numbers here are the headline: | Config | Throughput (ops/s) | Read P50 (µs) | Read P99 (µs) | Read P99.9 (µs) | Read P99.99 (µs) | |---|---|---|---|---|---| | buffered (default) | 68,959 (7.7%) | 44.81 | 541.22 | 2,225.2 | 11,334.5 | | direct_compaction_writes_only (existing knob alone) | 73,973 (10.3%) — +7.3% | 42.22 (−5.8%) | 456.27 (−15.7%) | 2,016.9 (−9.4%) | 9,190.0 (−18.9%) | | direct_compaction_read_only (new knob alone) | 84,337 (2.3%) — +22.3% | 38.66 (−13.7%) | 386.97 (−28.5%) | 1,644.8 (−26.1%) | 4,837.9 (−57.3%, 2.34×) | | direct_compaction_read_write (new + existing, recommended) | **104,923 (8.4%) — +52.2%** | **34.26 (−23.5%)** | **290.97 (−46.2%)** | **1,143.4 (−48.6%)** | **3,080.3 (−72.8%, 3.68×)** | | use_direct_reads=true (existing global) + write-side | 71,598 (9.1%) — +3.8% | 51.33 (+14.5%) | 297.91 (−45.0%) | 1,663.6 (−25.2%) | 6,530.0 (−42.4%) | Combined config gets a 3.68× p99.99 win, 1.86× p99, p50 down 23%, throughput up 52%. Same shape as the Cassandra blog's 1.93× p99 result — the improvement just lands at deeper percentiles for us because RocksDB's baseline data path is roughly 40× faster than Cassandra's (their buffered p99 was 35 ms, ours is 0.54 ms), so the cache-miss tail is further out. A few things worth calling out from this table: - The new flag is doing real work on top of the existing write-side flag here, not just shifting things around. Combined throughput is +42% over `direct_compaction_writes_only` alone, and combined p99.99 is 3× better. The existing knob alone gives a fairly modest +7% throughput / -19% p99.99 in this case — there's a clear gap that the new flag fills. - The new flag *alone* (no existing write-side flag) is also a real improvement here: +22% throughput, p99.99 down 57%. The P99 regression we saw in the small-hot-set case is gone, because the cache-protection effect now dominates the lost-readahead cost. - `use_direct_reads=true` (the existing global flag) actually regresses P50 by 14.5% in this workload — taking user reads off the page cache hurts you when the hot data could have been cached. It also gets the worst throughput of any direct config. It's not an equivalent way to get these gains. ### `compaction_readahead_size` matters when this flag is on Direct I/O bypasses kernel readahead, so RocksDB's own `DBOptions::compaction_readahead_size` becomes the only prefetch the iterator has. The default of 2 MB is enough and real users will get it automatically. **But `db_bench`'s `--compaction_readahead_size` CLI default is 0**, which defeats prefetch and makes direct compaction look slower than it actually is. If you're reproducing the numbers above, pass `--compaction_readahead_size=2097152` (or larger). - Recommended production config is `use_direct_io_for_compaction_reads=true` + `use_direct_io_for_flush_and_compaction=true`. Strongest configuration at every percentile and throughput in both benches. - The new flag is the read-side counterpart to `use_direct_io_for_flush_and_compaction`, which handles compaction-write cache pollution. They address different sources of pollution and compose. The gap between "combined" and "writes-only-alone" is 17 percentage points on p99.99 in the small-hot-set bench and 54 points in the larger one, so the new flag is contributing real value, especially as the hot set grows. - The new flag alone is also a real improvement when the hot set is big enough to compete with cache (+22% throughput, 2.34× p99.99 in the larger-hot-set bench). On a very small hot set it improves p99.99 but regresses p99, so pairing with the existing write-side flag is safer. - The benefit is workload-dependent. Small hot sets get modest tail-latency wins. Hot sets sized to actually compete for cache get the big multi-percentile wins shown above. Hot sets bigger than cache (not benched here but covered in the Cassandra blog) see no change either way — every read misses regardless. ### Reproducing Any Linux host (or a Linux VM on macOS via OrbStack / Multipass / lima): ```bash sudo apt-get install -y build-essential clang cmake git pkg-config \ libgflags-dev libsnappy-dev zlib1g-dev libbz2-dev liblz4-dev libzstd-dev cmake -DCMAKE_BUILD_TYPE=Release -DPORTABLE=1 -DWITH_GFLAGS=1 -DWITH_TESTS=0 .. make -j db_bench echo 0 | sudo tee /sys/kernel/mm/lru_gen/enabled ``` Build the source DB once, unrestricted memory: ```bash ./db_bench --benchmarks=fillrandom,compact,waitforcompaction,stats \ --db=/path/to/source_db --num=3500000 --key_size=16 --value_size=4096 \ --write_buffer_size=16777216 --target_file_size_base=16777216 \ --max_background_jobs=4 --compression_type=none --cache_size=4194304 \ --max_bytes_for_level_base=67108864 --disable_wal=1 --sync=0 ``` For each config, copy `source_db -> scratch_db`, run `sync && echo 3 > /proc/sys/vm/drop_caches`, then: ```bash sudo systemd-run --scope -p MemoryMax=1G -p MemorySwapMax=0 \ ./db_bench --use_existing_db=1 \ --benchmarks=readwhilewriting,stats --db=/path/to/scratch_db \ --threads=5 --duration=120 --statistics=true --histogram=1 \ --num=7500 --bgwriter_num=3500000 \ --key_size=16 --value_size=4096 \ --write_buffer_size=16777216 --target_file_size_base=16777216 \ --max_background_jobs=4 --compression_type=none \ --cache_size=4194304 --open_files=200 \ --skip_stats_update_on_db_open=true \ --max_bytes_for_level_base=67108864 \ --benchmark_write_rate_limit=209715200 \ --compaction_readahead_size=2097152 \ --rate_limiter_bytes_per_sec=0 \ --use_direct_reads={true|false} \ --use_direct_io_for_compaction_reads={true|false} \ --use_direct_io_for_flush_and_compaction={true|false} ``` For the larger hot-set table, change `--num=7500` to `--num=100000`. The five configs in the tables: - `buffered`: all three flags false. - `direct_compaction_writes_only`: `use_direct_io_for_flush_and_compaction=true`, the other two false. This is what users have today without this PR. - `direct_compaction_read_only`: `use_direct_io_for_compaction_reads=true`, the other two false. - `direct_compaction_read_write`: `use_direct_io_for_compaction_reads=true`, `use_direct_io_for_flush_and_compaction=true`, `use_direct_reads=false`. **Recommended.** - `direct_all`: `use_direct_reads=true`, `use_direct_io_for_flush_and_compaction=true`, `use_direct_io_for_compaction_reads=false`. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14743 Reviewed By: pdillinger Differential Revision: D108017601 Pulled By: xingbowang fbshipit-source-id: 4039d490d7e77b476db7a477a2f3d24738db6336 |
||
|
Anand Ananthabhotla
|
828f6d189e |
Mark remote filtered input counts as inaccurate (#14838)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14838 Remote compaction can pre-filter whole input files when universal compaction sees standalone range tombstones. In that case the worker's iterator count no longer describes the full original input set, so returning `has_accurate_num_input_records=true` can trigger a false-positive input-record verification on the primary. Mark the remote count inaccurate whenever the reconstructed compaction filtered input files before iteration, and add a regression test covering that path. Reviewed By: jaykorean Differential Revision: D107961493 fbshipit-source-id: 0c302081964ab1d894b34bddd2c55f5a4f06752f |
||
|
Xingbo Wang
|
2d68f30425 |
Support remote file system for blob direct write file (#14585)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14585 Adds typed file-open contracts and open-file size handling needed for blob direct-write files on remote or SHM-backed file systems. This diff: - Adds typed `FileOpenContract` semantics to RocksDB `FileOptions`, including `kNoReopenForWrite`, `kNoReadersWhileOpenForWrite`, bitwise helpers, and constructor propagation from `EnvOptions` to `FileOptions`. - Marks SST/table output creation paths with both `kNoReopenForWrite` and `kNoReadersWhileOpenForWrite`, including builder output, compaction output, and external `SstFileWriter` output. - Marks blob write paths with the appropriate contracts: regular blob file builders use both `kNoReopenForWrite` and `kNoReadersWhileOpenForWrite`, while blob direct-write partition output uses `kNoReopenForWrite` so active readers can still observe SHM-backed data through the read path. - Adds `GetFileSizeFromOpenFileOrPath` so readers can prefer an already-open `FSRandomAccessFile::GetFileSize` result and fall back to path-level `FileSystem::GetFileSize` according to the caller's fallback policy. - Updates blob file reader and table footer reader size checks to use the new open-file-or-path size helper, preserving malformed-file validation while supporting file systems where path-level size is not the only valid source. - Extends `FaultInjectionTestFS` to track file-open contracts, reject readers while a no-readers writer is open, reject reopened data writes for `kNoReopenForWrite`, allow `SyncFile` to use a reopened handle, and preserve contract state across create, reopen, reuse, rename, link, reset, and delete paths. - Keeps `IOStatus` return paths simple by returning status locals directly in the file-size helper and fault-injection contract validation paths. - Adds clang-format-compliant tests covering file-open contract enforcement, `SyncFile` being allowed while reopened data writes are rejected, blob direct-write behavior, blob reader behavior, and the new file-size fallback helper behavior. - Leaves WAL and MANIFEST contract assignment for follow-up work because live WAL/MANIFEST readers and `reuse_manifest_on_open` need separate handling before RocksDB can safely claim stronger file-open contracts for those file classes. Reviewed By: anand1976, pdillinger Differential Revision: D100002686 fbshipit-source-id: 26b4021bc13333ede1077f0ff8cbd71335c5f294 |
||
|
Peter Dillinger
|
8053b9414f |
Change default compression from Snappy to LZ4 (#14818)
Summary: The historical default block compression `kSnappyCompression` dates to when Snappy was the obvious fast/cheap choice. On modern server CPUs LZ4 matches or beats Snappy on compression ratio while decompressing substantially cheaper, so it is a better default. This changes the default `ColumnFamilyOptions::compression` to `kLZ4Compression`, with a runtime fallback of LZ4 -> Snappy -> none depending on what is compiled into the binary (new `GetDefaultCompressionType()` in util/compression.h). Only column families that do not explicitly set `compression` are affected (including compaction output when `CompactionOptions::compression == kDisableCompressionOption`), and only newly written SST files; existing data is read as before. Doc comments, the Java bindings, and the sorted_run_builder example are updated to describe the new default. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14818 Test Plan: Adjusted two unit tests that implicitly depended on the old Snappy default to pin `compression = kSnappyCompression`: db_iterator_test's ReadAhead (readahead byte thresholds assume Snappy-sized files) and compaction_service_test's CustomFileChecksum (LSM shape after auto-compaction determined whether a manual CompactRange had work to do). This change is primarily validated by performance testing. Added a `compressreject` db_bench benchmark (output buffer sized just below the predicted compressed size) to measure the cost of attempting then declining compression, alongside `compress`/`uncompress`. Both db_bench and sst_dump compress/decompress a modest block at a time, as in a real workload. sst_dump on SST files from production workloads (4 files, 16KB blocks, single thread) on recent server-class AMD, Intel, and ARM CPUs, LZ4 vs Snappy: - Compression ratio: comparable; LZ4 is slightly smaller on the more compressible files (up to ~8%) and within ~2% on the rest. - Compression (write) CPU: a wash, within ~2% either direction. - Decompression (read) CPU: the clear win -- Snappy costs ~1.2x-1.5x as much as LZ4, i.e. LZ4 saves ~25-30% read CPU, consistently across AMD, Intel, and ARM. db_bench synthetic workload (100-byte values), at 1 / 12 / 160 threads, LZ4 vs Snappy: - Compression throughput: LZ4 ~10-30% higher. - Decompression throughput: LZ4 much higher, and the advantage grows with core count -- from ~+12% at 12 threads to ~+45-50% at 160 threads, i.e. better multi-core scaling. - Rejection (insufficient ratio) path: comparable; LZ4 ~15-18% faster on compressible-but-rejected blocks and Snappy within ~10% on barely-compressible blocks. No meaningful regression, confirming incompressible data is still efficiently detected and stored raw. Reviewed By: xingbowang Differential Revision: D107536490 Pulled By: pdillinger fbshipit-source-id: f8abaee630d782674778338148e36ed0c84e3661 |
||
|
Josh Kang
|
768de6e50d |
Fix false-positive corruption error with remote compaction (#14808)
Summary: Fix a false-positive compaction corruption error in the remote compaction path. Remote workers can mark `CompactionJobStats::num_input_records` as unreliable when input iteration uses seek/skip behavior, but the remote result serialization was dropping `has_accurate_num_input_records`. The primary then deserialized the flag as its default `true`, trusted an unreliable zero input-record count, and raised `Compaction number of input keys does not match number of keys processed`. This change serializes the accuracy flag with the rest of `CompactionJobStats` so the primary preserves the remote worker's "do not verify this count" signal. It also adds a targeted regression test and a release note for the bug fix. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14808 Test Plan: New regression test that triggers corruption error without the fix by modifying `has_accurate_num_input_records=false` Reviewed By: xingbowang Differential Revision: D107128357 Pulled By: joshkang97 fbshipit-source-id: 3243c9c2ab18534652737f7410fe3c51724db549 |
||
|
Josh Kang
|
26a501b5d1 |
Fix tiered compaction incorrectly moving range tombstones upwards (#14795)
Summary: Fixes an off-by-one bug in how sequence numbers are handled when splitting range tombstones across output levels in per-key-placement (tiered) compaction. The bug was either introduced or propagated in https://github.com/facebook/rocksdb/issues/13256. Point entries move to proximal output only when their sequence number is strictly greater than `proximal_after_seqno_`, but range tombstones were previously split using an inclusive lower bound at that same seqno. That allowed a range tombstone at the boundary to be emitted to the proximal level while point keys at the same seqno stayed in the last level, which could create overlapping files in the proximal level (caught by `force_consistency_checks` as `L<n> has overlapping ranges`). This matters when `proximal_output_range_type_` is `kNonLastRange`: the compaction only owns the selected proximal-level input range, so existing last-level data at the split boundary must stay in the last level. In `kFullRange`, the compaction owns the relevant proximal-level range, so newer last-level data can be safely emitted to proximal output. The fix splits range tombstones at `proximal_after_seqno_ + 1` (saturating at `kMaxSequenceNumber`), so the half-open `[lower, upper)` tombstone filter lands on the same boundary as the strict `seqno > proximal_after_seqno_` rule used for point keys. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14795 Test Plan: New regression test `PrecludeLastLevelTestBase.RangeDelAtProximalSeqnoBoundaryStaysInLastLevel` uses a targeted manual compaction to exercise the `kNonLastRange` case directly, verifying a boundary range tombstone stays in the last level instead of widening the proximal output range. Worked example: ```text Initial state: L6 (last level): Put(Key 2)s1, Put(Key 12)s2, RangeDel[Key 2, Key 12)s3 L5 (proximal): file A [Key 0 .. Key 4]s4-s5 file B [Key 5 .. Key 9]s6-s7 preclude_last_level_min_seqno is forced to 0 via sync point. Manual CompactFiles selects only L5 file B + the L6 file (output level 6). Only part of the proximal level is selected, so this is the kNonLastRange case: max_last_level_seqno = 3 proximal_after_seqno_ = max(0, 3) = 3 Point keys Key 5 (s6) and Key 9 (s7): both seqno > 3 -> proximal output (L5) OK Range tombstone s3: Old (buggy): proximal keep range [3, MAX) includes s3, so the tombstone is emitted to the proximal output. The output is built from L5 input file B [Key 5 .. Key 9], but the tombstone covers [Key 2, Key 12), so the proximal output file starts at Key 2 and spills past Key 5 into the existing, untouched L5 file A [Key 0 .. Key 4]. Two overlapping files in L5 -> Corruption. New (fixed): split at proximal_after_seqno_ + 1 = 4. proximal keep [4, MAX) excludes s3; last-level keep [0, 4) includes s3 -> tombstone stays in the last level (L6). OK ``` Verification (debug build, `make -j64 tiered_compaction_test`): - Without the fix, the test fails at the `CompactFiles` call: ``` tiered_compaction_test.cc:2940: Failure Corruption: force_consistency_checks(DEBUG): VersionBuilder: L5 has overlapping ranges: file https://github.com/facebook/rocksdb/issues/11 largest key: Key(4) seq:5, type:1 (Put) vs. file https://github.com/facebook/rocksdb/issues/17 smallest key: Key(2) seq:3, type:15 (range deletion) ``` - With the fix, the test passes (range deletions absent from L5, still present in L6). Reviewed By: pdillinger Differential Revision: D106528471 Pulled By: joshkang97 fbshipit-source-id: a5f99b426d3a7a6253bc1972cf8cb60d1cb85089 |
||
|
Hui Xiao
|
a0db079fdd |
Fall back to local compaction and report kUseLocal on remote result parse failure (#14799)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14799 When `CompactionService::Wait()` returns `kSuccess` but `CompactionServiceResult::Read()` fails before the primary renames any remote output file from `CompactionServiceResult::output_path` into the DB directory, fall back to local compaction for the same job and notify the service with `OnInstallation(..., kUseLocal)`. At that point the remote SSTs are still in the service-managed output directory recorded in `CompactionServiceResult::output_path`, and the primary has not installed any of them into the DB yet. Update `CompactionServiceTest.InvalidResultFallsBackToLocal` to verify the fallback completes successfully, preserves the data, and invokes `OnInstallation()` exactly once with `kUseLocal`. Reviewed By: jaykorean Differential Revision: D106321319 fbshipit-source-id: 39d9206f0e3f62612a52c03462bd1bee69020b80 |
||
|
Peter Dillinger
|
805a476a5f |
Add OnCompactionPreCommit listener callback (#14740)
Summary: Adds a new `EventListener::OnCompactionPreCommit` callback that fires after a compaction job's output files are written but *before* the manifest write commits the new Version. At that point input files still have `FileMetaData::being_compacted == true`, so listeners that maintain bookkeeping of "files currently being compacted" can clean up that state without racing the compaction picker. A check implemented by a Meta-internal RocksDB user crashes when the same file appears as input to two concurrent compactions. Tracking that set in `OnCompactionBegin` / `OnCompactionCompleted` produces false positives because `Compaction::ReleaseCompactionFiles()` flips `being_compacted` back to false before `OnCompactionCompleted` fires, so another thread can pick the same file and trigger `OnCompactionBegin` before the previous compaction's `Completed` callback runs. Doing the cleanup in `OnCompactionPreCommit` closes that race. Default implementation is a no-op, so no API break. A trivial refactoring to split PerformTrivialMove ensures data is populated for the new callback, while calling back before the trivial move compaction is committed. Bonus: `CLAUDE.md` update for when to call `make clean` as I've recently had it get thoroughly confused TWICE mixing build modes. Task: T269479969 Pull Request resolved: https://github.com/facebook/rocksdb/pull/14740 Test Plan: - New unit test in `db/listener_test.cc` checks that `OnCompactionPreCommit` fires strictly between `OnCompactionBegin` and `OnCompactionCompleted` for the same compaction, and that input files still have `being_compacted == true` at the time it fires. - Crash test: adds a concurrent-compaction sanity check to `db_stress`'s listener resembling the Meta-internal intended usage: tracks input file numbers from `OnCompactionBegin` to `OnCompactionPreCommit`, aborting if the same file appears as input to two concurrent compactions. Also checks other ordering constraints and checks for "leaks" from possible failure to call OnCompactionPreCommit(). Exercises all compaction styles and trivial-move/FIFO paths under load. Reviewed By: hx235 Differential Revision: D105065326 Pulled By: pdillinger fbshipit-source-id: 8f606a7c0fac899574e7340c600b71fed902394a |
||
|
Peter Dillinger
|
795f3bd61f |
Fix check-sources.sh non-ASCII check and remove non-ASCII from sources (#14729)
Summary: The non-ASCII character check in check-sources.sh used git grep -P (Perl regex), which requires git compiled with PCRE support. On systems without it, the command fails with exit code 128, which is != 1 (no match), so the check always reported a violation -- effectively dead. Even in CI where git has PCRE2 support, the check was silently broken: git grep -P uses PCRE2 in UTF mode by default, which interprets [\x80-\xFF] as a Unicode codepoint range (U+0080 to U+00FF). Characters like em-dash (U+2014), arrows (U+2192), and math symbols (U+2248, etc.) fall outside that range and were not detected. Only Latin-1 Supplement characters (U+0080-U+00FF) would have been caught. Replace with LC_ALL=C git grep using bash $'[\x80-\xff]' literal byte range, which works with basic regex in the C locale, and replace all non-ASCII characters in non-excluded source files: - em-dash to -- - arrow to -> - math symbols to ASCII equivalents (~=, <=, >=) - box-drawing characters to ASCII art Also exclude .github/ from the check, as scripts there can use non-ascii without disrupting RocksDB builds on non-UTF-8 systems. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14729 Test Plan: manual / CI (make check-sources passes clean) Reviewed By: hx235 Differential Revision: D104692574 Pulled By: pdillinger fbshipit-source-id: 1d884c21056dcd83558b825a04b867f1c08e3f45 |
||
xingbowang
|
e0d549fbba |
Keep remote compaction stats serialization compatible with 11.1 (#14712)
Summary:
- Reapply
|
||
|
Xingbo Wang
|
d8fc727592 |
Revert temporary 11.1 remote compaction stats serialization workaround (#14663)
Summary: There is a separate way to handle incompatibility. Revert this. - revert the temporary workaround from https://github.com/facebook/rocksdb/issues/14656 that serialized `InternalStats::CompactionStats::counts` with the pre-11.2 compaction-reason count - restore remote compaction stats serialization to use the full `CompactionReason::kNumOfReasons` array size again ## Context https://github.com/facebook/rocksdb/issues/14656 temporarily shrank the serialized `counts` array to keep remote compaction metadata readable across the 11.1/11.2 boundary. This follow-up removes that special case because the compatibility issue is being handled separately. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14663 Test Plan: - Not run in this metadata-prep task Reviewed By: jaykorean Differential Revision: D102376258 Pulled By: xingbowang fbshipit-source-id: 55693ea25a97b70de9d53b0d6eebcbd88a325b1c |
||
|
Xingbo Wang
|
c4941760c9 |
Keep remote compaction stats serialization compatible with 11.1 (#14656)
Summary: - Keep remote compaction stats serialization compatible with RocksDB 11.1. - Serialize `InternalStats::CompactionStats::counts` using the pre-11.2 compaction-reason count so remote compaction metadata remains readable across the version boundary. - Preserve the existing formatting-only follow-up commit on the branch. ## Notes - Needs a better mechanism to make OptionTypeInfo::Array ser/des friendly. Will follow up after 11.2 release. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14656 Reviewed By: pdillinger Differential Revision: D102071507 Pulled By: xingbowang fbshipit-source-id: 6eeb87d86494f528f0d1ce46ce4c3e8e7dd2da53 |
||
|
Xingbo Wang
|
6ff96384dd |
db_iter: eagerly resolve blob-backed iterator columns (#14632)
Summary:
Restore the iterator contract for blob-backed wide-column entities by eagerly materializing all blob-backed columns before a `DBIter` entry is exposed as valid. Also capture similar error inside compaction filter V4, so that the error is captured a surfaced up like older compaction filter V3.
## Problem
Lazy wide-column blob resolution let `DBIter` report `Valid() == true` before all columns were actually prepared. Callers could read `value()` successfully, then hit a blob-read failure in `columns()`, which flipped the iterator to invalid after the entry had already been observed as valid.
## Solution
- Restore back to old behavior.
- Resolve and materialize all blob-backed wide columns during iterator positioning.
- Keep `columns()` as a pure accessor once `Valid()` is true.
- Add coverage for eager blob resolution on iterator scans.
- Add coverage for blob resolution failures invalidating the iterator before the entry is exposed.
- Capture the same error in compaction filter and set db to bg error status if the error is not retriable.
## Next step
If we want lazy iterator resolution in the future, we will need an API that can surface lazy resolution failures explicitly instead of hiding I/O and status transitions inside value() or columns().
## Testing
- `make -j14 db_wide_blob_direct_write_test`
- `timeout 60s ./db_wide_blob_direct_write_test --gtest_filter='*DirectWriteIteratorValueScanEagerlyResolvesBlobColumns:*DirectWriteIteratorBlobResolutionErrorInvalidatesEntry'`
## Task
T265294130
# Bonus fix
## Non-compaction blob filtering fixes
### 1. Flush-time wide-entity lazy blob resolution now works correctly
Blob direct-write can leave wide-entity blob references in memtables before flush. Flush already runs through `CompactionIterator`, but outside a real compaction it did not have blob read support. As a result, `FilterV4` lazy resolution on flush could fail immediately with
`NotSupported("Blob fetcher not available")` instead of either:
- resolving the blob successfully, or
- surfacing the real blob read error and failing the flush
This stack fixes that by plumbing blob read support into non-compaction table-file creation. `BuildTable()` now gives `CompactionIterator` enough context to construct a `BlobFetcher` for flush/recovery table building, including write-path fallback for direct-write blob files
that are not yet manifest-visible.
With that in place:
- flush-time `FilterV4` can lazily resolve blob-backed wide columns
- if resolution fails, the failure is latched and flush fails after `FilterV4()` returns
- `bg_error` is set instead of silently preserving the entry
### 2. Plain blob fallback outside compaction now uses the resolved user value
For plain blob-backed values, `FilterBlobByKey()` is allowed to return `kUndetermined`, which is supposed to fall back to the normal value-based filter path. That fallback worked in compaction, but the non-compaction `BuildTable()` path still treated plain blob indexes as
unsupported/corrupt because it could not read the blob.
This stack fixes that behavior for flush/recovery table-file creation:
- when `FilterBlobByKey()` returns `kUndetermined`, RocksDB now eagerly resolves the plain blob value
- `FilterV2`/`FilterV3`/`FilterV4` then see the actual user value as `ValueType::kValue`, not the `BlobIndex` encoding
- legacy filters therefore behave the same way outside compaction as they already do inside compaction
Wide-column entities still use the `FilterV4` lazy resolver path. The plain-blob fix is specifically about restoring the documented fallback behavior for blob-backed `kValue` records outside compaction.
## Why this matters
Without these last two commits, flush-time filtering still had two inconsistent behaviors:
- wide-entity lazy resolution could not actually read blob-backed columns in the direct-write case
- plain blob-backed values could not fall back to normal value-based filtering outside compaction
So even after fixing iterator validity and compaction error propagation, non-compaction table-file creation still had remaining contract holes. These commits make flush/recovery behave consistently with compaction.
## Coverage added
The tests added in these commits cover:
- flush-time plain-blob `FilterV3` fallback using the resolved user value
- flush-time plain-blob `FilterV4` fallback using the resolved user value
- flush-time direct-write wide-entity lazy resolver success
- flush-time direct-write wide-entity lazy resolver failure on missing blob file, including flush failure and `bg_error` latching
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14632
Reviewed By: anand1976
Differential Revision: D101425233
Pulled By: xingbowang
fbshipit-source-id: 695b2df5189033d30309b35849815e31fd965664
|
||
|
Anand Ananthabhotla
|
1cc216a45b |
Fast file open: retrieve, persist, and pass file open metadata (#14596)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14596 When DBOptions::fast_sst_open is enabled, RocksDB retrieves opaque file system metadata for SST files after flush, compaction, and external file ingestion via FSRandomAccessFile::GetFileOpenMetadata(). This metadata is persisted in the MANIFEST using a new forward-compatible NewFileCustomTag (kFileOpenMetadata = 17), and passed back to the file system via FileOptions::file_metadata on subsequent file opens. This accelerates DB open time on remote storage systems by allowing the file system to skip expensive metadata RPCs. The feature is gated by DBOptions::fast_sst_open (default false). Everything works seamlessly regardless of the option setting, file metadata support, or presence/absence of the metadata in the MANIFEST. The MANIFEST change is backward compatible - older RocksDB versions safely ignore the new tag. Reviewed By: xingbowang Differential Revision: D100220973 fbshipit-source-id: f52de9dd853a50653b3297ab4a37a868fe41cc04 |
||
|
Peter Dillinger
|
9f474a1034 |
Enforce < 4GB sizes for keys, values, and some BlockBuilder inputs (#14461)
Summary: BlockBuilder was somewhat inconsistent in its treatment of Slices whose size exceeds 4GB, which in a random corruption case could lead to (for example) serializing only the bottom 32 bits of a value size (uncorrupted) but appending the full multi-GB (corrupted) size. Because this is inner loop code, we don't want to pay CPU for extra conversions, data movements, or Status plumbing (already ruled out by Xingbo). In this change we make BlockBuilder more internally consistent and lift the 32-bit size requirement to callers (of a few functions specifically, for now). To ensure that's satisfied, I've added additional checks near the perimeter of RocksDB to ensure keys and values do not exceed 4GB, plus an extra random corruption or backstop check in BlockBasedTableBuilder. In detail, BlockBuilder (block_builder.cc/h): * Add API comments documenting the < 4GB assumption on Add/AddWithLastKey * Add debug assertions verifying input slice sizes < 4GB * Simplify AddWithLastKeyImpl to use uint32_t locals, reducing static_cast * Use only bottom 32 bits when appending derivative slices for consistency * Update MaybeStripTimestampFromKey to also truncate to 32-bit size * Add FIXME comments where buffer_/values_buffer_ sizes are truncated BlockBasedTableBuilder (block_based_table_builder.cc): * Add value size check (> uint32_t max) as a safety net against random corruption, since we have seen such corruptions in production WriteBatch (write_batch.cc): * Tighten existing key size checks to account for kNumInternalBytes (8-byte internal key suffix), using new kMaxWriteBatchKeySize constant * Add missing size checks to Delete, SingleDelete, and DeleteRange (both Slice and SliceParts variants) SstFileWriter (sst_file_writer.cc): * Add key and value size checks in AddImpl and DeleteRangeImpl, which bypass WriteBatch and go directly to the table builder MergeHelper (merge_helper.cc): * Add merge result size check (> uint32_t max) in both overloads of TimedFullMergeImpl, returning Corruption if exceeded * Add PartialMergeMulti result size check in MergeUntil CompactionIterator (compaction_iterator.cc): * Add size check on compaction filter output values (kChangeValue and kChangeWideColumnEntity), returning Corruption if > 4GB meta_blocks.cc: * Use Slice with uint32_t-truncated sizes in PropertyBlockBuilder::Finish to handle potentially oversized user property collector output Needed follow-up: * Check for blocks that are mis-encoded due to overflowing 32 bits for restart point offsets (and similar). See FIXME comments in block_builder.cc for why this is tricky. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14461 Test Plan: New extreme-size unit tests, along with some testing infrastructure improvements: Added test::HasBigMem() (in test_util/testharness.h) which returns true when the system has ≥128GB RAM (via sysconf(_SC_PHYS_PAGES) on Linux/macOS) or when ROCKSDB_BIGMEM_TESTS is set. All extreme-size tests use HasBigMem() to skip gracefully on smaller machines rather than being permanently disabled. Where possible, tests use MemMapping::AllocateLazyZeroed() to supply large keys/values as Slices backed by anonymous mmap (cleaner with new MemMapping::AsSlice()). On Linux, read-only access to these pages maps to the shared kernel zero page, so the source data consumes no physical RAM — only the destination copy (e.g., WriteBatch::rep_) materializes, cutting peak memory roughly in half vs. std::string. Tests (all enabled, skip via HasBigMem()): ./write_batch_test --gtest_filter='*LargeKeyValueSizeLimit*' ./external_sst_file_basic_test --gtest_filter='*LargeSizeSstFileWriter*' ./merge_test --gtest_filter='*LargeMergeResultRejected*' ./merge_helper_test --gtest_filter='*LargePartialMergeResultRejected*' ./db_compaction_test --gtest_filter='*CompactionFilterLargeValueRejected*' All 5 pass (verified on a 128GB+ machine). On smaller machines, all 5 bypass cleanly with "insufficient memory for reliable continuous testing". Reviewed By: xingbowang Differential Revision: D96521899 Pulled By: pdillinger fbshipit-source-id: 70f4b5e6a23ab074d60e653fbb7ddc5edbe162ab |
||
|
Xingbo Wang
|
3070f73e97 |
Wide-column blob separation: lazy resolution through read, compaction, and write paths (#14386)
Summary: Wide-column blob separation: lazy resolution through read, compaction, and write paths Extend blob direct write to support wide-column entities (PutEntity), and add lazy blob resolution for wide-column values across all read and compaction paths. **Write path -- PutEntity blob separation:** - BlobWriteBatchTransformer::PutEntityCF now extracts large column values (>= min_blob_size) to blob files and serializes V2 entities with BlobIndex references, matching the existing Put behavior. - Add MaybePreprocessWideColumns() static helper to share blob extraction logic between the WriteBatch transformer and the new PutEntity fast path. - Add PutEntityFastPath() in DBImpl that preprocesses columns (sort, blob extract, serialize) before calling WriteImpl, skipping the redundant WriteBatch transformation pass. Trace batch preserves the original columns. **Read path -- blob resolution for Get/MultiGet/Iterator:** - GetContext::SaveValue resolves V2 entity blob columns eagerly: for value (Get), resolves the default column's blob reference; for columns (GetEntity), resolves all blob columns and re-serializes as V1. - DBIter::SetValueAndColumnsFromEntity detects V2 entities, deserializes with DeserializeV2, and eagerly resolves all blob columns via a new ReadPathBlobResolver. Resolved values are cached in the resolver and wide_columns_ Slices point into the cache, avoiding copies. - Add ReadPathBlobResolver (new file) -- on-demand blob fetcher for the read path with per-column caching, used by both DBIter and GetContext. - BlobFetcher gains allow_write_path_fallback to read from in-flight direct-write blob files not yet visible through Version (pre-flush reads). - Memtable lookups for Get(key) on V2 entities with a blob default column now return the blob index with is_blob_index=true, triggering the existing BDW resolution in MaybeResolveWritePathValue. - MaybeResolveWritePathValue (renamed from MaybeResolveDirectWriteBlobIndex) now also resolves V2 entity blob columns for GetEntity/MultiGetEntity, re-serializing as V1 after resolution. **Compaction path -- filter, GC, and extraction:** - CompactionIterator::InvokeFilterIfNeeded handles V2 entities: FilterV3 gets eagerly-resolved column values for backward compatibility; FilterV4 gets a CompactionBlobResolver for lazy on-demand resolution. - Add CompactionFilter::FilterV4 with WideColumnBlobResolver* parameter and SupportsFilterV4() opt-in. Default delegates to FilterV3. - CompactionBlobResolver (new class) implements WideColumnBlobResolver for the compaction path with stats tracking. - ExtractLargeColumnValuesIfNeeded extracts inline columns to blob files during compaction (entities without existing blob columns only). - GarbageCollectEntityBlobsIfNeeded relocates blob values from old blob files to new ones during compaction GC, with helpers FetchBlobsNeedingGC, RelocateBlobValues, and SerializeEntityAfterGC. - PrepareOutput unified entity deserialization: single DeserializeV2 call reused by both filter and GC/extraction paths via entity_deserialized_ flag, avoiding redundant parsing. **Merge path -- V2 entity base value resolution:** - MergeHelper::MergeUntil, GetContext::MergeWithWideColumnBaseValue, and DBIter::MergeWithWideColumnBaseValue resolve V2 blob columns before calling TimedFullMerge, using ResolveEntityForMerge. **Blob garbage accounting:** - BlobGarbageMeter tracks blob file in/out flow for V2 entity blob columns via ForEachBlobFileNumber, used for accurate GC decisions. - FileMetaData::UpdateBoundaries tracks oldest_blob_file_number for V2 entities, ensuring blob files referenced by entities are not prematurely deleted. **Serialization improvements:** - WideColumnSerialization::SerializeV2Impl allocates serialized_blob_indices only for actual blob columns (not all columns) and uses autovector for name/value sizes. - Add ForEachBlobFileNumber for lightweight blob file number extraction without full deserialization. - Add ResolveEntityForMerge helper for merge-path resolution. - Add section-size validation in DeserializeV2Impl. - Add empty blob index and column type validation. - blob_column_resolver_util.h -- shared helpers (FindBlobColumn, FindInCache, CacheInlinedBlob) used by both ReadPathBlobResolver and CompactionBlobResolver. **Testing:** - db_blob_direct_write_test: end-to-end PutEntity with BDW before/after flush, verifying Get, GetEntity, MultiGetEntity, and Iterator. - db_blob_index_test: ~1550 lines covering V2 entity blob resolution through Get, GetEntity, MultiGet, Iterator, compaction filter (V3 compat and V4 lazy), merge with blob base, and compaction GC/extraction. - compaction_iterator_test: ~950 lines testing entity blob GC, extraction, filter interaction, and combined GC+filter scenarios. - db_wide_basic_test: ~1200 lines for wide-column lazy blob resolution through all read paths plus compaction round-trips. - db_open_with_config_test: ~450 lines for BDW entity config validation. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14386 Reviewed By: anand1976 Differential Revision: D99739701 Pulled By: xingbowang fbshipit-source-id: 6badd89b577f3054802eaaa654738468efb9dbdb |
||
|
Peter Dillinger
|
abc1f7ced5 |
Fix table cache leak when InstallCompactionResults fails (#14549)
Summary: When CompactionJob::Run() succeeds but Install() fails (e.g., LogAndApply MANIFEST I/O error), compact_->status was never updated with the install failure. CleanupCompaction() passed the stale OK status to SubcompactionState::Cleanup(), which skipped ReleaseObsolete -- leaking table cache entries for output files that were cached by VerifyOutputFiles but never installed into any Version. This is the same class of bug fixed in https://github.com/facebook/rocksdb/issues/14469 (where Run() failed after VerifyOutputFiles), but in the Install() failure path. The FindObsoleteFiles full-scan backstop would normally catch this, but fails under crash test metadata read fault injection (--open_metadata_read_fault_one_in), causing the TEST_VerifyNoObsoleteFilesCached assertion to fire during Close(). Fix: propagate Install()'s local status back to compact_->status before CleanupCompaction(), so Cleanup() sees the failure and calls ReleaseObsolete on the output files. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14549 Test Plan: New unit test DBCompactionTest.LeakedTableCacheEntryOnInstallFailure: - Without fix (ASAN): assertion fires -- "File 12 is not live nor quarantined" - With fix (ASAN): passes -- ReleaseObsolete properly cleans up the entry ``` COMPILE_WITH_ASAN=1 make -j db_compaction_test ./db_compaction_test --gtest_filter="DBCompactionTest.LeakedTableCacheEntry*" [ PASSED ] 2 tests. ``` Reviewed By: joshkang97 Differential Revision: D99155908 Pulled By: pdillinger fbshipit-source-id: ed5374a38d7903866a38a0fe0f5539e12321bc84 |
||
|
Josh Kang
|
5db0603613 |
Read-triggered compactions (#14426)
Summary: Add read-triggered compaction, a new feature that reduces read amplification by compacting SST files that receive high read traffic. When an SST file's read frequency (`num_reads_sampled / file_size`) exceeds a configurable threshold, it is marked for compaction to a lower level. The feature introduces two new options: a CF option `read_triggered_compaction_threshold` (default 0, disabled) and a DB option `max_periodic_compaction_trigger_seconds` (default 43200s) that controls how often the background thread re-evaluates compaction scores on quiet databases. Both options are dynamically changeable. Lowering `max_periodic_compaction_trigger_seconds` does add some overhead, but generally is minimal, so running this every couple of minutes in a production environment seems fairly reasonable. ## Key changes - **New CF option `read_triggered_compaction_threshold`** (`advanced_options.h`): When positive, files with `reads_per_byte > threshold` are marked for compaction. Files at the last non-empty level are skipped (bottommost compaction handles those separately). Marked files are sorted by hotness (reads_per_byte descending). - **New DB option `max_periodic_compaction_trigger_seconds`** (`options.h`): Replaces the hardcoded 12-hour ceiling in `ComputeTriggerCompactionPeriod()`. Essential for read-triggered compaction on quiet DBs since there are no writes to trigger score re-evaluation. - **Leveled compaction picker** (`compaction_picker_level.cc`): Adds read-triggered as the lowest-priority compaction reason in `SetupInitialFiles()`, using the existing `PickFileToCompact` helper. - **Universal compaction picker** (`compaction_picker_universal.cc`): Adds `PickReadTriggeredCompaction` as lowest priority. Refactors shared "find output level + compute overlapping inputs + create Compaction" logic from both `PickDeleteTriggeredCompaction` and `PickReadTriggeredCompaction` into `BuildCompactionToNextLevel`, handling both single-level and multi-level universal cases. - **Periodic trigger integration** (`db_impl.cc`): `TriggerPeriodicCompaction` now also fires for CFs with `read_triggered_compaction_threshold > 0`, even without time-based compaction configured. - **Stress test & db_bench support**: Both `db_stress` and `db_bench` support the new options. `db_crashtest.py` randomly enables read-triggered compaction and sets a short periodic trigger interval when enabled. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14426 Test Plan: **Unit tests**: - `compaction_picker_test` — 7 new tests: `ReadTriggeredCompactionDisabled`, `ReadTriggeredCompactionBelowThreshold`, `ReadTriggeredCompactionAboveThreshold`, `NeedsCompactionReadTriggered`, `ReadTriggeredPicksFile`, `UniversalReadTriggeredCompaction`, `ReadTriggeredSkipsLastLevel`, `UniversalReadTriggeredNoPickWhenNotMarked` - `db_compaction_test` — `ReadTriggeredCompaction` integration test verifying end-to-end behavior with sync points - Stress test coverage **Stress test**: ``` make V=1 -j "CRASH_TEST_EXT_ARGS=--duration=600 --max_key=2500000 --max_compaction_trigger_wakeup_seconds=10 --read_triggered_compaction_threshold=0.0001 --interval=600" blackbox_crash_test ``` - confirmed read triggered compactions from LOGS **Benchmark** (`db_bench`): Setup: 5M keys (100B values, 16B keys), leveled compaction, 5 levels, 4MB target file size. DB fully compacted, then 2M overlapping keys written without compaction to create L0/L1 overlap (82 files, ~294MB). LSM shape change during readrandom with read-triggered compaction: ``` BEFORE: L0=9 files (15MB), L1=4 (16MB), L2=20 (69MB), L3=49 (194MB) — 82 files, 294MB AFTER: L3=66 files (223MB) ``` | Benchmark | Config | avg ops/s | % change | |-----------|--------|-----------|----------| | readrandom (8 threads, 5M reads) | baseline (threshold=0) | 1,086,965 | — | | readrandom (8 threads, 5M reads) | threshold=0.000001, trigger=5s | 1,453,697 | **+33.7%** | Reviewed By: xingbowang Differential Revision: D97838716 Pulled By: joshkang97 fbshipit-source-id: a21fcb270c7fadd4f78d98b9c821982f220dd3f0 |
||
|
Josh Kang
|
3d99378791 |
Fix table handle leak (#14469)
Summary: Fix leaked table cache entries that cause `TEST_VerifyNoObsoleteFilesCached` assertion failure during `DB::Close()` in ASAN crash test builds (T258745630): ``` File 126519 is not live nor quarantined Assertion `cached_file_is_live_or_quar' failed. ``` When a compaction fails *after* `VerifyOutputFiles` succeeds (e.g., at `VerifyCompactionRecordCounts`), the overall `compact_->status` is set to error but each subcompaction's individual `status` remains OK. `SubcompactionState::Cleanup` only checked the individual subcompaction status, so it skipped calling `ReleaseObsolete` on the output files' table cache entries — leaking them. Normally, `Close()`'s backstop (`FindObsoleteFiles(force=true)` + `PurgeObsoleteFiles`) would catch this by finding the orphan file on disk, evicting the cache entry, and deleting the file. However, `FindObsoleteFiles` calls `FaultInjectionTestFS::GetChildren` which can fail under metadata read fault injection (`--open_metadata_read_fault_one_in=8`). The error is silently ignored (`s.PermitUncheckedError()` in db_impl_files.cc:199), so the orphan file is never found and the leaked cache entry is never evicted. The `Cleanup` bug is latent and predates recent changes. PR https://github.com/facebook/rocksdb/issues/14433 added `verify_output_flags` randomization to the crash test, and PR https://github.com/facebook/rocksdb/issues/14456 fixed false-positive corruptions that https://github.com/facebook/rocksdb/issues/14433 caused. Before https://github.com/facebook/rocksdb/issues/14456, `VerifyOutputFiles` would produce false corruption errors that *accidentally prevented the leak* by setting the subcompaction status to non-OK. ### How it triggers **Step 1 — VerifyOutputFiles adds cache entries for compaction output files:** ``` CompactionJob::Run() RunSubcompactions() // all subcompactions succeed // output file 12 written to disk // each sub_compact.status = OK SyncOutputDirectories() // status = OK VerifyOutputFiles() for each output_file: table_cache()->NewIterator(output_file.meta) FindTable() cache->Insert(file_number=12) // <<< ENTRY ADDED TO TABLE CACHE return iterator holding handle delete iter // releases handle, entry stays in LRU // status = OK ``` **Step 2 — A post-verification step fails, overall status set but NOT subcompaction status:** ``` CompactionJob::Run() VerifyCompactionRecordCounts() // returns Status::Corruption(...) FinalizeCompactionRun(status=Corruption) compact_->status = Corruption // <<< OVERALL status = error // each sub_compact.status is still OK! ``` **Step 3 — Install skips InstallCompactionResults (file 12 never enters a Version):** ``` CompactionJob::Install() status = compact_->status // Corruption if (status.ok()) // FALSE InstallCompactionResults() // <<< SKIPPED — file 12 not in any version ``` **Step 4 — CleanupCompaction skips ReleaseObsolete (THE BUG):** ``` CompactionJob::CleanupCompaction() for each sub_compact: sub_compact.Cleanup(table_cache) if (!status.ok()) // checks sub_compact.status = OK // <<< FALSE — individual status is OK! ReleaseObsolete(...) // <<< NEVER CALLED — cache entry leaked! ``` **Step 5 — Metadata read fault prevents backstop from finding the orphan:** `FindObsoleteFiles(force=true)` calls `GetChildren()` to scan the DB directory. `FaultInjectionTestFS::GetChildren` injects a metadata read error (`--open_metadata_read_fault_one_in=8`). The error is silently ignored (`s.PermitUncheckedError()`), so the directory listing is empty and the orphan file is never found. This happens both in the post-compaction cleanup (`BackgroundCallCompaction`) and in `Close()`'s backstop: ``` FindObsoleteFiles(force=true) fs->GetChildren(path, ...) // returns IOError (fault injected) s.PermitUncheckedError(); // error silently ignored // files vector is empty — orphan file 12 not found PurgeObsoleteFiles() // nothing to do — no Evict called ``` **Step 6 — Assertion fires during Close():** ``` CloseHelper() FindObsoleteFiles(force=true) // GetChildren fails again → orphan missed PurgeObsoleteFiles() // nothing to evict TEST_VerifyNoObsoleteFilesCached() for each cache entry: file_number = 12 live_and_quar_files.find(12) == end() // NOT in any version! >>> assert(cached_file_is_live_or_quar) FAILS <<< ``` **Crash test call stack:** ``` frame https://github.com/facebook/rocksdb/issues/9: __assert_fail_base("cached_file_is_live_or_quar", "db_impl_debug.cc", 389) frame https://github.com/facebook/rocksdb/issues/11: DBImpl::TEST_VerifyNoObsoleteFilesCached()::lambda // finds leaked entry frame https://github.com/facebook/rocksdb/issues/18: LRUCacheShard::ApplyToSomeEntries(...) // iterating cache shard frame https://github.com/facebook/rocksdb/issues/19: ShardedCache::ApplyToAllEntries(...) // iterating all shards frame https://github.com/facebook/rocksdb/issues/20: DBImpl::TEST_VerifyNoObsoleteFilesCached() // the verification frame https://github.com/facebook/rocksdb/issues/21: DBImpl::CloseHelper() // during Close frame https://github.com/facebook/rocksdb/issues/22: DBImpl::CloseImpl() frame https://github.com/facebook/rocksdb/issues/23: DBImpl::Close() frame https://github.com/facebook/rocksdb/issues/24: StressTest::Reopen() // crash test reopen frame https://github.com/facebook/rocksdb/issues/25: StressTest::OperateDb() // worker thread ``` ### Fix Pass the overall `compact_->status` to `SubcompactionState::Cleanup` and call `ReleaseObsolete` when *either* the subcompaction status or the overall status is non-OK: ```cpp // Before: if (!status.ok()) { ReleaseObsolete(...); } // After: if (!status.ok() || !overall_status.ok()) { ReleaseObsolete(...); } ``` ## Key changes - **`SubcompactionState::Cleanup`**: Now takes an `overall_status` parameter and calls `ReleaseObsolete` when *either* the subcompaction status or the overall compaction status is non-OK. - **`CompactionJob::CleanupCompaction`**: Passes `compact_->status` (the overall status) to each subcompaction's `Cleanup`. - **Sync point**: Added `CompactionJob::Run():AfterVerifyOutputFiles` for error injection in tests. - **Unit test**: `DBCompactionTest.LeakedTableCacheEntryOnCompactionFailure` uses `FaultInjectionTestFS` to reproduce the crash test scenario — injects error after `VerifyOutputFiles` and deactivates the filesystem so `GetChildren` fails in `FindObsoleteFiles`, preventing the backstop from evicting the leaked cache entry. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14469 Test Plan: - `DBCompactionTest.LeakedTableCacheEntryOnCompactionFailure`: - **Without fix (ASAN)**: assertion fires during `Close()` — `File 12 is not live nor quarantined` - **With fix (ASAN)**: passes — `ReleaseObsolete` properly cleans up the cache entry - **With fix (non-ASAN)**: passes ``` $ COMPILE_WITH_ASAN=1 make -j db_compaction_test $ ./db_compaction_test --gtest_filter="DBCompactionTest.LeakedTableCacheEntryOnCompactionFailure" [ PASSED ] 1 test. ``` Reviewed By: xingbowang Differential Revision: D97190944 Pulled By: joshkang97 fbshipit-source-id: fdfd481cc1e192803cfb7d64052ccb9162c21b94 |