mirror of
https://github.com/facebook/rocksdb.git
synced 2026-07-07 14:47:40 +08:00
update-folly-11.6
13973 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Josh Kang
|
3f51c0a185 |
Convert sequential single deletes into range tombstones (#14448)
Summary:
Add a read-path optimization that converts contiguous point tombstones into range tombstones during forward/reverse iteration. When a configurable threshold of consecutive point deletions (kTypeDeletion, kTypeDeletionWithTimestamp, kTypeSingleDeletion — with no live keys between them) is detected, a range tombstone covering `[first_tombstone_key, next_live_key)` is inserted into the active mutable memtable. This benefits future iterators by enabling efficient skipping via range tombstone fragmentation.
If there is a memtable switch during the read iteration, then the range deletion entry is discarded.
The inserted range tombstones are logically redundant (they don't delete anything that isn't already deleted by point tombstones), skip WAL (they're a derived optimization regenerated by future reads on crash), and use the max tombstone sequence number so they don't interfere with newer writes.
## Key changes
- **New option `min_tombstones_for_range_conversion`** (`AdvancedColumnFamilyOptions`): Threshold of contiguous point tombstones before converting to a range tombstone. Default 0 (disabled). Dynamically changeable via `SetOptions()`.
- **`DBIter` tracking logic** (`db/db_iter.cc`): Tracks contiguous tombstones during `FindNextUserEntryInternal()` (forward) and `PrevInternal()` (reverse). When a live key terminates a run that meets the threshold, `MaybeInsertRangeTombstone()` inserts `[first_tombstone, live_key)` into the active memtable.
- **`FindValueForCurrentKey` `found_visible` output** (`db/db_iter.cc`): Distinguishes "key deleted at this snapshot" from "no visible entries" so reverse tracking doesn't treat post-snapshot keys as tombstones.
- **`IterKey::Swap()`** (`db/dbformat.h`): Efficiently tracks reverse tombstone run end keys without extra allocations.
- **`MemTable::AddLogicallyRedundantRangeTombstone()`** (`db/memtable.cc`): Concurrent-safe range tombstone insertion into the active memtable. Range tombstone skiplist always uses concurrent inserts.
- **`ConstructFragmentedRangeTombstones` race fix** (`db/db_impl/db_impl_write.cc`): Moved after `MarkImmutable()` to prevent lost entries.
- **`MarkImmutable` ordering fix** (`db/memtable_list.cc`): Called before `current_->Add()` to close a race window.
- **Prefix filter awareness** (`db/db_iter.cc`): Tombstone tracking scoped to the seek prefix when prefix filtering is active. See dedicated section below.
- **Transaction awareness** (`db/db_iter.cc`): Tombstones with `seq > snapshot` excluded from tracking. `min_uncommitted` guard uses `insert_seq` (which may be bumped to `earliest_seq`) instead of `range_tomb_max_seq_`. See dedicated section below.
- **Duplicate range check**: Skips insertion if the memtable already covers `[start, end)`.
- **New statistics**: `READ_PATH_RANGE_TOMBSTONES_INSERTED` and `READ_PATH_RANGE_TOMBSTONES_DISCARDED`.
- **Memtable MultiGet batch lookup** (`memtable/inlineskiplist.h`, `db/memtable.cc`): `InlineSkipList::MultiGet()` with cached search path ("finger") for sorted key lookups.
- **New option `memtable_batch_lookup_optimization`** (`AdvancedColumnFamilyOptions`): Enables batch lookup for memtable MultiGet. Default false. Immutable.
## Deciding Range Tombstone Seqno
- The range tombstone is inserted with `insert_seq = max(range_tomb_max_seq_, earliest_seq)` where `range_tomb_max_seq_` is the maximum sequence number across all point tombstones in the contiguous run, and `earliest_seq` is the memtable's earliest sequence number. This preserves the memtable's `earliest_seqno_` invariant.
- If the iterator's snapshot sequence (`sequence_`) predates the memtable's `earliest_seq`, insertion is skipped entirely to avoid unintentionally covering entries between `sequence_` and `earliest_seq`.
## ConstructFragmentedRangeTombstones Race Fix
- `MarkImmutable()` and `ConstructFragmentedRangeTombstones()` are now called before `mutex_.Lock()` in `SwitchMemtable`, keeping this work outside the DB mutex. `MarkImmutable()` blocks concurrent `AddLogicallyRedundantRangeTombstone()` calls via `immutable_mutex_`, ensuring no range tombstones are inserted after the fragmented list is built. `MarkImmutable()` is idempotent, so `MemTableList::Add()` calling it again inside the mutex is harmless.
## Prefix Filter Safety
- When prefix filtering is active, the BBTI bloom filter may reject SST files outside the seek prefix, but the memtable (no bloom filter) returns keys across prefix boundaries. Tombstone tracking is scoped to the seek prefix so that converted range tombstones cannot cover live keys hidden in filtered files.
- `total_order_seek=true` disables prefix filtering — all files are visible, so tombstones safely span prefix boundaries.
- **Behavior change**: Seeking to an out-of-domain key with `total_order_seek=false` now treats it as total-order (prefix_ not set). When `prefix_same_as_start=true`, iterating past an out-of-domain key cleanly invalidates the iterator instead of calling `Transform()` on it (which was UB in release builds with `FixedPrefixTransform`). This is a requirement because an incorrect iterator scan could lead to a range tombstone covering a live key.
## Transaction Support
- Tombstones written by the transaction's own uncommitted writes (sequence > snapshot) are now excluded from contiguous tombstone tracking entirely at the tracking site in `FindNextUserEntryInternal()` and `PrevInternal()`. Previously, tracking relied on a `min_uncommitted` check at insertion time, but this was insufficient — a transaction's own Delete with `seq > snapshot` could extend a run of committed tombstones, and the resulting range tombstone would cover data visible to other snapshots.
- The fix skips any tombstone with `ikey_.sequence > sequence_` during tracking. If a transaction-owned tombstone appears mid-run, it flushes the accumulated committed run first, then resets tracking. This ensures only tombstones visible to the current snapshot are ever converted.
- Both WritePrepared and WriteUnprepared transactions are supported with dedicated test coverage:
- **WritePrepared**: When tombstones are committed before `Prepare()`, their seqnos are below `min_uncommitted` and insertion proceeds safely. When `Prepare()` happens first, tombstone seqnos exceed `min_uncommitted` and insertion is blocked.
- **WriteUnprepared**: Multiple unprepared batches with different seqno ranges are handled correctly. Own transaction Deletes that extend a committed tombstone run block insertion of the entire run. After rollback, data correctness is verified.
## UDT support
- When user-defined timestamps (UDT) are enabled, keys include an 8-byte timestamp suffix. The comparator, Put/Delete APIs, and ReadOptions all require timestamps.
- Forward exhaustion with UDT: `iterate_upper_bound_` is a plain user key without a timestamp suffix. It is padded with min timestamp via `AppendKeyWithMinTimestamp()` so it sorts after all entries with this user key, preserving the exclusive bound semantics.
- Reverse exhaustion with UDT: The end key comes from either the previous live key (which already has a proper timestamp suffix) or the seek target set by `SetSavedKeyToSeekForPrevTarget()` (which appends a timestamp via `SetInternalKey(..., timestamp_ub_)` and `UpdateInternalKey(..., ts)`). In both cases, the end key is already properly timestamped, so no additional padding is needed unlike forward exhaustion.
- Contiguous tombstone detection works correctly with UDT because the underlying `kTypeDeletionWithTimestamp` entries are tracked the same way as `kTypeDeletion`.
## Concurrent Iterators
- Should concurrent iterators happen to read the range at the same time, both will produce the same range and seqno entry. Only one will be accepted by the skip list and the others will be rejected. Future iterators will read the range and not even attempt to insert the range.
- There is nothing preventing similar ranges from being inserted however. Two iterators can produce overlapping ranges, but this protection would be complicated to implement and there is no evidence that it is a likely scenario yet.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14448
Test Plan:
- **Unit tests** (`db/db_iterator_test.cc`): `ReadPathRangeTombstoneTest` parameterized by forward/reverse with cases for basic insertion, non-contiguous (below threshold), memtable switch, exhausted iterator with/without bounds, direction change, mixed Delete/SingleDelete, single-delete-only runs, snapshot predating memtable, block cache tier incomplete, skip when covered by existing range, UDT basic scan, UDT exhaustion, prefix filter cross-prefix scan (`PrefixFilterCrossPrefixScanCoversLiveKey` with default/total_order_seek/prefix_same_as_start variants), stale ikey from forward-then-reverse scan (`StaleIkeyFromForwardThenReverse`), and reseek stale ikey (`ReseekStaleIkey`).
- **Concurrency test** (`db/db_test2.cc`): `DBTestConcurrentRangeTombstoneConversions` parameterized by `(allow_concurrent_memtable_write, min_tombstones_for_range_conversion)` with mixed writers, deleters, range deleters, and concurrent forward/reverse readers.
- **Transaction tests** (`utilities/transactions/write_prepared_transaction_test.cc`, `write_unprepared_transaction_test.cc`): Tests for WritePrepared (insertion allowed when tombstones committed before prepare, blocked when after; seqno bump shadowing prepared writes `RangeTombstoneSeqnoBumpShadowsPreparedWrite`) and WriteUnprepared (multiple batches, extended visibility with CalcMaxVisibleSeq, own deletions with rollback).
- **IterKey::Swap tests** (`db/dbformat_test.cc`): `IterKeySwapTest` parameterized over `(key_len, copy, use_secondary)` × 2 covering all inline/heap/pinned/secondary combinations.
- **InlineSkipList MultiGet tests** (`memtable/inlineskiplist_test.cc`): Basic, exact matches, empty, single key, randomized validation against `std::set::lower_bound`, duplicate keys with callback walk, and concurrent MultiGet with read-after-write consistency.
- **Memtable MultiGet tests** (`db/db_basic_test.cc`): Batch lookup, overwrite, flush, merge, disabled by default, paranoid checks, and snapshot tests.
- **Stress test coverage**: `min_tombstones_for_range_conversion` and `memtable_batch_lookup_optimization` options added to `db_crashtest.py` and `db_stress` flags.
- `make check` passes all tests.
### Benchmark results
Tombstones scattered randomly in clusters via `seek_nexts_to_delete` for realistic workloads.
**DB Setup A (scattered deletes, 100 per seek)**:
```
# Step 1: Create and compact
./db_bench --benchmarks=fillseq,compact --seed=1 --compression_type=none --num=1000000 --db=<DB>
# Step 2: Scatter tombstones (5000 seeks × 100 deletes ≈ 500k tombstones)
./db_bench --benchmarks=seekrandom,flush --seed=1 --compression_type=none --num=2000 \
--seek_nexts=0 --seek_nexts_to_delete=100 --use_existing_db=1 --threads=1 --db=<DB>
```
**DB Setup A2 (scattered deletes, 8 per seek)**:
```
# Step 1: Create and compact
./db_bench --benchmarks=fillseq,compact --seed=1 --compression_type=none --num=1000000 --db=<DB>
# Step 2: Scatter tombstones (5000 seeks × 8 deletes ≈ 40k tombstones)
./db_bench --benchmarks=seekrandom,flush --seed=1 --compression_type=none --num=2000 \
--seek_nexts=0 --seek_nexts_to_delete=8 --use_existing_db=1 --threads=1 --db=<DB>
```
**DB Setup B (no deletes)**:
```
./db_bench --benchmarks=fillseq,compact --seed=1 --compression_type=none --num=1000000 \
[--key_size=100] --db=<DB>
```
**Read workload** (same for all):
```
./db_bench --benchmarks=seekrandom --seek_nexts=100 --threads=8 \
--reverse_iterator={true,false} --seed=1 --use_existing_db=1 \
--compression_type=none --num=1000000 --duration=10 \
--disable_auto_compactions \
[--key_size=100] [--min_tombstones_for_range_conversion=X] --db=<DB_COPY>
```
Each workload averaged over 3 runs.
**Table 1: seekrandom forward, scattered deletes (2000 seeks × 100 deletes/seek)**
| Variant | avg ops/s | % vs main |
|---------|-----------|-----------|
| main | 2,895 | - |
| threshold=0 | 2,869 | -0.9% |
| threshold=8 | 287,334 | +9,824% |
**Table 2: seekrandom reverse, scattered deletes (2000 seeks × 100 deletes/seek)**
| Variant | avg ops/s | % vs main |
|---------|-----------|-----------|
| main | 544 | - |
| threshold=0 | 548 | +0.7% |
| threshold=8 | 206,491 | +37,860% |
**Table 3: seekrandom forward, scattered deletes (2000 seeks × 8 deletes/seek)**
| Variant | avg ops/s | % vs main |
|---------|-----------|-----------|
| main | 194,049 | - |
| threshold=0 | 195,703 | +0.9% |
| threshold=8 | 310,740 | +60.1% |
**Table 4: seekrandom reverse, scattered deletes (2000 seeks × 8 deletes/seek)**
| Variant | avg ops/s | % vs main |
|---------|-----------|-----------|
| main | 63,854 | - |
| threshold=0 | 69,266 | +8.5% |
| threshold=8 | 218,101 | +241.6% |
**Table 5: seekrandom forward, no deletes (regression check)**
| Variant | key=16B avg ops/s | % vs main | key=100B avg ops/s | % vs main |
|---------|-------------------|-----------|---------------------|-----------|
| main | 330,901 | - | 236,048 | - |
| threshold=0 | 328,398 | -0.8% | 238,055 | +0.9% |
| threshold=8 | 332,539 | +0.5% | 233,776 | -1.0% |
**Table 6: seekrandom reverse, no deletes (regression check)**
| Variant | key=16B avg ops/s | % vs main | key=100B avg ops/s | % vs main |
|---------|-------------------|-----------|---------------------|-----------|
| main | 261,445 | - | 192,177 | - |
| threshold=0 | 265,020 | +1.4% | 191,616 | -0.3% |
| threshold=8 | 250,881 | -4.0% | 189,239 | -1.5% |
Reviewed By: xingbowang
Differential Revision: D96203950
Pulled By: joshkang97
fbshipit-source-id: 06ba66ebde3c355f04671d1e681f1b1586e8751d
|
||
|
Jialiang Tan
|
65e77282ce |
Add block_decompress_count to PerfContext (#14557)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14557 RocksDB's PerfContext tracks block_decompress_time but has no counter for the number of block decompressions. The global Statistics ticker NUMBER_BLOCK_DECOMPRESSED exists but is not accessible through PerfContext, which is the thread-local, per-operation metric system used by benchmarks. Add block_decompress_count as a new PerfContext counter (gated at kEnableCount level) incremented in DecompressBlockData alongside the existing NUMBER_BLOCK_DECOMPRESSED ticker. This enables benchmarks and applications to observe how many blocks required decompression per operation, complementing the existing block_read_count. Reviewed By: anand1976 Differential Revision: D99233514 fbshipit-source-id: 40a68c1d9321f560cebdb7c30a544a0c62ae64f0 |
||
|
Jialiang Tan
|
a0ff5b143f |
Add num_data_blocks_compression_rejected/bypassed to TableProperties (#14551)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14551 Add two new per-SST TableProperties fields that track how many data blocks were stored uncompressed: - `num_data_blocks_compression_rejected`: blocks where compression was attempted but the compressed output exceeded the ratio limit set by `CompressionOptions::max_compressed_bytes_per_kb`. - `num_data_blocks_compression_bypassed`: blocks where compression was never attempted (e.g., kNoCompression type, no compressor available). Together with `num_data_blocks`, these give a full decomposition: num_data_blocks = compressed + rejected + bypassed Previously this information was only available via global Statistics tickers (NUMBER_BLOCK_COMPRESSION_REJECTED / BYPASSED) which aggregate across all SSTs. Now it is persisted per-SST in the properties block and visible via sst_dump --show_properties. Reviewed By: anand1976 Differential Revision: D99229339 fbshipit-source-id: 8333f51acff30a759d725e5e94bd80f4fcb18b57 |
||
|
Rajat Jain
|
64f8d5765b |
Add blob_compression_opts to ColumnFamilyOptions (#14568)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14568 Port of D98326754 to internal_repo_rocksdb (GitHub-synced repo). BlobDB blob file compression currently uses hardcoded default CompressionOptions{} in BlobFileBuilder, ignoring any user-specified compression options like level, window_bits, or strategy. This means ZSTD always uses level 3 (doubleFast strategy with two hash tables), even when a lower level would significantly reduce CPU usage. This diff adds a new `blob_compression_opts` field to AdvancedColumnFamilyOptions (and MutableCFOptions) and plumbs it into BlobFileBuilder::GetCompressor(). The option is registered as mutable and dynamically changeable via SetOptions(). For example, setting level=1 with ZSTD switches from "doubleFast" (two hash tables per thread) to "fast" (single hash table), reducing L3 cache pressure on flush-heavy workloads. There was an existing TODO in blob_file_builder.cc requesting exactly this feature — this diff fulfills it. This change is safe as the default value is always dFast. So this will not impact the current code. Reviewed By: xingbowang Differential Revision: D99478562 fbshipit-source-id: e79b847a854f60dea552442b18fd3f1384efac70 |
||
|
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
|
||
|
Peter Dillinger
|
ae0c9faaef |
Fix transaction lock timeout in crash test (#14540)
Summary: When BatchedOpsStressTest runs on a TransactionDB, db_->Write() creates internal transactions using default_lock_timeout (1 second), which is too short under heavy contention with many threads. This causes sporadic "Timeout waiting to lock key" errors printed to stderr, which the crash test framework treats as fatal failures. Increase default_lock_timeout to 600000ms (10 minutes) to match the lock_timeout already used for explicit transactions in NewTxn(). Pull Request resolved: https://github.com/facebook/rocksdb/pull/14540 Test Plan: Manual `make crash_test_with_wc_txn` as a sanity check Reviewed By: mszeszko-meta Differential Revision: D98944474 Pulled By: pdillinger fbshipit-source-id: 13b9e13fddc6332da010b01f7c41a51748f75624 |
||
|
Omkar Gawde
|
51561fc2cd |
Add explicit Close() call in WriteStringToFile (#14566)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14566 **What**: Adds an explicit `file->Close()` call in `WriteStringToFile()` before the function returns, instead of relying on the `unique_ptr` destructor to close the file. **How**: In `file_system.cc`, after the optional `Sync()` and before the error-cleanup path, calls `file->Close()`. If `Close()` fails, the file is deleted just like any other write failure. Adds two unit tests in `env_test.cc`: - `WriteStringToFileClosesFile`: uses `CountedFileSystem` to verify `Close()` is called and content is written correctly. - `WriteStringToFileCloseFailureDeletesFile`: uses a custom `CloseFailFS` wrapper to inject a `Close()` failure and verify the file is deleted on error. **Why**: The previous implementation never called `Close()` explicitly — it relied on the `unique_ptr` destructor which may silently swallow write errors that occur during close (e.g., flushing buffered data). Explicit `Close()` ensures such errors are detected and propagated to the caller. Reviewed By: archang19 Differential Revision: D99462173 fbshipit-source-id: 525b5489bd0dbfc3159b007a13f9474f84d3c84e |
||
|
Xingbo Wang
|
ebd118711c |
Temporarily disable trie UDI index in stress test (#14559)
Summary: **Summary:** Disable `use_trie_index` in `db_crashtest.py` to avoid trie UDI stress test failures. The default param was previously set to randomly enable trie index ~12.5% of the time (`random.choice([0, 0, 0, 0, 0, 0, 0, 1])`). Setting it to `0` until the underlying issues are resolved. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14559 Test Plan: Stress test runs without trie UDI failures. Reviewed By: hx235 Differential Revision: D99343747 Pulled By: xingbowang fbshipit-source-id: 6f86b5dd5ddb2869d797e93ddbda521981614bae |
||
|
Hui Xiao
|
ffe58fa976 |
Fix NULL persisted_seqno_ in AnonExpectedState (#14523) (#14523)
Summary: AnonExpectedState::Open() never initialized the base class persisted_seqno_ pointer, leaving it as nullptr. Any call to SetPersistedSeqno() or GetPersistedSeqno() on an AnonExpectedState (used when expected_values_dir is empty) would dereference a null pointer. Fix by allocating and assigning it in Open(), matching FileExpectedState's pattern. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14523 Reviewed By: mszeszko-meta Differential Revision: D98556119 Pulled By: hx235 fbshipit-source-id: c4c3103272735d36c1d05b96f7d009b6b750bf53 |
||
|
Xingbo Wang
|
d6b03d9c96 |
Add pre-push git hook to auto-format code (#14558)
Summary: Add a git pre-push hook that automatically runs `make format-auto` before every push. If formatting issues are found, the hook fixes them, creates a new commit, and retries the push — all in one shot. The hook is auto-configured on the first `make` build via `core.hooksPath`, so there is no manual install step needed. ## Changes **`githooks/pre-push`** — pre-push hook that: 1. Runs `make check-format` to detect formatting issues 2. If issues found, runs `make format-auto` to fix them 3. Creates a new "format" commit with the fixes 4. Retries the push automatically with the format commit included 5. Stashes/restores any uncommitted work so it is not affected 6. Skips in non-interactive environments (CI) unless `ROCKSDB_FORMAT_HOOK` is set 7. Can be skipped with `git push --no-verify` **`Makefile`**: - `setup-hooks` target: auto-sets `core.hooksPath=githooks` in the local repo config. Runs as a dependency of `all`, so any `make` build activates the hook with zero manual setup. - `install-hooks` / `uninstall-hooks` targets: manual copy-based alternative for those who prefer it Pull Request resolved: https://github.com/facebook/rocksdb/pull/14558 Reviewed By: archang19 Differential Revision: D99324330 Pulled By: xingbowang fbshipit-source-id: b824e56d572ad423fab7de7e15ead5fa8fd8e847 |
||
|
Peter Dillinger
|
abc1f7ced5 |
Fix table cache leak when InstallCompactionResults fails (#14549)
Summary: When CompactionJob::Run() succeeds but Install() fails (e.g., LogAndApply MANIFEST I/O error), compact_->status was never updated with the install failure. CleanupCompaction() passed the stale OK status to SubcompactionState::Cleanup(), which skipped ReleaseObsolete -- leaking table cache entries for output files that were cached by VerifyOutputFiles but never installed into any Version. This is the same class of bug fixed in https://github.com/facebook/rocksdb/issues/14469 (where Run() failed after VerifyOutputFiles), but in the Install() failure path. The FindObsoleteFiles full-scan backstop would normally catch this, but fails under crash test metadata read fault injection (--open_metadata_read_fault_one_in), causing the TEST_VerifyNoObsoleteFilesCached assertion to fire during Close(). Fix: propagate Install()'s local status back to compact_->status before CleanupCompaction(), so Cleanup() sees the failure and calls ReleaseObsolete on the output files. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14549 Test Plan: New unit test DBCompactionTest.LeakedTableCacheEntryOnInstallFailure: - Without fix (ASAN): assertion fires -- "File 12 is not live nor quarantined" - With fix (ASAN): passes -- ReleaseObsolete properly cleans up the entry ``` COMPILE_WITH_ASAN=1 make -j db_compaction_test ./db_compaction_test --gtest_filter="DBCompactionTest.LeakedTableCacheEntry*" [ PASSED ] 2 tests. ``` Reviewed By: joshkang97 Differential Revision: D99155908 Pulled By: pdillinger fbshipit-source-id: ed5374a38d7903866a38a0fe0f5539e12321bc84 |
||
|
Xingbo Wang
|
01ec15f328 |
Remove accidentally committed .claude/settings.local.json (#14554)
Summary: This file was accidentally included in https://github.com/facebook/rocksdb/issues/14535. It contains local Claude Code permission settings that are user-specific and should not be in the repo. Also adds it to `.gitignore` to prevent future accidental commits. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14554 Reviewed By: pdillinger Differential Revision: D99296922 Pulled By: xingbowang fbshipit-source-id: e229dd85179ae25bb15df1633bba7197dd2fa1f9 |
||
|
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 |
||
|
Hemal Shah
|
b611aa7309 |
Export header to add log data on a transaction (#14543)
Summary: Transactions in rockdb supported adding log data using the `put_log_data` api, but this was not exported for other language bindings. Exported this binding allows other languages like rust, go, etc add log data on an transaction Pull Request resolved: https://github.com/facebook/rocksdb/pull/14543 Reviewed By: joshkang97 Differential Revision: D99171663 Pulled By: xingbowang fbshipit-source-id: 24c94d71668a41cc7b2913972427255ab67d0863 |
||
|
Andrew Chang
|
830347c765 |
Fix null sqe crash in ReadAsync when io_uring submission queue is full (#14521)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14521 ReadAsync calls io_uring_get_sqe() without checking for nullptr. When the io_uring submission queue is full (outstanding completions not yet reaped), io_uring_get_sqe returns NULL and the subsequent io_uring_prep_readv dereferences it, causing a segfault. MultiRead already handles this correctly by using io_uring_sq_space_left() to cap submissions. ReadAsync submits exactly one SQE per call so a simple null check with error return is sufficient. On null sqe, clean up the already-allocated Posix_IOHandle and return IOStatus::Busy so the caller can retry after reaping completions. The io_uring queue depth is kIoUringDepth (256), and each thread gets its own io_uring instance via thread-local storage. In practice the SQ rarely fills because ReadAsync calls io_uring_submit() after each io_uring_get_sqe(), immediately flushing the SQE to the kernel. The null SQE would only occur under unusual kernel backpressure where the kernel cannot consume from the SQ ring fast enough. IOStatus::Busy was chosen (over IOError) because this is a transient condition. The caller has two options: 1. Call Poll() to reap outstanding completions from the CQ, then retry ReadAsync. This mirrors how MultiRead handles queue pressure internally by capping submissions and reaping between batches. 2. Fall back to synchronous Read(). Existing callers (FilePrefetchBuffer, IODispatcher) already have synchronous fallback paths for non-OK ReadAsync status, so IOStatus::Busy naturally triggers that fallback without additional code changes. Given the rarity of this condition, the synchronous fallback is pragmatic and avoids adding retry complexity. Also adds a TEST_SYNC_POINT_CALLBACK on io_uring_get_sqe to enable test injection, and a new ReadAsyncQueueFull unit test that uses SyncPoint to force a null SQE and verifies the Busy return, handle cleanup, and no crash. Reviewed By: xingbowang Differential Revision: D98533853 fbshipit-source-id: f6d181e5c0d5154b570ff6da39e2f52e2a6aea84 |
||
|
Andrew Chang
|
de6cdbb62d |
Fix SupportedOps to verify per-thread io_uring before advertising kAsyncIO (#14514)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14514 SupportedOps previously checked only that the ThreadLocalPtr container existed (non-null), which was set during construction based on a one-time probe on the main thread. However, CreateIOUring() can fail on other threads due to kernel resource limits or flag incompatibilities, causing ReadAsync to hit "failed to init io_uring" at runtime. Now SupportedOps eagerly initializes the thread-local io_uring instance via Get() + CreateIOUring() and only advertises kAsyncIO if it succeeds. Also logs to stderr (with thread id) when io_uring init fails, and adds a TEST_SYNC_POINT_CALLBACK for testing simulated CreateIOUring failures. Reviewed By: mszeszko-meta, xingbowang Differential Revision: D98409140 fbshipit-source-id: efa92d9ac920860e95a46710c4a87e36bacbb466 |
||
|
Maciej Szeszko
|
b8bbee0f3e |
Log IO uring init failures to stdout (instead of stderr) (#14548)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14548 Reviewed By: archang19 Differential Revision: D98667574 fbshipit-source-id: 4f01c9d8f2ae12051d4b3e4138258091b3c9eb3d |
||
|
Xingbo Wang
|
24fac74206 |
docs: add end-to-end flow documentation (read_flow, write_flow) (#14486)
Summary: Add comprehensive flow documentation tracing data through the full RocksDB system. ## Contents ### read_flow/ (11 chapters) Get/MultiGet/Iterator paths through SuperVersion, MemTable, block cache, SST files (L0→Ln), bloom filters, range deletions, prefetch, async I/O. ### write_flow/ (10 chapters) Put/Delete/Merge through WAL, MemTable, flush, compaction, delete cleanup, flow control, crash recovery, performance. ## Documentation Pattern Each component follows a consistent structure: - `index.md` — short overview (40-80 lines) with chapter table and key characteristics - `NN_topic.md` — deep-dive chapters with source file references and workflow descriptions All content validated against current codebase. No code snippets — refers to header files and struct/function names. Part 1 of 11 in the RocksDB AI documentation series. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14486 Reviewed By: anand1976 Differential Revision: D97795679 Pulled By: xingbowang fbshipit-source-id: 07c3be1a7f27e0a9cc1d82ccad16e8e4084f4ab0 |
||
|
Hui Xiao
|
f12465cb93 |
Add COERCE_CONTEXT_SWITCH and ASSERT_STATUS_CHECKED guidance to CLAUDE.md (#14522)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14522 Add two verification steps to RocksDB CLAUDE.md: 1. **Unit Test flakiness testing**: After writing a test, stress-test for flakiness with `COERCE_CONTEXT_SWITCH=1 make {test_binary}` followed by `--gtest_repeat=5`. This catches race conditions and context-switch-dependent failures early. 2. **Status object verification**: Run `ASSERT_STATUS_CHECKED=1 make check` to catch missing error handling that can lead to silent data corruption. These are compile-time checks enforced via a special build mode. These are existing RocksDB best practices that both the stress test agent and human developers should follow when writing or fixing code. Reviewed By: xingbowang Differential Revision: D98249994 fbshipit-source-id: 25ca6a628a82ecf968d5ed86aaa5c2f4b1060471 |
||
|
Xingbo Wang
|
7832ee886f |
db_stress: fix prefix scan correctness and crashtest stability (#14512)
Summary: This PR fixes several correctness and stability issues in the stress test and crash test infrastructure, plus adds regression coverage for trie UDI iterators. ### db_stress: constrain batched prefix scans `BatchedOpsStressTest::TestPrefixScan` opens 10 iterators with different prefixes and compares results in lockstep. Without `prefix_same_as_start`, unconstrained iterators could return keys beyond their seek prefix, producing more entries than bounded iterators and causing spurious assertion failures. Fix: set `prefix_same_as_start = true` on all iterators so every iterator stops at the seek prefix boundary. ### db_stress: skip invalid cross-prefix iterator checks When prefix iteration is enabled, `ReadOptions` requires that the seek key and `iterate_lower_bound` share the same prefix. Previously, stress test verification could run against configurations where they don't (e.g. when `lower_bound` spans a prefix boundary), producing false positives. Fix: detect this invalid configuration and mark the check as diverged (skip verification) rather than asserting. ### db_crashtest: disable BlobDB in best-efforts recovery BlobDB is not compatible with best-efforts recovery mode. Fix: explicitly disable blob file options when `best_efforts_recovery=True` in db_crashtest.py to avoid spurious failures. ### db_crashtest: preserve expected-state dirs across restarts The expected-state directory was being wiped on certain restart paths, causing verification failures on the next run. Fix: preserve expected-state dirs across db_crashtest restarts. Adds a new `db_crashtest_test.py` test to verify the behavior, runnable via `make db_crashtest_test`. ### Add trie UDI iterator regression coverage Adds regression tests for trie-based UserDefinedIndex (UDI) iterators covering snapshot-based reads, lower/upper bound iteration, `auto_refresh_iterator_with_snapshot`, and multi-version key handling. Also fixes an MSVC C4244 warning (int→char implicit narrowing) in the test. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14512 Reviewed By: hx235 Differential Revision: D98302018 Pulled By: xingbowang fbshipit-source-id: 84f5878665e5aeb61338ec2b6bfb2d2b077bb9f2 |
||
|
Danny Chen
|
ae322e0a73 |
Add SortedRunBuilder: use RocksDB as an external… (#14499)
Summary: CONTEXT: MyRocks and other third-party users currently need to understand deep RocksDB internals to sort unsorted data into SST files. The existing pattern requires: opening a full DB instance, configuring VectorRepFactory with universal compaction, disabling auto-compaction, writing unsorted data, manually triggering CompactRange with kForceOptimized, collecting output via GetColumnFamilyMetaData, and finally ingesting via IngestExternalFile with allow_db_generated_files. This is error-prone and requires ~30 lines of RocksDB configuration knowledge. WHAT: This adds a new utility class, SortedRunBuilder, that wraps all of the above into a simple Create/Add/Finish API. Callers feed in unsorted key-value pairs (in any order, from any number of threads) and receive sorted SST files with seqno=0 ready for ingestion -- or can iterate sorted output directly. The name SortedRunBuilder was chosen over alternatives like UnsortedSstFileWriter because this utility targets a broad audience: anyone wanting to use RocksDB as an external sort engine, not just users migrating from SstFileWriter. That said, this explicitly removes the sorted-input requirement that SstFileWriter enforces -- callers no longer need to pre-sort their data before writing SST files. KEY DESIGN DECISIONS: - Zero changes to core RocksDB internals. This is a pure utility layer that exclusively calls existing public APIs: * DB::Open(), DB::Put(), DB::Write(), DB::Flush(), DB::CompactRange(), DB::GetColumnFamilyMetaData(), DB::NewIterator(), DestroyDB() * VectorRepFactory (sort-on-flush memtable) * BottommostLevelCompaction::kForceOptimized (zero seqnos) - No modifications to: compaction logic, memtable implementation, SST file format, ingestion logic, or DB open/close paths - All new code lives in utilities/sorted_run_builder/ and the public header include/rocksdb/utilities/sorted_run_builder.h - Build file changes are limited to registering the new source/test files API SURFACE: SortedRunBuilderOptions opts; opts.temp_dir = "/tmp/sort_work"; std::unique_ptr<SortedRunBuilder> builder; SortedRunBuilder::Create(opts, &builder); builder->Add(key, value); // any order, thread-safe builder->AddBatch(&batch); // WriteBatch for throughput builder->Finish(); // flush + compact + collect builder->GetOutputFiles(); // sorted SSTs for ingestion builder->NewIterator(ro); // iterate sorted output FILES CHANGED: New: include/rocksdb/utilities/sorted_run_builder.h (public header) New: utilities/sorted_run_builder/sorted_run_builder.cc (implementation) New: utilities/sorted_run_builder/sorted_run_builder_test.cc (12 tests) New: docs/plans/sorted_run_builder_plan.md (design plan) New: docs/plans/sorted_run_builder_usage_guide.md (usage guide) Modified: src.mk, CMakeLists.txt, Makefile (register new files only) Pull Request resolved: https://github.com/facebook/rocksdb/pull/14499 Test Plan: $ make clean && make -j$(nproc) sorted_run_builder_test $ ./sorted_run_builder_test [==========] Running 12 tests from 1 test case. [ PASSED ] 12 tests. (688 ms total) Tests cover: basic sort correctness, empty builder, WriteBatch path, concurrent multi-threaded writes, ingestion into a target DB, large random dataset (10K keys), entry/size counters, error cases (Add after Finish, iterator before Finish, empty temp_dir), cleanup verification, and duplicate key handling. Reviewed By: xingbowang Differential Revision: D98935972 Pulled By: dannyhchen fbshipit-source-id: e3bb7f1ea5f6004aab697e4da667fa21292ca250 |
||
|
Maciej Szeszko
|
1e23ee053f |
skip stats if setup-ccache did not run or failed (#14542)
Summary: `teardown-ccache` runs with if: always(), but in folly jobs `setup-folly` precedes `setup-ccache`. When `setup-folly` fails, `setup-ccache` is skipped, so `CCACHE_DIR` is unset and ccache is not on `PATH`. This is exactly what happened [here](https://github.com/facebook/rocksdb/actions/runs/23660261947/job/68928337162). Fix guards `teardown-ccache` to exit gracefully when `CCACHE_DIR` is unset, and tolerate missing ccache binary for stats. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14542 Reviewed By: joshkang97 Differential Revision: D98964757 Pulled By: mszeszko-meta fbshipit-source-id: 8e83c1b62ef66130ba479142f002897d0a97f6c0 |
||
|
Maciej Szeszko
|
effac3a918 |
Update CLANGTIDY to latest stable & secure version (#14541)
Summary: As advertised. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14541 Reviewed By: xingbowang Differential Revision: D98961628 Pulled By: mszeszko-meta fbshipit-source-id: f0519089bb53649668e791b18428a65fc666b71c |
||
|
Peter Dillinger
|
c7f0e1fa93 |
Store UniqueIdVerifier file in expected_values_dir on local filesystem (#14539)
Summary: The warm_storage_crash_test was failing because UniqueIdVerifier stored its .unique_ids bookkeeping file in the DB directory, which lives on warm storage. After a crash, warm storage's weaker durability guarantees could cause flushed-but-not-synced data to be lost, making the file appear shorter on a second open within the same constructor. This caused CopyFile to return Corruption and trigger assert(false). Move the file to expected_values_dir (which is always on local filesystem) and always use Env::Default() for file operations, so that POSIX flush semantics are sufficient for read consistency without needing Sync. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14539 Test Plan: Full local run of `make blackbox_crash_test` Reviewed By: mszeszko-meta Differential Revision: D98934451 Pulled By: pdillinger fbshipit-source-id: 5497f23c2382cb5ac2c3d7f238c6c4ca1dd29f5b |
||
|
Xingbo Wang
|
141a1f0abf |
Add regression tests and diagnostic logging for trie UDI post-seek seqno correction (#14524)
Summary: Adds regression tests and failure-diagnostic tooling for the trie UDI post-seek seqno correction bug (T258590238). The core fix itself already landed in https://github.com/facebook/rocksdb/issues/14466. ## What's in this PR ### Regression tests - `TrieIndexFactoryTest.ZeroSeqMustNotSkipLeafForSmallerUserKey` — minimal unit test proving the original bug; exercises the seqno-based block advancement with a smaller user key that should NOT trigger advancement - `TrieIndexDBTest.AutoRefreshSnapshotNextAcrossSameUserKeyBoundaries` — DB-level test for snapshot-refresh iteration across same-user-key block boundaries - `TrieIndexDBTest.AutoRefreshSnapshotNextAfterCompactionAcrossSameUserKeyBoundaries` — same scenario after compaction reshapes SST layout - `TrieIndexDBTest.AutoRefreshSnapshotStressLikeSingleCfCoalescingIterator` — stress-like test exercising snapshot-refresh + coalescing-iterator interaction ### Diagnostic logging in db_stress Extracts the iterator verification failure dump into a `DumpIteratorVerificationFailure()` helper in `NonBatchedOpsStressTest`. On verification failure, logs: - Expected-state window (pre/post read values, raw state, pending flags) - Iterator config (UDI, trie, snapshot, multi-CF) - Replay comparison: creates fresh iterators with standard vs trie index, direct vs coalescing, seek-to-failure-key vs replay-from-mid — making it possible to identify which index/iterator combination diverges This logging was instrumental in triaging the original bug and will help with future trie UDI stress failures. ## Tests All existing trie index tests pass. New tests listed above all pass. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14524 Reviewed By: hx235 Differential Revision: D98583342 Pulled By: xingbowang fbshipit-source-id: 57e099055d6beae9b5f03f4e6605f0af6e65b94a |
||
|
zaidoon
|
41e5eb1a4d |
Support reverse iteration in UDI/trie index and optimize hot paths (#14466)
Summary: The following performance optimizations are included in this PR: - Inline NextSetBit/PrevSetBit into header (called on every trie level) - Unroll Rank1 popcount loop (the single hottest function) - Advance/Retreat: replace path stack top in-place instead of pop+push - std::swap for prev_key_scratch_ instead of O(n) string copy - assign() instead of ToString() to reuse buffer capacity - Cache IndexValue in wrapper to avoid repeated virtual dispatch - Mark TrieIndexIterator/TrieIndexBuilder as final for devirtualization Part of https://github.com/facebook/rocksdb/issues/12396 Pull Request resolved: https://github.com/facebook/rocksdb/pull/14466 Reviewed By: anand1976 Differential Revision: D97979699 Pulled By: xingbowang fbshipit-source-id: 7ec56ecc8b6d68548a336dd1aaccfb215189eff0 |
||
|
Hui Xiao
|
875b8544df |
Revert D98425822: Rocksdb Warm Storage Test failed: not implemented: SyncWAL() is not supported for this implementatio
Differential Revision: D98425822 Original commit changeset: ec17a15c837c Original Phabricator Diff: D98425822 fbshipit-source-id: 52c7c86d260bb7c239962c299841d62408ee5d88 |
||
|
generatedunixname3846135475516776
|
e96295a288 |
Rocksdb Warm Storage Test failed: not implemented: SyncWAL() is not supported for this implementatio (#14515)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14515 Reviewed By: xingbowang Differential Revision: D98425822 fbshipit-source-id: ec17a15c837c2b3e2fbe7ae6bc62aee3685e7bcc |
||
|
Josh Kang
|
f897c1b2ef |
Add uniform block tracking stats (#14513)
Summary: Add tracking of uniform index blocks as a table property (`num_uniform_blocks`). When `uniform_cv_threshold` is set, the block builder detects uniformly distributed keys via coefficient of variation of key gaps. This information is now surfaced end-to-end: from block building through index builders to SST table properties. ## Key changes - **BlockBuilder**: Persist the uniformity result from `ScanForUniformity()` as a member (`is_uniform_`) exposed via `IsUniform()`, rather than a local variable discarded after `Finish()`. - **Index builders**: All three index builder types (`ShortenedIndexBuilder`, `HashIndexBuilder`, `PartitionedIndexBuilder`) implement `NumUniformIndexBlocks()`. For partitioned indexes, the count accumulates across partition `Finish()` calls. - **Table property serialization**: Added `TablePropertiesNames::kNumUniformBlocks` (`"rocksdb.num.uniform.blocks"`) with full serialization/deserialization support. Without this, the property was computed in memory but never persisted to SST files. - **Table property aggregation**: `num_uniform_blocks` included in `Add()` and `GetAggregatablePropertiesAsMap()` for correct cross-SST aggregation. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14513 Test Plan: - **Unit tests**: `block_test` (7504 passed), `table_test` (6917 passed), `table_properties_collector_test` (8 passed). - **End-to-end verification via db_bench + sst_dump**: - Positive: `./db_bench --benchmarks=fillseq,compact --num=10000 --uniform_cv_threshold=0.2 --index_shortening_mode=0 --compression_type=none` produces SST with `# uniform blocks: 1`. - Negative: Same with `--uniform_cv_threshold=-1` (disabled) produces `# uniform blocks: 0`. Reviewed By: xingbowang Differential Revision: D98343170 Pulled By: joshkang97 fbshipit-source-id: ed08e83b2dcfb70f074bfb0f7bb5c31d75dc6da9 |
||
|
Xingbo Wang
|
dae450b78e |
Improve Claude review reliability (#14526)
Summary: ## Problems **(1) Claude review times out on large PRs** The `claude-code-base-action` defaults to `timeout_minutes=10`. Large PRs (e.g. https://github.com/facebook/rocksdb/issues/14499) get killed with exit code 124 after 600 seconds mid-review. **(2) Exported PRs never get reviewed** PRs exported from Meta's internal pipeline (e.g. https://github.com/facebook/rocksdb/issues/14515) trigger CI within milliseconds of PR creation. The `workflow_run` payload has `pull_requests=[]`, and the SHA-based fallback also misses because GitHub hasn't registered the PR yet — so Claude review never fires. ## Fixes - Set `timeout_minutes: "60"` on both auto-review and manual-review `Run Claude` steps - Retry the SHA→PR lookup up to 5 times with a 10s delay, giving GitHub up to ~50s to register the PR. Also bumped `per_page` from 30 to 100. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14526 Reviewed By: pdillinger Differential Revision: D98636217 Pulled By: xingbowang fbshipit-source-id: bba19095ab2ddf468c5c19cf5c77d19536f498b0 |
||
|
Andrew Chang
|
2af38206ad |
Log errno and thread ID on CreateIOUring failure (#14520)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14520 CreateIOUring() silently returns nullptr on failure, discarding the errno from io_uring_queue_init. This makes it impossible to diagnose why io_uring initialization fails on specific threads (e.g. ENOMEM from memlock limits, EINVAL from unsupported flags, EMFILE from fd exhaustion). Add a fprintf(stderr, ...) that logs strerror, errno, and pthread thread ID when io_uring_queue_init fails, so failures are diagnosable from logs without needing to reproduce. Reviewed By: xingbowang Differential Revision: D98526792 fbshipit-source-id: 1eb5042c8b62663c4d24c09f29ef7c55b90032f0 |
||
|
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
|
a965706e73 |
blob: skip empty values even with min_blob_size=0 (#14517)
Summary:
When `min_blob_size=0`, the existing guard condition:
```cpp
if (value.size() < min_blob_size_) {
return Status::OK(); // skip blob creation
}
```
is **false** for empty values (`0 < 0`), so empty values proceed to blob creation. This writes a 0-byte blob to disk and creates a `BlobContents` object with an empty `Slice`.
When that `BlobContents` is later evicted from the primary blob cache to `CompressedSecondaryCache`, the eviction handler calls `SaveTo(value, 0, 0, out)`, which hits:
```cpp
// typed_cache.h:230
assert(from_offset < slice.size()); // 0 < 0 → CRASH
```
This crash was found by the stress test with `--min_blob_size=0` and `--blob_cache` + secondary cache enabled (T261142690).
## Fix
Add an explicit `value.empty()` check before the blob path:
```cpp
if (value.empty() || value.size() < min_blob_size_) {
return Status::OK();
}
```
Empty values are now always stored inline in the SST, regardless of `min_blob_size`. This is also correct on principle: a `BlobIndex` reference is larger than an empty value, so storing an empty value as a blob is pure overhead with no benefit.
## Root Cause Chain
1. User writes `Put("key", "")` with `min_blob_size=0`
2. `BlobFileBuilder::Add()` — `0 < 0` is false, empty value proceeds to blob creation
3. 0-byte blob written to blob file; `BlobContents` created with 0-size slice
4. `BlobContents` inserted into primary blob cache (LRU)
5. Cache eviction triggers `CacheWithSecondaryAdapter::EvictionHandler()`
6. `CompressedSecondaryCache::InsertInternal()` → `SaveTo(value, 0, 0, out)`
7. `assert(from_offset < slice.size())` → `assert(0 < 0)` → **** assertion failure
## Test
Added `DBBlobBasicTest.EmptyValueNotStoredAsBlob` which:
- Writes an empty value and a non-empty value with `min_blob_size=0`
- Verifies both are readable
- Confirms the empty value is stored **inline** (readable from `kBlockCacheTier` without blob I/O)
- Confirms the non-empty value is stored as a **blob** (returns `IsIncomplete()` from `kBlockCacheTier`)
## Related
- Task: T261142690
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14517
Reviewed By: hx235
Differential Revision: D98499174
Pulled By: xingbowang
fbshipit-source-id: 5713923daec83db6491d00ce58acdf8231fabeba
|
||
|
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 |
||
|
Andrew Chang
|
47de3a3a2c |
Fix io_uring kernel resource leak in DeleteIOUring() (#14518)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14518 When RocksDB creates an io_uring instance via `CreateIOUring()`, it calls `io_uring_queue_init()` under the hood. This function asks the Linux kernel to set up a submission queue and completion queue for async I/O — the kernel allocates file descriptors and memory-mapped ring buffers to make this work. To properly release those kernel resources, you must call `io_uring_queue_exit()` before freeing the `io_uring` struct. This function tells the kernel "I'm done with this io_uring" — it unmaps the shared memory regions and closes the kernel file descriptors. Without it, those resources leak every time an io_uring instance is destroyed. `DeleteIOUring()` (the ThreadLocalPtr destructor callback) was only doing `delete iu` — freeing the C++ struct's heap memory but never telling the kernel to clean up. This meant every thread exit leaked kernel resources. The same bug also existed in the `PosixFileSystem` constructor, which creates a temporary test io_uring to check kernel support and then did `delete new_io_uring` without cleanup. Fix: 1. Add `io_uring_queue_exit(iu)` before `delete iu` in `DeleteIOUring()`. 2. Replace bare `delete new_io_uring` in fs_posix.cc with `DeleteIOUring(new_io_uring)` to reuse the now-correct helper. Note: the error-recovery path in io_posix.cc (~line 907) already correctly calls `io_uring_queue_exit()` before `delete`, confirming this was the intended pattern that was missed in these two spots. Reviewed By: anand1976 Differential Revision: D98500559 fbshipit-source-id: bd6c10c19d9fd67cf537dd7f100ef9dc49bfe77e |
||
|
Xingbo Wang
|
926e2b589f |
Fix nightly CI regressions, flaky tests, and Windows ccache from #14478 (#14508)
Summary: Fix build failures, flaky tests, and Windows ccache issues exposed by PR https://github.com/facebook/rocksdb/issues/14478. ### 1. Release build (NDEBUG) compile error **Job**: `build-linux-release-with-folly` The SyncPoint cleanup listener in testharness.cc referenced `SyncPoint::GetInstance()` unconditionally, but SyncPoint is only declared in debug builds. Wrapped with `#ifndef NDEBUG`. ### 2. Folly-lite link error **Job**: `build-linux-cmake-with-folly-lite` The `USE_FOLLY_LITE` cmake path unconditionally linked `-lglog`, which fails with lld (added in https://github.com/facebook/rocksdb/issues/14478) when glog isn't installed. Use `find_library()` to link only when available. ### 3. Flaky tests — leaked `Env::Default()` thread pool state Sharded execution runs multiple tests per process. Several tests in `db_compaction_test.cc` modified the global `Env::Default()` BOTTOM thread pool without resetting. With a leaked BOTTOM pool, subsequent tests' last-level compactions get forwarded to the bottom pool, freeing the LOW thread to pick additional compactions unexpectedly. Fix: add `TearDown()` override to `DBCompactionTest` that captures default thread pool sizes in the constructor and restores them after every test. This is more robust than per-test cleanup because: - It runs even when a test fails (gtest calls TearDown after assertion failures) - It catches all current and future leakers without per-test maintenance ### 4. Windows ccache — 0.26% hit rate → should be ~99% The Windows nightly build takes 57 minutes because ccache has near-zero hit rate. Two issues: - **Cache key**: The `hendrikmuhs/ccache-action` used a timestamp-based key, so each nightly run created a unique key and never found the previous run's cache (`No cache found.`). Fixed by using a stable key `ccache-windows-<workflow>` with prefix-based restore. - **Compiler check**: Missing `compiler_check=content` setting, so MSVC path/version changes between runners invalidated all cache entries. Added `compiler_check=content` (same fix as macOS in https://github.com/facebook/rocksdb/issues/14478). Pull Request resolved: https://github.com/facebook/rocksdb/pull/14508 Test Plan: - Release and debug builds compile cleanly - 70 consecutive shuffled runs of db_compaction_test (367 tests each) pass — 50 on full cores + 20 on 4 cores - Format check passes - Windows ccache fix requires CI run to verify (first run populates cache, second run should see high hit rate) Reviewed By: anand1976 Differential Revision: D98380757 Pulled By: xingbowang fbshipit-source-id: d74079b75786ba3299e335b145a6c5fdc81fd5c1 |
||
|
Andrew Chang
|
453ebd6f37 |
Fix MultiScan not respecting SupportedOps for async IO (#14510)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14510 When a filesystem does not return kAsyncIO in SupportedOps(), MultiScan still sends ReadAsync/Poll/AbortIO requests. Regular iterators check via CheckFSFeatureSupport in ArenaWrappedDBIter::Init and ForwardIterator, but MultiScan blindly trusted the caller-provided MultiScanArgs::use_async_io flag. Fix: In the MultiScan constructor, after scan_opts_ is initialized, check CheckFSFeatureSupport and disable use_async_io if the FS doesn't support it. Also pass the (potentially modified) scan_opts_ to Prepare() instead of the original scan_opts parameter. Reviewed By: mszeszko-meta Differential Revision: D97995735 fbshipit-source-id: 331639d950fd3cb9f491feed996d5820294beb4e |
||
|
Andrew Chang
|
24dc8306f8 |
Add atomic_flush to CreateBackupOptions (#14511)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14511 Add `atomic_flush` option to `CreateBackupOptions` that plumbs through `CheckpointImpl::CreateCustomCheckpoint` to `LiveFilesStorageInfoOptions`. When `flush_before_backup=true` and `atomic_flush=true`, the backup engine atomically flushes all column families before creating the backup. This ensures cross-CF consistency without needing WAL files. Combined with `BackupEngineOptions::backup_log_files=false`, this allows safely skipping WAL backup for multi-CF databases, reducing backup size and complexity. Changes: - `backup_engine.h`: Add `bool atomic_flush = false` to `CreateBackupOptions` - `checkpoint_impl.h/.cc`: Add `bool atomic_flush` parameter to `CreateCustomCheckpoint`, plumb to `LiveFilesStorageInfoOptions::atomic_flush` - `backup_engine.cc`: Pass `options.atomic_flush` through to `CreateCustomCheckpoint` - `checkpoint_test.cc`: Add `BackupWithAtomicFlushSkipsWAL` test verifying backup with atomic flush + no WAL creates a valid restorable multi-CF backup Reviewed By: xingbowang Differential Revision: D98208696 fbshipit-source-id: e818ba8669ac52a206e30e9dd56f1d7573cc0175 |
||
|
Xingbo Wang
|
0921ec1200 |
Improve Claude Code review: comprehensive prompt, incremental findings, recovery flow (#14507)
Summary: Improves the Claude Code review CI workflow to produce deeper, more reliable reviews. **Motivation:** The previous 30-turn limit caused reviews to silently produce "no output" on complex PRs (e.g. https://github.com/facebook/rocksdb/issues/14477). The review prompt was also too shallow — single-pass with no codebase context phase. **Changes:** **1. Comprehensive multi-agent review prompt** (`claude_md/ci_review_prompt.md`) - 9 specialized review agents: design, correctness, cross-component, invariant-adversary, caller-audit, performance, API, serialization, test coverage - Deep codebase context phase before agents spawn: caller-chain analysis (3-5 levels up), callee side-effect tracing, cross-component data consumer analysis, execution context verification, assumption stress-testing - Inter-agent debate with round-robin critique assignments - Final report quality rules: disproven findings removed, no stream-of-consciousness **2. Incremental findings + recovery flow** - `Write` tool added so Claude saves findings to `review-findings.md` after each phase - If the review hits the turn limit, a recovery step launches a Sonnet session to format partial findings into the standard output - Recovery file existence check prevents crash if recovery step fails - `getLastAssistantText` fallback truncated to 50KB to avoid enormous PR comments **3. Prompts extracted to files** (`claude_md/ci_*.md`) - `ci_review_prompt.md` — full review methodology - `ci_query_prompt.md` — `/claude-query` system prompt - `ci_recovery_prompt.md` — recovery formatting prompt - Secure: checkout is from base branch (main), not PR head **4. `max_turns` increased from 30 to 300** - Orchestrator budget for multi-agent workflow; sub-agents get their own turns - Recovery flow ensures partial results if limit is still hit Pull Request resolved: https://github.com/facebook/rocksdb/pull/14507 Reviewed By: archang19 Differential Revision: D98170111 Pulled By: xingbowang fbshipit-source-id: 390626a53e7a7f91c2d3e91ed4403494532425ed |
||
|
Josh Kang
|
f620a6d039 |
Support Pinnable Reads In SstFileReader (#14500)
Summary: Add `SstFileReader::Get` (single-key) and `SstFileReader::MultiGet` (PinnableSlice) overloads to enable zero-copy point lookups directly from SST files. The existing `MultiGet(std::string*)` is refactored to delegate to the new `MultiGet(PinnableSlice*)`, which writes results directly into caller-provided `PinnableSlice` values instead of copying through an intermediate buffer. The single-key `Get` uses `TableReader::Get` with a `GetContext` for efficient single-key lookups without the overhead of MultiGet's sorting and batching machinery. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14500 Test Plan: - New unit tests Reviewed By: xingbowang Differential Revision: D97825648 Pulled By: joshkang97 fbshipit-source-id: 17f3edd59bbf4747d17309c44ef12f0d952ea4eb |
||
|
anand76
|
c78144e452 |
Update version to 11.2.0 and add 11.1.fb to format compat (#14509)
Summary: - Bump version.h from 11.1.0 to 11.2.0 - Add `11.1.fb` to `check_format_compatible.sh` ## Remaining manual steps - [x] Cherry-pick HISTORY.md update from release branch - [ ] Update folly hash in Makefile to latest - Follow up in another PR Part of 11.1 release workflow. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14509 Reviewed By: archang19 Differential Revision: D98173862 Pulled By: anand1976 fbshipit-source-id: 020dbb30126d054788ed979f42fabd1be8d97abb |
||
|
Xingbo Wang
|
056a0f9664 |
Add atomic_flush option for checkpoint (#14477)
Summary: Add `force_atomic_flush` to `FlushOptions` so any flush caller (Checkpoint, `DB::Flush()`, etc.) can force atomic flush of all column families even when `DBOptions::atomic_flush` is disabled. ## Changes ### Public API - Added `bool force_atomic_flush = false` to `FlushOptions` - Added `bool atomic_flush = false` to `LiveFilesStorageInfoOptions` ### Flush dispatch - `FlushAllColumnFamilies()` — uses `immutable_db_options_.atomic_flush || flush_options.force_atomic_flush` - `DB::Flush()` (single-CF and multi-CF) — respects `force_atomic_flush` - `FlushForGetLiveFiles()` — constructs `FlushOptions` with `force_atomic_flush` from `LiveFilesStorageInfoOptions::atomic_flush` ### Per-request atomic flush in pipeline - `FlushRequest.atomic_flush` and `BGFlushArg.atomic_flush_` carry the per-request flag - `GenerateFlushRequest`, `EnqueuePendingFlush`, `PopFirstFromFlushQueue`, `BackgroundFlush`, `FlushMemTablesToOutputFiles` all use the per-request flag ### Stress test - Added `--checkpoint_atomic_flush` flag to db_stress, randomly enabled in db_crashtest.py ### Unit tests (6 new) - End-to-end checkpoint with atomic flush override - Negative test (default = no atomic flush) - Single-CF edge case - Interaction with `DBOptions::atomic_flush=true` - Mixed non-atomic then atomic flushes in queue - Mixed atomic then non-atomic flushes in queue Pull Request resolved: https://github.com/facebook/rocksdb/pull/14477 Reviewed By: joshkang97 Differential Revision: D97639233 Pulled By: xingbowang fbshipit-source-id: b924c044d6179e68b38c6604eb46c9140516d42a |
||
|
Hui Xiao
|
f8ff77db6a |
Fix FIFO crash test false verification failures by disabling file dropping (#14503)
Summary: FIFO compaction drops old SST files when total size exceeds `max_table_files_size` or `max_data_files_size`. The stress test's expected state can't track these drops, causing false "GetEntity returns NotFound" failures. Confirmed by `rocksdb.fifo.max.size.compactions COUNT: 7` in an asan_crash_test whitebox run (`compaction_style=2`, `fifo_compaction_max_data_files_size_mb=100`). This diff adds a `fifo_compaction_max_table_files_size_mb` flag and sets it to 100GB in db_crashtest.py. Since `max_table_files_size` is always active (default 1GB), it must always be set very high. `fifo_compaction_max_data_files_size_mb` is randomized between 0 (disabled, defers to `max_table_files_size`) and 100GB (overrides `max_table_files_size`) to exercise both code paths while preventing drops in either case. FIFO intra-L0 compaction (`fifo_allow_compaction`) is still exercised. Long-term TODO: handle FIFO drops in expected state via `OnCompactionBegin` listener calling `SetPendingDel()` on affected key ranges, treating drops as concurrent deletes. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14503 Reviewed By: joshkang97 Differential Revision: D97601382 Pulled By: hx235 fbshipit-source-id: 1a3badfa4de47efd73f965807982a966ebe135ef |
||
|
Josh Kang
|
dcf3db95a6 |
Make concurrent Memtable stats more robust (#14506)
Summary: Fix thread-safety issues in memtable stats tracking and `MaybeUpdateNewestUDT` to prepare for PR https://github.com/facebook/rocksdb/issues/14448, which introduces concurrent range tombstone insertion from the read path into the mutable memtable. Currently the non-concurrent write path updates memtable counters (`num_entries_`, `num_deletes_`, `num_range_deletes_`, `data_size_`) using non-atomic load/store pairs. While this is safe today PR https://github.com/facebook/rocksdb/issues/14448 will make range_tombstone memtable concurrent, but the main write path can still be non-concurrent. This fix ensures stats are tracked correctly. Similarly, `MaybeUpdateNewestUDT` was not thread-safe and was not called in the concurrent write path at all. This PR also fixes the `post_process_info` delete counter which missed `kTypeSingleDeletion` and `kTypeDeletionWithTimestamp`. ## Key changes - Switch non-concurrent counter updates from `LoadRelaxed`/`StoreRelaxed` pairs to `FetchAddRelaxed` to avoid races with concurrent range tombstone inserts (e.g. `AddLogicallyRedundantRangeTombstone` calling `BatchPostProcess`). - Make `MaybeUpdateNewestUDT` thread-safe by replacing `Slice newest_udt_` with `RelaxedAtomic<const char*> newest_udt_data_` and using a CAS loop. The pointed-to memory lives in the arena and remains valid for the memtable's lifetime. - Call `MaybeUpdateNewestUDT` in the concurrent write path (was previously skipped with a TODO). - Fix `post_process_info->num_deletes++` to also count `kTypeSingleDeletion` and `kTypeDeletionWithTimestamp`, matching the non-concurrent path. - Change `GetNewestUDT()` return type from `const Slice&` to `Slice` (returning by value) to avoid dangling reference issues with the new atomic pointer storage. Updated across `MemTable`, `ReadOnlyMemTable`, `MemTableListVersion`, and `WBWIMemTable`. - Remove unused `Slice newest_udt_` member from `WBWIMemTable`. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14506 Test Plan: - Added `ConcurrentWriteMemTableProperties` test in `db_properties_test.cc`: 4 threads writing puts, deletes, and single deletes concurrently, then verifying `rocksdb.num-entries-active-mem-table` and `rocksdb.num-deletes-active-mem-table` properties are correct. Flush with `flush_verify_memtable_count=true` validates integrity. - Added `ConcurrentGetTableNewestUDT` test in `memtable_list_test.cc`: 4 threads concurrently inserting entries with UDTs via `allow_concurrent=true`, verifying the newest UDT is correctly tracked. - Extended `DuplicateSeq` and `ConcurrentMergeWrite` tests in `db_memtable_test.cc` to verify stat counters after `BatchPostProcess`. - Benchmark results show no performance regression (3 runs each, averaged): **Workload 1: fillrandom** (pure Put workload) ``` ./db_bench -benchmarks=fillrandom -seed=1 -compression_type=none -threads=8 -db=<DB> ``` **Workload 2: readrandomwriterandom** (mixed read/write/delete) ``` # Setup: fillrandom,compact to populate DB (single-threaded) ./db_bench -benchmarks=readrandomwriterandom -seed=1 -compression_type=none \ -use_existing_db=1 -readwritepercent=50 -deletepercent=20 -threads=8 -db=<DB> ``` **Workload 3: fillrandom with range tombstones** (puts interleaved with range deletes) ``` ./db_bench -benchmarks=fillrandom -seed=1 -compression_type=none \ -writes_per_range_tombstone=100 -range_tombstone_width=10 \ -max_num_range_tombstones=1000 -threads=8 -db=<DB> ``` ``` | Benchmark | avg ops/s (main) | avg ops/s (feature) | % change | |------------------------------------|-----------------|--------------------:|----------| | fillrandom | 531,986 | 532,099 | +0.02% | | readrandomwriterandom (50r/30w/20d)| 786,400 | 815,143 | +3.65% | | fillrandom (range tombstones) | 541,656 | 531,008 | -1.97% | ``` Reviewed By: xingbowang Differential Revision: D97974456 Pulled By: joshkang97 fbshipit-source-id: ea95f23953fe37771a599aed61ece1a504b12c2e |
||
|
Xingbo Wang
|
ec31d0074f |
Accelerate build and test infrastructure (#14478)
Summary:
Reduce `make check` time from ~9.4 minutes to ~3.7 minutes (2.6x) on a 192-core machine, speed up CI by 1.8x across all jobs, and fix ccache hit rates (macOS cmake: 0.51% → 99.80%).
## Changes
### 1. Auto-detect lld linker (`build_detect_platform`)
- Probe for `lld` at configure time; fall back to default `ld.bfd` if unavailable
- Linux-only guard (`TARGET_OS = Linux`)
- **Measured: 12x faster linking** (7.4s → 0.6s per test binary)
- Add `-L/usr/local/lib` for lld library resolution on CI
- Install lld in CI pre-steps action
- Opt out with `ROCKSDB_NO_FAST_LINKER=1`
### 2. Sharded test execution (`Makefile`)
**Before:** `make check` enumerated every individual gtest case via `--gtest_list_tests`, then created a separate shell script for each case with `--gtest_filter=TestName`. For example, `block_based_table_reader_test` has 9,788 parameterized test cases — each spawned its own process, loading a ~50MB binary, initializing gtest, registering all ~10K tests, running exactly ONE, then tearing down. The per-process overhead was ~1.5s, so 9,788 × 1.5s ≈ 15,000s wasted on overhead alone. Across all 302 test binaries this produced 39,467 individual processes.
**After:** Use gtest's built-in sharding (`GTEST_TOTAL_SHARDS`/`GTEST_SHARD_INDEX`) to group ~10 test cases per process. Each shard loads the binary once and runs multiple tests sequentially — eliminating the per-process overhead. The 9,788 cases in `block_based_table_reader_test` become ~980 shards. Total process count drops from 39,467 to ~4,036. Same tests, same coverage, just fewer process spawns.
The shard count adapts to machine size: `min(ceil(test_count / GTEST_SHARD_SIZE), NCORES * 8)`. This ensures large machines (192 cores) get many small shards for parallelism, while small CI runners (4 cores) get fewer larger shards to avoid excessive overhead. Both `GTEST_SHARD_SIZE` (default 10) and `NCORES` can be overridden.
CI sharding uses round-robin distribution across 3 shards to balance heavy tests (db_test, db_compaction_test, etc.) instead of contiguous alphabetical ranges.
Note: gtest continues running all tests after a failure (`GTEST_THROW_ON_FAILURE=0`), so grouping tests does not mask failures — all failures within a shard are reported.
**Measured: 3.6x CPU reduction** (76,121s → 21,033s)
### 3. Fix SyncPoint leaks for test isolation (5 files)
Sharded execution exposed 43 test files that set SyncPoint callbacks but never clean them up. Stale callbacks with captured local variables cause segfaults or data corruption when subsequent tests in the same process trigger them.
**Systematic fix:** Global gtest `TestEventListener` in `testharness.cc` that calls `SyncPoint::DisableProcessing()` + `ClearAllCallBacks()` + `ClearTrace()` + `LoadDependency({})` after every test case. This cleans up all SyncPoint state: callbacks, dependency maps, cleared points, and the point filter. Registered via static initialization — no changes needed to individual test `main()` functions.
**SyncPoint infrastructure fixes:**
- `DisableProcessing()` now calls `cv_.notify_all()` to wake threads blocked in `Process()`. Previously, threads waiting for predecessor sync points that would never fire (because the test ended) would hang forever.
- `Process()` now rechecks `enabled_` after waking from `cv_.wait()`, so threads exit promptly when processing is disabled instead of looping forever.
**Specific fixes (defense-in-depth):**
- `SstFileReaderTest::VerifyNumEntriesCorruption` — leaked `PropertyBlockBuilder::AddTableProperty:Start` callback that corrupted SST entry counts
- `WritePreparedTransactionTest::CommitAndSnapshotDuringCompaction` — leaked `CompactionIterator:AfterInit` callback causing segfault via dangling pointers
- `TransactionTestBase` destructor — cleanup for 26 SyncPoint uses across transaction tests
- `RetriableLogTest` destructor — cleanup for log reader SyncPoint callbacks
### 4. Fix ccache for reliable cross-run caching (`setup-ccache`, `CMakeLists.txt`)
- **`CCACHE_COMPILERCHECK=content`**: Default `mtime` compared compiler binary modification time, which differs on every fresh CI runner → 0% hit rate. `content` hashes compiler output instead — stable across runner instances
- **`CCACHE_SLOPPINESS`**: CMake generates varying `-MF` paths and timestamps. Without sloppiness settings, ccache treated these as different compilations
- **`CMAKE_C/CXX_COMPILER_LAUNCHER`** instead of `RULE_LAUNCH_COMPILE`: Avoids double-wrapping ccache when also injected via PATH. Removes `RULE_LAUNCH_LINK` since ccache cannot cache link operations
- **macOS cmake hit rate: 0.51% → 99.80%**
### 5. Align GTEST_THROW_ON_FAILURE (`Makefile`)
- Set `GTEST_THROW_ON_FAILURE=0` in Makefile default to match CI's `pre-steps/action.yml`
- `=1` caused `std::terminate` in multi-threaded stress tests (e.g., `point_lock_manager_stress_test`, `rate_limiter_test`) when assertions fail — the gtest exception propagates across thread boundaries, triggering undefined behavior and heap-use-after-free under ASAN
### 6. Portability fixes
- `[[maybe_unused]]` instead of `__attribute__((unused))` for MSVC compatibility
- Remove blanket `-fPIC` from `COMMON_FLAGS` — only apply via `PLATFORM_SHARED_CFLAGS` for shared builds, preserving optimal codegen for release/static builds
- `make -s list_all_tests` to suppress make noise in test enumeration
## Results
### Local (192-core devvm, clean build)
| Metric | Before | After | Improvement |
|--------|--------|-------|-------------|
| Build time (clean) | 2m43s | ~2m | lld linker |
| Test wall clock | 400s | 200s | 2.0x |
| Test CPU total | 76,121s | 21,033s | 3.6x |
| Test failures | 0 | 0 | — |
| **Total make check** | **~9.4min** | **3m41s** | **2.6x** |
### CI (GitHub Actions)
| Metric | Before | After | Improvement |
|--------|--------|-------|-------------|
| Aggregate CI time | 15,220s | 8,425s | 1.8x |
| build-linux-arm | 763s | 221s | 3.5x |
| build-linux-release | 616s | 244s | 2.5x |
| build-windows-vs2022 | 2,868s | 1,205s | 2.4x |
| build-linux-java-static | 858s | 335s | 2.6x |
### ccache hit rates (all jobs)
| Job | Before | After |
|-----|--------|-------|
| macOS cmake (0-3) | 0.51% | **99.80%** |
| build-linux | 99.68% | **99.84%** |
| build-linux-clang18-asan-ubsan | 98.58% | **99.84%** |
| build-linux-clang18-mini-tsan | 98.74% | **99.84%** |
| build-windows-vs2022 | 99.22% | **99.83%** |
| All 27 ccache jobs | — | **>94%** |
## Remaining bottleneck
The critical path is now dominated by genuinely slow integration tests:
- `external_sst_file_test` shards: 97–113s each
- `db_test` shards: 109–124s each
- `db_bloom_filter_test`: 103s (non-parallel)
Further improvement would require test optimization or a `make check-fast` target.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14478
Test Plan:
- `make check -j192 SKIP_FORMAT_BUCK_CHECKS=1` — all 4,045 shards pass (ASAN+UBSAN), zero failures
- Verified SyncPoint fixes: each crash/corruption reproduces before fix, passes after
- Verified SyncPoint::DisableProcessing wakes blocked threads (previously caused indefinite hangs)
- Verified gtest does NOT stop after failure with `GTEST_THROW_ON_FAILURE=0` — all tests in a shard run and all failures are reported
- CI: all 34 checks pass across two consecutive runs
- ccache: validated hit rates >94% on second run after cache seeding
Reviewed By: hx235
Differential Revision: D97623305
Pulled By: xingbowang
fbshipit-source-id: b299e6d43c8713a9ef2b5659e8bbd74958afe155
|
||
|
Josh Kang
|
304ae7190a |
Support ExternalTable PinnableSlice Get (#14497)
Summary: The ExternalTableReader `Get` API has been modified to use PinnableSlice instead of std::string, this will allow implementations to utilize zero-copy Gets (e.g. reading from mmap or a cache). This is not a compatible change, but the API is marked as experimental, so should be allowed. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14497 Test Plan: - New unit tests Reviewed By: xingbowang Differential Revision: D97782011 Pulled By: joshkang97 fbshipit-source-id: 8a9e8c5bc5ff5e8dee6c0f2ee745521f09042cef |
||
|
Hui Xiao
|
9de0ea32e1 |
Fix crash_test_with_ts failure: disable use_trie_index with timestamps (#14502)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14502 The asan_crash_test_with_ts randomly selected use_trie_index=1 alongside user_timestamp_size=8. TrieIndexFactory requires plain BytewiseComparator, but user-defined timestamps wrap it as BytewiseComparator.u64ts, causing Status::NotSupported errors during flush/compaction. Fix: 1. Add use_trie_index=0 to ts_params in db_crashtest.py to prevent the incompatible combination (matches existing pattern for other features). 2. Add early validation in db_stress_tool.cc to reject this combination with a clear error message (defense in depth). Reviewed By: xingbowang Differential Revision: D97592648 fbshipit-source-id: 2259f82d95ea8410c4278b52f60983a4b5e84ec2 |
||
|
Xingbo Wang
|
77a0e347bb |
Add Claude code review CI workflow (#14480)
Summary: Add GitHub Actions workflows for AI-powered code review on PRs using Claude (Anthropic). Follows the clang-tidy security pattern with two separate workflows for privilege separation. ## Trigger Modes **1. Auto** — Runs after `pr-jobs` workflow completes successfully via `workflow_run`. Safe for fork PRs (runs from default branch, never executes PR code). **2. Manual** — Maintainers comment `/claude-review [focus area]` or `/claude-query <question>` on any PR. Restricted to 15 authorized team members. **3. workflow_dispatch** — For manual testing. ## Security Model (Two-Workflow Separation) Same pattern as clang-tidy: **`claude-review.yml`** (analysis): - Runs Claude with `ANTHROPIC_API_KEY` - Has ONLY `contents: read` — no PR write, no issue write - Saves review markdown + metadata as artifact **`claude-review-comment.yml`** (posting): - Triggers on `workflow_run` completion - Downloads artifact and posts/updates PR comment - Has `pull-requests: write` but never runs AI This separation prevents a crafted PR from tricking Claude into exfiltrating write tokens. ## Review Methodology Review prompt in `claude_md/code_review.md` (shared with local Claude Code reviews). Five perspectives: - Call-chain analysis (3-5 levels up/down) - Correctness & edge cases - Cross-component & adversarial (10 execution contexts) - Performance - API compatibility & test coverage ## Shared Scripts - `.github/scripts/post-pr-comment.js` — Create-or-update PR comment with marker-based dedup. Now used by both clang-tidy and Claude review. - `.github/scripts/parse-claude-review.js` — Parses `claude-code-base-action` execution log into markdown. ## Files Changed | File | Description | |------|-------------| | `.github/workflows/claude-review.yml` | Analysis workflow (476 lines) | | `.github/workflows/claude-review-comment.yml` | Comment posting workflow (146 lines) | | `.github/scripts/post-pr-comment.js` | Shared PR comment utility (57 lines) | | `.github/scripts/parse-claude-review.js` | Execution log parser (78 lines) | | `.github/workflows/clang-tidy-comment.yml` | Updated to use shared script | | `claude_md/code_review.md` | Review methodology (104 lines) | ## Setup Required Add `ANTHROPIC_API_KEY` secret to the repo settings. ## Testing Tested end-to-end on `xingbowang/rocksdb` fork — both auto and manual triggers, artifact upload/download, comment posting, and duplicate detection all verified working. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14480 Reviewed By: omkarhgawde Differential Revision: D97832666 Pulled By: xingbowang fbshipit-source-id: f80c7d8683ac980614dc4ca66c1e545deb3be504 |
||
|
Andrew Chang
|
89e384ca6f |
Support GetLiveFiles on secondary DB instances (#14475)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14475 GetLiveFiles was previously blocked on secondary instances with Status::NotSupported, even though the operation is safe to perform. The only reason GetLiveFiles was originally blocked is that it defaults to flushing the memtable (flush_memtable=true), which is a write operation. However, the actual implementation in DBImpl::GetLiveFiles (db_filesnapshot.cc) does two things: 1. Optionally flush the memtable — which we skip by passing flush_memtable=false. The secondary already overrides FlushForGetLiveFiles() as a no-op, so even if true were passed it would not actually flush. 2. Read live file state from versions_ under the mutex — this is purely read-only. It iterates the ColumnFamilySet to collect live table and blob file numbers, builds relative file paths for SST files, blob files, CURRENT, MANIFEST, and OPTIONS, and reads the manifest file size. The secondary maintains its own VersionSet which it keeps up to date via MANIFEST replay in TryCatchUpWithPrimary(). So all of this state is valid and accurate — it reflects exactly which files the secondary considers live at its current replay point. This is the same approach used by DBImplReadOnly and CompactedDBImpl, which both delegate to DBImpl::GetLiveFiles with flush_memtable=false. Reviewed By: xingbowang Differential Revision: D97563143 fbshipit-source-id: 8d5b52e26a478ef190eba598819de6527817bcfc |
||
|
Josh Kang
|
3d99378791 |
Fix table handle leak (#14469)
Summary: Fix leaked table cache entries that cause `TEST_VerifyNoObsoleteFilesCached` assertion failure during `DB::Close()` in ASAN crash test builds (T258745630): ``` File 126519 is not live nor quarantined Assertion `cached_file_is_live_or_quar' failed. ``` When a compaction fails *after* `VerifyOutputFiles` succeeds (e.g., at `VerifyCompactionRecordCounts`), the overall `compact_->status` is set to error but each subcompaction's individual `status` remains OK. `SubcompactionState::Cleanup` only checked the individual subcompaction status, so it skipped calling `ReleaseObsolete` on the output files' table cache entries — leaking them. Normally, `Close()`'s backstop (`FindObsoleteFiles(force=true)` + `PurgeObsoleteFiles`) would catch this by finding the orphan file on disk, evicting the cache entry, and deleting the file. However, `FindObsoleteFiles` calls `FaultInjectionTestFS::GetChildren` which can fail under metadata read fault injection (`--open_metadata_read_fault_one_in=8`). The error is silently ignored (`s.PermitUncheckedError()` in db_impl_files.cc:199), so the orphan file is never found and the leaked cache entry is never evicted. The `Cleanup` bug is latent and predates recent changes. PR https://github.com/facebook/rocksdb/issues/14433 added `verify_output_flags` randomization to the crash test, and PR https://github.com/facebook/rocksdb/issues/14456 fixed false-positive corruptions that https://github.com/facebook/rocksdb/issues/14433 caused. Before https://github.com/facebook/rocksdb/issues/14456, `VerifyOutputFiles` would produce false corruption errors that *accidentally prevented the leak* by setting the subcompaction status to non-OK. ### How it triggers **Step 1 — VerifyOutputFiles adds cache entries for compaction output files:** ``` CompactionJob::Run() RunSubcompactions() // all subcompactions succeed // output file 12 written to disk // each sub_compact.status = OK SyncOutputDirectories() // status = OK VerifyOutputFiles() for each output_file: table_cache()->NewIterator(output_file.meta) FindTable() cache->Insert(file_number=12) // <<< ENTRY ADDED TO TABLE CACHE return iterator holding handle delete iter // releases handle, entry stays in LRU // status = OK ``` **Step 2 — A post-verification step fails, overall status set but NOT subcompaction status:** ``` CompactionJob::Run() VerifyCompactionRecordCounts() // returns Status::Corruption(...) FinalizeCompactionRun(status=Corruption) compact_->status = Corruption // <<< OVERALL status = error // each sub_compact.status is still OK! ``` **Step 3 — Install skips InstallCompactionResults (file 12 never enters a Version):** ``` CompactionJob::Install() status = compact_->status // Corruption if (status.ok()) // FALSE InstallCompactionResults() // <<< SKIPPED — file 12 not in any version ``` **Step 4 — CleanupCompaction skips ReleaseObsolete (THE BUG):** ``` CompactionJob::CleanupCompaction() for each sub_compact: sub_compact.Cleanup(table_cache) if (!status.ok()) // checks sub_compact.status = OK // <<< FALSE — individual status is OK! ReleaseObsolete(...) // <<< NEVER CALLED — cache entry leaked! ``` **Step 5 — Metadata read fault prevents backstop from finding the orphan:** `FindObsoleteFiles(force=true)` calls `GetChildren()` to scan the DB directory. `FaultInjectionTestFS::GetChildren` injects a metadata read error (`--open_metadata_read_fault_one_in=8`). The error is silently ignored (`s.PermitUncheckedError()`), so the directory listing is empty and the orphan file is never found. This happens both in the post-compaction cleanup (`BackgroundCallCompaction`) and in `Close()`'s backstop: ``` FindObsoleteFiles(force=true) fs->GetChildren(path, ...) // returns IOError (fault injected) s.PermitUncheckedError(); // error silently ignored // files vector is empty — orphan file 12 not found PurgeObsoleteFiles() // nothing to do — no Evict called ``` **Step 6 — Assertion fires during Close():** ``` CloseHelper() FindObsoleteFiles(force=true) // GetChildren fails again → orphan missed PurgeObsoleteFiles() // nothing to evict TEST_VerifyNoObsoleteFilesCached() for each cache entry: file_number = 12 live_and_quar_files.find(12) == end() // NOT in any version! >>> assert(cached_file_is_live_or_quar) FAILS <<< ``` **Crash test call stack:** ``` frame https://github.com/facebook/rocksdb/issues/9: __assert_fail_base("cached_file_is_live_or_quar", "db_impl_debug.cc", 389) frame https://github.com/facebook/rocksdb/issues/11: DBImpl::TEST_VerifyNoObsoleteFilesCached()::lambda // finds leaked entry frame https://github.com/facebook/rocksdb/issues/18: LRUCacheShard::ApplyToSomeEntries(...) // iterating cache shard frame https://github.com/facebook/rocksdb/issues/19: ShardedCache::ApplyToAllEntries(...) // iterating all shards frame https://github.com/facebook/rocksdb/issues/20: DBImpl::TEST_VerifyNoObsoleteFilesCached() // the verification frame https://github.com/facebook/rocksdb/issues/21: DBImpl::CloseHelper() // during Close frame https://github.com/facebook/rocksdb/issues/22: DBImpl::CloseImpl() frame https://github.com/facebook/rocksdb/issues/23: DBImpl::Close() frame https://github.com/facebook/rocksdb/issues/24: StressTest::Reopen() // crash test reopen frame https://github.com/facebook/rocksdb/issues/25: StressTest::OperateDb() // worker thread ``` ### Fix Pass the overall `compact_->status` to `SubcompactionState::Cleanup` and call `ReleaseObsolete` when *either* the subcompaction status or the overall status is non-OK: ```cpp // Before: if (!status.ok()) { ReleaseObsolete(...); } // After: if (!status.ok() || !overall_status.ok()) { ReleaseObsolete(...); } ``` ## Key changes - **`SubcompactionState::Cleanup`**: Now takes an `overall_status` parameter and calls `ReleaseObsolete` when *either* the subcompaction status or the overall compaction status is non-OK. - **`CompactionJob::CleanupCompaction`**: Passes `compact_->status` (the overall status) to each subcompaction's `Cleanup`. - **Sync point**: Added `CompactionJob::Run():AfterVerifyOutputFiles` for error injection in tests. - **Unit test**: `DBCompactionTest.LeakedTableCacheEntryOnCompactionFailure` uses `FaultInjectionTestFS` to reproduce the crash test scenario — injects error after `VerifyOutputFiles` and deactivates the filesystem so `GetChildren` fails in `FindObsoleteFiles`, preventing the backstop from evicting the leaked cache entry. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14469 Test Plan: - `DBCompactionTest.LeakedTableCacheEntryOnCompactionFailure`: - **Without fix (ASAN)**: assertion fires during `Close()` — `File 12 is not live nor quarantined` - **With fix (ASAN)**: passes — `ReleaseObsolete` properly cleans up the cache entry - **With fix (non-ASAN)**: passes ``` $ COMPILE_WITH_ASAN=1 make -j db_compaction_test $ ./db_compaction_test --gtest_filter="DBCompactionTest.LeakedTableCacheEntryOnCompactionFailure" [ PASSED ] 1 test. ``` Reviewed By: xingbowang Differential Revision: D97190944 Pulled By: joshkang97 fbshipit-source-id: fdfd481cc1e192803cfb7d64052ccb9162c21b94 |