mirror of
https://github.com/facebook/rocksdb.git
synced 2026-07-07 14:47:40 +08:00
4707775ae9d46363157ca7b45a85155712f950d5
406 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Xingbo Wang
|
4707775ae9 |
Fix GetContext status propagation and blob-backed wide-column merge operands (#14640)
Summary: - propagate lower-level read and merge failures through `GetContext` via `read_status`, so `Get` and `GetEntity` preserve the original error instead of synthesizing `Corruption` when blob-backed reads or merge resolution fail - teach `GetMergeOperands` to resolve blob-backed default columns from wide-column entities, covering both the direct base-value path and the merge-plus-base path - add regression coverage for blob-read IO errors during `Get`/`GetEntity` merge resolution and for `GetMergeOperands` on blob-backed wide-column entities - fix the `DBFlushTest.MemPurgeCorrectLogNumberAndSSTFileCreation` test race by waiting for flush callbacks and cleaning up sync points ## Testing - `make db_blob_basic_test -j14` - `/usr/bin/perl -e 'alarm shift; exec ARGV' 60 ./db_blob_basic_test --gtest_filter='DBBlobBasicTest/DBBlobBasicIOErrorTest.GetBlob_IOError/*:DBBlobBasicTest/DBBlobBasicIOErrorTest.GetEntityMergeWithBlobBaseIOError/*'` ## Task T265824017, T265415808 Pull Request resolved: https://github.com/facebook/rocksdb/pull/14640 Reviewed By: anand1976 Differential Revision: D101690700 Pulled By: xingbowang fbshipit-source-id: 2b6fc357b37a01efa72a2d54dcff55be8992f42a |
||
|
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 |
||
|
Josh Kang
|
e07ccc3528 |
block GetCreationTimeOfOldestFile on async file open completion (#14723)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14723 ### Context `GetCreationTimeOfOldestFile()` assumed `max_open_files` = -1 meant every live SST had a pinned table reader. That is not true with `open_files_async`: recovery intentionally skips loading table files, `DB::Open()` returns, and `BGWorkAsyncFileOpen()` pins readers later. Any caller — e.g. fb_rocksdb's daily report at `FbRocksDb.cpp:1025` — invoking the API in the window between `DB::Open` returning and the background opener completing trips a debug assert. Reported in https://fb.workplace.com/groups/rocksdb/permalink/31668956482726231/. Removing the assert alone is insufficient. For legacy DBs whose manifest does not carry `file_creation_time`, `FileMetaData::TryGetFileCreationTime()` falls back to the pinned reader; with no reader, it returns `kUnknownFileCreationTime` and the function silently returns 0 (the "info unavailable" sentinel). The caller cannot distinguish "no info" from "raced with async open." ### Changes - Remove the invalid debug assert in `Version::GetCreationTimeOfOldestFile`. - Add a private helper `DBImpl::WaitForAsyncFileOpen()` that blocks on `bg_cv_` while `bg_async_file_open_state_ == kScheduled`. The synchronization machinery (`bg_async_file_open_state_` + `bg_cv_`) already exists — the destructor wait loop in `db_impl.cc:674-687` uses the same pattern. The helper is a no-op when `open_files_async = false`, and bails on `shutting_down_` so `DB::Close()` is not blocked by an in-flight caller. - Call `WaitForAsyncFileOpen()` at the top of `DBImpl::GetCreationTimeOfOldestFile()` (inside the `max_open_files == -1` branch). - Document the blocking behavior in `include/rocksdb/db.h`. - Replace the regression test with one that uses a `DBImpl::WaitForAsyncFileOpen::BeforeWait` sync point: spawn a thread that calls `GetCreationTimeOfOldestFile`, deterministically confirm it blocks inside the wait, release async open, confirm the caller wakes with the real value. ### Potential Followups (not included here) - Apply the same wait to `GetLiveFilesMetaData` and `GetColumnFamilyMetaData` — both zero out `oldest_ancester_time` / `file_creation_time` in the SST metadata they return during the async-open window (`db/version_set.cc:7877-7878`, `2090-2091`, `2172-2173`). - Address compaction-picker effects: TTL/periodic file selection (`db/version_set.cc:4039`, `4092-4094`), bottommost over-marking (`db/version_set.cc:4700-4701`), FIFO TTL/temperature pickers (`db/compaction/compaction_picker_fifo.cc:105-107`, `167-170`, `401-411`), and tiered-compaction output time inheritance (`db/compaction/compaction.cc:981`, `1000`). - Harden `FbRocksDb.cpp:1025` to check `status.ok()` instead of `status.code() != kNotSupported`. Reviewed By: mszeszko-meta Differential Revision: D104285992 fbshipit-source-id: ea46375ea1b3ba77fe6b548071aee1101ac0da77 |
||
|
Anand Ananthabhotla
|
60b34f30c4 |
Extend optimize_manifest_for_recovery through DB::Close (#14703)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14703 Extend `optimize_manifest_for_recovery` so a clean `DB::Close` can persist up-to-date WAL recovery markers when that can be done safely. Combined with the recovery-side optimization in the previous diff, a clean close/reopen can avoid recovery-time MANIFEST appends. This remains best-effort: if the close-time write is disabled, skipped, or fails, RocksDB falls back to the standard recovery path on the next open. The option stays mutable so it can be turned off before close to suppress the optimization without restarting the DB. The close-time path respects the existing recovery constraints for 2PC, non-empty column families, dropped column families, and WAL tracking, and preserves the existing file-number invariants. Reviewed By: pdillinger, hx235 Differential Revision: D103568449 fbshipit-source-id: ae62867507a8a87640a2c140bea852b7c608cb66 |
||
|
Anand Ananthabhotla
|
4e2fe35bbb |
Gate file_open_metadata consumption on fast_sst_open option (#14676)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14676 When fast_sst_open is disabled, RocksDB was still passing previously-persisted file_open_metadata from the MANIFEST to NewRandomAccessFile. This could cause failures when the metadata becomes stale (e.g. expired filesystem credentials). This change gates the consumption of file_open_metadata in TableCache::GetTableReader on the fast_sst_open option. When fast_sst_open is false, previously persisted metadata is ignored and not passed to the filesystem via FileOptions::file_metadata. The fast_sst_open flag is threaded from MutableDBOptions through VersionSet -> ColumnFamilySet -> ColumnFamilyData -> TableCache at construction time, ensuring the gate is active before any table readers are opened during recovery. Dynamic changes via SetDBOptions are also propagated to all existing TableCache instances. Reviewed By: mszeszko-meta, xingbowang Differential Revision: D102735581 fbshipit-source-id: 9a2c4dc0644a2f65c36b2468605df57779e127cd |
||
|
Josh Kang
|
03e5a0b9a8 |
Fix range tombstone conversion + ingest sst and re-enable crash tests (#14654)
Summary:
Fixes a correctness bug in read-path range-tombstone synthesis when it races with `IngestExternalFile`. The synthesis path could insert a tombstone into the active memtable at a snapshot's sequence number, while ingestion installed an L0 SST at `LastSequence + 1` — a higher seqno than the synthesized tombstone. This breaks the main assumption of range tombstone reads that all lower levels have lower seqno.
The fix introduces a per-CF `port::RWMutex` (`ColumnFamilyData::ingest_sst_lock_`) plus a per-memtable `ingest_seqno_barrier_`. Ingestion takes the read lock and range tombstone synthesis **tries** to take a write lock.
If iterator lock is successful, then we have a new updated barrier seqno that we can validate the iterator seqno against. An added benefit is we no longer need to gate against empty memtable. This was originally added as an easy fix to prevent memtables from being inserted into while ingestion was happening.
## The bug, by example
`ReadPathRangeTombstoneTest.NewerPointInOlderFileStillVisible` (`db/db_iterator_test.cc:6929`):
1. L0 file `b@1, c@2, d@3`, then L0 file `Delete(b)4, Delete(c)5`.
2. Active memtable: `Put(z)6`. Snapshot taken at seq 6.
3. `IngestExternalFile({c → "vc_live"})` → installed at L0 with seq 7.
4. Iterator at snap 6 walks the deletion run and synthesizes `[b, d) @ seq 6` into the active memtable via `MemTable::AddLogicallyRedundantRangeTombstone`.
5. `Get("c")` at the latest snapshot: memtable returns covering tombstone (seq 6), `Version::Get` short-circuits, **never reads `c@7`** — returns `NotFound` instead of `"vc_live"`.
The invariant `Version::Get` relies on (memtable seqs ≥ any L0 seq for the same key) is broken because synthesis writes at the *snapshot's* seq while ingestion writes at `LastSequence + 1`.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14654
Test Plan:
- Updated regression test
- Re-enable crashtests and manually run
| Flavor | Jobs |
|---|---:|
| `fbcode_blackbox_crash_test` | 200 |
| `fbcode_whitebox_crash_test` | 30 |
| `fbcode_asan_blackbox_crash_test` | 30 |
| `fbcode_tsan_blackbox_crash_test` | 30 |
| `fbcode_crash_test_with_atomic_flush` | 30 |
| `fbcode_crash_test_with_wc_txn` | 30 |
| `fbcode_crash_test_with_ts` | 30 |
| **Total** | **380** |
Reviewed By: xingbowang
Differential Revision: D102044512
Pulled By: joshkang97
fbshipit-source-id: 0c69187595edc5a5fa80be24bffdba710a92e56e
|
||
|
Josh Kang
|
790294f3ad |
Disallow table_filters entirely when range tombstone conversion is enabled (#14618)
Summary: The prior fix (https://github.com/facebook/rocksdb/issues/14586) disabled read-path range tombstone synthesis at the `DBIter` level when `table_filter` was set. However, synthesized tombstones persist in the memtable beyond the lifetime of the iterator that created them. A prior unfiltered iterator can synthesize a range tombstone that then silently affects a subsequent filtered iterator's results — the filtered scan sees the tombstone but not the SSTs it was derived from, allowing hidden SST state to corrupt the filtered view. See failed crash test in T264151327. This PR takes the stronger approach of rejecting iterator creation outright (`InvalidArgument`) when `ReadOptions::table_filter` is used on a column family with `min_tombstones_for_range_conversion > 0`. This eliminates the entire class of interaction bugs between the two features rather than trying to suppress conversion in individual code paths. ## Example - `min_tombstones_for_range_conversion = 2` - L1 SST_keep: `Put(a), Put(b), Put(c)` (`c` is the live boundary) - L0 SST_dels: `Delete(a), Delete(b)` (2 contiguous tombstones, newer) ### Step 1 — Iterator A (no filter) A walks `Del(a) Del(b)` (2 contig) then `c` (live). Synthesizes range tombstone `[a, c)` into the active memtable. ### Step 2 — Iterator B (`table_filter` skips SST_dels) B's filter excludes SST_dels, so `a, b` from SST_keep should appear live. But the memtable now holds range tombstone `[a, c)` from step 1, which applies to B regardless of `table_filter`. B sees only `c` — **a, b are silently hidden**. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14618 Test Plan: - `TableFilterNotAllowed` test validates the rejection path end-to-end: unfiltered scan synthesizes the tombstone, filtered iterator is rejected with `InvalidArgument`, and the full-DB view remains correct via `VerifyIteration`. - Crash test sanitization ensures `db_stress` won't hit the incompatible configuration. - Run crash test sanitization unit test: `python3 tools/db_crashtest_test.py` - Run iterator tests: `make db_iterator_test && ./db_iterator_test --gtest_filter="*TableFilterNotAllowed*"` Reviewed By: xingbowang Differential Revision: D100873156 Pulled By: joshkang97 fbshipit-source-id: dd0d123d94b1bfda2a4a5040681fcc7cf14f760e |
||
|
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 |
||
|
Xingbo Wang
|
4cf28450a5 |
Add customizable blob partition strategy for blob direct write (#14565)
Summary:
This PR adds a `BlobFilePartitionStrategy` interface that allows users to plug in custom partition selection logic for blob direct writes. The default behavior (round-robin across partitions) is preserved as the built-in `RoundRobinBlobFilePartitionStrategy`.
### Motivation
The existing blob direct write implementation distributes writes across partitions using a fixed round-robin strategy (atomic counter mod num_partitions). While this works well for uniform workloads, some use cases benefit from custom routing:
- **Key-based routing**: co-locate related keys in the same blob partition for locality-aware reads.
- **CF-based routing**: direct writes for different column families to dedicated partitions.
- **Value-size routing**: send large vs. small values to different partitions.
- **Application-defined affinity**: any domain-specific grouping that the default round-robin cannot express.
### Design
A new pure-virtual interface `BlobFilePartitionStrategy` is added to `include/rocksdb/advanced_options.h`:
```cpp
class BlobFilePartitionStrategy {
public:
virtual ~BlobFilePartitionStrategy() = default;
// Select a partition for the given blob direct write.
// The return value can be any uint32_t; the caller applies
// modulo num_partitions internally.
// Implementations must be thread-safe.
virtual uint32_t SelectPartition(uint32_t num_partitions,
uint32_t column_family_id,
const Slice& key,
const Slice& value) = 0;
};
```
The strategy receives:
- `num_partitions`: the configured partition count (for implementations that want to be partition-count-aware).
- `column_family_id`: useful for CF-based routing.
- `key` / `value`: the key and value being written.
The return value is taken modulo `num_partitions` internally, so implementations can return any `uint32_t` without worrying about bounds.
A new column family option `blob_direct_write_partition_strategy` (type `std::shared_ptr<BlobFilePartitionStrategy>`, default `nullptr`) wires the strategy into `BlobFilePartitionManager`. When `nullptr`, the built-in `RoundRobinBlobFilePartitionStrategy` is used, preserving existing behavior exactly.
### Changes
| File | Change |
|---|---|
| `include/rocksdb/advanced_options.h` | New `BlobFilePartitionStrategy` interface; new `blob_direct_write_partition_strategy` option |
| `db/blob/blob_file_partition_manager.h/.cc` | Accept strategy in constructor; replace atomic counter with strategy call; move `RoundRobinBlobFilePartitionStrategy` here as default |
| `options/cf_options.h/.cc` | Thread strategy through `ImmutableCFOptions` |
| `options/options_helper.cc` | Propagate strategy in `UpdateColumnFamilyOptions` |
| `options/options_settable_test.cc` | Register new option field in options settability test |
| `db/db_impl/db_impl.cc` | Pass strategy when constructing `BlobFilePartitionManager` |
| `db/blob/db_blob_direct_write_test.cc` | New test with `FixedBlobDirectWritePartitionStrategy` |
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14565
Test Plan:
New test `DirectWriteCustomPartitionStrategyRoutesWritesToOneBlobFile` verifies that a `FixedBlobDirectWritePartitionStrategy` that always returns partition 3 correctly routes all writes to a single blob file across 4 configured partitions.
```
make -j128 db_blob_direct_write_test && ./db_blob_direct_write_test --gtest_filter='*CustomPartition*'
make -j128 db_blob_direct_write_test && ./db_blob_direct_write_test
```
Reviewed By: joshkang97
Differential Revision: D99458813
Pulled By: xingbowang
fbshipit-source-id: 54cfbb0f75e24b58a8db61ad8755204d0db6ece7
|
||
|
Xingbo Wang
|
af4e32945b |
Blob direct write v1: write-path blob separation with partitioned files (reduced scope) (#14535)
Summary: This PR introduces **blob direct write v1**, a reduced-scope write-path optimization where large values (>= `min_blob_size`) are written directly to blob files during `Put()` and replaced in the memtable with compact `BlobIndex` references. This avoids holding full values in memory until flush time. ### Motivation In the existing BlobDB architecture, values are written to the WAL and memtable in their full form and separated into blob files only at flush time. This means: - Large values are held in memory twice (raw in memtable + blob file at flush) - Blob I/O is serialized through a single flush thread per column family Blob direct write addresses both: values leave the write path as small `BlobIndex` references, and multiple **partitions** (configurable via `blob_direct_write_partitions`) allow concurrent blob writes with independent locks. ### Design (v1 — single-writer, WAL-disabled, reduced scope) The v1 design intentionally keeps scope narrow for correctness and reviewability: - **Single writer thread assumption**: no concurrent writes to the same partition file. One logical writer serializes the batch. - **WAL-disabled**: direct-write blob files are only registered in MANIFEST at flush time. WAL replay cannot recover unregistered blob references, so WAL is disabled for this v1. - **Flush-on-write**: each `AddRecord` call flushes to the OS immediately. - **FIFO generation batching**: each memtable switch creates one generation batch. Direct-write files for that memtable are sealed and registered atomically when the batch is flushed to MANIFEST. - **Round-robin partitions**: blob writes are distributed across `blob_direct_write_partitions` files using an atomic counter. ### New components | Component | Description | |---|---| | `BlobFilePartitionManager` | Owns N partition files per CF. Manages open/seal/register lifecycle tied to memtable generations. | | `BlobWriteBatchTransformer` | A `WriteBatch::Handler` that rewrites qualifying `Put` values as `BlobIndex` entries before the batch enters the write group. | ### Write path integration 1. `DBImpl::WriteImpl` calls `BlobWriteBatchTransformer::TransformBatch` before entering the writer group (for default write path), or before joining the batch group (for pipelined/unordered write). 2. Values >= `min_blob_size` are written to a partition file; the key is stored with a `BlobIndex` in the transformed batch. A rollback guard marks blob bytes as initial garbage if the write fails. 3. On `SwitchMemtable`, `RotateCurrentGeneration` moves active partitions into the next immutable batch. 4. `FlushMemTableToOutputFile` / `AtomicFlushMemTablesToOutputFiles` call `PrepareFlushAdditions` to seal partition files and collect `BlobFileAddition` + `BlobFileGarbage` entries registered to MANIFEST alongside the flush. 5. Shutdown paths (`CancelAllBackgroundWork`, `WaitForCompact` with `close_db=true`) force-flush all CFs with active direct-write managers to ensure blob files are registered before close. ### Read path - **Get/MultiGet**: `MaybeResolveBlobForWritePath` resolves `BlobIndex` references found in memtable or immutable memtable via `BlobFilePartitionManager::ResolveBlobDirectWriteIndex`, which first checks manifest-visible state and falls back to direct blob-file reads via `BlobFileCache`. - **Iterator**: `DBIter::BlobReader` is extended with a `BlobFilePartitionManager*` to resolve direct-write blob indexes during iteration. The unified `ResolveBlobDirectWriteIndex` path handles both manifest-visible and not-yet-flushed files. ### New options | Option | Default | Description | |---|---|---| | `enable_blob_direct_write` | `false` | Enable write-path blob separation for this CF. Requires `enable_blob_files = true`. Not dynamically changeable. | | `blob_direct_write_partitions` | `1` | Number of parallel partition files per CF. Not dynamically changeable. | ### Feature incompatibilities (reduced v1 scope) The following features are *not supported* when `enable_blob_direct_write = true`, and are enforced both in `db_stress_tool` validation and `db_crashtest.py` sanitization: **Write model constraints:** - `threads` must be 1 (single writer assumption) - `allow_concurrent_memtable_write` = 0 - `enable_pipelined_write` = 0 (transformation done before batch group, but pipelined path supported with pre-transform) - `two_write_queues` = 0 - `unordered_write` = 0 (transformation done before batch group, but unordered path supported with pre-transform) **WAL and recovery:** - `disable_wal` = 1 (required — WAL replay of unregistered blob files is out of v1 scope) - `best_efforts_recovery` = 0 - `reopen` = 0 (no crash-restart with WAL replay) - All WAL-related stress features disabled: `manual_wal_flush_one_in`, `sync_wal_one_in`, `lock_wal_one_in`, `get_sorted_wal_files_one_in`, `get_current_wal_file_one_in`, `track_and_verify_wals`, `rate_limit_auto_wal_flush`, `recycle_log_file_num` **Blob GC and dynamic options:** - `use_blob_db` = 0 (stacked BlobDB not supported) - `allow_setting_blob_options_dynamically` = 0 - `enable_blob_garbage_collection` = 0 - `blob_compaction_readahead_size` = 0 - `blob_file_starting_level` = 0 **Unsupported value types and APIs:** - Merge (`use_merge`, `use_full_merge_v1`) — merge values pass through untransformed - Entity APIs (`use_put_entity_one_in`, `use_get_entity`, `use_multi_get_entity`, `use_attribute_group`) - `use_timed_put_one_in` - User-defined timestamps (`user_timestamp_size`, `persist_user_defined_timestamps`, `create_timestamped_snapshot_one_in`) - Transactions (`use_txn`, `use_optimistic_txn`, `test_multi_ops_txns`, `commit_bypass_memtable_one_in`) — though `WriteCommittedTxn::CommitInternal` falls back from bypass-memtable to normal path when BDW is active - `IngestWriteBatchWithIndex` returns `NotSupported` - `inplace_update_support` = 0 **Fault injection:** - All write/read/metadata fault injection disabled (`sync_fault_injection`, `write_fault_one_in`, `metadata_write_fault_one_in`, `read_fault_one_in`, `metadata_read_fault_one_in`, `open_*_fault_one_in`) **Infrastructure/snapshot APIs:** - `remote_compaction_worker_threads` = 0 - `test_secondary` = 0 - `backup_one_in` = 0 - `checkpoint_one_in` = 0 - `get_live_files_apis_one_in` = 0 - `ingest_external_file_one_in` = 0 - `ingest_wbwi_one_in` = 0 ### Tests - `db/blob/db_blob_basic_test.cc`: ~660 lines of new direct-write unit tests covering basic put/get, multi-partition, flush/compaction, recovery, and error injection. - `db/blob/blob_file_cache_test.cc`: ~96 lines of new tests for direct-write blob file cache behavior. - `db/write_batch_test.cc`: ~96 lines of tests for WriteBatch with blob index entries. - `utilities/transactions/transaction_test.cc`: verifies transaction commit path falls back correctly with direct write enabled. - `db_stress_tool/`: full stress test support with `--enable_blob_direct_write` and `--blob_direct_write_partitions` flags, integrated into `db_crashtest.py` with 10% random selection alongside regular blob params. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14535 Test Plan: ``` make -j128 db_blob_basic_test && ./db_blob_basic_test make -j128 blob_file_cache_test && ./blob_file_cache_test make -j128 write_batch_test && ./write_batch_test make -j128 transaction_test && ./transaction_test make -j128 check ``` Stress test: ``` python3 tools/db_crashtest.py blackbox --enable_blob_direct_write=1 \ --enable_blob_files=1 --blob_direct_write_partitions=4 \ --disable_wal=1 --threads=1 ``` Reviewed By: pdillinger Differential Revision: D98766843 Pulled By: xingbowang fbshipit-source-id: 1577653826913a59d05680a87bce5534ac5a5e69 |
||
|
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 |
||
|
Xingbo Wang
|
1a4b1e42cc |
Add include_blob_files option to GetApproximateSizes (#14501)
Summary: **Summary:** Add a new boolean flag `include_blob_files` (default: `false`) to `SizeApproximationOptions` and a corresponding `INCLUDE_BLOB_FILES` enum value to `SizeApproximationFlags`. When set to `true`, the returned size includes an approximation of blob file data in the queried key range. **Algorithm:** The blob file size contribution is prorated using the SST size ratio: ``` blob_size_in_range ≈ total_blob_size * (sst_size_in_range / total_sst_size) ``` The blob-to-SST ratio (`total_blob_size / total_sst_size`) is computed once before the per-range loop, so iterating levels and blob files only happens once per `GetApproximateSizes` call regardless of how many ranges are queried. The per-range SST size (`ApproximateSize`) is computed once and shared between `include_files` and `include_blob_files`. **Limitations:** - Assumes blob data is distributed proportionally to SST data across the key space. May be inaccurate if blob value sizes vary significantly across different key ranges (e.g., one range has large blobs while another has small ones). - If there are no SST files (all data in memtables), the blob size contribution will be 0 even if blob files exist on disk. **Changes:** - `include/rocksdb/options.h`: New `include_blob_files` field in `SizeApproximationOptions`; updated doc comments for `include_memtables`/`include_files` - `include/rocksdb/db.h`: New `INCLUDE_BLOB_FILES` in `SizeApproximationFlags` enum, updated flags-to-options mapping - `include/rocksdb/c.h`: New `rocksdb_size_approximation_flags_include_blob_files` C API enum value - `java/`: Added `INCLUDE_BLOB_FILES` to `SizeApproximationFlag.java` and JNI flag mapping in `rocksjni.cc` - `db/db_impl/db_impl.cc`: Blob-to-SST ratio computed once before loop, SST range size computed once per range and shared - `db_stress_tool/db_stress_test_base.cc`: Randomized `include_blob_files` in stress test Pull Request resolved: https://github.com/facebook/rocksdb/pull/14501 Test Plan: - New `DBBlobBasicTest.GetApproximateSizesIncludingBlobFiles` — verifies: - Size with blobs > without (full range) - Non-overlapping range returns 0 - Partial range returns proportionally less than full range - `SizeApproximationFlags` API works - Multi-range query: two sub-ranges sum approximately to the full-range result - Stress test now exercises the new option randomly Reviewed By: hx235 Differential Revision: D97984211 Pulled By: xingbowang fbshipit-source-id: e9127eac3308687fd4f0b17a771fd61fba6a8380 |
||
|
Hui Xiao
|
63c86160cd |
Add OnBackgroundJobPressureChanged listener callback (#14474)
Summary: **Summary:** Add a new EventListener callback `OnBackgroundJobPressureChanged` that fires after every flush or compaction background job completes. The callback delivers a `BackgroundJobPressure` snapshot containing: - Compaction scheduling counters (scheduled/running, combined and per-priority LOW/BOTTOM breakdown) - Flush scheduling counters (scheduled/running) - Write stall proximity percentage (0=healthy, 100=at stall threshold, can exceed 100 when stalling) - Whether compaction speedup is active `CaptureBackgroundJobPressure()` reads scheduling counters and computes write stall proximity from L0 sorted run count and pending compaction bytes (same inputs as `RecalculateWriteStallConditions()`). TODO: add memory-related write stall triggers later. Introduces `num_running_bottom_compactions_` counter to track BOTTOM- priority compactions separately from LOW, enabling per-pool breakdown in the pressure snapshot. The callback fires on the background thread after counter decrements and `MaybeScheduleFlushOrCompaction()`, so the snapshot reflects post- completion state. Uses the same mutex unlock/lock pattern as `NotifyOnFlushCompleted`. A `bg_pressure_callback_in_progress_` counter ensures destructor safety since the callback fires after `bg_flush_scheduled_`/`bg_compaction_scheduled_` are decremented. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14474 Test Plan: - listener_test BackgroundJobPressure: 3-phase test verifying no pressure, pressure build-up (speedup, scheduling, proximity), and pressure relief after compaction completes - db_compaction_test CompactRangeBottomPri: verifies num_running_bottom_compactions_ via sync point - db_stress_tool exercises the callback with RandomSleep() Reviewed By: xingbowang Differential Revision: D97423623 Pulled By: hx235 fbshipit-source-id: 07003c8de226ec29d32b8a88e2d86e5de85cd2cc |
||
|
Josh Kang
|
f25fb41da6 |
Add option to validate sst files in the background on DB open (#14322)
Summary: Add `open_files_async` option for faster DB startup. When enabled, SST file opening and validation is deferred to a background thread after `DB::Open` returns, reducing startup latency for databases with many SST files. WAL recovery remains synchronous. To support this, `FindTable` is extended with a pinning mechanism that stores the cache handle directly on `FileMetaData` via a new `PinnedTableReader` class, and sets the table reader atomically so subsequent reads skip cache lookups. `FileDescriptor::table_reader` is replaced with `PinnedTableReader pinned_reader` which wraps a `std::atomic<TableReader*>` with acquire/release ordering to safely handle concurrent access between the background opener and read threads. Should validations fail, the background opener sets a `kAsyncFileOpen` background error. Future read requests will look up the table reader again via the cache, and if any validations fail there it will get propagated to the user (existing behavior when `max_open_files > 0`). This feature is most useful when `max_open_files=-1`, because otherwise file opening is already capped at 16 files and DB open should be fast. ## Restrictions - This feature also is incompatible with fifo compaction because fifo compaction requires reading table properties under DB mutex. When table reader is unpinned, this may cause a DB hang. - This feature is also incompatible with `skip_stats_update_on_db_open=false` because it will result in even longer DB open ## Key changes - New `open_files_async` DB option with C, Java, and `db_bench` bindings - `BGWorkAsyncFileOpen` background worker that opens all SST files post-`DB::Open`, with shutdown awareness via `shutting_down_` flag - New `PinnedTableReader` class in `version_edit.h` — thread-safe wrapper holding `std::atomic<TableReader*>` and `Cache::Handle*` with proper acquire/release ordering. Replaces the old `FileDescriptor::table_reader` raw pointer and `FileMetaData::table_reader_handle` - Extract `LoadTableHandlersHelper` into `db/version_util.cc` — shared between `VersionBuilder::LoadTableHandlers` (for version edits during recovery) and `BGWorkAsyncFileOpen` (for base storage post-open) - `FindTable` extended with `pin_table_handle` and `out_table_reader` params — when pinning is enabled, the table reader is stored on `FileMetaData` so Get/MultiGet/Iterator skip redundant cache lookups. `FindTable` now performs the pinned-reader fast-path check internally instead of requiring callers to check `fd.table_reader` beforehand - Note: pinning is explicit (not default) because some callers create temporary `FileMetaData`s that would need to properly clean up table handles - `CompactedDBImpl` updated to use `FindTable` + pinning instead of raw `fd.table_reader` access for Get/MultiGet - New `kAsyncFileOpen` background error reason in `listener.h` and `error_handler.cc` - Add a check in ~DBImpl to ensure async file open task has not been forgotten to be scheduled in (future) subclasses of DBImpl. Certain subclasses that never use it will need to explicitly mark it. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14322 Test Plan: - `OpenFilesAsyncTest` parameterized over `num_flushes` (1, 20), `ReadType` (Get, MultiGet, Iterator), `max_open_files` (-1, 10), and `read_only` (true, false) - **ConcurrentFileAccess**: concurrent reads and compactions race with async opener - **AfterRead**: reads happen before async opener, verifying lazy open and that the opener sees already-pinned readers - **BeforeRead**: async opener completes first, verifying reads use pre-loaded table readers - **Shutdown**: DB closes before async opener starts, verifying clean cancellation with 0 file opens - **Error**: corrupted SST files, verifying `kAsyncFileOpen` background error is set and reads return corruption - **DropColumnFamily**: CF dropped before async opener runs, verifying the opener gracefully skips dropped CFs - Added to crash test ### Benchmark To simulate a high-latency remote filesystem, I set up a virtual filesystem with dm-delay using 10ms reads, 0 ms writes. ``` # Generate a DB with many L0 files TEST_TMPDIR=/data/users/jkangs/dm-delay-test/mnt ./db_bench -benchmarks=fillseq -disable_auto_compactions=true -write_buffer_size=1000 -num=1000000 ``` ``` ./db_bench -use_existing_db=true -db=/data/users/jkangs/dm-delay-test/mnt/dbbench -benchmarks=readrandom -reads=1 -report_open_timing=true -open_files_async=true -use_direct_reads -file_opening_threads=1 -skip_stats_update_on_db_open OpenDb: 25.1419 milliseconds ``` ``` ./db_bench -use_existing_db=true -db=/data/users/jkangs/dm-delay-test/mnt/dbbench -benchmarks=readrandom -reads=1 -report_open_timing=true -open_files_async=false -use_direct_reads -file_opening_threads=1 -skip_stats_update_on_db_open OpenDb: 23109.4 milliseconds ``` ### No read regressions On main branch ``` ./db_bench -use_existing_db=true -db=/dev/shm/dbbench -benchmarks=readrandom -seed=1 -threads=8 -duration=30 readrandom : 4.827 micros/op 1657100 ops/sec 30.005 seconds 49720992 operations; 183.3 MB/s (6198999 of 6198999 found) ``` On this branch ``` ./db_bench -use_existing_db=true -db=/dev/shm/dbbench -benchmarks=readrandom -seed=1 -threads=8 -duration=30 readrandom : 4.863 micros/op 1644808 ops/sec 30.007 seconds 49354992 operations; 182.0 MB/s (6099999 of 6099999 found) ./db_bench -use_existing_db=true -db=/dev/shm/dbbench -benchmarks=readrandom -seed=1 -threads=8 -duration=30 -open_files_async=true readrandom : 4.803 micros/op 1665392 ops/sec 30.004 seconds 49968992 operations; 184.2 MB/s (6222999 of 6222999 found) ``` Reviewed By: pdillinger, xingbowang Differential Revision: D93538033 Pulled By: joshkang97 fbshipit-source-id: 32ac70c112cd733b7c1e1c1e2e7ce6422318a5ae |
||
|
Peter Dillinger
|
da2c3c0ee6 |
Fix & improve compaction trigger on a "quiet" DB (#14396)
Summary: As a follow-up to https://github.com/facebook/rocksdb/issues/13736, allow a "quiet" DB to react much sooner to time-based compaction triggers. For details see DBImpl::ComputeTriggerCompactionPeriod() implementation. Also based on review feedback, fixing a bug where only column families setting periodic compaction would be triggered, rather than any time-based compaction. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14396 Test Plan: extended+added unit tests to cover much of the logic Reviewed By: xingbowang Differential Revision: D94626166 Pulled By: pdillinger fbshipit-source-id: de9ca19e46bdba5d9715474efbc61805354d730d |
||
|
Hui Xiao
|
29819f37e1 |
Remove deprecated ReadOptions::managed, `ColumnFamilyOptions::snap_refresh_nanos (#14350)
Summary: **Context/Summary:** Remove deprecated, unused APIs and options: - ReadOptions::managed: This option was not used anymore. The functionality it controlled has been removed long ago. - ColumnFamilyOptions::snap_refresh_nanos: Deprecated and unused option. Corresponding C API (rocksdb_readoptions_set_managed) and Java API (ReadOptions.managed/setManaged) are also removed. All related checks an db_impl and db_impl_secondary iterators are cleaned up. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14350 Test Plan: make check Reviewed By: pdillinger Differential Revision: D93812438 Pulled By: hx235 fbshipit-source-id: e4a9d21c65f83294b6d0878286ba14024f049bac |
||
|
anand76
|
653fd9c65b |
Bug fix for bg error recovery in TransactionDB (#14313)
Summary: This PR fixes a bug in the interaction between WritePrepared/WriteUnprepared TransactionDB (with two_write_queues=true) and background error recovery. This bug caused crash tests to fail with a "sequence number going backwards" error during DB open. Root Cause ------------ When two_write_queues=true, sequence numbers are allocated via FetchAddLastAllocatedSequence() before a write completes, but are only published via SetLastSequence() after the write succeeds. If a background error occurs (e.g., a MANIFEST write failure during flush), the error recovery path in DBImpl::ResumeImpl creates new memtables and WAL files. The new WAL's starting sequence number is based on LastSequence() (the published value), which can be lower than already-allocated sequence numbers that were written to the old WAL. On subsequent recovery, RocksDB detects that sequence numbers in the new WAL are lower than those in the old WAL and reports a "sequence number going backwards" corruption error, causing the DB to fail to open. Fix --- The fix adds a call to a new VersionSet::SyncLastSequenceWithAllocated() method at the beginning of DBImpl::ResumeImpl, before any new memtables or WALs are created. This method advances last_sequence_ to match last_allocated_sequence_ if the latter is higher, ensuring the new WAL starts with a sequence number that is at least as high as any previously allocated one. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14313 Test Plan: --------- Add new unit tests in write_prepared_transaction_test_seqno Reviewed By: pdillinger Differential Revision: D92746944 Pulled By: anand1976 fbshipit-source-id: 34385fc13fd74435dd1c3283637eb118f45d887e |
||
|
Andrew Chang
|
09bda51c50 |
Propagate file_checksum through FileOptions on NewRandomAccessFile (#14321)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14321 Add file_checksum and file_checksum_func_name fields to FileOptions so that downstream FileSystem implementations can access per-file checksum metadata when SST files are opened. The fields are populated from FileMetaData at all call sites where SST files are opened via NewRandomAccessFile: TableCache::GetTableReader, Version::GetTableProperties, and CompactionJob::ReadTablePropertiesDirectly. Also fixes the fallback path in TableCache::GetTableReader to use the local fopts (with temperature and checksum) instead of the original file_options. Added a kNoFileChecksumFuncName which is distinct from kUnknownFileChecksumFuncName: - kUnknownFileChecksumFuncName ("Unknown"): We have FileMetaData for this file, and the metadata says no checksum was computed (no factory was configured when the file was written). This is a property of the file itself. - kNoFileChecksumFuncName ("Unavailable"): We don't even have FileMetaData — we're opening this file in a context where there's no checksum metadata to propagate at all (e.g., SstFileDumper, SstFileReader, checksum generation). It's a property of the call site, not the file. So the assertion file_checksum.empty() is correct for both, but for different reasons — one says "the file has no checksum," the other says "we have no idea about this file's checksum." Reviewed By: pdillinger Differential Revision: D92728944 fbshipit-source-id: 8fd34ea22ca87090b26d0a55c921f354f97f1ffc |
||
|
Xingbo Wang
|
ea5e649225 |
Fix an infinite compaction loop bug with udt (#14228)
Summary: Problem The TEST_WaitForCompact in TimestampCompatibleCompactionTest.UdtTombstoneCollapsingTest would sometimes run forever, indicating an infinite compaction loop. Issue https://github.com/facebook/rocksdb/issues/14223 Root Cause In ComputeBottommostFilesMarkedForCompaction(), files were marked for bottommost compaction based only on the condition largest_seqno < oldest_snapshot_seqnum. However, for User-Defined Timestamps (UDT) columns, compaction can only zero sequence numbers when the file's maximum timestamp is below full_history_ts_low. When timestamps were above this threshold: 1. File gets marked for compaction (seqno condition met) 2. Compaction runs but cannot zero seqno (timestamp condition not met) 3. Output file immediately gets re-marked for compaction 4. Infinite loop Solution Added timestamp range tracking to FileMetaData and updated the marking logic to check timestamps before marking files. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14228 Test Plan: Unit test Reviewed By: pdillinger Differential Revision: D90586045 Pulled By: xingbowang fbshipit-source-id: addfa4f988db8c87fb513a1bf58ee54623a6c210 |
||
|
Josh Kang
|
4bf2bcdbb3 |
Allow setting options for multiple column families (#14201)
Summary: Currently to set options for multiple CFs, the caller must repeatedly call SetOptions() for each CF. This in turn serializes the entire options file each time. This PR exposes a new API that allows SetOptions to be called on multiple CFs at once, thus only paying the OPTIONS file serialization once. Also added a new unit test for SetOptions. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14201 Reviewed By: pdillinger Differential Revision: D89735181 Pulled By: joshkang97 fbshipit-source-id: 9b7a721b7e8769b653243b1581678ffd05d038e8 |
||
|
Ranjan Banerjee
|
9fbb68be17 |
Api to get SST file with key ranges for a particular level and key range (startKey, EndKey)rocksdb [Internal version] (#14009)
Summary: There are instances where an application might be interested in knowing the distribution in SST files for a key range in a particular level. This implementation creates an overloaded GetColumnFamilyMetaData api where (startKey, EndKey) can be passed along with level information to filter the necessary sst files along with the keyranges for each sst file Pull Request resolved: https://github.com/facebook/rocksdb/pull/14009 Reviewed By: anand1976 Differential Revision: D83389707 fbshipit-source-id: 6df1dc1f9233efe9000b03cc1831b3c618cbcef3 |
||
|
Peter Dillinger
|
37176a4a44 |
Auto-tune manifest file size (#14076)
Summary:
Adds auto-tuning of manifest file size to avoid the need to scale `max_manifest_file_size` in proportion to things like number of SST files to properly balance (a) manifest file write amp and new file creation, vs. (b) manifest file space amp and replay time, including non-incremental space usage in backups. (Manifest file write amp comes from re-writing a "live" record when the manifest file is re-created, or "compacted"; space amp is usage beyond what would be used by a compacted manifest file.) In more detail,
* Add new option `max_manifest_space_amp_pct` with default value of 500, which defaults to 0.2 write amp and up to roughly 5.0 space amp, except `max_manifest_file_size` is treated as the "minimum" size before re-creating ("compacting") the manifest file.
* `max_manifest_file_size` in a way means the same thing, with the same default of 1GB, but in a way has taken on a new role. What is the same is that we do not re-create the manifest file before reaching this size (except for DB re-open), and so users are very unlikely to see a change in default behavior (auto-tuning only kicking in if auto-tuning would exceed 1GB for effective max size for the current manifest file). The new role is as a file size lower bound before auto-tuning kicks in, to minimize churn in files considered "negligibly small." We recommend a new setting of around 1MB or even smaller like 64KB, and expect something like this to become the default soon.
* These two options along with `manifest_preallocation_size` are now mutable with SetDBOptions. The effect is nearly immediate, affecting the next write to the current manifest file.
Also in this PR:
* Refactoring of VersionSet to allow it to get (more) settings from MutableDBOptions. This touches a number of files in not very interesting ways, but notably we have to be careful about thread-safe access to MutableDBOptions fields, and even fields within VersionSet. I have decided to save copies of relevant fields from MutableDBOptions to simplify testing, etc. by not saving a reference to MutableDBOptions but getting notified of updates.
* Updated some logging in VersionSet to provide some basic data about final and compacted manifest sizes (effects of auto-tuning), making sure to avoid I/O while holding DB mutex.
* Added db_etc3_test.cc which is intended as a successor to db_test and db_test2, but having "test.cc" in its name for easier exclusion of test files when using `git grep`. Intended follow-up: rename db_test2 to db_etc2_test
* Moved+updated `ManifestRollOver` test to the new file to be closer to other manifest file rollover testing.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14076
Test Plan:
As for correctness, new unit test AutoTuneManifestSize is pretty thorough. Some other unit tests updated appropriately. Manual tests in the performance section were also audited for expected behavior based on the new logging in the DB LOG. Example LOG data with -max_manifest_file_size=2048 -max_manifest_space_amp_pct=500:
```
2025/10/24-11:12:48.979472 2150678 [/version_set.cc:5927] Created manifest 5, compacted+appended from 52 to 116
2025/10/24-11:12:49.626441 2150682 [/version_set.cc:5927] Created manifest 24, compacted+appended from 2169 to 1801
2025/10/24-11:12:52.194592 2150682 [/version_set.cc:5927] Created manifest 91, compacted+appended from 10913 to 8707
2025/10/24-11:13:02.969944 2150682 [/version_set.cc:5927] Created manifest 362, compacted+appended from 52259 to 13321
2025/10/24-11:13:18.815120 2150681 [/version_set.cc:5927] Created manifest 765, compacted+appended from 80064 to 13304
2025/10/24-11:13:35.590905 2150681 [/version_set.cc:5927] Created manifest 1167, compacted+appended from 79863 to 13304
```
As you can see, it only took a few iterations of ramp-up to settle on the auto-tuned max manifest size for tracking ~122 live SST files, around 80KB and compacting down to about 13KB. (13KB * (500 + 100) / 100 = 78KB). With the default large setting for max_manifest_file_size, we end up with a 232KB manifest, which is more than 90% wasted space. (A long-running DB would be much worse.)
As for performance, we don't expect a difference, even with TransactionDB because actual writing of the manifest is done without holding the DB mutex. I was not able to see a performance regression using db_bench with FIFO compaction and >1000 ~10MB SST files, including settings of -max_manifest_file_size=2048 -max_manifest_space_amp_pct={500,10,0}. No "hiccups" visible with -histogram either.
I also tried seeding a 1 second delay in writing new manifest files (other than the first). This had no significant effect at -max_manifest_space_amp_pct=500 but at 100 started causing write stalls in my test. In many ways this is kind of a worst case scenario and out-of-proportion test, but gives me more confidence that a higher number like 500 is probably the best balance in general.
Reviewed By: xingbowang
Differential Revision: D85445178
Pulled By: pdillinger
fbshipit-source-id: 1e6e07e89c586762dd65c65bb7cb2b8b719513f9
|
||
|
Peter Dillinger
|
befa6b8050 |
Fix and check for potential ODR violations (#14096)
Summary: ... caused by public headers depending on build parameters (macro definitions). This change also adds a check under 'make check-headers' (already in CI) looking for potential future violations. I've audited the uses of '#if' in public headers and either * Eliminated them * Systematically excluded them because they are intentional or similar (details in comments in check-public-header.sh * Manually excluded them as being ODR-SAFE In the case of ROCKSDB_USING_THREAD_STATUS, there was no good reason for this to appear in public headers so I've replaced it with a static bool ThreadStatus::kEnabled. I considered getting rid of the ability to disable this code but some relatively recent PRs have been submitted for fixing that case. I've added a release note and updated one of the CI jobs to use this build configuration. (I didn't want to combine with some jobs like no_compression and status_checked because the interaction might limit what is checked. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14096 Test Plan: manual 'make check-headers' + manual cmake as in new CI config + CI Reviewed By: jaykorean Differential Revision: D86241864 Pulled By: pdillinger fbshipit-source-id: d16addc9e3480706b174a006720a4def0740bf2e |
||
|
Hui Xiao
|
f722e68d88 |
New FlushWAL() API to take extra fields such as rate limiter priority (#14037)
Summary: **Context/Summary:** There is no way to tag or rate-limit write IO occurs during FlushWAL() with priority. Under `Options::manual_wal_flush=true`, it is the major source of write IO during user writes so we decide to add that support. A new option struct `FlushWALOptions` is introduced to avoid making the API ugly for future new fields. Also, we can't use the WriteOptions (https://github.com/facebook/rocksdb/blob/main/include/rocksdb/options.h#L2293-L2302 i) since is associated with that particular Put/Merge/.. associated with that option but FlushWAL() can happen after that write. There is no way to carry that write option over in RocksDB. I also avoided using the WriteOptions since it's mostly for live write. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14037 Test Plan: New UTs `TEST_P(DBRateLimiterOnManualWALFlushTest, ManualWALFlush)` Reviewed By: archang19 Differential Revision: D84193522 Pulled By: hx235 fbshipit-source-id: 18feb5235672010d19a101ce52c8abdcc4a789f2 |
||
|
Peter Dillinger
|
2950e99219 |
Require C++20 (#13904)
Summary: I am wanting to use std::counting_semaphore for something and the timing seems good to require C++20 support. The internets suggest: * GCC >= 10 is adequate, >= 11 preferred * Clang >= 10 is needed * Visual Studio >= 2019 is adquate And popular linux distributions look like this: * CentOS Stream 9 -> GCC 11.2 (CentOS 8 is EOL) * Ubuntu 22.04 LTS -> GCC 11.x (Ubuntu 20 just ended standard support) * Debian 12 (oldstable) -> GCC 12.2 * (Debian 11 has ended security updates, uses GCC 10.2) This required generating a new docker image based on Ubuntu 22 for CI using gcc. The existing Ubuntu 20 image works for covering appropriate clang versions (though we should maybe add a much later version as well, in the next increment of our Ubuntu 22 image; however the minimum available clang build from apt.llvm.org for Ubuntu 22 is clang 13). Update to SetDumpFilter is to quiet a mysterious gcc-13 warning-as-error. Removed --compile-no-warning-as-error from a cmake command line because cmake in the new docker image is too old for this option. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13904 Test Plan: CI, one minor unit test added to verify std::counting_semaphor works Reviewed By: xingbowang Differential Revision: D81266435 Pulled By: pdillinger fbshipit-source-id: 26040eeccca7004416e29a6ff4f6ea93f2052684 |
||
|
Hui Xiao
|
b67149a55e |
Skip DumpStats() on dropped CF (#13900)
Summary: **Context/Summary:** DumpStats() do not skip dropped CF and can run into a seg fault like below ``` 2025-08-23T06:44:05.0469230Z �[0;32m[ RUN ] �[mFormatLatest/ColumnFamilyTest.LiveIteratorWithDroppedColumnFamily/0 2025-08-23T06:44:05.0470050Z Received signal 11 (Segmentation fault: 11) 2025-08-23T06:44:05.0470510Z #0 0x7000069305e0 2025-08-23T06:44:05.0471070Z https://github.com/facebook/rocksdb/issues/1 rocksdb::DBImpl::DumpStats() (in librocksdb.10.6.0.dylib) (db_impl.cc:1076) ``` This PR skipped it. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13900 Test Plan: - Deterministically repro-ed the seg fault before the fix and ensure it doesn't happen after the fix ``` diff --git a/db/column_family_test.cc b/db/column_family_test.cc index 3a2ca0617..f57d6f757 100644 --- a/db/column_family_test.cc +++ b/db/column_family_test.cc @@ -2372,11 +2372,17 @@ TEST_P(ColumnFamilyTest, LiveIteratorWithDroppedColumnFamily) { int kKeysNum = 10000; PutRandomData(1, kKeysNum, 100); { + ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->LoadDependency( + {{"PostDrop", "BeforeAccessCFD"}, {"PostAccessCFD", "BeforeGo"}}); + + ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing(); std::unique_ptr<Iterator> iterator( db_->NewIterator(ReadOptions(), handles_[1])); iterator->SeekToFirst(); DropColumnFamilies({1}); + TEST_SYNC_POINT("PostDrop"); + TEST_SYNC_POINT("BeforeGo"); // Make sure iterator created can still be used. int count = 0; @@ -2386,6 +2392,9 @@ TEST_P(ColumnFamilyTest, LiveIteratorWithDroppedColumnFamily) { } ASSERT_OK(iterator->status()); ASSERT_EQ(count, kKeysNum); + + ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->DisableProcessing(); + ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->ClearAllCallBacks(); } Reopen(); diff --git a/db/db_impl/db_impl.cc b/db/db_impl/db_impl.cc index a8e4f5f8f..a8a0499c0 100644 --- a/db/db_impl/db_impl.cc +++ b/db/db_impl/db_impl.cc @@ -1073,8 +1073,10 @@ void DBImpl::DumpStats() { continue; } - auto* table_factory = - cfd->GetCurrentMutableCFOptions().table_factory.get(); + TEST_SYNC_POINT("BeforeAccessCFD"); + auto moptions = cfd->GetCurrentMutableCFOptions(); + auto* table_factory = moptions.table_factory.get(); + TEST_SYNC_POINT("PostAccessCFD"); assert(table_factory != nullptr); // FIXME: need to a shared_ptr if/when block_cache is going to be mutable Cache* cache = ~ ``` Reviewed By: archang19 Differential Revision: D81003739 Pulled By: hx235 fbshipit-source-id: bdf3c4cc45988f43e79ebc191a20af5b70ac289f |
||
|
Changyu Bi
|
82b5a2d3fc |
Allow ingestion of any DB generated SST file (#13878)
Summary: `IngestExternalFileOptions::allow_db_generated_files` requires SST files to have zero sequence number. This PR opens it up for any DB generated SST files. Currently we don't do global sequence number assignment when `allow_db_generated_files` is true, so we require that files do not overlap with any key in the CF. One behavior difference is that now we allow ingesting overlapping files when `allow_db_generated_files` is true. Users need to ensure that files are ordered such that later files have more recent updates. Intended follow ups: - Record smallest seqno in table property, so that we don't need to scan the file for it. - Cover allow_db_generated_files in crash test. We may create a new DB and ingest all files from a CF for verification. - Add APIs that uses allow_db_generated_files. For example, an API for ingesting SST files from a source CF, so that we take care of ingestion file ordering for user. If we are already getting metadata from the source CF, we may be use it as a hint for level placement instead of dividing input files into batches again (`ExternalSstFileIngestionJob::DivideInputFilesIntoBatches`). Pull Request resolved: https://github.com/facebook/rocksdb/pull/13878 Test Plan: two new unit tests. Reviewed By: hx235, xingbowang Differential Revision: D80233727 Pulled By: cbi42 fbshipit-source-id: 74209386d8426c434bff2d9a734f06db537eb50c |
||
|
Ryan Hancock
|
0b44282a9d |
Introduction of MultiScanOptions (#13837)
Summary: To better support future options, and changes, we need to convert the std::vector<ScanOptions> to something more malleable. This diff introduces the MultiScanOptions structure and pipes it through the various points in the code in the Prepare path. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13837 Test Plan: Ensure all associated tests pass ``` make check all ``` Reviewed By: cbi42 Differential Revision: D79655229 Pulled By: krhancoc fbshipit-source-id: 3a90fb7420e9655021de85ed0158b866f8bfba05 |
||
|
Changyu Bi
|
3bd7d968e1 |
Introduce column family option cf_allow_ingest_behind (#13810)
Summary: this option has the same functionality as DBOptions::allow_ingest_behind but allows the feature at per CF level. `DBOptions::allow_ingest_behind` is deprecated after this PR and users should use `cf_allow_ingest_behind` instead. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13810 Test Plan: updated some existing tests to use the new option. Reviewed By: xingbowang Differential Revision: D79191969 Pulled By: cbi42 fbshipit-source-id: 0da45f6be472ace6754ad15df93d45ac86313837 |
||
|
Xingbo Wang
|
11a259a5f0 |
Support GetFileSize API in FSRandomAccessFile (#13676)
Summary:
Add file size validation in ReadFooterFromFile function.
Deprecate skip_checking_sst_file_sizes_on_db_open option.
This change is used to address this issue
https://github.com/facebook/rocksdb/issues/13619
It supports file size validation in ReadFooterFromFile. In favor of this
change, CheckConsistency function and
skip_checking_sst_file_sizes_on_db_open flag are deprecated.
The CheckConsistency function checks each file size matches what was
recorded in manifest during DB open. Meantime, ReadFooterFromFile was
called for each file in LoadTables function. Since ReadFooterFromFile
always validates file size, the CheckConsistency is redundant.
In addtion, CheckConsistency is executed in a single thread. This could
slow down DB open when a network file system is used. Therefore, the
flag skip_checking_sst_file_sizes_on_db_open was added to skip this
check. After this change, ReadFooterFromFile was executed in parallel
through multiple threads. Therefore, the concern of DB open slowness is
eliminated, and the flag could be deprecated.
When paranoid check flag is set to true, corrupted file will fail to open the DB.
When paranoid check flag is set to false, DB will still be able to open, the
healthy ones can be accessed, while the corrupted ones not.
There is 2 slight concerns of this change.
*If max_open_files is set with smaller value, engine will not open all
the files during DB open. This means if there is a corruption on file
size, it will not be detected during DB open, but rather at a later
time. Since the default is -1, which means open all the files, and it is
rarely overridden and a lot of new features rely on it to be -1, the
risk is very low.
*If FIFO compaction is used, engine could fail to open DB unnecessarily
on the corrupted files that would never be used again. However, this is
a very rare case as well. The error could still be ignored by setting
paranoid_checks operationally. The risk is very low.
To remain backward compatibility. The public facing flag was kept and
marked as no-op internally. Another change is required to fully remove
the flag.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/13676
Test Plan:
make check
A new unit test was added to validate file size check API works as
expected.
Reviewed By: pdillinger
Differential Revision: D76168033
Pulled By: xingbowang
fbshipit-source-id: 8ceacf39bcfe02ff7aa289868c341366ee9f3a8e
|
||
|
Changyu Bi
|
4f7d3a0cb2 |
Add a new periodic task to trigger compactions (#13736)
Summary: address an existing limitation on compaction triggering mechanism that relies on events like flush/compaction/SetOptions. This is important for periodic compactions where files can become eligible without any of these events. The periodic task now runs every 12 hours and check CFs that enables `periodic_compaction_second` (TBD if we want to expand to all CFs) for eligible compactions. Some of the periodic tasks probably don't need to run immediately after Register(). I'm keeping the existing behavior for now for patch release and to makes tests happy. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13736 Test Plan: - new unit test that fails before this change. - ran crash test for hours with the periodic task running every 5 seconds: `python3 ./tools/db_crashtest.py blackbox --test_batches_snapshot=0 --periodic_compaction_seconds=10` Reviewed By: pdillinger Differential Revision: D77460715 Pulled By: cbi42 fbshipit-source-id: 00f61502753185e76830c9ed44c5ccc4f4f16bfa |
||
|
anand76
|
8b84390517 |
Add upper bound support for forward scans in MultiScan (#13723)
Summary: Respect the scan upper bound/limit, if specified, in `MultiScan`. This applies to block based table and other native RocksDB SSTs. In order to properly support it, the `MultiScan` object caches the `ReadOptions` passed by the user and sets the `iterate_upper_bound` as appropriate. We optimize for the case of either all scans specifying the upper bound, or none of them. In case of mixed scans, we reallocate the DB iterator anytime `ReadOptions` has to be updated. Tests: New unit tests in `db_iterator_test` Pull Request resolved: https://github.com/facebook/rocksdb/pull/13723 Reviewed By: cbi42 Differential Revision: D77385049 Pulled By: anand1976 fbshipit-source-id: 9c02d125770cbedbe6e8c10767ba537e7f7540e1 |
||
|
Changyu Bi
|
0119a8c78b |
Fix Checkpoint::ExportColumnFamily() returning staled data (#13654)
Summary: `Checkpoint::ExportColumnFamily()` calls DB::Flush() before getting all SST file metadata through `GetColumnFamilyMetaData()`. `GetColumnFamilyMetaData()` gets metadata through the SuperVersion but Flush() does not guarantee the flush result is reflected in SuperVersion upon return (explained below). This PR updates `GetColumnFamilyMetaData()` to get metadata from version instead. Since `GetColumnFamilyMetaData()` [acquires db mutex](https://github.com/facebook/rocksdb/blob/0c533e61bc6d89fdf1295e8e0bcee4edb3aef401/db/db_impl/db_impl.cc#L5023-L5031), it should not need to acquire SV anyway. Fixes https://github.com/facebook/rocksdb/issues/13652. Here we explain how Flush(wait=true) does not guarantee that the flush result is in SuperVersion when the call returns. - RocksDB uses group commit to do MANIFEST update. - When a flush tries to install its flush result, it may be done by another MANIFEST writer. - MANIFEST write is done atomically together with updating Version and cfd->imm() (the list of immutable memtables), but it does not install new SuperVresion - When the MANIFEST writer releases db mutex, the flush wait thread finds that cfd->imm() does not have the relevant memtable anymore: https://github.com/facebook/rocksdb/blob/09175119d2464d7ceecdf1cb7d6d5b517b730965/db/db_impl/db_impl_compaction_flush.cc#L2739-L2742 Pull Request resolved: https://github.com/facebook/rocksdb/pull/13654 Test Plan: the repro in https://github.com/pcholakov/rocksdb/commit/a52d426e82ff5a3dd181dbd5d676dbb54080f5fa pass after this change. Reviewed By: hx235 Differential Revision: D75795658 Pulled By: cbi42 fbshipit-source-id: 4f10baff67944bcd762cf0d237d653a8a35dbca3 |
||
|
Changyu Bi
|
f49d76b7ad |
Clarify that memtable_op_scan_flush_trigger does not support tailing iterator (#13586)
Summary: clarify in comments and fix one implementation under NewIterator where option `memtable_op_scan_flush_trigger` does not work correctly with tailing iterator yet. This is because tailing iterator can rebuild iterator internally which reads from a newer memtable, and DBIter's reference to active memtable needs to be refreshed. This PR clarifies that `memtable_op_scan_flush_trigger` will have no effect on tailing iterator. We can add the support in the future if needed. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13586 Test Plan: existing tests. Reviewed By: jaykorean Differential Revision: D74108099 Pulled By: cbi42 fbshipit-source-id: 7c6608485d57755abc44f3be0b3c5d82a7bc5ca9 |
||
|
Yu Zhang
|
476a98ca30 |
Add a new GetNewestUserDefinedTimestamp API (#13547)
Summary: This PR adds a DB::GetNewestUserDefinedTimestamp API to get the newest timestamp of the column family. This is only for when the column family enables user defined timestamp. It checks the mutable memtable, the immutable memtable and the SST files, and returns the first newest user defined timestamp found. When user defined timestamp is not persisted in SST files, there is metadata in MANIFEST tracking upperbound of flushed timestamps, so the newest timestamp in SST files can be found. If user defined timestamps are persisted in SST files, currently no timestamp metadata info is persisted. A NotSupported status will be returned if SST files need to be checked in that case. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13547 Test Plan: Added tests Reviewed By: cbi42 Differential Revision: D73123575 Pulled By: jowlyzhang fbshipit-source-id: 460ac4f9c96926d3c8fcf7944edab8dc0feae1dd |
||
|
Peter Dillinger
|
2a0ee4ddd8 |
Refactor wal related naming and more (#13490)
Summary: * Clarify in API comments which `log_` options in DBOptions relate to WALs, info log, and/or manifest files. * Rename a bunch of "log" things to "wal" for clarity, especially in DBImpl. (More to go, especially some more challenging cases like `DBImpl::logs_`, but a step in the right direction IMHO) * Simplify DBImpl ctor by moving constant initializers to field definitions. * Use RelaxedAtomic for (renamed) `wals_total_size_` Pull Request resolved: https://github.com/facebook/rocksdb/pull/13490 Test Plan: existing tests Reviewed By: cbi42 Differential Revision: D71939382 Pulled By: pdillinger fbshipit-source-id: 852f4737eca83e6ad653010cc197ad1b6e6bae13 |
||
|
Changyu Bi
|
56359da691 |
Trigger memtable flush based on number of hidden entries scanned (#13523)
Summary: Introduce a mutable CF option `memtable_op_scan_flush_trigger`. When a DB iterator scans this number of hidden entries (tombstones, overwritten puts) from the active memtable in a Seek() or Next() operation, it marks the memtable to be eligible for flush. Subsequent write operations will schedule the marked memtable for flush. The main change is small and is in db_iter.cc. Some refactoring is done to consolidate and simplify creation of `ArenaWrappedDBIter` and `DBIter`. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13523 Test Plan: - new unit tests added. - added `memtable_op_scan_flush_trigger` in crash test - benchmark: The following benchmark was done with a previous version of the PR where the option was `memtable_tombstone_scan_limit` and it concerns tombstone only. The results should still be applicable for the case when there's no overwritten puts. Tests that when memtable has many tombstones, the option helps to improve scan performance: ``` TEST_TMPDIR=/dev/shm ./db_bench --benchmarks=seekrandomwhilewriting --expand_range_tombstones=true --writes_per_range_tombstone=1 --max_num_range_tombstones=10000000 --perf_level=2 --range_tombstone_width=100 --memtable_tombstone_scan_limit= memtable_tombstone_scan_limit = 10000 seekrandomwhilewriting : 18.527 micros/op 53973 ops/sec 18.527 seconds 1000000 operations; (7348 of 1000000 found) next_on_memtable_count = 122305248 grep "flush_started" /dev/shm/dbbench/LOG | wc 8 200 2417 memtable_tombstone_scan_limit=200 seekrandomwhilewriting : 4.918 micros/op 203315 ops/sec 4.918 seconds 1000000 operations; (4510 of 1000000 found) next_on_memtable_count = 1853167 grep "flush_started" /dev/shm/dbbench/LOG | wc 184 4600 54121 When memtable_tombstone_scan_limit=200, more flush is trigged to drop tombstones sooner and improve scan performance. ``` Tests that the new option does not introduce noticeable regression: ``` TEST_TMPDIR=/dev/shm ./db_bench --benchmarks=seekrandomwhilewriting[-X5] --expand_range_tombstones=true --writes_per_range_tombstone=1 --max_num_range_tombstones=10000000 --perf_level=2 --range_tombstone_width=100 --seed=123 Main: seekrandomwhilewriting [AVG 5 runs] : 46049 (± 4512) ops/sec PR: seekrandomwhilewriting [AVG 5 runs] : 46100 (± 4470) ops/sec The results are noisy with this PR performing better and worse in different runs, with no noticeable regression. ``` Reviewed By: pdillinger Differential Revision: D72596434 Pulled By: cbi42 fbshipit-source-id: 2d51a0221dc20dac844aeba2ad3999d075a4cf91 |
||
|
anand76
|
f7764cb6b2 |
Remove fail_if_options_file_error DB option (#13504)
Summary: The fail_if_options_file_error has been deprecated for more than a year. This PR removes it from the code base. https://github.com/facebook/rocksdb/issues/12056 fixed a bug that was blocking the option from removal. https://github.com/facebook/rocksdb/issues/12249 marked it as deprecated. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13504 Reviewed By: hx235 Differential Revision: D72194063 Pulled By: anand1976 fbshipit-source-id: 0aa7cf56e60c48c7e7654743d3e64922ce65225d |
||
|
anand76
|
24e2b05e61 |
Multi scan API (#13473)
Summary: A multi scan API for users to pass a set of scan ranges and have the table readers determine the optimal strategy for performing the scans. This might include coalescing of IOs across scans, for example. The requested scans should be in increasing key order. The scan start keys and other info is passed to NewMultiScanIterator, which in turn uses the newly added Prepare() interface in Iterator to update the iterator. The Prepare() takes a vector of ScanOptions, which contain the start keys and optional upper bounds, as well as user defined parameters in the property_bag taht are passed through as is to external table readers. The initial implementation plumbs this through to the ExternalTableReader. This PR also fixes an issue of premature destruction of the external table iterator after the first scan of the multi-scan. The `LevelIterator` treats an invalid iterator as a potential end of file and destroys the table iterator in order to move to the next file. To prevent that, this PR defines the `NextAndGetResult` interface that the external table iterator must implement. The result returned by `NextAndGetResult` differentiates between iterator invalidation due to out of bound vs end of file. Eventually, I envision the `MultiScanIterator` to be built on top of a producer-consumer queue like container, with RocksDB (producer) enqueueing keys and values into the container and the application (consumer) dequeueing them. Unlike a traditional producer consumer queue, there is no concurrency here. The results will be buffered in the container, and when the buffer is empty a new batch will be read from the child iterators. This will allow the virtual function call overhead to be amortized over many entries. TODO (in future PRs): 1. Update the internal implementation of Prepare to trim the ScanOptions range based on the intersection with the table key range, taking into consideration unbounded scans and opaque user defined bounds. 2. Long term, take advantage of Prepare in BlockBasedTableIterator, atleast for the upper bound case. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13473 Reviewed By: pdillinger Differential Revision: D71447559 Pulled By: anand1976 fbshipit-source-id: 31668abb0c529aa1ac1738ae46c36cbddf9148f1 |
||
|
Peter Dillinger
|
82794e0a4f |
Deprecate RangePtr, favor new RangeOpt and OptSlice (#13481)
Summary: The new API in https://github.com/facebook/rocksdb/issues/13453 is awkward and precarious because of using RangePtr, which encodes optional keys using raw pointers to Slice. We could use `std::optional<Slice>` instead but that is unsatisfyingly a larger object with an inefficient size (typically 17 bytes). Here I introduce a custom optional Slice type, `OptSlice`, that is the same size as a Slice, and use it in a number of places to clean up code and make some public APIs easier to work with. This includes * `atomic_replace_range` (not yet released, OK to change) * `GetAllKeyVersions()` which gets a behavior change because of its unusual handling of empty keys. * `DeleteFilesInRanges()` * TODO in follow-up: `CompactRange()` Most of the diff is associated updates and refactorings. Also * Move some relevant things out of db.h to keep it as tidy as possible. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13481 Test Plan: tests updated Reviewed By: hx235 Differential Revision: D71747774 Pulled By: pdillinger fbshipit-source-id: b4c8519608d119b8bceca9bb0fd778608f62a141 |
||
|
Yu Zhang
|
934cf2d40d |
Implement the DB::GetPropertiesOfTablesForLevels API (#13469)
Summary: As titled. This API returns the table properties of files per level. It can be handy for use cases that needed file's leveling info while retrieving TableProperties. We will use this API to later aggregate per level data write time info. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13469 Test Plan: Added unit tests Reviewed By: pdillinger Differential Revision: D71353096 Pulled By: jowlyzhang fbshipit-source-id: dc1fbb2c97e4365fc8d7241f9a59c65fbf4fb766 |
||
|
Peter Dillinger
|
7f3ee34cdf |
Experimental ingestion option atomic_replace_range (#13453)
Summary: Adding a new option (argument) for file ingestion `atomic_replace_range` which is intended to support a couple forms of "atomic replacement of a key range": * (Experimental implementation here) With snapshot_consistency=false, the feature acts like an atomic DeleteFilesInRange prior to the ingestion, though requires no existing files to partially overlap the range. (Consider using SstPartitioner.) This is especially useful for "always compacted" workloads, perhaps along with CF option `disallow_memtable_writes` and ingestion option `fail_if_not_bottommost_level`. If both bounds are nullptr, the whole CF is replaced. * (To implement in follow-up) With snapshot_consistency=true (and perhaps in some fallback cases from above such as partial overlap), a "giant tombstone file" as in https://github.com/facebook/rocksdb/issues/13078 is generated and ingested at the beginning of the list. Because I see this as a more elaborate DeleteRange, I would naturally expect the upper bound/limit key to be exclusive, but it has been challenging getting that to work. The inclusive/exclusive handling is currently a documented bug for the experimental feature to sort out in follow-up work. (I would love to take advantage of proposed SliceBound, but that would be ambitious to adapt to DeleteRange. Even getting the "replace whole CF" variant of the functionality might be difficult to get worthing with DeleteRange underneath. Nevertheless, I feel it's best to consolidate these two forms of "atomic replacement" under variants of the same API.) Pull Request resolved: https://github.com/facebook/rocksdb/pull/13453 Test Plan: Unit tests added / updated. db_stress integration left as follow-up work (experimental feature, will be challenging) Reviewed By: anand1976 Differential Revision: D71584295 Pulled By: pdillinger fbshipit-source-id: 307abff426e4b7d0a340008918ebcddc896ef747 |
||
|
Peter Dillinger
|
da8eba8b49 |
Improve consistency of SeqnoToTime tracking in SuperVersion (#13316)
Summary: This is an unexpectedly complex follow-up to https://github.com/facebook/rocksdb/issues/13269. This change solves (and detects regressed) inconsistencies between whether a CF's SuperVersion is configured with a preserve/preclude option and whether it gets a usable SeqnoToTimeMapping. Operating with preserve/preclude and no usable mapping is degraded functionality we need to avoid. And no mapping is useful for actually disabling the feature (except with respect to existing SST files, but that's less of a concern for now). The challenge is that how we maintain the DB's SeqnoToTimeMapping can depend on all the column families, and we don't want to iterate over all column families *for each column family* (e.g. on initially creating each). The existing code was a bit relaxed: * On initially creating or re-configuring a CF, we might install an empty mapping, but soon thereafter (after releasing and re-acquiring the DB mutex) re-install another SuperVersion with a useful mapping. The solution here is to refactor the logic so that there's a distinct but related workflow for (a) ensuring a quality set of mappings when we might only be considering a single CF (`EnsureSeqnoToTimeMapping()`), and (b) massaging that set of mappings to account for all CFs (`RegisterRecordSeqnoTimeWorker`) which doesn't need to re-install new SuperVersions because each CF already has good mappings and will get updated SuperVersions when the periodic task adds new mappings. This should eliminate the extra SuperVersion installs associated with preserve/preclude on CF creation or re-configure, making it the same as any other CF. Some more details: * Some refactorings such as removing new_seqno_to_time_mapping from SuperVersionContext. (Now use parameter instead of being stateful.) * Propagate `read_only` aspect of DB to more places so that we can pro-actively disable preserve/preclude on read-only DBs, so that we don't run afoul of the assertion expecting SeqnoToTime entries. * Introduce a utility struct `MinAndMaxPreserveSeconds` for aggregating preserve/preclude settings in a useful way, sometimes on one CF and sometimes across multiple CFs. Much cleaner! (IMHO) * Introduce a function `InstallSuperVersionForConfigChange` that is a superset of `InstallSuperVersionAndScheduleWork` for when a CF is new or might have had a change to its mutable options. * Eliminate redundant re-install SuperVersions of created "missing" CFs in DBImpl::Open. Intended follow-up: * Ensure each flush has an "upper bound" SeqnoToTime entry, which would resolve a FIXME in tiered_compaction_test, but causes enough test churn to deserve its own PR + investigation. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13316 Test Plan: This change is primarily validated by a new assertion in SuperVersion::Init to ensure consistency between (a) presence of any SeqnoToTime mappings in the SuperVersion and (b) preserve/preclude option being currently set. One unit test update was needed because we now ensure at least one SeqnoToTime entry is created on any DB::Open with preserve/preclude, so that there is a lower bound time on all the future data writes. This required a small hack in associating the time with Seqno 1 instead of 0, which is reserved for "unspecified old." Reviewed By: cbi42 Differential Revision: D70540638 Pulled By: pdillinger fbshipit-source-id: bb419fdbeb5a1f115fc429c211f9b8efaf2f56d7 |
||
|
Changyu Bi
|
7e272d2032 |
Update MultiGet to provide consistent CF view for kPersistedTier (#13433)
Summary: when reading with ReadOptions::read_tier = kPersistedTier and with a snapshot, MultiGet allows the case where some CF is read before a flush and some CF is read after the flush. This is not desirable, especially when atomic_flush is enabled and users use MultiGet to do some consistency checks on the data in SST files. This PR updates the code path for SuperVersion acquisition to get a consistent view across when kPersistedTier is used. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13433 Test Plan: a new unit test that could be flaky without this change. Reviewed By: jaykorean Differential Revision: D70509688 Pulled By: cbi42 fbshipit-source-id: 80de96f94407af9bb2062b6a185c61f65827c092 |
||
|
Peter Dillinger
|
c9ce4a3d6b |
Improve atomicity of SetOptions, skip manifest write (#13384)
Summary: Motivated by code review issue in https://github.com/facebook/rocksdb/issues/13316, we don't want to release the DB mutex in SetOptions between updating the cfd latest options and installing the new Version and SuperVersion. SetOptions uses LogAndApply to install a new Version but this currently incurs an unnecessary manifest write. (This is not a big performance concern because SetOptions dumps a new OPTIONS file, which is much larger than the redundant manifest update.) Since we don't want IO while holding the DB mutex, we need to get rid of the manifest write, and that's what this change does. We introduce a kind of dummy VersionEdit that allows the existing code paths of LogAndApply to install a new Version (with the updated mutable options), recompute resulting compaction scores etc., but without the manifest write. Part of the validation for this is new assertions in SetOptions verifying the consistency of the various copies of MutableCFOptions. (I'm not convinced we need it in SuperVersion in addition to Version, but that's not for here and now.) These checks depend on defaulted `operator==` so depend on C++20. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13384 Test Plan: New unit test in addition to new assertions. SetOptions already tested heavily in crash test. Used `ROCKSDB_CXX_STANDARD=c++20 make -j100 check` to ensure the new assertions are verified Reviewed By: cbi42 Differential Revision: D69408829 Pulled By: pdillinger fbshipit-source-id: 4cf026010c6bb381e0ea27567cce2708d4678e7d |
||
|
Changyu Bi
|
dd01f73e26 |
Fix a bug in GetMergeOperands() with continue_cb set (#13383)
Summary: Noticed this while I was working on memtable code. Wrong status (MergeInProgress()) and wrong number of merge operands can be returned if the `continue_cb` stop at an immutable memtable. This is due to 1. Get from memtable sets MergeInProress() status https://github.com/facebook/rocksdb/blob/302254d928402c80ecabb0cf2b76d18be35a87b9/db/memtable.cc#L1461 2. Get from immutable memtable does not update status but stops the get: https://github.com/facebook/rocksdb/blob/302254d928402c80ecabb0cf2b76d18be35a87b9/db/memtable.cc#L1364 3. GetImpl() only returns merge_operands for OK status: https://github.com/facebook/rocksdb/blob/302254d928402c80ecabb0cf2b76d18be35a87b9/db/db_impl/db_impl.cc#L2552 Also updated some comments for GetMergeOperands(). Pull Request resolved: https://github.com/facebook/rocksdb/pull/13383 Test Plan: added a unit test that fails GetMergeOperands() with MergeInProgress() status before this fix. Reviewed By: jowlyzhang Differential Revision: D69322133 Pulled By: cbi42 fbshipit-source-id: aebfccd8446e9640cff02877915076e2d10f7a5b |
||
|
Yu Zhang
|
354025fc86 |
Fix flaky test ExternalSSTFileBasicTest.Basic (#13374)
Summary: This test is flaky likely due to synchronization of the file ingestion thread and the live write thread with test sync points are not working as expected sometimes. Very occasionally, the live write thread can enter the write queue after file ingestion job already dequeued. Or it entered and waited for a very short period of time and quickly returned in the fast path: https://github.com/facebook/rocksdb/blob/833a2266a394fe5f140d2a22f406c82bb605c726/db/write_thread.cc#L83-L86 To fix the flakiness, I moved the test sync points to make sure the write thread is already linked into the write queue before the file ingestion writer get dequeued, so it definitely would need to wait some time in order to do its write. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13374 Test Plan: I'm able to reproduce the flakiness with this command before the fix with every two or three runs: ./gtest-parallel external_sst_file_basic_test --gtest_filter=ExternalSSTFileBasicTest.Basic --repeat=10000 --workers=100 After the fix, I have tried the command for 10 runs, and there is no failure detected. Reviewed By: cbi42 Differential Revision: D69258712 Pulled By: jowlyzhang fbshipit-source-id: adcbad4dd53ccddab5c137d3f9d740b9f9623207 |
||
|
Maciej Szeszko
|
591f5b1266 |
Remove deprecated DB::DeleteFile API references (#13322)
Summary: Cleanup post https://github.com/facebook/rocksdb/pull/13284. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13322 Test Plan: 1. We did not find any evidence of breakage in internal pre-release integration pipeline runs after renaming the deprecated API in `9.10`. 2. _To the extent possible_, we manually validated partner use cases of file deletion and confirmed deprecated API is no longer in use. Reviewed By: jaykorean Differential Revision: D68476852 Pulled By: mszeszko-meta fbshipit-source-id: fbe1f873e16ae7c60d7706a3c44ecc695ab86a4b |
||
|
Peter Dillinger
|
780280b52e |
Reduce unnecessary MutableCFOptions copies and parameters (#13301)
Summary: As follow-up to https://github.com/facebook/rocksdb/issues/13239, this change is primarily motivated by simplifying the calling conventions of LogAndApply. Since it must be called while holding the DB mutex, it can read safely read cfd->GetLatestMutableCFOptions(), until it releases the mutex within ProcessManifestWrites. Before it releases the mutex, it makes a copy of the mutable options in a new, unpublished Version object, which can be used when not holding the DB mutex. This eliminates the need for callers of LogAndApply to copy mutable options for its sake, or even specify mutable options at all. And it eliminates the need for *another* copy to be saved in ManifestWriter. Other functions that don't need the mutable options parameter: * ColumnFamilyData::CreateNewMemtable() * CompactionJob::Install() / InstallCompactionResults() * MemTableList::*InstallMemtable*() * Version::PrepareAppend() Pull Request resolved: https://github.com/facebook/rocksdb/pull/13301 Test Plan: existing tests, CI with sanitizers Reviewed By: mszeszko-meta Differential Revision: D68234865 Pulled By: pdillinger fbshipit-source-id: 6ce95f9cc479834e09ffc8ce93cbae7b664329e5 |
||
|
Peter Dillinger
|
b3333587eb |
Clean up some CFOptions code hygiene, fix SetOptions() bug (#13294)
Summary: To start, I wanted to remove the unnecessary new_options parameter of `InstallSuperVersionAndScheduleWork()`. Passing it something other than the latest mutable options would be inconsistent/outdated. There was even a comment "Use latest MutableCFOptions" on a place that was using the saved options in effect for the compaction. On investigation, this fixes an undiagnosed but longstanding serious bug in SetOptions() where the new settings can be reverted if a flush or compaction started before the SetOptions() finishes after. Fix confirmed with new unit test in db_test.cc. I also got tired of seeing the cumbersome usage of pointer rather than const reference for related options accesses, so there's kind of a large (but trivial) refactoring tied in here as well. (Sorry for combining them; wasn't planning a major bug fix) Intended follow-up: Clarify/simplify the crazy calling conventions of LogAndApply, and remove some unnecessary copying of MutableCFOptions (see new FIXMEs) Pull Request resolved: https://github.com/facebook/rocksdb/pull/13294 Test Plan: test for bug fix, confirmed fails on main and at least as far back as version 8.10. Plus existing tests and CI Reviewed By: mszeszko-meta Differential Revision: D68141563 Pulled By: pdillinger fbshipit-source-id: f6c3290145afa06cc2fe8b485a5de17560a5deea |