Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14881
Read-only DB instances can share a directory with a live writer, so their view of live files is only a snapshot from the time they opened. After read-only DB open began setting `opened_successfully_` for its intended open-success semantics, that state had an unintended side effect: the common close path treated a successful read-only open like a read-write open and ran obsolete-file cleanup. If the writer created or made files live after the read-only handle opened, that cleanup could use the stale read-only live set and delete files still needed by the writer.
This change records whether a `DBImpl` was opened read-only and skips close-time obsolete-file cleanup for read-only DBs instead of simply keeping `opened_successfully_=false`, which could be confusing and easily mistaken in the future again.
For completeness I also updated other callsites of `opened_successfully_`, but those should not be real bugs as ReadOnly DBs do not run flushes/compactions or write to WALs.
Reviewed By: xingbowang
Differential Revision: D109622445
fbshipit-source-id: d7be4b20fce86ccb218a63ac6f5b707b316aac79
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
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
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
Summary:
Adds a new `EventListener::OnCompactionPreCommit` callback that fires after a compaction job's output files are written but *before* the manifest write commits the new Version. At that point input files still have `FileMetaData::being_compacted == true`, so listeners that maintain bookkeeping of "files currently being compacted" can clean up that state without racing the compaction picker.
A check implemented by a Meta-internal RocksDB user crashes when the same file appears as input to two concurrent compactions. Tracking that set in `OnCompactionBegin` / `OnCompactionCompleted` produces false positives because `Compaction::ReleaseCompactionFiles()` flips `being_compacted` back to false before `OnCompactionCompleted` fires, so another thread can pick the same file and trigger `OnCompactionBegin` before the previous compaction's `Completed` callback runs. Doing the cleanup in `OnCompactionPreCommit` closes that race. Default implementation is a no-op, so no API break.
A trivial refactoring to split PerformTrivialMove ensures data is populated for the new callback, while calling back before the trivial move compaction is committed.
Bonus: `CLAUDE.md` update for when to call `make clean` as I've recently had it get thoroughly confused TWICE mixing build modes.
Task: T269479969
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14740
Test Plan:
- New unit test in `db/listener_test.cc` checks that `OnCompactionPreCommit` fires strictly between `OnCompactionBegin` and `OnCompactionCompleted` for the same compaction, and that input files still have `being_compacted == true` at the time it fires.
- Crash test: adds a concurrent-compaction sanity check to `db_stress`'s listener resembling the Meta-internal intended usage: tracks input file numbers from `OnCompactionBegin` to `OnCompactionPreCommit`, aborting if the same file appears as input to two concurrent compactions. Also checks other ordering constraints and checks for "leaks" from possible failure to call OnCompactionPreCommit(). Exercises all compaction styles and trivial-move/FIFO paths under load.
Reviewed By: hx235
Differential Revision: D105065326
Pulled By: pdillinger
fbshipit-source-id: 8f606a7c0fac899574e7340c600b71fed902394a
Summary:
- Add experimental immutable `DBOptions::async_wal_precreate` to reserve and open one future WAL on a background HIGH-priority task, with sanitization that disables the optimization when WAL recycling is configured.
- Split WAL creation into open/preallocate and start phases so `SwitchMemtable()` can consume a prepared WAL after writing normal WAL metadata, wait for in-flight precreation, fall back to synchronous creation, and delete an unstarted prepared WAL on start failure.
- Keep WAL numbering, close, recovery, and read-only open safe for empty future WAL files left by async precreation; `error_if_wal_file_exists=true` now rejects non-empty WALs while tolerating empty WALs.
- Add public option plumbing for the C API, options parsing/stringification, random option testing, `db_bench`, `db_stress`, and crash-test configuration.
- Add WAL precreate statistics counters plus Java `TickerType`/JNI mappings, and update C++, C, and Java read-only-open documentation for the empty-WAL behavior.
- Add focused WAL/option/C/Java tests for async precreate ready/wait/failure/recovery paths, read-only WAL detection, option sanitization, and API plumbing, plus write-flow docs and unreleased history entries for the new feature and behavior change.
PR https://github.com/facebook/rocksdb/pull/14738
Reviewed By: pdillinger
Differential Revision: D105020559
fbshipit-source-id: 5059b424702e021abb8de65ceeb6d3b975280ffc
Summary:
**Summary:**
Adds a --listener_uri flag to db_stress that creates an EventListener via ObjectLibrary from the given URI and attaches it to the DB options.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14741
Test Plan:
- Compilation
- e2e test will be done next internally with a customized listener
Reviewed By: anand1976
Differential Revision: D104750476
Pulled By: hx235
fbshipit-source-id: cdc00191de6b7434e4b373db30769ff34d99b80d
Summary:
last_compacted_manifest_file_size_ drives TuneMaxManifestFileSize() to compute the manifest rotation threshold, but it started at 0 on every DB::Open and was only populated after the first manifest rotation. This is really only a problem with reuse_manifest_on_open, because no fresh manifest is created on open.
Add a new forward-compatible (safe-to-ignore) MANIFEST tag kLastCompactedManifestFileSize that records the approximate compacted manifest size at the end of WriteCurrentStateToManifest. During recovery, the value is loaded and used to immediately tune the rotation threshold.
The record includes a rough estimate of its own overhead (~15 bytes) and must be the last record written by WriteCurrentStateToManifest for accurate estimation.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14725
Test Plan:
Extended AutoTuneManifestSize in db_etc3_test to close and reopen with reuse_manifest_on_open after establishing a known auto-tuning state. Verifies that the manifest file number is preserved (no spurious rotation) and that subsequent CF additions don't trigger rotation -- proving the persisted compacted size keeps the tuned threshold correct. Verified the test fails when the recovery loading is disabled.
Relax a fragile Java test that was dependent on the exact size of the manifest file.
SHORT_TEST=1 ./tools/check_format_compatible.sh
Reviewed By: anand1976
Differential Revision: D104464522
Pulled By: pdillinger
fbshipit-source-id: 4f5d22d2e149bd40a523ee11780e5e3344803c19
Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14726
Add SstFileReader::ParseTableIteratorKey() so callers of NewTableIterator() have a public way to decode raw table keys without duplicating RocksDB internal-key layout. The implementation delegates to the existing public ParsedEntryInfo parser using the reader comparator.
Reviewed By: pdillinger
Differential Revision: D104584393
fbshipit-source-id: 98e21c4d6676fbba69e533376b3da67539dd8fad
Summary:
Previously, calling `DB::CreateColumnFamily(opts, "", &handle)` returned `Status::OK()` with a usable handle, but the column family was not persisted in the manifest. Any data written to the empty-named CF was silently lost on DB reopen, and `ListColumnFamilies` would not show it.
The empty string is also reserved as a sentinel meaning "no/unknown column family" in various RocksDB APIs and serialization formats (e.g. `TablePropertiesCollectorFactory::Context::kUnknownColumnFamily` and table properties), so allowing it as a real CF name is ambiguous in addition to being broken.
This change rejects an empty CF name with `Status::InvalidArgument` at the top of `DBImpl::CreateColumnFamilyImpl`, which covers the single-CF `CreateColumnFamily` API as well as both `CreateColumnFamilies` overloads (by-names and by-descriptors).
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14732
Test Plan: * Added `ColumnFamilyTest.EmptyNameRejected` covering all three Create entry points; verifies `IsInvalidArgument()` and that no spurious handles are returned.
Reviewed By: hx235
Differential Revision: D104753911
Pulled By: pdillinger
fbshipit-source-id: e52d792830b965484a618f4e55981eee4eb6f515
Summary:
- propagate lower-level read and merge failures through `GetContext` via `read_status`, so `Get` and `GetEntity` preserve the original error instead of synthesizing `Corruption` when blob-backed reads or merge resolution fail
- teach `GetMergeOperands` to resolve blob-backed default columns from wide-column entities, covering both the direct base-value path and the merge-plus-base path
- add regression coverage for blob-read IO errors during `Get`/`GetEntity` merge resolution and for `GetMergeOperands` on blob-backed wide-column entities
- fix the `DBFlushTest.MemPurgeCorrectLogNumberAndSSTFileCreation` test race by waiting for flush callbacks and cleaning up sync points
## Testing
- `make db_blob_basic_test -j14`
- `/usr/bin/perl -e 'alarm shift; exec ARGV' 60 ./db_blob_basic_test --gtest_filter='DBBlobBasicTest/DBBlobBasicIOErrorTest.GetBlob_IOError/*:DBBlobBasicTest/DBBlobBasicIOErrorTest.GetEntityMergeWithBlobBaseIOError/*'`
## Task
T265824017, T265415808
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14640
Reviewed By: anand1976
Differential Revision: D101690700
Pulled By: xingbowang
fbshipit-source-id: 2b6fc357b37a01efa72a2d54dcff55be8992f42a
Summary:
### Motivation
Claude's auto review workflow classifies the PR as complex and sets `MAX_THINKING_TOKENS`=**32000** in `.github/workflows/ai-review-analysis.yml:534`. The Claude action then sends a request where `thinking.budget_tokens` is **32000**, but the request `max_tokens` is not greater than that, so Anthropic rejects it before the review starts ([example](https://github.com/facebook/rocksdb/actions/runs/25691112276/job/75427435133)).
### Fix
Reduce the "complex" thinking budget from **32000** to **24000** tokens and clamps manual overrides to the same ceiling, preventing the thinking budget from exceeding or equalling the action's effective `max_tokens`. 24k is still generous — enough for deep reasoning on complex PRs while guaranteeing the model can emit the formatted review.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14731
Reviewed By: xingbowang
Differential Revision: D104749426
Pulled By: mszeszko-meta
fbshipit-source-id: bf94d0f50e7c2c9bc9e4d4cdffd61087d739018a
Summary:
The non-ASCII character check in check-sources.sh used git grep -P (Perl regex), which requires git compiled with PCRE support. On systems without it, the command fails with exit code 128, which is != 1 (no match), so the check always reported a violation -- effectively dead.
Even in CI where git has PCRE2 support, the check was silently broken: git grep -P uses PCRE2 in UTF mode by default, which interprets [\x80-\xFF] as a Unicode codepoint range (U+0080 to U+00FF). Characters like em-dash (U+2014), arrows (U+2192), and math symbols (U+2248, etc.) fall outside that range and were not detected. Only Latin-1 Supplement characters (U+0080-U+00FF) would have been caught.
Replace with LC_ALL=C git grep using bash $'[\x80-\xff]' literal byte range, which works with basic regex in the C locale, and replace all non-ASCII characters in non-excluded source files:
- em-dash to --
- arrow to ->
- math symbols to ASCII equivalents (~=, <=, >=)
- box-drawing characters to ASCII art
Also exclude .github/ from the check, as scripts there can use non-ascii without disrupting RocksDB builds on non-UTF-8 systems.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14729
Test Plan: manual / CI (make check-sources passes clean)
Reviewed By: hx235
Differential Revision: D104692574
Pulled By: pdillinger
fbshipit-source-id: 1d884c21056dcd83558b825a04b867f1c08e3f45
Summary:
Previously, `Classify PR complexity (Codex)` ran under `bash -e`, so any `codex exec` failure aborted the entire Codex review before the real review step could run. The classifier only selects the review budget, so on failure we now log the classifier output tail, default to the `complex` review budget, and continue. This keeps actual Codex review failures visible through the existing review exit-code/log handling while preventing the auxiliary classifier from blocking review generation.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14721
Reviewed By: xingbowang
Differential Revision: D104326081
Pulled By: mszeszko-meta
fbshipit-source-id: c17388bbf576f71ff85ded320e0740f89072f8c1
Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14723
### Context
`GetCreationTimeOfOldestFile()` assumed `max_open_files` = -1 meant every live SST had a pinned table reader. That is not true with `open_files_async`: recovery intentionally skips loading table files, `DB::Open()` returns, and `BGWorkAsyncFileOpen()` pins readers later. Any caller — e.g. fb_rocksdb's daily report at `FbRocksDb.cpp:1025` — invoking the API in the window between `DB::Open` returning and the background opener completing trips a debug assert. Reported in https://fb.workplace.com/groups/rocksdb/permalink/31668956482726231/.
Removing the assert alone is insufficient. For legacy DBs whose manifest does not carry `file_creation_time`, `FileMetaData::TryGetFileCreationTime()` falls back to the pinned reader; with no reader, it returns `kUnknownFileCreationTime` and the function silently returns 0 (the "info unavailable" sentinel). The caller cannot distinguish "no info" from "raced with async open."
### Changes
- Remove the invalid debug assert in `Version::GetCreationTimeOfOldestFile`.
- Add a private helper `DBImpl::WaitForAsyncFileOpen()` that blocks on `bg_cv_` while `bg_async_file_open_state_ == kScheduled`. The synchronization machinery (`bg_async_file_open_state_` + `bg_cv_`) already exists — the destructor wait loop in `db_impl.cc:674-687` uses the same pattern. The helper is a no-op when `open_files_async = false`, and bails on `shutting_down_` so `DB::Close()` is not blocked by an in-flight caller.
- Call `WaitForAsyncFileOpen()` at the top of `DBImpl::GetCreationTimeOfOldestFile()` (inside the `max_open_files == -1` branch).
- Document the blocking behavior in `include/rocksdb/db.h`.
- Replace the regression test with one that uses a `DBImpl::WaitForAsyncFileOpen::BeforeWait` sync point: spawn a thread that calls `GetCreationTimeOfOldestFile`, deterministically confirm it blocks inside the wait, release async open, confirm the caller wakes with the real value.
### Potential Followups (not included here)
- Apply the same wait to `GetLiveFilesMetaData` and `GetColumnFamilyMetaData` — both zero out `oldest_ancester_time` / `file_creation_time` in the SST metadata they return during the async-open window (`db/version_set.cc:7877-7878`, `2090-2091`, `2172-2173`).
- Address compaction-picker effects: TTL/periodic file selection (`db/version_set.cc:4039`, `4092-4094`), bottommost over-marking (`db/version_set.cc:4700-4701`), FIFO TTL/temperature pickers (`db/compaction/compaction_picker_fifo.cc:105-107`, `167-170`, `401-411`), and tiered-compaction output time inheritance (`db/compaction/compaction.cc:981`, `1000`).
- Harden `FbRocksDb.cpp:1025` to check `status.ok()` instead of `status.code() != kNotSupported`.
Reviewed By: mszeszko-meta
Differential Revision: D104285992
fbshipit-source-id: ea46375ea1b3ba77fe6b548071aee1101ac0da77
Summary:
- Use the liburing TSAN suppressions in `tools/tsan_suppressions.txt` instead of defining the process-wide `__tsan_default_suppressions()` hook, avoiding conflicts with downstream applications.
- Wire RocksDB TSAN make and crash-test flows to use that suppressions file by default without overriding caller-provided `TSAN_OPTIONS`.
- Cover direct `db_crashtest.py` launches by passing the default suppressions to `db_stress` subprocesses.
- Fix the GCC 16 unity-build warning in `CacheItemHelper` by directly initializing the no-secondary-cache helper fields instead of delegating with `this`.
Imported from D101303486.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14710
Test Plan:
- `COMPILE_WITH_TSAN=1 make -j128 env_test`
- `timeout 60s ./env_test --gtest_filter=EnvPosixTest.IOUringAddressReuseNoTsanFalsePositive`
- `python3 tools/db_crashtest_test.py`
- `python3 -m py_compile tools/db_crashtest.py tools/db_crashtest_test.py`
- CI: `build-linux-unity-and-headers` passed after the `CacheItemHelper` fix
Reviewed By: hx235
Differential Revision: D104103800
Pulled By: xingbowang
fbshipit-source-id: 6066d9abe02a3c44d75f9ce449889468c927ce56
Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14704
Add an immutable DBOption `reuse_manifest_on_open` (default false). When enabled, `DB::Open` can keep using the recovered MANIFEST for the first post-open metadata update instead of rebuilding a fresh MANIFEST, which can reduce warm-open latency for DBs whose MANIFEST is expensive to regenerate.
Reuse is still best-effort. If RocksDB cannot safely resume appending to the recovered MANIFEST, it falls back to the existing fresh-MANIFEST path. The option is also disabled under `best_efforts_recovery`.
This diff also teaches the reopened MANIFEST writer to adopt the existing file size before appending, documents the small-`max_manifest_file_size` caveat for the reused path, and keeps the full warm-reopen composition working with `optimize_manifest_for_recovery`.
Reviewed By: hx235, pdillinger
Differential Revision: D103568447
fbshipit-source-id: f4f5c35ea3ef0b80a0d52d94be40c6bd11505999
Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14703
Extend `optimize_manifest_for_recovery` so a clean `DB::Close` can persist up-to-date WAL recovery markers when that can be done safely. Combined with the recovery-side optimization in the previous diff, a clean close/reopen can avoid recovery-time MANIFEST appends.
This remains best-effort: if the close-time write is disabled, skipped, or fails, RocksDB falls back to the standard recovery path on the next open. The option stays mutable so it can be turned off before close to suppress the optimization without restarting the DB.
The close-time path respects the existing recovery constraints for 2PC, non-empty column families, dropped column families, and WAL tracking, and preserves the existing file-number invariants.
Reviewed By: pdillinger, hx235
Differential Revision: D103568449
fbshipit-source-id: ae62867507a8a87640a2c140bea852b7c608cb66
Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14702
Add a mutable DBOption `optimize_manifest_for_recovery` (default false) as a temporary rollout / kill switch for warm-reopen MANIFEST optimizations.
In this diff, enabling the option lets recovery skip MANIFEST updates during `DB::Open` when the recovered state is already reflected on disk, which reduces metadata appends after a clean shutdown and can lower warm-reopen latency on storage where MANIFEST appends are expensive.
If the option is disabled, RocksDB follows the existing recovery path unchanged. The optimization is disabled under `best_efforts_recovery`, where recovery intentionally rewrites metadata as part of salvage, and the option is mutable so later diffs in this stack can share the same rollout knob.
Reviewed By: pdillinger, hx235
Differential Revision: D103568448
fbshipit-source-id: 9ec930343e434f1bee6130bcdbd7738dddd92b6d
Summary:
This fixes a GCC 16 unity-build failure in `Cache::CacheItemHelper`. The no-secondary-cache constructor delegated to the full constructor while passing this as `without_secondary_compat`. GCC 16 reports that pattern as `-Wmaybe-uninitialized` because the delegated constructor receives a pointer to the object under construction and its debug assertions dereference it. Since RocksDB builds with `-Werror`, the warning is promoted to a build error. We see it now because the unity job uses `gcc:latest`, which now has `GCC 16.1.0`. The code pattern itself dates back to PR https://github.com/facebook/rocksdb/issues/11299 / ccaa3225b from 2023.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14713
Reviewed By: xingbowang
Differential Revision: D104162382
Pulled By: mszeszko-meta
fbshipit-source-id: d6cb579e37bed0d833c6427c3b0541c857425d42
Summary:
Add MLIR-compatible PrefixVarint32/64 helpers in util/prefix_varint.h. Document the format, split decode API, and hot paths, and cover the codec in coding_test with round-trip, disk-read, overflow, and truncation cases.
I'm intending to use this in the new blog file format because you only need to read the first byte to know how many varint bytes to read total, and it might be more CPU efficient in other uses as well (to be determined in follow-up work).
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14692
Test Plan: Unit tests included
Reviewed By: joshkang97
Differential Revision: D103245079
Pulled By: pdillinger
fbshipit-source-id: dca435beccb6e666d864a31d8683ad19e2121c34
Summary:
Reduces per-step heap allocations in the external table iterator's hot path
by replacing the `InternalKey` member of `ExternalTableIteratorAdapter` with
`IterKey`. `InternalKey` stores its bytes in a `std::string`, which
heap-allocates whenever a key exceeds the small-string threshold. `IterKey`
keeps a 39-byte inline buffer and only spills to the heap for larger keys.
`UpdateKey` runs on every `Next` / `Prev` / `Seek*`, so this swap removes a
recurring per-step allocation for typical key sizes.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14695
Test Plan: CI
Reviewed By: anand1976
Differential Revision: D103440780
Pulled By: joshkang97
fbshipit-source-id: deb0a8ea04110e3dff0a1bcad767e7136192fdc6
Summary:
See SC crash test failure 2729181374202803470.
Flush/compaction retention was protecting only explicit snapshots, while readers without an explicit snapshot were still bounded by GetLastPublishedSequence(). That mismatch becomes observable in WRITE_COMMITTED when commit_bypass_memtable runs: a newer version can already be installed into WBWI-backed immutable memtables before SetLastSequence() publishes it. This can happen with two-write-queue enabled OR disabled.
Example setup:
- K@P = "old_value" is the current published version of key K.
- K@U = "new_value" is written later with U > P.
- commit_bypass_memtable ingests K@U into immutable memtables, but publication is still at P.
- flush starts in that window and builds its snapshot context.
Before this change, if there was no explicit snapshot at P, flush/compaction was free to collapse K@P under K@U. A reader at the published boundary P then saw neither version: K@P had been discarded, while K@U was still too new, so Get(K) returned NotFound.
A simple solution is to add a "fake" snapshot boundary for pessimistic write committed txns.
NOTE: A behavioral side effect is that this will prevent a merge operand at snapshot seqno from being filtered via compaction filter because of the new managed snapshot. But this keeps it aligned with the behavior that write prepared and write unprepared have.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14681
Test Plan: Regression test that fails without fix, specifically K@P gets deleted and a snapshot read returns NotFound, when it should exist.
Reviewed By: hx235
Differential Revision: D102856943
Pulled By: joshkang97
fbshipit-source-id: 46cb3936e9d4e771966d39fd0d15436335bf6ccf
Summary:
This commit fixes several potential issues introduced by cc8d9ea04 (CI: AI review workflow improvements) that broke the agent review jobs in GitHub CI.
1. Codex workflows fail when OPENAI_API_KEY is not configured:
- Added Codex review workflows fail with exit code 1 if the API key secret is missing
- Added script-level checks to skip Codex steps gracefully when OPENAI_API_KEY is unset, avoiding CI noise
2. Missing pull-requests: read permission in manual-review job:
- The manual-review job calls github.rest.pulls.get() but lacked the required permission
- Added 'pull-requests: read' to the manual-review job permissions
3. Potential crash when headSha is undefined:
- parse-claude-review.js and parse-codex-review.js accessed meta.headSha.substring() without null check
- Added defensive checks to handle undefined headSha gracefully
4. Claude model claude-opus-4-7 incompatible with thinking API:
- The commit upgraded default model to claude-opus-4-7, but this model doesn't support the 'thinking.type.enabled' API used by the Claude Code action
- Error: 'thinking.type.enabled' is not supported for this model. Use 'thinking.type.adaptive' and 'output_config.effort' to control thinking behavior
- Reverted default model to claude-opus-4-6 and removed claude-opus-4-7 from options
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14696
Test Plan: Created a draft PR with this change and draft production code changes, including a branch on facebook/rocksdb. Go to https://github.com/facebook/rocksdb/actions/workflows/claude-review.yml and choose "Run workflow" with that branch and PR. (Changes in the PR do are not picked up for the agent code review workflows; the only way to test what is outside of main is with a branch on facebook/rocksdb)
Reviewed By: joshkang97
Differential Revision: D103453631
Pulled By: pdillinger
fbshipit-source-id: 15a82257b5762e9e0b1b393dc45c4b343c866f7a
Summary:
Two CPU-side optimizations to `DBIter`'s hot iteration path.
1. `PrepareValueInternal` always re-parses the internal key after `iter_.PrepareValue()`, even though most inner iterators report their value was already prepared and cannot have moved `iter_.key()`.
2. `ResetValueAndColumns` and `ResetBlobData` unconditionally clear wide-column / blob state at the top of every `Next()`, even on plain-value scans where neither has been populated since the last reset.
Both wins are pure CPU on hot, fully-cached workloads. The dirty-flag bookkeeping introduced for (2) is extracted into a small reusable `DirtyTracked<T>` utility so the same pattern can be reused for any T whose `Reset()` is non-trivial.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14684
Test Plan:
**Benchmark — `db_bench seekrandom`.** Each op = 1 Seek + 100 Next on a fully cached 10M-key DB (16 B key, 100 B value, no compression).
| | ops/sec |
|---|---:|
| BEFORE | 42,131 |
| AFTER | 45,269 |
| **Δ** | **+3,138 (+7.4%)** |
Reviewed By: xingbowang
Differential Revision: D103070805
Pulled By: joshkang97
fbshipit-source-id: 6b184a1aa559649c575d95be28ab43bbee9ffe64
Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14685
D100235293 and D102823090 migrated the generated RocksDB BUCK file from
`//folly/experimental/coro:*` to `//folly/coro:*`, but did not update
the buckifier script that generates it in internal_repo_rocksdb. The next release would revert the change.
Update buckify_rocksdb.py to match, so the generated BUCK file stays consistent with the folly coro migration.
Reviewed By: nmk70
Differential Revision: D103096688
fbshipit-source-id: 9055769ed5e9893397c7504ada22e21980f59dd2
Summary:
- Upgrade the Claude review workflow models and add complexity-based thinking budget selection for the main review run.
- Keep AI review comments tied to the reviewed commit and restructure long reviews so high-severity findings stay visible while details live in a collapsible section.
- Add early auto-trigger gating plus Codex review/comment workflows, including shared comment-building and parsing helpers.
## Testing
- Not run. The branch refresh was a no-op rebase onto current `upstream/main`, and no new code changes were made during this task.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14659
Reviewed By: pdillinger
Differential Revision: D102224743
Pulled By: xingbowang
fbshipit-source-id: 39c25494062cbcd52d3e85f8a859b5a3907c758e
Summary: Migrate coro references across the repo to the non shim version.
Differential Revision: D102823090
fbshipit-source-id: 3bec59aed1f2645139a1ad9c18363e6af462e61f
Summary:
The previous pre-push hook auto-formatted, committed, and re-pushed on behalf of the user. This was fragile: it's not clear whether the format fix should amend the current commit or create a new one. Adding a commit breaks populating the summary for creating a new PR. Also, the hook's internal re-push would drop flags like --set-upstream from the original command. Replace with a simple check-only approach that blocks the push and tells the user what to fix.
Also add a new check for untracked source files (.cc, .h, .py, etc.) in tracked directories (excluding third-party/). These typically indicate files that were forgotten in the commit, which would cause the pushed code to fail to build.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14680
Test Plan: manual
Reviewed By: xingbowang
Differential Revision: D102855227
Pulled By: pdillinger
fbshipit-source-id: 7dc2c4e7a2b2c392bf8da74d7ea43883c8c075a9
Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14676
When fast_sst_open is disabled, RocksDB was still passing previously-persisted
file_open_metadata from the MANIFEST to NewRandomAccessFile. This could cause
failures when the metadata becomes stale (e.g. expired filesystem credentials).
This change gates the consumption of file_open_metadata in
TableCache::GetTableReader on the fast_sst_open option. When fast_sst_open is
false, previously persisted metadata is ignored and not passed to the filesystem
via FileOptions::file_metadata.
The fast_sst_open flag is threaded from MutableDBOptions through VersionSet ->
ColumnFamilySet -> ColumnFamilyData -> TableCache at construction time, ensuring
the gate is active before any table readers are opened during recovery. Dynamic
changes via SetDBOptions are also propagated to all existing TableCache instances.
Reviewed By: mszeszko-meta, xingbowang
Differential Revision: D102735581
fbshipit-source-id: 9a2c4dc0644a2f65c36b2468605df57779e127cd
Summary:
Fix false internal warnings about skipped tests (seen on D102718613).
ROCKSDB_GTEST_SKIP signals a gap in test coverage due to the build/execution
environment, while ROCKSDB_GTEST_BYPASS is for intentionally and permanently
omitting certain parameterizations. These six tests skip based on the test
parameterization (forward vs reverse, primary vs secondary mode), not due to
any environment limitation, so BYPASS is the correct macro.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14679
Test Plan: these are tests, look at internal signals
Reviewed By: joshkang97
Differential Revision: D102833346
Pulled By: pdillinger
fbshipit-source-id: e57bd1e217ca63aaf418b68c788563e67bbb07e3
Summary:
- External Table is only PUTs with no seqno, so we can route it to the simpler more efficient `GetContext::SaveValue`.
- `SstFileReader::Get` was allocating a string to append key footer, instead use LookUpKey to allocate on stack for short keys.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14673
Test Plan: CI passes
Reviewed By: anand1976
Differential Revision: D102659533
Pulled By: joshkang97
fbshipit-source-id: e55127548c9e4b7bdeb63c46acfdb8eb5883db14
Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14672
This fixes the forward path introduced in the original pull request: https://www.internalfb.com/diff/D102409405
That change tried to avoid using user-owned `iterate_upper_bound_` memory by reading `iter_.key()` when the internal iterator was still valid and using it as the exclusive end key for the synthetic range tombstone. That is not safe: after the upper-bound break, `iter_.key()` is only the current merged child position, not a key the bounded scan proved safe to read or use as a range endpoint.
Example: run a prefix-bounded scan over prefix `b` with tombstones `ba`, `bb`, `bc`, `iterate_upper_bound = "bd"`, live key `be` in an SST, and later same-prefix key `bz` in the memtable. The SST child can drop out when it notices `be >= bd`, while the memtable child is not upper-bound aware and can still surface `bz`. If cleanup uses `iter_.key() == "bz"`, it inserts `[ba, bz)` and covers the live `be`, even though that key was never safe for the bounded scan to reason about.
The fix is to always end the forward range at `saved_key_`, which is the last tombstone user key the iterator actually proved deleted. That is conservative but safe. The regression test builds the mixed SST/memtable example above and asserts that the live `be` remains visible. The diff also re-enables randomized `min_tombstones_for_range_conversion` in `db_crashtest.py` and forces it back to `0` when `use_multiscan == 1`, since multiscan still does not support read-path range conversion.
Reviewed By: xingbowang
Differential Revision: D102493855
fbshipit-source-id: 45bdb86464268e2c4fa2ea735dbb059a5e054f28
Summary:
Replace raw owning pointers with std::unique_ptr for StreamingCompress/Uncompress in Create functions and log::Writer/Reader, eliminating manual delete calls in destructors. Also update the StreamingCompressionTest to use unique_ptr for its local StreamingCompress, StreamingUncompress, and MemoryAllocator objects.
Bonus: improve error message for missing/broken ruby (new build requirement)
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14665
Test Plan: ./log_test - all 211 tests pass.
Reviewed By: xingbowang
Differential Revision: D102381893
Pulled By: pdillinger
fbshipit-source-id: 02806b46c8371474a969d6fa3012fff5eb7886c3
Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14662
When a file becomes obsolete, `ReleaseObsolete()` calls `ReleaseAndEraseIfLastRef()` to remove it from the table cache. However, if concurrent readers hold references to the cache entry, this is not the last ref, so the entry stays. When those readers later call plain `Release()`, the entry becomes unreferenced but remains in the cache — there is no mechanism to trigger cleanup.
The fix is to call `cache->Erase()` in `ReleaseObsolete()` instead of relying solely on `ReleaseAndEraseIfLastRef()`. `Erase()` marks the entry as Invisible in the cache. The cache's `Release()` implementation already checks `IsInvisible()` and erases Invisible entries when the last reference is released. This guarantees that obsolete file entries are cleaned up regardless of concurrent reader timing.
Also reverts the D102158618 workaround that reordered `EraseUnRefEntries()` before `TEST_VerifyNoObsoleteFilesCached` in `CloseHelper`. With the root cause fixed, the assertion correctly passes in its original position.
Reviewed By: pdillinger
Differential Revision: D102158618
fbshipit-source-id: 5796abee916dfbd99554a36d3bbf579842d2fb8b
Summary:
Seems like crashtests are failing more often now, disabling again to investigate.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14670
Reviewed By: archang19
Differential Revision: D102479287
Pulled By: joshkang97
fbshipit-source-id: 3f147f58b0cf2901ffe3b99e8681ed5a6085e7c8
Summary:
`blob_compression_opts` is dynamically changeable through `SetOptions()`, but
blob direct write cached its per-thread compressor by `CompressionType` only.
After updating `blob_compression_opts`, new blob records on an existing writer
thread could keep using stale compression settings.
This change threads `CompressionOptions` through the direct-write settings and
rebuilds the cached per-thread compressor whenever the published options for
that compression type change. When the options stay the same, the steady-state
cache behavior is unchanged.
It also adds a regression test that toggles the ZSTD checksum flag through
`SetOptions()` and inspects the raw blob records to confirm each direct-write
record used the latest compression options.
## Testing
- `timeout 60s ./db_blob_direct_write_test --gtest_filter='DBBlobDirectWriteTest.DirectWriteCompressionOptionsUpdateRebuildsCachedCompressor' --gtest_repeat=5`
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14669
Reviewed By: jainraj91
Differential Revision: D102423376
Pulled By: xingbowang
fbshipit-source-id: 504243d72cf22e62097f7e88ac1cc36c5bf29eee
Summary:
kNoChecksum was not being tested in db_crashtest.py despite being a supported checksum type. Add it to the random selection to ensure crash test coverage of the no-checksum code path.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14667
Test Plan: Ran multiple blackbox and whitebox crash test validation runs with kNoChecksum forced and with random selection including kNoChecksum. All passed crash-recovery verification.
Reviewed By: hx235
Differential Revision: D102404837
Pulled By: pdillinger
fbshipit-source-id: 0120f1269c917dd6e347764f2bdc0095d46ff71a
Summary:
`DBIter::FindNextUserEntryInternal` previously used `iterate_upper_bound_` (user-controlled memory) as the exclusive end-key when flushing an accumulated tombstone run. That pointer is dangerous as it points to user memory space, which can be modified at any moment.
This PR removes that dependency: the end-key is always derived from RocksDB-owned data — either the live `iter_` key that broke the run, or `saved_key_` when `iter_` is exhausted.
The tradeoff here is that there is potentially 1 less tombstone to get covered in this path, which is reasonable.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14664
Test Plan: Updated unit tests, CI passes
Reviewed By: xingbowang
Differential Revision: D102409405
Pulled By: joshkang97
fbshipit-source-id: b2beed73e5fe5b955b2d782fc467eda637c4fcf2
Summary:
CONTEXT: The manifest validation on close feature (verify_manifest_content_on_close) detects corruption but does not increment any statistics counter, making it harder to monitor in production.
WHAT: Add a new ticker MANIFEST_VALIDATION_FAILURE_COUNT that is incremented each time content validation detects manifest corruption during DB::Close(). The counter fires per corruption detection, so it can increment up to 2 times per close (once on initial check, once after rewrite attempt). Updated all existing manifest validation tests to verify the counter value.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14657
Test Plan:
- All 7 manifest validation tests pass with new stat assertions
- 5x repeat with COERCE_CONTEXT_SWITCH=1 shows no flakiness
- Full version_set_test suite (212 tests) passes
Reviewed By: anand1976
Differential Revision: D102404260
Pulled By: dannyhchen
fbshipit-source-id: 21a0aa1ad8de12a935caf5642e41ccf2a47b46d9
Summary:
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14658
Follow up to D101463511. Add a hook and a manager-aware overload of so custom CompressionManagers can provide human-readable names for custom compression types while preserving the existing generic fallback when no compatible manager is available.
Reviewed By: pdillinger
Differential Revision: D102201365
fbshipit-source-id: 0c7456bb9db2e54927a4349d12c035fc8b5ad562
Jay Huh