mirror of
https://github.com/facebook/rocksdb.git
synced 2026-07-07 14:47:40 +08:00
main
13970 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Maciej Szeszko
|
8d33fe7c70 |
fix use-after-free: drain background purge in CloseHelper (#14842)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14842 ### Problem A process using RocksDB can hit an ASAN `heap-use-after-free` in `rocksdb::InstrumentedMutex::Lock()` when a `DBImpl` is closed while obsolete-file purge work is still in flight. This can happen during close storms with `avoid_unnecessary_blocking_io=true` on a shared Env, where a queued or late handoff purge can run after `~DBImpl` has destroyed `mutex_`. ### Root Cause `CloseHelper()` has several mutex-unlocked windows late in shutdown. A dropped column-family handle or SuperVersion cleanup can run in one of those windows and start obsolete-file purge work. `FindObsoleteFiles()` marks that work by incrementing `pending_purge_obsolete_files_`, but `PurgeObsoleteFiles(..., true)` can then spend time outside the DB mutex before transferring the work to `bg_purge_scheduled_` via `SchedulePurge()`. During that handoff, `bg_purge_scheduled_` is still zero even though purge work is pending. ### Fix Make the final `CloseHelper()` drain wait for both `pending_purge_obsolete_files_` and `bg_purge_scheduled_` before destroying `versions_` / `DBImpl`. This covers both the pending handoff and the scheduled `BGWorkPurge` state. Also move `TEST_VerifyNoObsoleteFilesCached()` and `table_cache_->EraseUnRefEntries()` after the final drain, so debug/ASAN table-cache verification runs only after close-time purge work has settled. The regression test parks a dropped-CF cleanup in the pending handoff window and verifies `CloseHelper()` blocks in the final drain until that handoff is released. Reviewed By: xingbowang Differential Revision: D107920881 fbshipit-source-id: f73cd79afe50fc30e0f5d14889dd4285bf350a58 |
||
|
Peter Dillinger
|
01084e7e8e |
Disable parallel compression for fast built-in compressors (#14841)
Summary: Use Compressor::GetRecommendedParallelThreads() overrides to disable parallel compression for Snappy, LZ4 (accelerated, not LZ4HC), and for accelerated levels of ZSTD (level < 0). These do not generally benefit from parallel compression. Also add --verify_compression option to sst_dump for some basic "recompress" benchmarking with that option. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14841 Test Plan: unit test updated. In planning the scope of this change, I manually tested some production SST files with release build sst_dump --command=recompress and various settings for --compression_parallel_threads, --compression_types, and --compression_level, while I had the CPUs mostly busy using cache_bench in the background. Here's an example, before the change to override parallel_threads: ``` $ for PT in 1 8; do /usr/bin/time ./sst_dump --command=recompress --compression_parallel_threads=$PT --block_size=16384 --compression_types=kLZ4Compression --verify_compression=1 test.sst; done ... Compression: kLZ4Compression Block Size: 16384 Threads: 1 Cx level: 32767 Cx size: 168501634 Uncx size: 791721664 Ratio: 4.698599 Write usec: 2345894 Read usec: 429022 Cx count: 48661 (100.0%) Not cx for ratio: 0 ( 0.0%) Not cx otherwise: 0 ( 0.0%) 2.54user 0.29system 0:02.84elapsed 99%CPU (0avgtext+0avgdata 460816maxresident)k ... Compression: kLZ4Compression Block Size: 16384 Threads: 8 Cx level: 32767 Cx size: 168501634 Uncx size: 791721664 Ratio: 4.698599 Write usec: 2476459 Read usec: 439464 Cx count: 48661 (100.0%) Not cx for ratio: 0 ( 0.0%) Not cx otherwise: 0 ( 0.0%) 3.95user 0.33system 0:02.98elapsed 143%CPU (0avgtext+0avgdata 455504maxresident)k ``` Here as in many of the cases I'm changing, it actually takes longer to compress with parallel, despite the added parallel opportunity of verify_compression. And overall CPU is much higher from 2.54 `CPU*s` to 3.95 `CPU*s`. The difference disappears with the change, because both use single-threaded SST construction. Reviewed By: joshkang97 Differential Revision: D108075938 Pulled By: pdillinger fbshipit-source-id: 5d1fc77ddbccf9f3b24a4f0b20b2b3c43074e89d |
||
|
zaidoon
|
7affaee1c4 |
Add use_direct_io_for_compaction_reads option (#14743)
Summary: Adds a new `DBOption use_direct_io_for_compaction_reads` (default false). When on, compaction-input SST files are opened with `O_DIRECT` so the sequential read-once data from compaction doesn't pollute the OS page cache and evict the hot user-read working set. User reads keep going through the buffered fast path. This protects user-read tail latency on write-heavy workloads without forcing user reads onto the existing global `use_direct_reads` knob (which pays in throughput and P50 — see the bench below). The interesting bit is that just flipping the FileOptions returned by `FileSystem::OptimizeForCompactionTableRead` doesn't actually trigger `O_DIRECT` at the kernel level. The TableCache (and `FileMetaData::pinned_reader`) is already holding buffered handles opened at flush time or at `DB::Open` via `LoadTableHandlers`. When compaction asks for an iterator, it gets back the cached buffered handle and the kernel never sees the `O_DIRECT` flag. So this PR also adds a small bypass path: - `TableCache::FindTable` / `NewIterator` learn a `open_ephemeral_table_reader` mode. When set, the pinned-reader fast path and the shared cache are skipped, `GetTableReader` is called directly with the caller's FileOptions, and ownership of the freshly opened TableReader is handed back via a `unique_ptr`. The iterator takes ownership via `RegisterCleanup` and frees the reader on destruction. - `VersionSet::MakeInputIterator` and `LevelIterator` plumb the flag through both L0 and L1+ compaction-input paths. - `CompactionJob::ProcessKeyValueCompaction` turns the bypass on when `use_direct_io_for_compaction_reads` is set, the global `use_direct_reads` is off, and `OptimizeForCompactionTableRead` produced `use_direct_reads=true` in the compaction-read FileOptions. The option is opt-in: when off, nothing changes for existing users. When on, only the compaction-input opens take the bypass path; user reads keep hitting the TableCache and the buffered fast path normally. There's also a small db_bench helper in the same PR: a new `--bgwriter_num` flag that lets the writer thread in `readwhilewriting` (and the other "while writing" variants) spread its puts across `[0, bgwriter_num)` instead of `[0, num)`. Without this the readers and writer share a key range and you can't have both a hot read subset and meaningful compaction work — this lets you have both. ### Benchmark Setup: Ubuntu 24.04 (kernel 7.0.5, OrbStack Linux VM on Apple Silicon), 14 vCPUs, virtio-blk disk, btrfs. MGLRU disabled (`echo 0 > /sys/kernel/mm/lru_gen/enabled`) so the kernel uses the classic active/inactive LRU. 14 GB DB (3.5M keys × 4 KB values), no compression. Each measurement run is pinned to a 1 GB cgroup via `systemd-run --scope -p MemoryMax=1G -p MemorySwapMax=0`. Page cache is dropped between configs. db_bench is Release build. Workload: `readwhilewriting` for 120s. 4 reader threads doing random reads over a hot key subset, plus 1 writer thread spreading overwrites across the full 3.5M-key keyspace (via `--bgwriter_num=3500000`) throttled at 200 MB/s, so there's continuous compaction running while the readers go. The size of the hot reader subset relative to available page cache controls how visible the optimization is. The Cassandra blog ([Lightfoot 2026](https://lightfoot.dev/direct-i-o-for-cassandra-compaction-cutting-p99-read-latency-by-5x/)) documented the same thing: biggest wins when the hot set is big enough to actually compete for cache, smaller wins when the hot set trivially fits, neutral when the hot set is way bigger than cache. So I ran two hot-set sizes. #### Small hot set: ~30 MB (~3% of the 1 GB cgroup) — N=5 iterations, mean (CV) `--num=7500`. The hot set is small enough that the page cache holds it without much trouble even under compaction, so the wins here are real but on the modest side. | Config | Throughput (ops/s) | Read P50 (µs) | Read P99 (µs) | Read P99.9 (µs) | Read P99.99 (µs) | |---|---|---|---|---|---| | buffered (default) | 233,477 (8.2%) | 16.09 | 82.24 | 721.0 | 2,102.5 | | direct_compaction_writes_only (existing knob alone) | 287,405 (2.8%) — **+23.1%** | 13.00 (−19.2%) | **66.77 (−18.8%)** | 553.9 (−23.2%) | 1,787.6 (−15.0%) | | direct_compaction_read_only (new knob alone) | 250,669 (2.4%) — +7.4% | 14.16 (−12.0%) | 102.99 (+25.2%) | 689.8 (−4.3%) | 1,801.3 (−14.3%) | | direct_compaction_read_write (new + existing, recommended) | 277,920 (3.3%) — **+19.0%** | **12.99 (−19.3%)** | 84.23 (+2.4%) | 613.4 (−14.9%) | **1,738.2 (−17.3%)** | | use_direct_reads=true (existing global) + write-side | 249,014 (2.5%) — +6.7% | 15.95 (−0.9%) | 68.78 (−16.4%) | **450.8 (−37.5%)** | 1,814.5 (−13.7%) | CV is 2.4–3.3% on the optimized configs (8.2% on buffered), so the deltas are real. With a hot set this small, the existing `use_direct_io_for_flush_and_compaction` knob is already doing most of the work — the new flag's main extra contribution here is P99.99 (combined wins it by ~2 points vs writes-only-alone). Worth noting: the new flag *alone* (without the existing write-side flag) improves P99.99 but regresses P99 by 25% on this small-hot-set workload, because direct compaction reads lose kernel readahead and compaction-output writes are still hitting the page cache. That regression goes away once you combine with the existing write-side flag, or once the hot set is bigger (see next table). So if you're using just one knob, use the existing one. If you're using this PR's flag, pair it with `use_direct_io_for_flush_and_compaction=true`. #### Larger hot set: ~400 MB (~40% of cache) — N=5 iterations, mean (CV) `--num=100000`. This is the case the Cassandra blog calls out — hot set big enough to actually fight compaction for cache. Their analogous setup (1M hot partitions, ~33% hot/cache) reported 1.93× p99 improvement. Numbers here are the headline: | Config | Throughput (ops/s) | Read P50 (µs) | Read P99 (µs) | Read P99.9 (µs) | Read P99.99 (µs) | |---|---|---|---|---|---| | buffered (default) | 68,959 (7.7%) | 44.81 | 541.22 | 2,225.2 | 11,334.5 | | direct_compaction_writes_only (existing knob alone) | 73,973 (10.3%) — +7.3% | 42.22 (−5.8%) | 456.27 (−15.7%) | 2,016.9 (−9.4%) | 9,190.0 (−18.9%) | | direct_compaction_read_only (new knob alone) | 84,337 (2.3%) — +22.3% | 38.66 (−13.7%) | 386.97 (−28.5%) | 1,644.8 (−26.1%) | 4,837.9 (−57.3%, 2.34×) | | direct_compaction_read_write (new + existing, recommended) | **104,923 (8.4%) — +52.2%** | **34.26 (−23.5%)** | **290.97 (−46.2%)** | **1,143.4 (−48.6%)** | **3,080.3 (−72.8%, 3.68×)** | | use_direct_reads=true (existing global) + write-side | 71,598 (9.1%) — +3.8% | 51.33 (+14.5%) | 297.91 (−45.0%) | 1,663.6 (−25.2%) | 6,530.0 (−42.4%) | Combined config gets a 3.68× p99.99 win, 1.86× p99, p50 down 23%, throughput up 52%. Same shape as the Cassandra blog's 1.93× p99 result — the improvement just lands at deeper percentiles for us because RocksDB's baseline data path is roughly 40× faster than Cassandra's (their buffered p99 was 35 ms, ours is 0.54 ms), so the cache-miss tail is further out. A few things worth calling out from this table: - The new flag is doing real work on top of the existing write-side flag here, not just shifting things around. Combined throughput is +42% over `direct_compaction_writes_only` alone, and combined p99.99 is 3× better. The existing knob alone gives a fairly modest +7% throughput / -19% p99.99 in this case — there's a clear gap that the new flag fills. - The new flag *alone* (no existing write-side flag) is also a real improvement here: +22% throughput, p99.99 down 57%. The P99 regression we saw in the small-hot-set case is gone, because the cache-protection effect now dominates the lost-readahead cost. - `use_direct_reads=true` (the existing global flag) actually regresses P50 by 14.5% in this workload — taking user reads off the page cache hurts you when the hot data could have been cached. It also gets the worst throughput of any direct config. It's not an equivalent way to get these gains. ### `compaction_readahead_size` matters when this flag is on Direct I/O bypasses kernel readahead, so RocksDB's own `DBOptions::compaction_readahead_size` becomes the only prefetch the iterator has. The default of 2 MB is enough and real users will get it automatically. **But `db_bench`'s `--compaction_readahead_size` CLI default is 0**, which defeats prefetch and makes direct compaction look slower than it actually is. If you're reproducing the numbers above, pass `--compaction_readahead_size=2097152` (or larger). - Recommended production config is `use_direct_io_for_compaction_reads=true` + `use_direct_io_for_flush_and_compaction=true`. Strongest configuration at every percentile and throughput in both benches. - The new flag is the read-side counterpart to `use_direct_io_for_flush_and_compaction`, which handles compaction-write cache pollution. They address different sources of pollution and compose. The gap between "combined" and "writes-only-alone" is 17 percentage points on p99.99 in the small-hot-set bench and 54 points in the larger one, so the new flag is contributing real value, especially as the hot set grows. - The new flag alone is also a real improvement when the hot set is big enough to compete with cache (+22% throughput, 2.34× p99.99 in the larger-hot-set bench). On a very small hot set it improves p99.99 but regresses p99, so pairing with the existing write-side flag is safer. - The benefit is workload-dependent. Small hot sets get modest tail-latency wins. Hot sets sized to actually compete for cache get the big multi-percentile wins shown above. Hot sets bigger than cache (not benched here but covered in the Cassandra blog) see no change either way — every read misses regardless. ### Reproducing Any Linux host (or a Linux VM on macOS via OrbStack / Multipass / lima): ```bash sudo apt-get install -y build-essential clang cmake git pkg-config \ libgflags-dev libsnappy-dev zlib1g-dev libbz2-dev liblz4-dev libzstd-dev cmake -DCMAKE_BUILD_TYPE=Release -DPORTABLE=1 -DWITH_GFLAGS=1 -DWITH_TESTS=0 .. make -j db_bench echo 0 | sudo tee /sys/kernel/mm/lru_gen/enabled ``` Build the source DB once, unrestricted memory: ```bash ./db_bench --benchmarks=fillrandom,compact,waitforcompaction,stats \ --db=/path/to/source_db --num=3500000 --key_size=16 --value_size=4096 \ --write_buffer_size=16777216 --target_file_size_base=16777216 \ --max_background_jobs=4 --compression_type=none --cache_size=4194304 \ --max_bytes_for_level_base=67108864 --disable_wal=1 --sync=0 ``` For each config, copy `source_db -> scratch_db`, run `sync && echo 3 > /proc/sys/vm/drop_caches`, then: ```bash sudo systemd-run --scope -p MemoryMax=1G -p MemorySwapMax=0 \ ./db_bench --use_existing_db=1 \ --benchmarks=readwhilewriting,stats --db=/path/to/scratch_db \ --threads=5 --duration=120 --statistics=true --histogram=1 \ --num=7500 --bgwriter_num=3500000 \ --key_size=16 --value_size=4096 \ --write_buffer_size=16777216 --target_file_size_base=16777216 \ --max_background_jobs=4 --compression_type=none \ --cache_size=4194304 --open_files=200 \ --skip_stats_update_on_db_open=true \ --max_bytes_for_level_base=67108864 \ --benchmark_write_rate_limit=209715200 \ --compaction_readahead_size=2097152 \ --rate_limiter_bytes_per_sec=0 \ --use_direct_reads={true|false} \ --use_direct_io_for_compaction_reads={true|false} \ --use_direct_io_for_flush_and_compaction={true|false} ``` For the larger hot-set table, change `--num=7500` to `--num=100000`. The five configs in the tables: - `buffered`: all three flags false. - `direct_compaction_writes_only`: `use_direct_io_for_flush_and_compaction=true`, the other two false. This is what users have today without this PR. - `direct_compaction_read_only`: `use_direct_io_for_compaction_reads=true`, the other two false. - `direct_compaction_read_write`: `use_direct_io_for_compaction_reads=true`, `use_direct_io_for_flush_and_compaction=true`, `use_direct_reads=false`. **Recommended.** - `direct_all`: `use_direct_reads=true`, `use_direct_io_for_flush_and_compaction=true`, `use_direct_io_for_compaction_reads=false`. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14743 Reviewed By: pdillinger Differential Revision: D108017601 Pulled By: xingbowang fbshipit-source-id: 4039d490d7e77b476db7a477a2f3d24738db6336 |
||
|
Anand Ananthabhotla
|
828f6d189e |
Mark remote filtered input counts as inaccurate (#14838)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14838 Remote compaction can pre-filter whole input files when universal compaction sees standalone range tombstones. In that case the worker's iterator count no longer describes the full original input set, so returning `has_accurate_num_input_records=true` can trigger a false-positive input-record verification on the primary. Mark the remote count inaccurate whenever the reconstructed compaction filtered input files before iteration, and add a regression test covering that path. Reviewed By: jaykorean Differential Revision: D107961493 fbshipit-source-id: 0c302081964ab1d894b34bddd2c55f5a4f06752f |
||
|
Josh Kang
|
1ede57e5b0 |
Add file ingestion histograms (#14836)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14836 Adds a public `Statistics` histogram, `INGEST_EXTERNAL_FILE_TIME` (`"rocksdb.ingest.external.file.micros"`), recording the end-to-end latency in microseconds of each `IngestExternalFile(s)` call. Ingestion timing was previously only available through per-thread `perf_context` counters, which require setting a `PerfLevel` and are not aggregated, so there was no process-wide latency distribution (p50/p99/max) for dashboards. It is recorded with an RAII `StopWatch` at the top of `DBImpl::IngestExternalFiles` -- one sample per call (not per column family), covering all return paths. It is null-safe and self-gating on the stats level, so there is no cost when statistics are off, and ingestion is not a hot path. Java bindings are kept in sync per the `statistics.h` requirement; the C API needs no change. Reviewed By: xingbowang Differential Revision: D107721260 fbshipit-source-id: 0705f9e0f7392329a7bcbdbd9f3afd34594d20bb |
||
|
Xingbo Wang
|
82085868e2 |
Add read-scoped block buffers for scan reads (#14806)
Summary: Add read-scoped block buffers for scan reads. Introduce an experimental read-scoped block buffer provider API, configured through ReadOptions::read_scoped_block_buffer_provider, so supported block-based table iterator scans and MultiScan data-block reads can use caller-provided read-scoped storage for final data-block contents. When configured, supported provider-backed scan data-block reads bypass the data-block cache while preserving normal index/filter block-cache behavior. Known-uncompressed reads can attach provider cleanup to provider-backed read buffers without copying. Compressed reads decompress directly into provider-backed output, while maybe-compressed reads that turn out to be uncompressed copy once into provider-backed final contents. mmap reads ignore the provider. Extend AlignedBuffer and RandomAccessFileReader direct-I/O paths to support external aligned allocations, then use that support for read-scoped iterator, async I/O, and MultiRead scratch buffers. Centralize read-scoped I/O policy, keep coalesced async reads safe when blocks are released before completion, and validate provider lease contracts. Add focused coverage for read-scoped ownership, compressed and uncompressed blocks, direct I/O, data-block cache bypass behavior, invalid provider leases, async release handling, and stress-test provider invariants. Add public API release notes for read-scoped block buffers. Bonus change: Fixed a flaky test in ReserveThread ## Testing - CI Pull Request resolved: https://github.com/facebook/rocksdb/pull/14806 Reviewed By: anand1976 Differential Revision: D106999951 Pulled By: xingbowang fbshipit-source-id: b1f23d4bab6318b6373ba2ca99a5c4d6a842dc5a |
||
|
Raj Suvariya
|
0493154a73 |
Expose BackupEngine C API additions: StopBackup and rate limiters (#14722)
Summary: Extends RocksDB's C API with two `BackupEngine` capabilities needed by language bindings (e.g. Rust via librocksdb-sys) that consume the C API: - **StopBackup**: Add `rocksdb_backup_engine_stop_backup()` to allow cancelling an in-progress backup. - **Rate limiters**: Add `rocksdb_backup_engine_options_set_backup_rate_limiter()` and `rocksdb_backup_engine_options_set_restore_rate_limiter()` to expose the `shared_ptr<RateLimiter>` fields on `BackupEngineOptions`. The existing `uint64_t` setters only throttle writes; these expose the richer `RateLimiter` object that supports read+write throttling (e.g. `kAllIo` mode). Pull Request resolved: https://github.com/facebook/rocksdb/pull/14722 Test Plan: - [x] New tests in `db/c_test.c` cover `StopBackup` and rate limiter setter/getter roundtrips, plus opening a real backup engine with rate limiters set and running a backup end-to-end - [x] `make check` passes with no regressions Reviewed By: pdillinger Differential Revision: D107654882 Pulled By: xingbowang fbshipit-source-id: f50c3989779e6a099113fec203231d47b9480cb9 |
||
|
anand76
|
a214d3f1f8 |
Update version to 11.5.0 for 11.4 release (#14820)
Summary: - Bump version.h from 11.4.0 to 11.5.0 - Add `11.4.fb` to `check_format_compatible.sh` - Cherry-pick HISTORY.md from `11.4.fb` - Delete `unreleased_history/` note files Part of 11.4 release workflow. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14820 Reviewed By: joshkang97 Differential Revision: D107681545 Pulled By: anand1976 fbshipit-source-id: ce34d01f3e7fb336cc7ebf3450ccbe97a8628eca |
||
|
Erin Gao
|
2b9e8dc925 |
Create a C shim for setting TransactionDB write policy (#14810)
Summary: `TransactionDB` supports different write policies (e.g. `WritePrepared`), but this functionality is not currently accessible via the C API. Creating a C shim to expose the functionality for setting the write policy. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14810 Reviewed By: pdillinger Differential Revision: D107654915 Pulled By: xingbowang fbshipit-source-id: b0d915a3420057de5236fe9f6cb47d291294788c |
||
|
Xingbo Wang
|
8c3bd2c3e9 |
Fix stale no-reopen tracking in fault injection FS (#14822)
Summary: PR https://github.com/facebook/rocksdb/issues/14585 added FileOpenContract enforcement to FaultInjectionTestFS. The no-reopen-for-write check recorded contracts by path but did not clear a stale contract after the file was deleted. A later SST create that reused the same path could be rejected as a forbidden reopen, causing DBWALTest.WALWithChecksumHandoff to fail with "NewWritableFile violates no-reopen-for-write contract". If the ASSERT exited the test early, the local Env stack object could also be destroyed before DB fixture teardown closed the DB, producing the follow-on TSAN heap-use-after-free. When opening a file for write, drop stale no-reopen tracking if the target file no longer exists, while still rejecting writes through reopened handles. Also close the DB before the WAL test's local Env unwinds on assertion failure. A separate TSAN report in DBBlobBasicIOErrorTest.GetEntityMergeWithBlobBaseIOError exposed unsynchronized access to FaultInjectionTestEnv::error_: SetFilesystemActive() updated the stored error under mutex_, while background purge work read it through GetError() without that mutex. Copy injected errors under the same mutex and read the no-space state in GetFreeSpace() while protected. Apply the same synchronization to FaultInjectionTestFS. Bonus fix: Fix another TSAN data race in FaultInjectionTestEnv GetError not synchronized under mutex_ Pull Request resolved: https://github.com/facebook/rocksdb/pull/14822 Test Plan: CI Reviewed By: mszeszko-meta Differential Revision: D107689662 Pulled By: xingbowang fbshipit-source-id: 6d4f8fdc8b898d3ddcad7816e385f3b7f20c4727 |
||
|
Xingbo Wang
|
76b8cce2f7 |
Fix spelling of memtable_verify_per_key_checksum_on_seek (#14811)
Summary: - Rename the misspelled `memtable_veirfy_per_key_checksum_on_seek` option and related flags/config keys to `memtable_verify_per_key_checksum_on_seek`. - Update memtable option plumbing, options serialization/logging, db_bench/db_stress/crash-test flags, tests, and the option-addition guide to use the corrected name. - Keep the checksum-on-seek behavior unchanged. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14811 Test Plan: CI Reviewed By: pdillinger Differential Revision: D107382276 Pulled By: xingbowang fbshipit-source-id: 7621dc718d61503b982a7e3f65cc9293a1ad085b |
||
|
Xingbo Wang
|
10e4f4745e |
Add contract boundary guidance to CLAUDE.md (#14821)
Summary: - Add a contract-boundaries section to the RocksDB code review checklist in `CLAUDE.md`. - Document common review prompts for keeping caller-specific policy out of reusable lower layers. - Add "Contract Boundary Leaks" to the common review feedback patterns. ## Testing CI Pull Request resolved: https://github.com/facebook/rocksdb/pull/14821 Reviewed By: pdillinger Differential Revision: D107652555 Pulled By: xingbowang fbshipit-source-id: 383bd78fec6e3ecc251c752d30f9de3ea44a10de |
||
|
Xingbo Wang
|
2d68f30425 |
Support remote file system for blob direct write file (#14585)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14585 Adds typed file-open contracts and open-file size handling needed for blob direct-write files on remote or SHM-backed file systems. This diff: - Adds typed `FileOpenContract` semantics to RocksDB `FileOptions`, including `kNoReopenForWrite`, `kNoReadersWhileOpenForWrite`, bitwise helpers, and constructor propagation from `EnvOptions` to `FileOptions`. - Marks SST/table output creation paths with both `kNoReopenForWrite` and `kNoReadersWhileOpenForWrite`, including builder output, compaction output, and external `SstFileWriter` output. - Marks blob write paths with the appropriate contracts: regular blob file builders use both `kNoReopenForWrite` and `kNoReadersWhileOpenForWrite`, while blob direct-write partition output uses `kNoReopenForWrite` so active readers can still observe SHM-backed data through the read path. - Adds `GetFileSizeFromOpenFileOrPath` so readers can prefer an already-open `FSRandomAccessFile::GetFileSize` result and fall back to path-level `FileSystem::GetFileSize` according to the caller's fallback policy. - Updates blob file reader and table footer reader size checks to use the new open-file-or-path size helper, preserving malformed-file validation while supporting file systems where path-level size is not the only valid source. - Extends `FaultInjectionTestFS` to track file-open contracts, reject readers while a no-readers writer is open, reject reopened data writes for `kNoReopenForWrite`, allow `SyncFile` to use a reopened handle, and preserve contract state across create, reopen, reuse, rename, link, reset, and delete paths. - Keeps `IOStatus` return paths simple by returning status locals directly in the file-size helper and fault-injection contract validation paths. - Adds clang-format-compliant tests covering file-open contract enforcement, `SyncFile` being allowed while reopened data writes are rejected, blob direct-write behavior, blob reader behavior, and the new file-size fallback helper behavior. - Leaves WAL and MANIFEST contract assignment for follow-up work because live WAL/MANIFEST readers and `reuse_manifest_on_open` need separate handling before RocksDB can safely claim stronger file-open contracts for those file classes. Reviewed By: anand1976, pdillinger Differential Revision: D100002686 fbshipit-source-id: 26b4021bc13333ede1077f0ff8cbd71335c5f294 |
||
|
Xingbo Wang
|
b5922eab11 |
Add FileSystem SyncFile API (#14739)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14739 Adds public path-level file sync APIs to RocksDB so callers can ask `Env` or `FileSystem` to sync a named file without hand-rolled reopen logic. This diff: - Adds public `Env::SyncFile` and `FileSystem::SyncFile` APIs for syncing or fsyncing a file by name without requiring callers to reopen it directly. - Documents the `SyncFile` contract: filesystems may override it as a no-op when flush/close already provides durability, and RocksDB callers should use it instead of hand-rolled `ReopenWritableFile()+Sync()/Fsync()` so filesystems can reject post-close data-write reopen paths. - Implements the default `Env` and `FileSystem` behavior by reopening the file as writable, calling `Sync` or `Fsync`, closing the handle, returning the sync/fsync error ahead of a close error when both fail, and explicitly consuming the close status on the sync-failure path. - Wires `SyncFile` through the Env/FileSystem bridge and wrapper layers, including `EnvWrapper`, `FileSystemWrapper`, `CompositeEnvWrapper`, `EncryptedFileSystem`, `RemapFileSystem`, read-only file systems, mock file systems, and fault-injection wrappers. - Keeps fault-injection `SyncFile` overrides on the base default implementation so the operation still exercises the wrapper's `ReopenWritableFile`, wrapped-file `Sync`/`Fsync`, and `Close` hooks instead of bypassing them through target forwarding. - Updates external SST ingestion to call `FileSystem::SyncFile` instead of manually reopening an ingested file as writable, while preserving the `NotSupported` skip behavior and failure logging. - Adds release-note coverage for the new public API and unit coverage for default `Env`/`FileSystem` success, reopen failure, close failure, sync-versus-close failure precedence, and external SST sync behavior. There was an earlier version of this API which got reverted (PR #13987) due to internal change broken. Re-apply the change. The internal issue will be resolved in next release. Reviewed By: pdillinger Differential Revision: D104918547 fbshipit-source-id: 3f8d2d127fc962b68b423bbb90de551fe6706224 |
||
|
Peter Dillinger
|
8053b9414f |
Change default compression from Snappy to LZ4 (#14818)
Summary: The historical default block compression `kSnappyCompression` dates to when Snappy was the obvious fast/cheap choice. On modern server CPUs LZ4 matches or beats Snappy on compression ratio while decompressing substantially cheaper, so it is a better default. This changes the default `ColumnFamilyOptions::compression` to `kLZ4Compression`, with a runtime fallback of LZ4 -> Snappy -> none depending on what is compiled into the binary (new `GetDefaultCompressionType()` in util/compression.h). Only column families that do not explicitly set `compression` are affected (including compaction output when `CompactionOptions::compression == kDisableCompressionOption`), and only newly written SST files; existing data is read as before. Doc comments, the Java bindings, and the sorted_run_builder example are updated to describe the new default. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14818 Test Plan: Adjusted two unit tests that implicitly depended on the old Snappy default to pin `compression = kSnappyCompression`: db_iterator_test's ReadAhead (readahead byte thresholds assume Snappy-sized files) and compaction_service_test's CustomFileChecksum (LSM shape after auto-compaction determined whether a manual CompactRange had work to do). This change is primarily validated by performance testing. Added a `compressreject` db_bench benchmark (output buffer sized just below the predicted compressed size) to measure the cost of attempting then declining compression, alongside `compress`/`uncompress`. Both db_bench and sst_dump compress/decompress a modest block at a time, as in a real workload. sst_dump on SST files from production workloads (4 files, 16KB blocks, single thread) on recent server-class AMD, Intel, and ARM CPUs, LZ4 vs Snappy: - Compression ratio: comparable; LZ4 is slightly smaller on the more compressible files (up to ~8%) and within ~2% on the rest. - Compression (write) CPU: a wash, within ~2% either direction. - Decompression (read) CPU: the clear win -- Snappy costs ~1.2x-1.5x as much as LZ4, i.e. LZ4 saves ~25-30% read CPU, consistently across AMD, Intel, and ARM. db_bench synthetic workload (100-byte values), at 1 / 12 / 160 threads, LZ4 vs Snappy: - Compression throughput: LZ4 ~10-30% higher. - Decompression throughput: LZ4 much higher, and the advantage grows with core count -- from ~+12% at 12 threads to ~+45-50% at 160 threads, i.e. better multi-core scaling. - Rejection (insufficient ratio) path: comparable; LZ4 ~15-18% faster on compressible-but-rejected blocks and Snappy within ~10% on barely-compressible blocks. No meaningful regression, confirming incompressible data is still efficiently detected and stored raw. Reviewed By: xingbowang Differential Revision: D107536490 Pulled By: pdillinger fbshipit-source-id: f8abaee630d782674778338148e36ed0c84e3661 |
||
|
Hui Xiao
|
5177a8e6c2 |
Add remote compaction format compatibility test to check_format_compatible.sh (#14798)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14798 Add cross-version format compatibility testing for remote compaction to `check_format_compatible.sh` as primary and worker RocksDB instances can run different versions. Two new ldb commands coordinate via local files: - `remote_compaction_primary`: opens an existing DB and runs `CompactRange()` through `LocalFileCompactionService`, which writes `input.bin` via `Schedule()` and polls for `result.bin` via `Wait()`. - `remote_compaction_worker`: polls for `input.bin`, calls `OpenAndCompact()`, writes `result.bin`. The test script creates a DB using `generate_random_db.sh` with the primary's ldb binary (new optional 3rd argument) so the OPTIONS file matches the primary's version. An overlap key is written to ensure `CompactRange` triggers a real compaction (not a trivial move). For each old ref in `db_forward_with_options_refs`, the script tests both directions -- current primary + old worker and old primary + current worker -- to catch wire-format incompatibilities in `CompactionServiceInput`/`CompactionServiceResult`. Old refs lacking the commands are skipped gracefully. Reviewed By: pdillinger Differential Revision: D106321150 fbshipit-source-id: e0341b57c1b12e1fa5296609f0463f77484c1a6e |
||
|
Peter Dillinger
|
3883a8d05e |
Rename db_test2 -> db_etc2_test (#14218)
Summary: When searching/grepping through unit tests, it's convenient to use test.cc suffix to match all unit tests, or to exclude test.cc when excluding unit tests from a code search. (I've even seen my AI assistant grep through `*test.cc`.) So I'm renaming db_test2.cc (as planned in https://github.com/facebook/rocksdb/issues/14076) Pull Request resolved: https://github.com/facebook/rocksdb/pull/14218 Test Plan: existing tests + CI Reviewed By: xingbowang Differential Revision: D90140624 Pulled By: pdillinger fbshipit-source-id: 78aa099290670bbb4093b2c7c02bc47ab3bf5f8e |
||
|
Josh Kang
|
768de6e50d |
Fix false-positive corruption error with remote compaction (#14808)
Summary: Fix a false-positive compaction corruption error in the remote compaction path. Remote workers can mark `CompactionJobStats::num_input_records` as unreliable when input iteration uses seek/skip behavior, but the remote result serialization was dropping `has_accurate_num_input_records`. The primary then deserialized the flag as its default `true`, trusted an unreliable zero input-record count, and raised `Compaction number of input keys does not match number of keys processed`. This change serializes the accuracy flag with the rest of `CompactionJobStats` so the primary preserves the remote worker's "do not verify this count" signal. It also adds a targeted regression test and a release note for the bug fix. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14808 Test Plan: New regression test that triggers corruption error without the fix by modifying `has_accurate_num_input_records=false` Reviewed By: xingbowang Differential Revision: D107128357 Pulled By: joshkang97 fbshipit-source-id: 3243c9c2ab18534652737f7410fe3c51724db549 |
||
|
Peter Dillinger
|
62f05627be |
Reduce manifest rotation for foreground metadata ops (#14797)
Summary: Async WAL precreation in https://github.com/facebook/rocksdb/pull/14738 / D105020559 was motivated by slow file creation time on remote storage. MANIFEST does not need the same precreation treatment as WAL because most MANIFEST writes come from background flush and compaction work, but user-facing metadata operations can still pay MANIFEST rotation file creation latency inline. File ingestion performance is a particular concern to some Meta users. Relax the effective MANIFEST rotation limit by 25% for MANIFEST write batches containing any foreground VersionEdit, while keeping background-only flush/compaction batches on the configured or auto-tuned limit. This covers column family manipulation, external file ingestion and import, and DeleteFilesInRange(s). SetOptions remains expected to avoid MANIFEST writes; the test keeps a regression guard for that behavior. The relaxation is intentionally bounded. It reduces the chance that foreground metadata operations create a new MANIFEST inline, while still allowing foreground operations to rotate once the current MANIFEST is beyond the relaxed threshold. Heavier blocking operations like manual Flush or CompactRange already trigger additional file creation and do not get this treatment here, though that could be reconsidered later. This should reduce a potential latency hazard of manifest file size auto-tuning: more frequent MANIFEST rotations. With this change, rotation latency is shifted toward background-only MANIFEST batches when possible. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14797 Test Plan: Expanded DBEtc3Test.AutoTuneManifestSize to cover the foreground threshold behavior and the original auto-tuning behavior in separate phases: - verifies foreground-only CreateColumnFamily writes get only bounded 25% headroom by asserting the first four large-CF additions do not rotate and the fifth does; - verifies auto-tuned background thresholds still prevent excessive rotation; - verifies foreground operations stay below the relaxed threshold for CreateColumnFamily, IngestExternalFile, CreateColumnFamilyWithImport, and DeleteFilesInRanges; - verifies SetOptions still does not write to MANIFEST; - verifies a following background flush still rotates at the normal threshold; - preserves the persisted compacted manifest size close/reopen coverage. Reviewed By: xingbowang Differential Revision: D106578771 Pulled By: pdillinger fbshipit-source-id: f8e274032cd9e7f50e95b685c949242f95351498 |
||
|
Peter Dillinger
|
9ef369c606 |
Make StringToMap entries self-contained for direct round-trip (#14805)
Summary:
StringToMap previously stripped the outer braces from nested values,
making single-entry round-trip via `key=value;` (e.g. SetOptions)
silently corrupt values that contain ';'. For example, a filter_policy
with sub-options serialized as
filter_policy={id=ribbonfilter:10:-1;bloom_before_level=-1;}
parsed to map[filter_policy] = "id=ribbonfilter:10:-1;bloom_before_level=-1;",
and embedding that map entry directly into another `key=value;` string
re-exposed the inner ';' as a top-level delimiter.
This change is a step toward the "essential view": the outer `{...}` of
a nested value is part of the value's identity. StringToMap preserves it, so
each map entry is in self-contained form (a simple value or a single
balanced `{...}` block) and can be embedded directly without further
escaping by the caller. However some permissiveness is kept for
compatibility: list-style typed parsers (ParseVector, ParseArray,
kStringMap, the listener parser) and scalar dispatch in
OptionTypeInfo::Parse accept either the bare or the wrapped form via a
new OptionTypeInfo::StripOuterBraces helper that peels at most one
outer-pair-matched layer. So braced scalar input like `key={42};` and
`key={true};` still parses, and `key={};` denotes an empty list,
distinct from `key={{}};` which denotes a one-element list with an
empty element.
A new public MapToString is added to convenience.h as the symmetric
inverse of StringToMap: a trivial `key=value;` joiner that relies on
each value already being self-contained (which StringToMap guarantees).
Also fixed and refactored our trim() function because it would do the wrong
thing for a single space. This problem was detected by expanding the
StringToMapRandomTest based on code review feedback, and should only
improve existing callers to trim().
The OPTIONS-file serializer code is untouched, so the on-disk format
is byte-for-byte identical and format-compatibility is preserved.
Bonus: fixes / clarifications to CLAUDE.md's build-system note.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14805
Test Plan:
db_bloom_filter_test extends MutableFilterPolicy with the OPTIONS-file
format for filter_policy to confirm that form round-trips through
SetOptions.
options_test updates StringToMapTest expectations for the new
brace-preserving behavior, and adds:
- MapToStringTest covering the new joiner.
- StringToMapMapToStringRoundTripTest including a single-entry
pull-and-embed scenario that previously corrupted on round-trip.
- FullOptionsStringToMapRoundTripTest that drives a populated
DBOptions and CFOptions through GetStringFromXxx -> StringToMap ->
per-entry single round-trip -> MapToString -> GetXxxFromString ->
VerifyXxxOptions, catching any custom serializer that emits a
value with ';' without enclosing it in '{}'.
- EmptyBracedVectorAndBracedScalarTest covering `key={};` (empty
list) and braced scalar input like `key={42};`, `key={true};`.
- Expanded StringToMapRandomTest with round-tripping.
Some explicit trim() tests added to string_util_test.cc
Format compatibility verified with
SHORT_TEST=1 tools/check_format_compatible.sh
Other existing tests in options_test, customizable_test, options_settable_test,
listener_test, options_file_test, options_util_test, db_options_test,
and compaction_service_test have extensive coverage of serialization /
deserialization logic.
Reviewed By: hx235, xingbowang
Differential Revision: D106855466
Pulled By: pdillinger
fbshipit-source-id: 872aac8d819c7b90c807b92bab85569b48fa2aa0
|
||
|
Xingbo Wang
|
30dba7f41a |
Fix compaction abort rescheduling before queue pick (#14800)
Summary: - AbortAllCompactions can race with an already-scheduled automatic compaction before the background worker calls PickCompactionFromQueue(). MaybeScheduleFlushOrCompaction() has already consumed one unscheduled_compactions_ credit when it scheduled the worker, but the CF is still present in compaction_queue_ with queued_for_compaction=true. If the worker returns kCompactionAborted before popping the CF, ResumeAllCompactions() can see unscheduled_compactions_ == 0 and fail to schedule the still-queued work, leaving compaction permanently stalled until DB restart. - Restore the unscheduled compaction credit in the non-prepicked abort path, matching the existing BG-work-stopped handling for the same scheduled-before-pick state. Prepicked/manual compactions are not adjusted because they do not represent an unpopped automatic compaction_queue_ entry whose scheduling credit was consumed. - Add AbortScheduledAutomaticCompactionBeforePick to deterministically reproduce the lost-credit race with a sync point at BackgroundCallCompaction:0. The test verifies that ResumeAllCompactions() schedules the still-queued automatic compaction by checking COMPACT_WRITE_BYTES advances and L0 file count drops. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14800 Test Plan: - Without the implementation fix, `DBCompactionAbortTest.AbortScheduledAutomaticCompactionBeforePick` fails because `COMPACT_WRITE_BYTES` remains unchanged after `ResumeAllCompactions()`. Reviewed By: pdillinger Differential Revision: D106684155 Pulled By: xingbowang fbshipit-source-id: 6dacea473ef481eb3122eb15e296e5b69635413f |
||
|
Xingbo Wang
|
023fbb074a |
Optimize MultiScan dispatch for sorted blocks (#14783)
Summary: - Propagate validated, sorted MultiScan range state from `DBIter::Prepare()` through `MultiScanArgs`. - Mark block-based table IO jobs as already sorted when the public MultiScan ranges have been validated. - Keep `IODispatcher::SubmitJob()` as the normalization boundary for unsorted callers, while allowing sorted callers to skip the defensive block-handle sort. - Update private dispatcher coalescing helpers to consume sorted block indices and add debug assertions for that precondition. ## Testing CI, new unit test Pull Request resolved: https://github.com/facebook/rocksdb/pull/14783 Reviewed By: anand1976 Differential Revision: D106301516 Pulled By: xingbowang fbshipit-source-id: 99b7ffcaecbf27cb79f15feb4af8680ff1e422d9 |
||
|
Josh Kang
|
26a501b5d1 |
Fix tiered compaction incorrectly moving range tombstones upwards (#14795)
Summary: Fixes an off-by-one bug in how sequence numbers are handled when splitting range tombstones across output levels in per-key-placement (tiered) compaction. The bug was either introduced or propagated in https://github.com/facebook/rocksdb/issues/13256. Point entries move to proximal output only when their sequence number is strictly greater than `proximal_after_seqno_`, but range tombstones were previously split using an inclusive lower bound at that same seqno. That allowed a range tombstone at the boundary to be emitted to the proximal level while point keys at the same seqno stayed in the last level, which could create overlapping files in the proximal level (caught by `force_consistency_checks` as `L<n> has overlapping ranges`). This matters when `proximal_output_range_type_` is `kNonLastRange`: the compaction only owns the selected proximal-level input range, so existing last-level data at the split boundary must stay in the last level. In `kFullRange`, the compaction owns the relevant proximal-level range, so newer last-level data can be safely emitted to proximal output. The fix splits range tombstones at `proximal_after_seqno_ + 1` (saturating at `kMaxSequenceNumber`), so the half-open `[lower, upper)` tombstone filter lands on the same boundary as the strict `seqno > proximal_after_seqno_` rule used for point keys. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14795 Test Plan: New regression test `PrecludeLastLevelTestBase.RangeDelAtProximalSeqnoBoundaryStaysInLastLevel` uses a targeted manual compaction to exercise the `kNonLastRange` case directly, verifying a boundary range tombstone stays in the last level instead of widening the proximal output range. Worked example: ```text Initial state: L6 (last level): Put(Key 2)s1, Put(Key 12)s2, RangeDel[Key 2, Key 12)s3 L5 (proximal): file A [Key 0 .. Key 4]s4-s5 file B [Key 5 .. Key 9]s6-s7 preclude_last_level_min_seqno is forced to 0 via sync point. Manual CompactFiles selects only L5 file B + the L6 file (output level 6). Only part of the proximal level is selected, so this is the kNonLastRange case: max_last_level_seqno = 3 proximal_after_seqno_ = max(0, 3) = 3 Point keys Key 5 (s6) and Key 9 (s7): both seqno > 3 -> proximal output (L5) OK Range tombstone s3: Old (buggy): proximal keep range [3, MAX) includes s3, so the tombstone is emitted to the proximal output. The output is built from L5 input file B [Key 5 .. Key 9], but the tombstone covers [Key 2, Key 12), so the proximal output file starts at Key 2 and spills past Key 5 into the existing, untouched L5 file A [Key 0 .. Key 4]. Two overlapping files in L5 -> Corruption. New (fixed): split at proximal_after_seqno_ + 1 = 4. proximal keep [4, MAX) excludes s3; last-level keep [0, 4) includes s3 -> tombstone stays in the last level (L6). OK ``` Verification (debug build, `make -j64 tiered_compaction_test`): - Without the fix, the test fails at the `CompactFiles` call: ``` tiered_compaction_test.cc:2940: Failure Corruption: force_consistency_checks(DEBUG): VersionBuilder: L5 has overlapping ranges: file https://github.com/facebook/rocksdb/issues/11 largest key: Key(4) seq:5, type:1 (Put) vs. file https://github.com/facebook/rocksdb/issues/17 smallest key: Key(2) seq:3, type:15 (range deletion) ``` - With the fix, the test passes (range deletions absent from L5, still present in L6). Reviewed By: pdillinger Differential Revision: D106528471 Pulled By: joshkang97 fbshipit-source-id: a5f99b426d3a7a6253bc1972cf8cb60d1cb85089 |
||
|
Xingbo Wang
|
e492562651 |
Prune MultiScan blocks using first internal key (#14784)
Summary: - Use `IndexValue::first_internal_key` from `kBinarySearchWithFirstKey` index entries to decide whether the final bounded MultiScan candidate block starts at or beyond the scan limit. - Skip that block when its first user key compares greater than or equal to the range limit with `CompareWithoutTimestamp`. - Preserve existing conservative behavior for unbounded ranges, index entries without first-key metadata, and normal `kBinarySearch` indexes. - Add parameterized coverage for boundary limits, in-block limits, bytewise and reverse comparators, and stripped/persisted user-defined timestamp modes. ## Testing CI Pull Request resolved: https://github.com/facebook/rocksdb/pull/14784 Reviewed By: pdillinger Differential Revision: D106302205 Pulled By: xingbowang fbshipit-source-id: 1acebdf48bf7c18d35a781ca41c7bfd5c4ab8f47 |
||
|
Hui Xiao
|
a0db079fdd |
Fall back to local compaction and report kUseLocal on remote result parse failure (#14799)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14799 When `CompactionService::Wait()` returns `kSuccess` but `CompactionServiceResult::Read()` fails before the primary renames any remote output file from `CompactionServiceResult::output_path` into the DB directory, fall back to local compaction for the same job and notify the service with `OnInstallation(..., kUseLocal)`. At that point the remote SSTs are still in the service-managed output directory recorded in `CompactionServiceResult::output_path`, and the primary has not installed any of them into the DB yet. Update `CompactionServiceTest.InvalidResultFallsBackToLocal` to verify the fallback completes successfully, preserves the data, and invokes `OnInstallation()` exactly once with `kUseLocal`. Reviewed By: jaykorean Differential Revision: D106321319 fbshipit-source-id: 39d9206f0e3f62612a52c03462bd1bee69020b80 |
||
|
Michael Huang
|
9270dc8149 |
Add blob_cache_read_byte perf context counter (#14792)
Summary: Added blob_cache_read_byte in the rocksdb::PerContextBase to expose the blob cache read bytes when blob cache is enabled. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14792 Reviewed By: xingbowang Differential Revision: D106528572 Pulled By: mikechuangmeta fbshipit-source-id: 555b2f01785bb819e62ed834ee45f0436dfb2875 |
||
|
xingbowang
|
638354e766 |
Fix getdeps fallback mirror downloads (#14763)
Summary: - parse folly getdeps manifests with bare package entries so fallback prefetching actually runs - validate and remove bad cached/downloaded archives before trying fallback mirrors - download through temporary files and include libiberty in the GNU toolchain fallback set Context: Nightly test failed with dependency download failure in folly. ``` Assessing autoconf... Download with https://ftpmirror.gnu.org/gnu/autoconf/autoconf-2.69.tar.gz -> /tmp/fbcode_builder_getdeps-Z__wZrocksdbZrocksdbZthird-partyZfollyZbuildZfbcode_builder-root/downloads/autoconf-autoconf-2.69.tar.gz ... [Complete in 136.616022 seconds] raise Exception( Exception: https://ftpmirror.gnu.org/gnu/autoconf/autoconf-2.69.tar.gz: expected sha256 954bd69b391edc12d6a4a51a2dd1476543da5c6bbf05a95b59dc0dd6fd4c2969 but got e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 make: *** [folly.mk:152: build_folly] Error 1 ##[error]Process completed with exit code 2. ``` Pull Request resolved: https://github.com/facebook/rocksdb/pull/14763 Test Plan: 1. Connection failure: - Forced first mirror to http://127.0.0.1:1/... - It logged connection refused. - It then tried https://mirrors.kernel.org/gnu/... - Download succeeded, size 1927468, SHA matched 954bd69b... 2. Empty file / bad hash: - Ran a local HTTP server returning a zero-byte autoconf-2.69.tar.gz - Script logged mismatch with actual=e3b0c442... size=0 - It removed the bad download and fell back to mirrors.kernel.org - Download succeeded with the expected SHA. 3. Existing zero-byte cache: - Seeded cache with an empty tarball. - Script removed invalid cache and downloaded a verified copy. Reviewed By: mszeszko-meta Differential Revision: D105859558 Pulled By: xingbowang fbshipit-source-id: ff1f20f87debad561610271ce99b8b8de2d4264f |
||
|
Hui Xiao
|
c724aeb67e |
Add multi-DB stress testing support (--num_dbs flag) (#14749)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14749 Add `--num_dbs` flag to run N independent DB instances in parallel. Each `StressTest` instance has its own DB with isolated fault injection (from D104959945). `db_crashtest.py` defaults to `num_dbs=1`. For `num_dbs=1`: `--db` and `--expected_values_dir` are paths used as-is. For `num_dbs>1`: they are parent directories; C++ creates `db_0/`, `db_1/`, ... subdirs underneath. Path ownership: C++ owns DB and secondary dir creation (supports remote env). Python owns EV dir creation (always local). C++ also creates EV dirs as fallback for direct CLI usage. `DestroyAllDbs` cleans up subdirs and the parent dir. Per-DB: `threads`, `max_key`, `ops_per_thread`, `reopen`, `column_families`, and all DB options. Shared: background env threads (compaction, flush pool), `block_cache`, `write_buffer_manager`, `compressed_secondary_cache`, `rate_limiter`, `compaction_thread_pool_adjust_interval`. Reviewed By: anand1976 Differential Revision: D104959942 fbshipit-source-id: 3d0d60101e7f2e600306e5a9c4018686bf649658 |
||
|
Xingbo Wang
|
364eb88151 |
Keep prepared transactions rollbackable after commit write failure (#14778)
Summary: - Restore prepared transactions to `PREPARED` state when writing the commit marker fails, so callers can still roll them back. - Preserve `PREPARED` state when rollback of a prepared transaction hits a retryable write error, allowing rollback to be retried after `DB::Resume()`. - Update `db_stress` to clean up prepared transactions after failed commits and report detailed rollback cleanup failure diagnostics. - Add a WritePrepared regression test covering retryable commit write failure, retryable rollback write failure, successful rollback retry, and DB reopen. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14778 Test Plan: CI Reviewed By: pdillinger Differential Revision: D106202437 Pulled By: xingbowang fbshipit-source-id: b0b52e1d14f39b023b9692dd8fc44060fa35c446 |
||
|
Xingbo Wang
|
d7afd3bb3b |
Sync recovery SST directory before reused MANIFEST append (#14780)
Summary: - When `reuse_manifest_on_open` reuses the current MANIFEST, `DB::Open` recovery can flush WAL data into a new L0 SST and append the corresponding `VersionEdit` to that already-current MANIFEST. - If open later fails and the process crashes, the MANIFEST edit can be durable while the recovered SST directory entry is not, leaving the DB pointing at a missing SST. - Fsync the recovered SST's data directory before adding the file to the recovery edit when appending to a reused MANIFEST. - Add a regression test that injects failure after MANIFEST sync, simulates crash cleanup of files created after the last directory sync, and verifies the recovered key remains readable. ## Task - T272584339 Pull Request resolved: https://github.com/facebook/rocksdb/pull/14780 Test Plan: CI Reviewed By: anand1976 Differential Revision: D106201774 Pulled By: xingbowang fbshipit-source-id: a44a7d1263d5bc1d82b995c90eef1a825eab4182 |
||
|
Xingbo Wang
|
ae30c71c6b |
Fix secondary WAL tailing with precreated future WAL (#14781)
Summary: - Fix secondary catch-up WAL discovery to retain existing WAL readers until MANIFEST replay advances min_log_number_to_keep past them. - Do not treat a higher-number WAL appearing in the directory as proof that a lower-number current WAL is obsolete; async WAL precreation can expose that shape while the lower-number WAL is still growing. - Continue scanning from the smallest retained reader so later appends to the current WAL are replayed, and add a regression test that precreates an empty future WAL before secondary catch-up. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14781 Test Plan: - make -j128 db_secondary_test - ./db_secondary_test --gtest_filter=DBSecondaryTest.CatchUpTailsCurrentWalWhenFutureWalExists - ./db_secondary_test - make check-sources Reviewed By: hx235 Differential Revision: D106296411 Pulled By: xingbowang fbshipit-source-id: acc850a177c02968372981d1407721540bc164f5 |
||
|
zaidoon
|
91b31112ed |
Expose AdvancedColumnFamilyOptions::memtable_batch_lookup_optimization in the C API (#14776)
Summary: `AdvancedColumnFamilyOptions::memtable_batch_lookup_optimization` ([advanced_options.h](https://github.com/facebook/rocksdb/blob/main/include/rocksdb/advanced_options.h)) gates the skip-list memtable's batch-lookup optimization for `MultiGet`. When enabled, the search path is cached between consecutive keys, reducing per-key cost from `O(log N)` to `O(log d)` where `d` is the distance between consecutive keys. The C++ field exists; the C API setter does not. This PR adds the missing pair, mirroring the existing `rocksdb_options_{set,get}_memtable_huge_page_size` shape exactly — the closest sibling on both axes: - C API: adjacent memtable knob, same `rocksdb_options_t*` receiver. - C++: same `AdvancedColumnFamilyOptions` parent struct, same immutability semantics. ## Motivation Without this setter, C API consumers and downstream bindings cannot opt into the batch-lookup optimization. Non-skip-list memtable implementations fall back to per-key lookups, so the flag is a no-op for them. The field is immutable on the C++ side, so calling the setter on options that are already in use by an open DB has no effect on that DB — same constraint as the underlying C++ field. This matches the behavior of every other immutable-options setter in the C API. No change to the C++ API. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14776 Reviewed By: joshkang97 Differential Revision: D106364224 Pulled By: xingbowang fbshipit-source-id: 90946af498fba51581a1e7d493c9e5c9b98472a2 |
||
|
zaidoon
|
7f32e2cabc |
Expose BlockBasedTableOptions::uniform_cv_threshold and BlockSearchType::kAuto in the C API (#14775)
Summary: The block-based table format gained an "auto" index-block search mode ([table.h](https://github.com/facebook/rocksdb/blob/main/include/rocksdb/table.h)) that selects binary vs interpolation search per index block based on key uniformity. The C++ surface exposes this as two coupled knobs: - `BlockSearchType::kAuto = 0x02` selects the per-block adaptive search at read time. - `BlockBasedTableOptions::uniform_cv_threshold` (default `-1`, i.e. disabled) is the coefficient-of-variation threshold checked on the write path to set the per-block `is_uniform` footer bit that `kAuto` reads. This PR adds the missing C API coverage for both: 1. **`rocksdb_block_based_table_index_block_search_type_auto = 2`** enum constant. The existing setter `rocksdb_block_based_options_set_index_block_search_type` already does `static_cast<BlockSearchType>(v)`, so `kAuto = 2` was reachable today by passing the raw int — only the named constant was missing. 2. **`rocksdb_block_based_options_set_uniform_cv_threshold(...)` setter**. The field had no C wrapper, so C/binding users could select `kAuto` but the `is_uniform` bit was never set on the write path, making `kAuto` degenerate to `kBinary`. No getter is added: the surrounding `rocksdb_block_based_options_set_*` functions in `c.h` do not expose getters either, so adding one only here would be inconsistent with the local style. ## Motivation Without both pieces, `kAuto` is effectively unreachable from C. This matters for binding consumers (Rust, Go, Java-via-JNI shim, etc.) who want to opt index-block search into the per-block adaptive mode. The setter mirrors the existing `rocksdb_block_based_options_set_data_block_hash_ratio` shape exactly (same struct, same `double` payload, same naming pattern), so review surface is minimal. No change to the C++ API. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14775 Reviewed By: joshkang97 Differential Revision: D106364288 Pulled By: xingbowang fbshipit-source-id: bb532eac6d4c04d032a7235f25ac29ab74f636f2 |
||
|
Xingbo Wang
|
81fa943ca0 |
Avoid retryable io_uring wait_cqe stderr in crash tests (#14779)
Summary: - Suppress retryable `io_uring_wait_cqe()` errors in `PosixFileSystem::Poll()` and `AbortIO()` before they reach stderr during crash-test SIGTERM timeout handling. - Keep terminal `wait_cqe` failures logged and fatal. - Extend the `db_crashtest.py` SIGTERM stderr filter only for retryable `Poll`/`AbortIO` wait_cqe errors and add regression coverage. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14779 Test Plan: CI ## Related - T272682963 Reviewed By: hx235 Differential Revision: D106201579 Pulled By: xingbowang fbshipit-source-id: 2331d5cdca064ad901f6af7341b4e8f15b418663 |
||
|
zaidoon
|
a3ba3e8a6e |
Expose ReadOptions::optimize_multiget_for_io in the C API (#14752)
Summary:
The async MultiGet support introduced two `ReadOptions` flags: `async_io` and `optimize_multiget_for_io`. The setter/getter for `async_io` was exposed in the C API in [
|
||
|
Peter Dillinger
|
e82af29adb |
Makefile fix and speed up 'clean' (#14767)
Summary: * Fix Makefile default target (was ordered after a folly target) * Improved the speed of `make clean` by using just one `find` and by pruning "hidden" .* and third-party directories that should not be modified anyway. * Reduce excessive output from `make clean` Pull Request resolved: https://github.com/facebook/rocksdb/pull/14767 Test Plan: manual Reviewed By: mszeszko-meta Differential Revision: D105972958 Pulled By: pdillinger fbshipit-source-id: 2c0f6097c74c3129b815450f23c19ef07bfbe656 |
||
|
Peter Dillinger
|
acfa68ccff |
Fix flaky preallocation tests on btrfs, zfs, tmpfs, and overlayfs (#14744)
Summary: Fix flaky test failures in EnvPosixTestWithParam.AllocateTest and DBWALTest.TruncateLastLogAfterRecoverWithFlush that occur when running on filesystems where preallocated space is not reliably reflected in st_blocks. The tests use stat() to check st_blocks and verify that fallocate with FALLOC_FL_KEEP_SIZE actually preallocates disk space. However, on certain filesystems, this check is unreliable: 1. btrfs and zfs: Copy-on-write filesystems where preallocated extents are not reliably reflected in st_blocks, especially under load. 2. tmpfs: Memory filesystem that may not report preallocated blocks in st_blocks. 3. overlayfs: Union filesystem common in containers that may not pass through fallocate properly or report preallocated space. 4. Any filesystem where FALLOC_FL_KEEP_SIZE is not supported: The preallocation will fail silently (error ignored with PermitUncheckedError), leaving only the written data in st_blocks. Changes made: env/env_test.cc: - Added filesystem magic number definitions for TMPFS_MAGIC, OVERLAYFS_SUPER_MAGIC, and ZFS_SUPER_MAGIC - Extended the AllocateTest to skip block count checks on zfs, tmpfs, and overlayfs in addition to btrfs - Added runtime fallback: if st_blocks is less than expected, print a warning and skip the check instead of failing. This handles unknown filesystems or configurations where preallocation isn't supported. db/db_wal_test.cc: - Added includes and filesystem magic number definitions - Added ShouldSkipAllocationCheck() helper function to detect problematic filesystems - Modified TruncateLastLogAfterRecoverWithoutFlush, TruncateLastLogAfterRecoverWithFlush, TruncateLastLogAfterRecoverWALEmpty, and ReadOnlyRecoveryNoTruncate tests to skip allocation checks on problematic filesystems - Added runtime fallback checks similar to env_test.cc These changes make the tests robust against filesystem differences while still validating preallocation behavior on filesystems where it works correctly (ext4, xfs). Pull Request resolved: https://github.com/facebook/rocksdb/pull/14744 Test Plan: Many local 'make -j100 check' runs that would previously fail with good probability. Reviewed By: hx235 Differential Revision: D105331256 Pulled By: pdillinger fbshipit-source-id: 862a1512da1466cb037af15342404939b677c02a |
||
|
Xingbo Wang
|
8bf167a194 |
Notify listeners before DB shutdown begins (#14769)
Summary: ### Notify listeners before DB shutdown begins db_stress listener bookkeeping correlates compaction callbacks with file deletion callbacks. DBImpl intentionally skips some compaction listener callbacks once shutdown starts, but file deletion callbacks can still be delivered. That mismatch is normally tolerable for production listeners, but db_stress uses listener-local tracking to detect callback consistency and can report a false positive when shutdown interrupts a compaction callback sequence. The failed-open case is the important gap. DB::Open() can create a DBImpl, recover or flush files, schedule background compaction, and then fail during late open work such as persisting OPTIONS or waiting for open-time compaction under fault injection. At that point DB::Open() tears down the internal DBImpl before returning an error to StressTest::Open(). If OnCompactionBegin already recorded an input file or job in DbStressListener, DBImpl shutdown can suppress the later OnCompactionPreCommit/OnCompactionCompleted callbacks that would normally clear that state. Since control has not returned to the stress harness yet, StressTest::CleanUp()/Reopen() cannot notify the listener in time. A later shutdown-time callback, or the next open retry reusing the same listener, can then observe stale tracking and abort even though RocksDB did not compact the same SST concurrently. Add EventListener::OnDBShutdownBegin and fire it once from DBImpl::CancelAllBackgroundWork() before publishing shutting_down_. The callback also covers cleanup of a failed DB::Open() attempt, where the DB pointer refers to the internal DBImpl that was never returned to the caller. Track shutdown_notification_sent_ separately from shutting_down_ because listeners are invoked with mutex_ released, and a concurrent or reentrant cancellation must not deliver the callback twice. Update DbStressListener to consume the DBImpl-driven shutdown notification instead of relying on StressTest::CleanUp()/Reopen() to manually notify it. This lets db_stress mark itself as shutting down before DBImpl starts skipping shutdown-sensitive compaction notifications, including during failed-open cleanup. ### Also keep listener state scoped to one db_stress open attempt. Initialize listeners at the top of each non-transactional open retry before enabling open fault injection, so retry attempts get fresh listener state without widening open fault injection to listener construction. Factor the open fault setup into a small helper to keep the retry loop readable. The transaction open path still initializes listeners once because it does not use this open-fault retry loop. ### Also fix multi-ops transaction listener checks during shutdown A TSAN race was reported where MultiOpsTxnsStressListener::OnCompactionCompleted called VerifyPkSkFast on a background compaction thread while the main thread was destroying the transaction DB wrapper in StressTest::CleanUp(). The reported read was a virtual call through the DB object and the write was the WritePreparedTxnDB/WriteUnpreparedTxnDB destructor updating the vptr. The vulnerable ordering is that DBImpl can already be inside NotifyOnCompactionCompleted before shutdown is requested. It unlocks db mutex while iterating listeners; another listener such as DbStressListener can spend time in its callback, giving the main thread time to enter CleanUp()/Close(). The DB object is still shutting down, but MultiOpsTxnsStressListener may be invoked later in the same callback iteration and call VerifyPkSkFast through stress_test_->db_aptr_, racing with DB wrapper destruction. With DBImpl-owned EventListener::OnDBShutdownBegin callback, we have MultiOpsTxnsStressListener consume that callback directly. Once DBImpl begins shutdown, the listener skips both flush-completed and compaction-completed verification callbacks, avoiding DB access during teardown. This also covers failed-open cleanup without name-based downcasts in StressTest. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14769 Test Plan: CI Reviewed By: pdillinger Differential Revision: D106011452 Pulled By: xingbowang fbshipit-source-id: 768838ddcd9910de5d1b5204c990a4d88dbc850c |
||
|
Hui Xiao
|
97551b72d7 |
Per-StressTest fault injection with env/fs cleanup (#14757)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14757 Pull Request resolved: https://github.com/facebook/rocksdb/pull/14750 Prerequisite for multi-DB stress test support where each StressTest instance owns one DB. Moves fault injection from a single global to per-StressTest instance so each DB gets isolated fault injection; errors injected into one DB do not leak to others. FS/Env architecture change: Before (upstream): raw_fs → FaultInjectionTestFS (global) → DbStressFSWrapper → db_stress_env → DB::Open After (this diff): Global: raw_env (no wrappers) Used outside StressTest: non-FS ops (threads, time, sleep), test framework FS setup (dirs), DB destruction Used inside StressTest: cleanup ops that must succeed (external file delete) Per StressTest: raw_fs → DbStressFSWrapper (db_stress_fs_, always) → FaultInjectionTestFS (db_fault_injection_fs_, optional) → CompositeEnvWrapper (db_env_, always) → DB::Open Expected values state: Env::Default() (always local PosixEnv even when raw_env is remote) FS layer order swapped (DbStressFSWrapper now innermost). Safe because: - Error injection returns early; inner wrapper never executes (same behavior) - DbStressFSWrapper assertions do not modify data (checksum validation, IOActivity checks) - MANIFEST rename tracking slightly better for crash simulation in new order Stored members (per StressTest): - db_stress_fs_: DbStressFSWrapper. Always active regardless of fault injection flags. - db_fault_injection_fs_: FaultInjectionTestFS. Only when fault injection flags set. Direct access for Enable/Disable/SetThreadLocal. - db_env_: CompositeEnvWrapper. Always present. THE env for all DB I/O (options_.env). Eliminated globals: db_stress_env → renamed to raw_env (no wrappers, DbStressFSWrapper moved to per-StressTest); db_stress_listener_env, db_stress_raw_fs removed; fault_fs_guard, fault_env_guard moved to per-StressTest. Other changes: - CleanupOutputDirectory simplified (uses raw_env, no disable/enable needed) - SstFileManager recreated with per-StressTest env in Open() - Remote compaction override env uses options.env directly (fixes pre-existing silent bug) - Comments added: Env::Default() always local, DbStressDestroyDb MANIFEST explanation, fault injection log path in TEST_TMPDIR - TestFSWritableFile::Close() now mirrors the production FSWritableFile close boundary. After the first Close() attempt, later explicit or destructor Close() calls are wrapper-level no-ops, while FaultInjectionTestFS still records the first close attempt for crash/recovery simulation. - Added targeted fault_injection_fs_test coverage for injected metadata-close failures to ensure FaultInjectionTestFS does not retry the inner Close() path. Reviewed By: anand1976 Differential Revision: D104959945 fbshipit-source-id: 7cf9bb494dec2b372528d5f119c023b6d392ffca |
||
|
Maciej Szeszko
|
d1ab4bf12c |
Guard db_bench DB lifetime with RWMutex against shutdown race (#14754)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14754 **Problem** When db_bench shuts down (fatal IO error, unknown benchmark name, etc.), ErrorExit() or ~Benchmark() destroys db_/multi_dbs_ while worker threads may still be inside benchmark methods holding raw DBWithColumnFamilies pointers returned by SelectDBWithCfh(). In single-DB mode, db_.db becomes nullptr and multi_dbs_ is empty, so workers evaluate rand_int % 0 and SIGFPE. The cascade across all workers masks whatever originally triggered the shutdown. Observed in the reliable_volumes crash-fault test at ~5 SIGFPE events per hour. **Approach** Guard DB lifetime at the worker-thread boundary with a reader-writer lock (port::RWMutex): workers acquire a read lock for the duration of their benchmark method; mutators acquire the write lock (which waits for active readers to drain) before destroying DBs. This ensures raw pointers returned by SelectDBWithCfh() remain valid for their entire usage scope. **Fix** - Workers hold a DbUseGuard (RAII wrapper over a read lock on db_lifecycle_rwlock_) for the entire benchmark method in ThreadBody. Cost: one rdlock/unlock pair per worker lifetime, zero hot-path overhead. - Every DB mutation path (ErrorExit, ~Benchmark, fresh-DB reopen) takes DbStateMutationGuard, which acquires the write lock and then stops the secondary update thread before mutation may proceed. - ErrorExit() routes to std::_Exit(1) when called from any thread that cannot safely run the mutation cleanup path: a DbUseGuard holder (would self-wait on the write lock) or the secondary update thread (would self-join via StopSecondaryUpdateThread). Tracked via two thread-locals: holds_db_use_guard_ and is_secondary_update_thread_. Main thread takes DbStateMutationGuard, cleans up, dispatches through db_bench_exit() / ToolHooks::Exit. - StopSecondaryUpdateThread() resets secondary_update_stopped_ to 0 after joining, so a replacement thread created by a subsequent Open() is not immediately killed. - Protocol is mechanically enforced in debug builds: * SelectDBWithCfh asserts holds_db_use_guard_ (read-side ownership) * DeleteDBs asserts holds_db_state_mutation_guard_ (write-side ownership) * DbUseGuard and DbStateMutationGuard ctor/dtor assert correct imbalance (no nested or stray release). * DbStateMutationGuard ctor additionally asserts !holds_db_use_guard_ and !is_secondary_update_thread_, catching the new deadlock modes the single-lock design makes reachable (WriteLock while holding ReadLock; secondary thread self-joining via StopSecondaryUpdateThread). These convert "trust me" invariants into "the assert fires if you break it." Direct field accesses (e.g. db_.db->NewIterator inside a benchmark method, multi_dbs_.clear() in the reopen branch) are protected by the enclosing guard scope but are not per-site asserted. **Caveat** port::RWMutex is pthread_rwlock_t with default attrs -> reader-preferred on Linux/glibc. The current call graph has no concurrent worker spawn during mutator wait (mutator paths run only from the main thread, not concurrently with RunBenchmark spawning workers), so writer starvation is not reachable. Documented as a constraint; revisit if that invariant changes. The port layer doesn't expose pthread_rwlockattr_setkind_np cross-platform. **Alternatives considered** - std::_Exit(1) on every shutdown path, skip cleanup entirely. Loses ToolHooks::Exit dispatch, flushed traces, and end-of-run stats -- those matter for the RV crash-fault test image which consumes db_bench output. Rejected. - shared_ptr<DBWithColumnFamilies> from SelectDBWithCfh, let DB lifetime extend naturally to the last reader. Adds a per-call atomic refcount bump in the hot path; the RWMutex approach is per-method-call instead of per-op, making the hot-path cost zero. Rejected for hot-path neutrality. Reviewed By: xingbowang Differential Revision: D104974784 fbshipit-source-id: 3a04d9e1c0b5042436d690f573cf369de6b4c9df |
||
|
Xingbo Wang
|
e42af37ea7 |
Avoid reusing db_stress listeners across open retries (#14765)
Summary: - Rebuild db_stress event listeners through a shared `InitializeListenersForOpen()` helper. - Reinitialize listeners before retrying `DB::Open()` after injected open or open-compaction failures. ## Context - A stress test failed with "Concurrent compaction of SST file detected". - Root cause: StressTest::Open() built DbStressListener once before its DB::Open() retry loop. With open fault injection, a DB::Open() attempt can create a DBImpl, schedule background compaction, and then fail during late open work such as persisting OPTIONS. During teardown of that failed DBImpl, DBImpl's shutdown flag can suppress later compaction callbacks, leaving listener-local compaction bookkeeping stale. - Diagnosis: Sandcastle DB LOGs showed file 16821 flushed, then a failed open attempt with an injected read error and a background compaction picking 16821. The crash was in DbStressListener::OnCompactionBegin, so this was stale db_stress listener state across open attempts rather than DBImpl allowing a real concurrent compaction of the same SST. - Fix: factor listener construction into InitializeListenersForOpen() and call it before each DB::Open() attempt, including the retry path after open/open-compaction failure. Each DBImpl open attempt now gets fresh listener state. - Verification: make clean; make db_stress -j192; make check-sources; git diff --check. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14765 Test Plan: - `make clean` - `make db_stress -j192` - `make check-sources` - `git diff --check` Reviewed By: pdillinger Differential Revision: D105969381 Pulled By: xingbowang fbshipit-source-id: 759e2be7e1215a498ed449ab36f13e8c7975f4a4 |
||
|
xingbowang
|
54d13c0af9 |
Fix DBTest cleanup for alternate log dirs (#14764)
Summary: - Clean DBTestBase's fixture-owned alternate WAL and db_log_dir paths during setup and teardown. - Prevent kDBLogDir option tests from leaving dbname_/db_log_dir behind and polluting later DBTest cases in the same gtest shard. - Preserve production DestroyDB() behavior while making the test fixture cleanup complete. Context: - The ARM nightly failure was exposed by the 32-shard db_test layout running DBTest.GetPicksCorrectFile before DBTest.PurgeInfoLogs in the same process. - GetPicksCorrectFile can use kDBLogDir, which creates logs under dbname_/db_log_dir. DestroyDB() intentionally ignores DeleteDir() failures when unknown children remain, so the next fixture could observe dbname_ still existing. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14764 Test Plan: - make -j14 db_test - TEST_TMPDIR=/tmp/rocksdb_arm_fix_test ./db_test --gtest_filter=DBTest.GetPicksCorrectFile:DBTest.PurgeInfoLogs - TEST_TMPDIR=/tmp/rocksdb_arm_fix_test_shard GTEST_TOTAL_SHARDS=32 GTEST_SHARD_INDEX=18 ./db_test Reviewed By: mszeszko-meta Differential Revision: D105860476 Pulled By: xingbowang fbshipit-source-id: b2685063ca6c4eedec589697b439fe4aee4eda1a |
||
|
Hui Xiao
|
2904bc64dc |
Encapsulate path access in StressTest via accessors (#14756)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14756 Pure mechanical refactor: replace all direct FLAGS_db / FLAGS_expected_values_dir / FLAGS_secondaries_base reads with accessor methods on StressTest. No new flags, no parameters, no behavior change. Prepares for multi-DB stress test where each StressTest instance has its own DB. Changes: - GetDbPath(), GetExpectedValuesDir(), GetSecondariesBase() accessors return the corresponding FLAGS values directly - Replace ~15 FLAGS_db references with GetDbPath() in db_stress_test_base.cc - Move SharedState constructor from .h to .cc (needs full StressTest type for GetExpectedValuesDir()) - Move DbStressListener constructor from .h to .cc (same reason) - Replace FLAGS_db / FLAGS_expected_values_dir in db_stress_driver.cc with accessor calls - NO changes to db_crashtest.py or db_stress_gflags.cc Reviewed By: anand1976 Differential Revision: D104959943 fbshipit-source-id: d7ef6a39d4c2ed467b2960417629c09f3988faf5 |
||
|
Anand Ananthabhotla
|
88d7d2df75 |
Enable MANIFEST optimization options in db_crashtest.py and add db_stress verification (#14742)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14742 This change enables testing of the two new MANIFEST optimization options introduced in D103568447: 1. optimize_manifest_for_recovery - Skips unnecessary MANIFEST edits during recovery 2. reuse_manifest_on_open - Reuses existing MANIFEST file on DB open Changes: - tools/db_crashtest.py: Add both options with 20% probability to default_params - db_stress_tool/db_stress_test_base.h: Add ManifestVerifyMode enum and member variables for tracking MANIFEST state - db_stress_tool/db_stress_test_base.cc: Implement RecordManifestStateBeforeReopen() and VerifyManifestNotRewritten() methods to validate MANIFEST reuse on DB reopen The verification logic handles 4 combinations: - Both disabled: No verification (baseline) - Only optimize enabled: No verification (hard to measure without sync points) - Only reuse enabled: Verify MANIFEST file is reused - Both enabled: Verify MANIFEST reused AND CURRENT unchanged Verification is warning-only (not fatal) to account for legitimate fallback cases like corruption or size limits. Reviewed By: hx235 Differential Revision: D105224068 fbshipit-source-id: 5baa65680fdd639674d87ff1e9187b743e691bc1 |
||
|
Hui Xiao
|
367f2b0fdf |
Resumable Remote Compaction Blog Post (#14759)
Summary: **Summary:** as titled Pull Request resolved: https://github.com/facebook/rocksdb/pull/14759 Test Plan: local server rendering test <img width="914" height="764" alt="Screenshot 2026-05-19 at 2 18 41 PM" src="https://github.com/user-attachments/assets/0e5353d4-df41-476c-8902-28d7975330d6" /> <img width="914" height="764" alt="Screenshot 2026-05-19 at 2 18 59 PM" src="https://github.com/user-attachments/assets/a0f8692d-6ca8-4c67-b7a0-b47146080c57" /> <img width="895" height="477" alt="Screenshot 2026-05-19 at 2 19 20 PM" src="https://github.com/user-attachments/assets/7479dd54-5bc4-41da-99f3-f081a714ba7c" /> Reviewed By: jaykorean Differential Revision: D105665739 Pulled By: hx235 fbshipit-source-id: 0701a627eb2b18b9bfd3dd22397aeae9553f1903 |
||
|
Peter Dillinger
|
07a5a0a804 |
Fix "too many open files" failures in GitHub CI (#14755)
Summary: seen several times in the build-linux-mini-crashtest job. Raise ulimit in container spec. Task: T271298423 Pull Request resolved: https://github.com/facebook/rocksdb/pull/14755 Test Plan: look at reported ulimits, watch CI Reviewed By: hx235 Differential Revision: D105664569 Pulled By: pdillinger fbshipit-source-id: 6f7f3976bd73ecc86509ac955d5e190316e98ba3 |
||
|
Peter Dillinger
|
01d8c7720f |
Fix crash test failures in OnCompactionPreCommit listener checks (#14753)
Summary: NotifyOnCompactionPreCommit initially omitted the shutting_down_ guard to avoid a false positive abort in the db_stress OnTableFileDeleted check, where stale compacting_files_ entries from skipped notifications would be mistaken for a bug. The better fix is to add the shutting_down_ guard for consistency with Begin and Completed, and instead make OnTableFileDeleted tolerate stale tracking during shutdown by checking a new atomic bool in the listener intended to track DBImpl's shutting_down_. Also fix lint: release mutex before RandomSleep() in PreCommit; use char literal for find_last_of; avoid unnecessary string copy. Document WART in listener.h: all three compaction callbacks are skipped during DB shutdown, so a committed compaction may go unobserved. Bonus: update CLAUDE.md with instructions on avoiding non-ASCII characters Pull Request resolved: https://github.com/facebook/rocksdb/pull/14753 Test Plan: manually trigger many crash test runs Reviewed By: hx235 Differential Revision: D105591913 Pulled By: pdillinger fbshipit-source-id: 32029fea4c2571d88f645eb325db2e25a94e0d26 |
||
|
Hui Xiao
|
cba33621bd |
Fix BackgroundJobPressure flakiness with TEST_WaitForBackgroundWork (#14708)
Summary: BackgroundJobPressure test was flaky because Phase 3 used TEST_WaitForCompact() which waits for bg_compaction_scheduled_ but NOT bg_pressure_callback_in_progress_. The final "healthy" pressure callback could still be in-flight when the test checked snapshots.back(), causing compaction_scheduled=1 instead of 0. Fix: replace TEST_WaitForCompact() with TEST_WaitForBackgroundWork() which explicitly checks bg_pressure_callback_in_progress_ (db_impl.cc:478-484). Also add TEST_WaitForBackgroundWork() before Phase 1 and Phase 2 snapshot checks for consistency, ensuring all pressure callbacks are delivered before assertions. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14708 Reviewed By: jaykorean Differential Revision: D103784101 fbshipit-source-id: 275802b5bb70094af62486bde26b599a292e71fa |
||
|
Hui Xiao
|
554a123295 |
Clean up failed regression test workdirs (#14751)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14751 `regression_test.sh` already cleans the current run's `TEST_PATH` on normal success and on the early-exit path when a recent `db_bench` is still running. But a hard benchmark failure goes through `exit_on_error`, which exits before the end-of-`main()` cleanup runs. This change adds an `EXIT` trap and a single-shot finalizer so the current invocation still cleans its own `TEST_PATH` on hard failure. It does not reintroduce any sibling-directory cleanup, and it preserves the existing success-path cleanup, early-exit cleanup, and debug preservation behavior. | Scenario | Previous code | New code | | --- | --- | --- | | Normal success | Cleans current `TEST_PATH` at end of `main()` | Still cleans current `TEST_PATH` | | Hard benchmark failure via `exit_on_error` | Can leave current `TEST_PATH` behind | `EXIT` trap cleans current `TEST_PATH` | | Early exit because recent `db_bench` exists | Cleans current `TEST_PATH`, exits `2` | Same behavior | | Debug mode / `DELETE_TEST_PATH=0` | Preserves artifacts | Same behavior | Reviewed By: jaykorean Differential Revision: D105411220 fbshipit-source-id: 37a335b87faaeee86d44ef2e24bebf1b7b9626d6 |
||
|
Maciej Szeszko
|
459236341c |
Start development 11.4 (#14747)
Summary: * Release notes from 11.3 branch * Update version.h * Add [11.3.fb](https://github.com/facebook/rocksdb/tree/11.3.fb) (to check_format_compatible.sh) * Update folly commit hash to: https://github.com/facebook/folly/releases/tag/v2026.05.11.00 (see [comment](https://github.com/facebook/rocksdb/pull/14747#issuecomment-4480217173) for more) * Copyright headers refresh * Decouple 11.1.fb from 11.2.fb in `db_forward_with_options_refs` * Add validation step in `check_format_compatible.sh` to prevent gluey release strings Pull Request resolved: https://github.com/facebook/rocksdb/pull/14747 Reviewed By: xingbowang Differential Revision: D105443941 Pulled By: mszeszko-meta fbshipit-source-id: 8499b18cc2ff118d805b3865463ad2a999868de4 |