mirror of
https://github.com/facebook/rocksdb.git
synced 2026-10-23 08:09:02 +03:00
main
14217 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
cm-sally-mate
|
94c7f76da0 |
Keep read-path tombstone conversion from deleting ingested files an older iterator cannot see (#15461)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15461 **What:** With `min_tombstones_for_range_conversion` set, an iterator whose view predates an ingestion can delete the ingested keys for good. **Why it matters:** Silent, permanent data loss. `Get` returns the ingested key right after ingestion. After one scan from the older iterator, `Get` returns NotFound, also after flush and reopen. Every MyRocks bulk load with the option set takes this path: MyRocks ingests with `allow_global_seqno=false` and `snapshot_consistency=false` (seqno 0). **Root cause:** - The older iterator does not see the ingested file. It sees the deleted keys on both sides of the file as one run, and inserts a range tombstone at its read sequence number. The tombstone covers the ingested keys. - `ingest_seqno_barrier_` (D102044512) refuses a conversion only when the iterator's sequence number is below the ingested seqno. That misses two cases: 1. The file gets no new seqno: no overlap (seqno 0), `ingest_behind`, or `allow_db_generated_files`. Ingestion does not move the barrier at all (`assigned > 0`). 2. `NewIterator()` references the SuperVersion before it reads the sequence number. If an ingestion and a write happen in between, the iterator reads at a sequence number above the ingested file, with a SuperVersion that does not have it. This also loses files ingested with a new seqno. - Ingestions that flush the memtable are safe: the older iterator's memtable becomes immutable. **Fix:** Barrier on the Version, not only on the sequence number. - After `LogAndApply()`, ingestion records the new Version number on the active memtable (`ingest_version_barrier_`), while it still holds the per-CF `ingest_sst_lock`. - `DBIter` passes the number of the Version it reads. `AddLogicallyRedundantRangeTombstone()` refuses the conversion if that number is below the barrier. - Version numbers only go up, and every later Version of the column family has the ingested files. So the check is true exactly when the iterator cannot see an ingestion into this memtable. Iterators created after the ingestion convert as before. - The seqno barrier stays: it still covers old explicit snapshots that read a newer Version. **Related:** D126796014 (`Disable tombstone conversion for kMemtableTier iterators`) fixes a different trigger of the same feature. Reviewed By: joshkang97, archang19 Differential Revision: D126875720 fbshipit-source-id: 5767b866c6174d791f9ddae78311816300ac49f3 |
||
Yoshinori Matsunobu
|
d6a7847d37 |
Add a DB option to log long-held global mutexes and release sites (#15436)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15436 ## Why Long-held global mutexes can stall foreground and background work. Existing mutex statistics measure acquisition waits, not continuous ownership. This adds opt-in logging to identify the mutex, owning thread, operation and release site. ## Enable for an investigation Provide an INFO logger and shared `Statistics` object when opening the DB, then: - Set the shared statistics level to `StatsLevel::kAll`. - Set `mutex_hold_log_threshold_us` through `DB::SetDBOptions`; `100000` means 100 ms. - Restore threshold 0 and the previous statistics level afterward. The threshold defaults to **0 (disabled)**. `kAll` alone does not enable these hold probes; threshold 0 leaves existing statistics timers unchanged. ## Output Illustrative records, not production observations: ``` SLOW_GLOBAL_MUTEX mutex=DBImpl::mutex_ mutex_addr=0x12340000 owner_thread_id=12345 thread_operation=Compaction hold_us=200000 threshold_us=100000 observed_end_us=1792205400000000 buffer_dropped_records=0 unlock=db_impl_compaction_flush.cc:4660 BackgroundCallCompaction() SLOW_GLOBAL_MUTEX mutex=DBImpl::wal_write_mutex_ mutex_addr=0x12340100 owner_thread_id=12346 thread_operation= hold_us=35000 threshold_us=25000 observed_end_us=1792205401000000 buffer_dropped_records=0 scope_exit=db_impl_write.cc:2250 PreprocessWrite() ``` The first proves a completed 200 ms DB-mutex ownership interval on a thread tagged `Compaction`. The second uses a 25 ms threshold; an empty operation tag means none was available. Neither record reports acquisition wait or proves the cause of a stall. Release attribution uses compiler-embedded strings available in optimized, stripped builds, with a GCC/Clang fallback where standard source locations are unavailable. Direct unlock and condition wait identify their call sites; scope exit and temporary unlock identify guard declarations, not closing braces. No stack unwinding or symbol lookup. ## Scope and safety - Track six selected mutexes: DB, WAL, point-lock wait graph, two prepare-log bookkeeping mutexes and transaction name map. This is not comprehensive synchronization tracing. - Acquisition waits and condition-variable release gaps are excluded from hold duration. Configuration is pre-first-use; repeated setup retains the first valid context. - Sample at most once per mutex per second, with eight deferred records per thread. `buffer_dropped_records` counts capacity overflow only; rate limiting, pause and emission contention can omit samples. - Emit after covered outer lock scopes release. Emission is nonblocking best-effort; shutdown drains/pauses emission and stops diagnostics before the owned logger closes. Closing from that logger's own callback is unsupported. - No data-file format change. The mutable option is persisted in OPTIONS and wired into bench/stress tools. Typed C binding is deferred; string DB-option APIs are available. Disabled bookkeeping still has a cost. Release A/B measurements found about 3 ns added per selected mutex pair at level 1, and about 70 ns for hold timing over `kAll`/0. Get and reused-iterator Seek showed no resolved small probe effect; fresh iterator+Seek at 16 threads showed +8.7% CPU/query and -9.4% QPS. No logs were emitted in those benchmarks; logging cost and broader workload qualification remain open. Reviewed By: xingbowang Differential Revision: D125790955 fbshipit-source-id: 1096163f3bd25ffee585dc6ec8ebfd7f1759851a |
||
Andrew Chang
|
431ce83b9b |
Disable tombstone conversion for kMemtableTier iterators (#15451)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15451 Fix a data loss bug in read-path range tombstone conversion. Why the change is required: With `min_tombstones_for_range_conversion` > 0, a DBIter that scans a run of contiguous point tombstones inserts a range tombstone `[first_tombstone_key, next_live_key)` at its read sequence number into the active memtable. This is only logically redundant if the iterator observed every live key inside that range. An iterator created with `ReadOptions::read_tier = kMemtableTier` reads only the mutable and immutable memtables (`DBImpl::NewInternalIterator` adds no SST iterators for that tier), but DBIter still enabled conversion for it: the gates were the active memtable, `table_filter`, prefix mode, and timestamp visibility, not the read tier. Previous behavior: A live key that exists only in SST files and sorts between memtable tombstones is invisible to a memtable-only scan, so the scan treats the tombstones around it as contiguous and inserts a range tombstone covering it. The key is then deleted for all reads at or above the iterator's sequence number, and the deletion persists through flush and compaction. Example with `min_tombstones_for_range_conversion = 2`: `Put(c)`, `Flush()`, `Put(a)`, `Delete(b)`, `Delete(d)`, `Put(e)`; a `kMemtableTier` scan returns `a, e` and inserts `[b, e)`; afterwards `Get(c)` returns NotFound. Both forward and reverse scans trigger it. Affected: column families with `min_tombstones_for_range_conversion` > 0 (EXPERIMENTAL, default 0) that create iterators with `read_tier = kMemtableTier`. Not affected: `kBlockCacheTier` (the scan stops with `Incomplete` before a run can be flushed, already covered by `NoInsertionOnBlockCacheTierIncomplete`), `kPersistedTier` (not supported for iterators), and all other iterators. New behavior: Conversion runs only for iterators with `read_tier = kReadAllTier`, and stays disabled for the iterator lifetime for every other tier. This also turns it off for `kBlockCacheTier` scans that are served fully from cache, which could convert before. The option documentation now lists reads with `read_tier` other than `kReadAllTier` among the configurations where conversion is skipped. Implementation: One more condition, `read_tier == kReadAllTier`, in the DBIter constructor's `min_tombstones_for_range_conversion_` gate, next to the existing `table_filter` and timestamp-visibility checks. Release note added. Safety, compatibility, and limitations: No API, format, or default changes. The change only drops a logically redundant optimization for read tiers that cannot guarantee a complete scan. A new test checks that a range tombstone inserted by a full-tier scan does not change what later `kMemtableTier` and `kBlockCacheTier` reads return. The only difference it found: with a cold block cache, a `kBlockCacheTier` `Get` of a covered key returns NotFound (the full-tier answer) instead of `Incomplete`. Reviewed By: joshkang97, jaykorean Differential Revision: D126796014 fbshipit-source-id: 70f5184dbf5bb9eff765c232ac130ab43379a814 |
||
Peter Dillinger
|
539532377d |
Remove Solaris support (#15449)
Summary: Solaris support was contributed by outside users starting with |
||
zaidoon
|
553b52b4a7 |
Wire IndexFactory through block-based tables (#14963)
Summary: Build block-based table indexes through `IndexFactory`, with modes for writing both indexes or only the custom index and a per-read index selector. Built-in reads retain their existing cache and prefetch paths. The review fixes preserve the existing UDI behavior at the migration and rollback boundaries: - Reopening an SST with index caching enabled reuses the serialized custom block. Each table reader constructs its own factory reader over those bytes and passes `NewReader()` a `Slice` view stored in the table reader. That view stays valid for the reader's lifetime, so a legacy reader can advance it or keep a pointer to it without changing the cached block. The view and the raw block are destroyed after the custom reader. - `IndexFactoryReader::ApproximateMemoryUsage()` keeps its documented meaning: the reader's own allocations, excluding the serialized block. `MemoryUsageIncludesIndexBlock()` defaults to false. A reader whose estimate includes the block opts in, and the wrapper subtracts the block size from that estimate. - Copying factory options and clearing a legacy UDI flag now rolls back that flag's policy. Legacy-derived options keep the requested default and resolve their effective mode without changing the inputs. Explicit factory modes retain precedence through cloning, failed updates, reconstruction, and the C API, including an explicitly selected `kStandardDefault`. - OK empty custom-index output still produces a zero-size meta block and reads through the full standard index. This covers point data and files containing only range tombstones. Custom-only mode rejects empty output because it has no standard fallback. - Default primary reads retain the table factory's precedence over a legacy per-read factory pointer. Explicit `kPreferCustom` still checks a supplied factory name; `kBuiltin` ignores that pointer. - OPTIONS files write equivalent legacy booleans for `kStandardDefault`, `kStandardRequired`, and `kCustomDefault`. Older releases retain those policies when loading newer files with `ignore_unknown_options=true`; the explicit-mode marker remains authoritative for current readers. Custom-only SSTs carry a footer feature bit so older readers reject them. Reject unsupported filter/index combinations before constructing filters. Trie size estimates account for shared prefixes, and block offsets or sizes beyond the 32-bit format limit return `NotSupported` instead of truncating. ## Safety and compatibility - Custom-only files require format version 6 or later and the factory. Rewrite every custom-only SST with a full standard index before removing the factory or downgrading. The other modes have no equivalent legacy configuration beyond the three listed above. - A raw C++ options struct cannot distinguish an explicitly assigned `kStandardDefault` from its default value. Configure the factory to make that choice explicit. `GetEffectiveIndexMode()` reports the resolved policy while preserving legacy inputs for copy-and-clear rollback. - A custom factory with user-defined timestamps is rejected in every mode except `kStandardOnly`, which ignores the factory. - Trie construction rejects a data-block offset or size greater than `UINT32_MAX`, including in `kStandardDefault`. That can fail flush, compaction, or `SstFileWriter::Finish()`; total SST size above 4 GiB alone is not the rejection condition. File-size targets do not split L0 or `SstFileWriter` outputs. - Custom-index cache hits, misses, and adds are reported under `BLOCK_CACHE_INDEX_*` instead of `BLOCK_CACHE_DATA_*`. The existing cache setting still controls ownership and cache use. - A table reader now charges a custom index block it owns (`cache_index_and_filter_blocks=false`); before, that block was not charged anywhere. Cached blocks are still charged only to the block cache. A reader whose `ApproximateMemoryUsage()` includes the serialized block, such as IntegerTrie or TimeSeries, must override `MemoryUsageIncludesIndexBlock()` to return true, or that block is charged twice. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14963 Test Plan: For this commit, on macOS: `table_test`, `trie_index_test`, `trie_index_db_test`, and `block_based_table_reader_test` pass on the full stack (11,416 cases), and the full `table_test` passes at this PR's head alone (6,969 cases). The four custom-reader compatibility tests pass 100 times with `ASSERT_STATUS_CHECKED=1` and `COERCE_CONTEXT_SWITCH=1`, and 84 UDI table cases pass under ASAN with stack-use-after-return detection. Release-mode syntax checks with `-DNDEBUG -fno-rtti` on the changed sources, source checks, formatting checks, and `git diff --check` pass. Two new regressions cover the latest review, and both were checked failing before the fix: - `RetainedReaderSliceOutlivesTableOpen`: a reader keeps a pointer to the `Slice` it received, the stack is overwritten after open, and the reader then serves a seek and a full scan. It covers all three custom-index modes with owned and cached blocks, including a cached reopen. Without the fix, the retained `Slice` no longer matched the block. - `AuxiliaryReaderMemoryIsFullyCharged`: a reader that implements only the older reader methods reports a fixed allocation of half and twice the raw block size. Table usage must grow by that amount with owned and cached blocks, and a strict cache sized just below the required reservation must reject the open with `MemoryLimit`. Without the fix, usage fell short by `min(allocation, raw block size)` and the strict-cache open succeeded. The consuming-reader and strict-cache accounting tests are kept, now on shared helpers. From the previous revision: 498 focused C++ cases passed (82 table/factory, 156 trie, and 260 trie DB), along with the C API test and generated C API verification. Coverage includes both comparators, copied-options rollback, C factory reconstruction, explicit modes, OPTIONS serialization, SST Get/iteration, point and range-only flushes, compaction, reopen, cache disabled/enabled/evicted, and legacy factory precedence. The rollback, empty-output, primary-selector, and OPTIONS regressions were checked failing before their fixes. The cache test measures file reads at the custom block's actual offset: one on a cold open, zero on cached reopens, and one after eviction or with caching disabled. Disabling custom-block cache reuse makes that assertion fail. The full suite and Linux/Windows/TSAN coverage are left to CI. Earlier full-suite runs had failures/timeouts (173/2095 jobs normally and 152/2088 with Status checking); that broad suite was not rerun for this commit. ## Series Previous: https://github.com/facebook/rocksdb/issues/14962 (landed through import). Next: https://github.com/facebook/rocksdb/issues/14965. Reviewed By: jaykorean Differential Revision: D122796629 Pulled By: xingbowang fbshipit-source-id: 4c9d6f3f548c588a37b08fd94de25e8c0b5ef743 |
||
Xingbo Wang
|
81369fc170 |
Fix async secondary catch-up on deleted SSTs (#15438)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15438 Fix a RocksDB secondary with `open_files_async = true` latching on `PathNotFound` during `TryCatchUpWithPrimary()`. **Why:** With `open_files_async = true` and `max_open_files = -1`, a secondary's catch-up could fail with `PathNotFound` on a table that was live at open time (most reachably in a non-default column family) but which the primary compacted away before the background async opener pinned it. The error latched in the tailer's status, so the secondary never advanced again and had to be closed and reopened. With `open_files_async = false` the same sequence caught up fine. This blocks enabling async secondary open where it would otherwise cut cold-open time substantially. **Previous behavior:** In `VersionEditHandlerPointInTime::MaybeCreateVersionBeforeApplyEdit`, a negative-edge `LoadTableHandlers()` failure skipped the unrecoverable point-in-time only when the status was `Corruption`; a `PathNotFound` was returned as-is and latched. During a secondary's catch-up, `open_files_async` leaves recovery-time tables unpinned until the background opener reaches them, so `LoadTableHandlers()` could try to open a file the primary had already deleted. **New behavior:** That negative-edge failure path now also treats `PathNotFound` as a missing file (skip this point-in-time and continue) when `no_error_if_files_missing_` is set, mirroring the tolerance `VersionEditHandler::LoadTables()` already applies. Catch-up continues past the transient state and installs the current version, so the secondary advances instead of latching. **Implementation:** - `db/version_edit_handler.cc`: broaden the negative-edge skip condition in `VersionEditHandlerPointInTime::MaybeCreateVersionBeforeApplyEdit` from `s.IsCorruption()` to also include `s.IsPathNotFound() && no_error_if_files_missing_`. - `db/db_secondary_test.cc`: add `DBSecondaryTest.OpenFilesAsyncNonDefaultCfCatchUp`, a regression test that opens an async secondary with a non-default column family, holds the background opener back so the open-time table stays unpinned, has the primary flush and `CompactRange(kForce)` to replace and delete that table, then asserts `TryCatchUpWithPrimary()` succeeds and reads the new value. **Safety boundaries:** The change only affects point-in-time handlers that opted into missing-file tolerance (`no_error_if_files_missing_`, set by secondary/follower tailing and best-efforts recovery); it does not relax error handling for strict recovery. It does not change the `trust_manifest_recovery_` / `defer_sst_file_opening_` paths, which already skip opening. **Validation and limitations:** The regression test covers async secondary catch-up after deletion of an open-time SST in a non-default column family. Validation covers async opening, catch-up, memtable retention, and repeated scheduling changes. Strict recovery, API and file formats are unchanged. The full supported platform matrix has not been run locally. Reviewed By: jaykorean Differential Revision: D125378257 fbshipit-source-id: 3dd23c2f763e4e744c9f38fb77dfdfb3f554ee7f |
||
Josh Kang
|
babce11705 |
Add public internal key encoding utilities (#15396)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15396 ## Summary `ParseEntry()` exposes the components of an internal key, but constructing one with a chosen sequence number and entry type currently requires private RocksDB utilities or duplicating the encoding. Add public `EncodeEntry()` to construct a stored internal key from `ParsedEntryInfo`, including its user key, timestamp, sequence number, and entry type. Also expose `PackSequenceAndType()` and `UnPackSequenceAndType()` to work with the sequence/type footer as a `uint64_t`. The helpers map all nine representable public entry types to their internal types and use the existing footer encoding; `EncodeEntry()` reuses the public packer. Packing `kEntryOther` or a sequence number above the 56-bit limit returns `InvalidArgument` without changing the output. Unpacking reports internal value types without a public equivalent as `kEntryOther`. Timestamp size remains the caller's responsibility, and input slices must not alias the output string. These are additive C++ utilities using the existing internal-key format. Reviewed By: anand1976, xingbowang Differential Revision: D125082298 fbshipit-source-id: 6c09a31695f1d377b9cc208fa5b6edd3ad6638e2 |
||
|
Andrew Chang
|
ee305ac1c3 |
Avoid a TLS init call on every perf counter update (#15428)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15428 RocksDB counts per-operation statistics (blocks read, bytes read, mutex waits, timings) in `perf_context`. It is a `thread_local` variable: each thread has its own copy, so threads never contend on it. This per-thread copy is "thread-local storage" (TLS). **Problem**: `PerfContext` has a non-trivial destructor. For such a variable, C++ must make sure the calling thread's copy is constructed before each use from another source file, so the compiler emits a call to a small "TLS init" function. That call goes through the PLT (procedure linkage table, the jump table used to call functions that may live in another shared library). Every `PERF_COUNTER_ADD` therefore paid a real function call before incrementing a counter, and these macros run in hot loops such as key comparisons during block iteration. `perf_level`, the on/off switch, has a smaller cost: it is `extern thread_local` without `constinit`, so each read also tests whether it has been initialized. **Fix**: - The perf macros read a `constinit thread_local PerfContext*` that each thread sets on first use. After that, a counter update is a pointer load, a null check and the increment. - `perf_level` is declared and defined `constinit`, so reading it is a plain thread-local load. - No public API change. `perf_context`, `get_perf_context()` and macro behavior are unchanged, and so is thread-exit behavior. **Results**: - Microbenchmark (`PERF_COUNTER_ADD` in a loop): 2.10 -> 0.84 ns per update with counting enabled; unchanged at 0.42 ns with counting disabled. - Calls to the TLS init functions drop from 40 and 96 to 0 in block.cc.o and memtable.cc.o. - In CPU profiles of large production RocksDB deployments, these accessors cost roughly 0.3-0.6% of total CPU. This change removes most of the `perf_context` part. Reviewed By: xingbowang Differential Revision: D125982209 fbshipit-source-id: f38b4fffda09611d28322e7534b135fb35631389 |
||
Anand Ananthabhotla
|
074134d98f |
Stop writes on retryable background IO errors when auto resume is disabled (#15444)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15444 **Why the change is required** `max_bgerror_resume_count <= 0` turns off automatic `DB::Resume()`, so the application is responsible for recovering from background errors. Before automatic recovery was introduced, a background error under `paranoid_checks` latched `bg_error_` and made the DB read-only. Since then, two classes of retryable IO error are either never latched or latched as soft errors that leave the DB accepting writes. When auto resume is disabled, no one recovers from them and the application gets no clear signal. **Previous behavior** (with `max_bgerror_resume_count <= 0`) - A retryable or file-scoped IO error during compaction was never set in `bg_error_`. Compaction rescheduled itself and retried indefinitely. A persistent failure was visible only through `EventListener::OnBackgroundError` and the LOG, while compaction debt grew until writes slowed or stopped on L0/pending-compaction triggers. - A retryable or file-scoped IO error while writing a flush SST or the MANIFEST after a successful WAL sync (`kFlushNoWAL` / `kManifestWriteNoWAL`, which covers almost every retryable flush failure, not only `disableWAL` writes) was a soft error with `soft_error_no_bg_work_` set. Background work stopped, but writes kept succeeding into the memtable. Nothing retried the flush, so writers eventually blocked indefinitely on the memtable limit instead of failing. **New behavior** With `max_bgerror_resume_count <= 0`, every retryable or file-scoped background IO error that reaches the retryable path in `ErrorHandler::SetBGErrorImpl` is latched as `kHardError`, whatever the reason (compaction, flush, MANIFEST, WAL). Writes fail immediately and reads continue to work. `DB::Resume()` clears the error, flushes and reschedules compaction as before. **Implementation** In `SetBGErrorImpl`, the compaction special case (notify the listener and return without setting `bg_error_`) now applies only when auto resume is enabled, and the severity selection maps everything to `kHardError` when auto resume is disabled. Because `kErrorRecoveryRetryFlush` is no longer selected in that mode, `soft_error_no_bg_work_` is not set. The option documentation in `include/rocksdb/options.h` and the comment above `SetBGError` are updated. **Safety, compatibility, and limitations** - This is an intentional behavior change, limited to DBs that set `max_bgerror_resume_count <= 0`. With the default (`INT_MAX`) or any positive value, nothing changes. - fbcode callers that set the option to 0, and their apparent expectations: - `unicorn/live_updates/dispatcher/MessageStore.cpp`: the comment says "we do not allow any [background errors] as they should be retried by the underlying FS". Stopping on retryable compaction errors fits that intent better than silent retries. - `reliable_volumes/client/index_manager/RocksDBIndexStorage.cpp`: a JustKnob (`disableRocksDbBackgroundErrorAutoResume`) disables auto resume alongside other corruption-detection options, which suggests it wants background errors surfaced rather than masked. - `zippydb/server/store` (`RocksDBStoreResumeWritesTest`): uses 0 with `disableWAL` and drives recovery through its own `ResumeWrites` task calling `DB::Resume()`, so it already expects writes to stop until resume. I read the test and expect it to pass, but did not run it. No production zippydb configuration setting 0 was found. - Out-of-space errors (`kNoSpace`/`kSpaceLimit`) are untouched: they skip this path and are still recovered through `SstFileManager`, which does not consult `max_bgerror_resume_count`. Errors ignored under `paranoid_checks=false` are also unchanged. - No API, ABI or on-disk format change. - Found during testing, pre-existing and not fixed here: `SetBGErrorImpl` releases the DB mutex in `NotifyOnBackgroundError` before calling `StartRecoverFromRetryableBGIOError`. If the last failed auto-resume attempt's flush error is reported in that window, the recovery thread can give up and clear `recovery_in_prog_` first, and the late report then starts a new recovery with a fresh retry budget, so `max_bgerror_resume_count` can be exceeded. This only applies with auto resume enabled. Worth a separate fix. Reviewed By: joshkang97 Differential Revision: D126453053 fbshipit-source-id: 3f4be1d12a8d44552313b022db5afe200bc1c6ce |
||
|
Xingbo Wang
|
7454655969 |
Small fix and stress coverage for MANIFEST-bound OPTIONS recovery (#15431)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15431 A filesystem failure during column family creation can leave the MANIFEST and OPTIONS snapshots at different points in the column family lifecycle. Recovery must select the snapshot consistent with the MANIFEST without deleting a newer valid legacy snapshot. Fix: When the MANIFEST committed an older tracked OPTIONS file, recovery previously rejected the highest-numbered candidate without reading its tracking marker. Writable open then deleted a newer legacy file, or an unreadable highest file, as a rejected tracked orphan; read-only open also reported the older file number. Recovery now inspects every higher candidate. A newer legacy snapshot supersedes the older committed snapshot, while an unreadable higher candidate is retained rather than deleted. Add: Crash-test invocations randomize `track_options_file_number_in_manifest`. Before writable open can replace evidence, db_stress compares the public latest-OPTIONS choice with read-only recovery, using only currently live column family descriptors and running before requested missing families are appended. After open, it verifies the persisted tracking marker and live column family set. TransactionDB still skips the unsupported read-only probe, but it now reads the MANIFEST first so an existing commit enables the post-open verification. The regression test also checks that writable open preserves the newer legacy file before creating another column family. Change: Reopen MANIFEST-avoidance checks use the loaded `options_` value directly. This is a behavior-preserving simplification, not a separate correctness fix. Safety and limitations: There is no API, file-format, or production hot-path change. Ordinary writable open still receives the caller options and complete requested column family set. The extra read-only open is stress-only and runs when tracking verification is active. The pre-open comparison remains skipped for best-efforts recovery, TransactionDB, and stacked BlobDB; post-open verification remains skipped for best-efforts recovery, TTL, and stacked BlobDB. Random crash coverage is probabilistic, and this parent diff does not add randomized column family operations; the dependent lifecycle diff adds that profile. Reviewed By: pdillinger Differential Revision: D125029493 fbshipit-source-id: 5e291deff735e3cbe6d7df0513f940e84b20f387 |
||
Jay Huh
|
a7f6f33ccc |
Keep OPTIONS file alive while GetLiveFilesStorageInfo reads its size (#15430)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15430 A db_stress blackbox crash test failed in verification with: `GetLiveFilesStorageInfo failed: IO error: No such file or directory: while stat a file for size: .../OPTIONS-857089` Since D123104845, `GetLiveFilesStorageInfoImpl` picks the OPTIONS file number while holding the DB mutex, releases the mutex, and only then calls `GetFileSize` on that file. If a `SetOptions()` runs in that window, it writes a new OPTIONS file, and `MaybeDeleteObsoleteOptionsFiles()` keeps only the newest two plus the MANIFEST-committed one. So the file that was just picked can be deleted before it is stat'ed. Checkpoint calls `DisableFileDeletions()` first and is not affected, but a plain `GetLiveFilesStorageInfo()` call, as db_stress makes, gets a spurious IO error. The fix increments `disable_delete_obsolete_files_` (`DisableFileDeletionsWithLock()`) before releasing the mutex, and decrements it again under the mutex right after the size read. `MaybeDeleteObsoleteOptionsFiles()` already skips cleanup while the counter is non-zero. Any OPTIONS cleanup skipped during that window happens on the next OPTIONS write. The decrement is done directly rather than through `EnableFileDeletions()`. That method is virtual, and `DBImplReadOnly` and `CompactedDBImpl` override it to return `NotSupported` without decrementing. Read-only DBs do reach this code (their OPTIONS file number is set at open), so going through `EnableFileDeletions()` left the counter stuck at 1 after every call. Calling the non-virtual `DBImpl::EnableFileDeletions()` instead is not safe either: when the counter reaches 0 it runs a forced `FindObsoleteFiles` scan and `PurgeObsoleteFiles`, which a read-only DB must never do. Secondary DBs never take this path, because `ReactiveVersionSet` does not record an OPTIONS file number, so `options_number` is 0 there. Cost: no extra I/O or scan. Checkpoint already disables deletions before calling, so for it this only bumps the counter. For a caller that has not, such as db_stress, obsolete files that become deletable during the short window are not purged at the decrement. They stay queued and are removed by the next `FindObsoleteFiles` (end of the next flush or compaction, or the periodic full scan). Reading the cached `VersionSet::options_file_size_` under the mutex, as before D123104845, would avoid the stat entirely. I kept the stat because D123104845 chose it deliberately. The cached size is the better option if it is always in sync with `options_file_number_`. Reviewed By: xingbowang Differential Revision: D126001916 fbshipit-source-id: 60f4f31c73e48b374f4015ed2847d45e706b85de |
||
generatedunixname3846135475516776
|
61831d47be |
Rocksdb Crash Test failed: stderr: Failure: latest seqno changed from <N> to <N> with W (#15433)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15433 Reviewed By: jaykorean, xingbowang Differential Revision: D125887282 fbshipit-source-id: 611491c1ece2c6e3bc74f22545bd5d93467cf9b9 |
||
|
Peter Dillinger
|
8121c7e254 |
Expose kLatestSupportedFormatVersion in BlockBasedTableOptions (#15397)
Summary: Before this change, the highest supported block-based table format_version was only available as the internal constant kLatestBbtFormatVersion in table/format.h. Applications that accept a format_version setting from users or configuration (especially for SetOptions()) had no public way to validate it against the linked RocksDB build before applying it. This adds the public constant BlockBasedTableOptions::kLatestSupportedFormatVersion (currently 8) and removes kLatestBbtFormatVersion, redirecting all internal and test uses to the new constant. Supported-version checks and option validation are unchanged: format_version above the constant is still rejected as InvalidArgument, and draft format_versions remain usable only under the TEST-only TEST_AllowUnsupportedFormatVersion() hook. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15397 Test Plan: Same value and same checks, so no new tests. Existing format_version validation coverage (e.g. TableTest.UnsupportedFormatVersionConfigTest) and the FormatLatest test instantiations now go through the public constant. Reviewed By: rban1 Differential Revision: D125117976 Pulled By: pdillinger fbshipit-source-id: 67e0c5b7eade4bf649957435aa0b28b187793f7c |
||
|
Jay Huh
|
54732b0c8f |
Tolerate CacheLib startup INFO in crash tests (#15416)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15416 Why the change is required: A crash test using `secondary_cache_uri=cachelibwrapper://...` constructs and resets CacheLib/Navy. Sandcastle suppresses CacheLib `XLOG(INFO)` by patching the fbcode XLOG header, but commit `831f00db6a47` enabled the exported xplat header unit. The build therefore consumed an unpatched XLOG header, normal CacheLib startup INFO reached stderr, and `db_crashtest` exited 2 before it could accept the intentional whitebox SIGTERM. Previous behavior: Any residual child stderr was fatal. `sanitize_known_stderr()` covered expected SIGTERM and io_uring diagnostics but not CacheLib construction/reset INFO, so an otherwise healthy secondary-cache whitebox iteration failed. New behavior and implementation: When `secondary_cache_uri` starts with `cachelibwrapper://`, the sanitizer moves only INFO-severity lines from an explicit CacheLib construction/reset source allowlist to stdout. Warnings, errors, fatal logs, unrelated INFO, and identical lines without the CacheLib URI remain fatal. Safety and compatibility: Default and non-CacheLib configurations are unchanged. No API, ABI, persisted format, database behavior, or performance-sensitive engine path changes. ___ landed-with-radar-review Differential Revision: D123695714 fbshipit-source-id: 844ecd2cbf1b3c1415629d787748b5edeb63d456 |
||
|
Anand Ananthabhotla
|
a3b0f187ff |
Add FileSystem::IsPreallocationSupported() to skip WAL truncation on open (#15395)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15395 **Why the change is required:** During `DB::Open`, RocksDB reopens the last WAL file and truncates it to its logical size. This is done even when every column family's log number has already advanced past that WAL, so the file is obsolete and will be deleted later in the same open. The only purpose of the truncation is to release space preallocated beyond the logical end of file (e.g. by `fallocate` with `FALLOC_FL_KEEP_SIZE`); `GetFileSize()` reports the apparent size, so truncating to it can only free preallocated blocks. On a FileSystem that never preallocates, this work is pure overhead: a `GetFileSize`, a `ReopenWritableFile`, a no-op `Truncate` (the default `FSWritableFile::Truncate` returns OK without doing anything), and a `Close` on every open. On remote or append-oriented FileSystems the reopen can be expensive. **Previous behavior:** - If recovery flushed all data, or the WALs contained no data (including when all WALs are older than the min log number to keep), `MaybeFlushFinalMemtableOrRestoreActiveLogFiles()` always called `GetLogSizeAndMaybeTruncate()` on the last WAL. In read-only mode it still issued a `GetFileSize()` whose result was discarded. - With `avoid_flush_during_recovery`, `RestoreAliveLogFiles()` always reopened and truncated the last live WAL. **New behavior:** A FileSystem can report that it never preallocates space by returning false from the new `FileSystem::IsPreallocationSupported()`. When it does, DB open skips reopening and truncating the WAL. In the flushed / no-data path it also skips the unused `GetFileSize()`, so no I/O is issued against the old WAL. The read-only no-data path no longer issues the discarded `GetFileSize()` either. **Implementation:** - `include/rocksdb/file_system.h`: new `virtual bool IsPreallocationSupported()`, defaulting to `true`. `FileSystemWrapper` forwards it to the target. - The policy lives in the two callers in `db_impl_open.cc`; `GetLogSizeAndMaybeTruncate()` is unchanged. `RestoreAliveLogFiles()` still calls `GetFileSize()` because the size is needed for `alive_wal_files_` and `wals_total_size_`. **Safety, compatibility, and limitations:** - The default is `true`, so every existing FileSystem, including ones that override `SupportedOps()` without calling the base class, keeps the current behavior. This is why it is a separate virtual function instead of an opt-in `FSSupportedOps` bit, which would silently disable truncation for those FileSystems. - No built-in FileSystem overrides the default: Posix, Windows, `MockFileSystem`, and `LegacyFileSystemWrapper` all report `true`, so their WAL truncation behavior is unchanged. Posix deliberately reports `true` even in builds without `ROCKSDB_FALLOCATE_PRESENT`: WALs may have been preallocated by a different build running on the same machine, and a build-dependent answer would leave that space unreclaimed. - The function describes what the FileSystem can do, not per-file options such as `allow_fallocate` or build configuration. A WAL may have been preallocated by an earlier process with different options, so gating on the current options could leak space. - A FileSystem that wrongly returns false can only leak preallocated space, in archived WALs (`WAL_ttl_seconds` / `WAL_size_limit_MB`) or across a crash loop before obsolete WALs are deleted. Data is unaffected. Reviewed By: joshkang97 Differential Revision: D125049741 fbshipit-source-id: ac9795f2045c46911cd86b8faacef5aa49043496 |
||
Ranjan Banerjee
|
a215c38a8e |
Fix libaio download after Folly update (#15404)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15404 The RocksDB Folly checkout rewrites a dead libaio source URL to Debian's mirror. The pinned Folly revision now uses releases.pagure.org, so the rewrite still matched the older pagure.io archive URL and became a no-op. Public Folly builds then downloaded an empty file and failed SHA-256 verification. Update the rewrite to match Folly's current libaio URL. The Debian archive has the same libaio 0.3.113 contents and SHA-256 already recorded in the Folly manifest, so the checksum and extraction directory remain unchanged. This only affects the public Folly checkout path; RocksDB runtime behavior and system-package users are unaffected. ___ landed-with-radar-review Differential Revision: D125347462 fbshipit-source-id: 4203b6a0b6037efb23aa1f44306b0ca0cd73a43c |
||
|
Anand Ananthabhotla
|
df90749560 |
Wire compaction to retry TableBuilder file cuts (#15344)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15344 Add the internal TableBuilder::TryAdd() contract so RocksDB-owned table builders can distinguish accepted rows, real builder errors, and clean requests to cut the current output table and retry the same row. Add diagnostic reason codes for retryable table boundaries; existing internal table builders inherit a default TryAdd() implementation that forwards to Add(). Translate the public ExternalTableBuilderBase::TryAdd() result in ExternalTableBuilderAdapter so external table implementations can request a file cut while RocksDB compaction uses the internal TableBuilder contract. Teach compaction output writing to honor kRequiresNewTable by closing the current output file, opening a new one through the existing callbacks, and retrying the same key without advancing the input iterator. Split OutputValidator validation into CanAdd() and AddValidated() so validator state is not updated until the key is actually accepted by a table builder. Add a DB compaction test with a custom TableFactory that wraps block-based tables and requests one mid-compaction file cut. The test verifies the cut produces two readable output files and preserves all key/value results. Reviewed By: jaykorean, xingbowang Differential Revision: D123932406 fbshipit-source-id: 9ed82c3782787de192bca15e40bfbf014ef28c15 |
||
|
Josh Kang
|
014e1c43c1 |
Apply readability fixes (isolate decls, braces) (#15394)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15394 ## Summary RocksDB's `.clang-tidy` enables several readability checks that existing code still violates: `readability-isolate-declaration` (476 findings), `readability-braces-around-statements` (49), `readability-redundant-string-init` (25), `modernize-use-using` (10), `misc-definitions-in-headers` (3) and `readability-duplicate-include` (1). They keep getting flagged whenever nearby lines change. This diff fixes 553 of the 564; upstream has since fixed the other 11. What changed: - Multi-variable declarations such as `std::string a, b;` are split into one declaration per variable. Pointer and reference declarators are kept per variable. - Braces are added around single-statement `if`/`else`/`for`/`while` bodies, and empty spin loops get an empty `{}` body. - `std::string s = ""` / `member("")` initializers are removed. - Remaining `typedef`s in the touched C++ files become `using`. C headers and `extern "C"` code are untouched. - `inline` is added to non-template function definitions in test headers. - A duplicate `#include "util/udt_util.h"` in `db_impl.cc` is removed. These are clang-tidy's own fix-its, applied mechanically and then clang-formatted on the changed lines. Three kinds of broken fix-it output were repaired by hand. Removing a `member("")` initializer left a dangling comma in constructor initializer lists (`ExternalSstFileInfo`, `ColumnFamilyMetaData` and others). The function-type typedefs in `block_test.cc` came out malformed. Empty `while (cond);` spin loops got a `{;` body. Splitting declarations also exposed two vectors in `db_etc2_test.cc` to `performance-inefficient-vector-operation`, so a `reserve()` was added for each. The lint bot reports every finding in a file a diff touches. This diff splits declarations in `options/configurable_test.cc`, so it also fixes that file's 14 older `cert-err58-cpp` and `cppcoreguidelines-avoid-non-const-global-variables` findings, which come from seven mutable file-level maps. Six are now `const` and built where they are used (function-local statics, or locals in their only user). The unused `imm_struct_option_info` is deleted. Compatibility: no behavior change. In public headers, the only edits are removed redundant `""` initializers, split declarations and added braces. Reviewed By: rban1 Differential Revision: D123186310 fbshipit-source-id: f9c8e88b00e997db0196f8725c7c8d880cf798da |
||
|
Jay Huh
|
956c7042da |
Check async I/O support on the MultiGet thread and fall back to sync (#15337)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15337 ## Why RocksDB crash tests repeatedly fail with `MultiGetEntity error: Not implemented: ReadAsync: failed to init io_uring` when synchronous `MultiGet` requests async I/O on a thread that cannot create an io_uring. The same `NotSupported` status can reach users if capability detection succeeds but an individual asynchronous submission later fails. ## Previous behavior `Version` cached `FSSupportedOps::kAsyncIO` when it was constructed. Posix io_uring instances are thread-local, so that result described the construction thread rather than the thread later executing MultiGet. A cached positive result could route a different thread into `ReadAsync`, where io_uring creation failed and the error reached the caller. A cached negative result could also unnecessarily disable async I/O on a capable read thread. ## New behavior and implementation Synchronous `Version::MultiGet` now checks async-I/O support on the thread executing the read. Both the cross-level `optimize_multiget_for_io` path and the within-level overlap path enter their coroutine-based I/O only when that thread supports it. Posix `SupportedOps` eagerly creates and retains that thread-local io_uring, so subsequent submissions reuse the verified ring. The capability remains advisory: if `ReadAsync` nevertheless returns `NotSupported`, `AsyncFileReader` completes the `FSReadRequest` synchronously, including requests that use filesystem-owned buffers. Poll errors are applied only to requests that were actually submitted asynchronously, so synchronous fallbacks in a mixed batch retain their successful status. Regression coverage forces io_uring capability failure only on a dedicated read thread and exercises both synchronous MultiGet branches. Separate tests cover all-request fallback and a batch mixing submitted and synchronously fallen-back requests under an injected Poll error. A one-line bug-fix release note records the user-visible correction. ## Safety and compatibility This changes no API, ABI, or persisted format. The support check is only performed when synchronous `MultiGet` receives `ReadOptions::async_io`; the Posix steady-state cost is a virtual call and thread-local ring lookup per such MultiGet. Direct I/O continues to use the existing synchronous `MultiRead` route before `AsyncFileReader`, and native coroutine APIs are unchanged. Reviewed By: pdillinger Differential Revision: D117802488 fbshipit-source-id: 0f2991a523607d894436cd53123e7a1b2fe93167 |
||
|
Anand Ananthabhotla
|
9198b11b46 |
Make InjectedErrorLog::Entry 256 bytes on 32-bit targets (#15388)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15388 `InjectedErrorLog::Entry` in `utilities/fault_injection_fs.h` is written to the injected error log in its native layout, and a `static_assert` requires `sizeof(Entry) == 256` so the record size stays stable. That assert fails on i386 builds. Root cause: the fields add up to only 252 bytes. On targets where `alignof(uint64_t)` is 8 (x86_64, aarch64, armv7), the compiler adds 4 bytes of implicit tail padding, so the size is 256. On i386, `uint64_t` members are 4-byte aligned inside a struct, so there is no tail padding and the size is 252. Fix: add an explicit trailing `uint8_t padding[4]` field so the size no longer depends on `uint64_t` alignment. Compatibility: on 64-bit targets the new field takes the place of the old implicit tail padding, so every field offset and the 256-byte record layout are unchanged. `Record()` value-initializes the entry (`Entry entry{}`), so the padding bytes are always written as zero. No decoder in the repo depends on the padding bytes. ___ landed-with-radar-review Differential Revision: D124873369 fbshipit-source-id: f5c6f43ca85013b0e769853ad13b8d4f3f89b958 |
||
|
Yoshinori Matsunobu
|
77eef9fa2e |
Make RocksDB reverse iteration honor query aborts (#15376)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15376 RocksDB `DBIter` polled `ROCKSDB_THREAD_YIELD_CHECK_ABORT()` while scanning ordinary forward entries, but reverse `Prev()` / `SeekForPrev()` traversal and `MergeValuesNewToOld()` could scan many internal keys without polling. Long tombstone, version, or merge-operand runs could therefore keep a MyRocks query running after `KILL QUERY` until the scan finished. Poll the existing hook before each potentially expensive internal iterator movement in the forward scan, forward merge-operand scan, and four reverse traversal loops. Mark the abort branch `UNLIKELY`; on abort, invalidate the iterator and return `Status::Aborted("Query abort.")`. MyRocks already defines the hook and maps `Status::Aborted` for a killed THD to `HA_ERR_QUERY_INTERRUPTED`, so no MyRocks change is required. There are no API, ABI, or persisted-format changes. The hook remains Linux-only, and cancellation remains cooperative: this cannot preempt an individual underlying iterator operation that never returns. The change is rebased onto current master, including D124377137's fix for the unrelated `WriteBufferManagerTest.FlushPolicyRetiresAndRestartsHandoffWorkers` failures seen on V1. Reviewed By: pdillinger Differential Revision: D124441004 fbshipit-source-id: 53bf83a00121b42566dbd93de232e973cbe78688 |
||
|
Josh Kang
|
4b983da055 |
Spell out special member functions (rule of five) (#15387)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15387 ## Summary RocksDB's `.clang-tidy` enables `cppcoreguidelines-special-member-functions`, `cppcoreguidelines-virtual-class-destructor` and the noexcept-move checks, and clang warns `-Wdeprecated-copy-with-user-provided-dtor`. A full scan reports 476 findings. In most of them, a class declares a destructor (or copy operations) but leaves the rest of copy/move implicit. That has two costs. Readers can't tell whether a class may be copied or moved. And resource-owning classes keep an implicit copy that would double-free if anyone ever used it. This diff resolves 453 of the 476. Approach, picked per class from least to most invasive: 1. An empty user-provided destructor becomes `= default`. This covers most interface classes, such as `virtual ~X() {}` -> `= default`. If the class is still copied, that would only rename the deprecated-copy warning, so the destructor is removed instead when nothing needs it (`Customizable`, `TableFactory` and `InternalKeyComparator` inherit a virtual one; `IteratorWrapperBase` isn't polymorphic), and `MemTableRep::KeyComparator` gets defaulted copy and move. 2. Classes whose copy is already deleted get explicitly deleted move operations too. Move was already unavailable, so this only documents it. 3. Resource-owning classes that are never copied get deleted copy and move operations. Examples: the C API wrapper structs, which run a `destructor_` callback (edited in `tools/c_api_gen/c_base.cc` and regenerated into `db/c.cc`); `ShardedCache`, which frees its shards; iterator wrappers; and RAII handles. 4. Value types that are copied or moved get the missing operations as `= default`, which keeps their current behavior. 5. Exception specifications of existing functions are unchanged, except for `WriteBatch`'s move assignment (below): no other `noexcept` is added or removed. Only the move operations this diff newly declares (`Comparator`, `FileSampledStats` and the thread-local `Entry`) are `noexcept`; they copy only scalars or atomics. `WriteBatch`'s copy and move assignment destroyed `*this` and placement-new'ed a new `WriteBatch` over it. That is undefined behavior when `*this` is a subclass, and if the copy constructor threw, `*this` was left destroyed and was later destroyed again. Move assignment now assigns member-wise and ends in the same state as the move constructor, which leaves `has_key_with_ts_`, the timestamp-size tracking and the other fields it doesn't copy at their defaults. `rep_` is swapped through a temporary, because a plain string move-assignment can keep the old buffer (e.g. a large batch reset with `batch = WriteBatch()` would keep its memory). Nothing in it can throw, so it is now `noexcept`. Copy assignment copies into a temporary and moves it in, so a throwing copy leaves `*this` unchanged. Bugs this exposed and fixed: - `RestoreAfterCopyOrCreateWorkItem` (backup engine): the hand-written move constructor and assignment moved only some fields, so `from_file` and `to_file` were empty after `push_back(std::move(...))`. A restore that hit a checksum mismatch then reported "While restoring -> : ..." without file names. It now uses the compiler-generated moves, which are still `noexcept`. - `ExternalSstFileIngestionJob` lived in a `std::vector`, and its destructor frees raw-owned pointers, so the implicit copy used on vector reallocation would double-free. Its file batches also point into its own inline file storage, so neither a copy nor a move is safe after `Prepare()`. Copy and move are now deleted, and `DBImpl` holds jobs through `std::unique_ptr`. Compatibility: no public class loses an operation users can call. Public interfaces mostly only get `virtual ~X() = default;`. `WriteBatch`'s move assignment becomes `noexcept`, which is not part of the mangled name, so the ABI doesn't change. `SubcompactionJobInfo` and `CompactionJobInfo` intentionally stay C++20 aggregates, so users' aggregate and designated initializers keep compiling. `Configurable`, `CompareInterface`, `WriteBatchBase`, `ObjectLibrary::Entry` and `MemTableRep::KeyComparator` declare defaulted copy and move, and `Customizable` no longer declares a destructor. As a result, `Configurable` and `Customizable` subclasses that declare no destructor or copy operations of their own now move their registered-options list instead of copying it. In-tree, these are the mutable-options adapters, the table factories, the memory allocators and test classes; all are held through smart pointers or used in place, and `Clone()` copies, so none is moved by value. Not changed (23): `IODebugContext` (custom deep copy plus a `std::shared_mutex`), `DBWithColumnFamilies` in db_bench (stored in a `std::vector` with non-movable members), `Snapshot`'s protected virtual destructor (intended), `ManagedSnapshot`, the `SubcompactionJobInfo` and `CompactionJobInfo` aggregates, and the 16 noexcept-move-constructor and noexcept-swap findings on existing functions, including `autovector`'s move assignment (adding `noexcept` would change their exception specifications). Copying `ManagedSnapshot` already double-releases the snapshot. Fixing that needs a real move implementation and is a behavior change, so it is left for a separate diff. Reviewed By: rban1 Differential Revision: D123186309 fbshipit-source-id: 6f8e57e233e12199a007407130bb9a1f6d52e8d1 |
||
|
Josh Kang
|
ddac37a780 |
Serialize subset-checkpoint OPTIONS persistence with SetOptions (#15386)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15386 Fixes crash-test failure T292562874: `Checkpoint failed: Corruption: [RocksDBOptionsParser]:failed the verification on BlockBasedTable::: filter_policy.id` Root cause: - D123104845 ("Commit authoritative OPTIONS file numbers in MANIFEST", 77d94c989243) made subset checkpoints (`CreateCheckpoint` with `include_cf_ids` that exclude some CFs) write a new OPTIONS file. `DBImpl::GetLiveFilesStorageInfoImpl` captures the snapshot under the DB mutex with `cfd->GetLatestCFOptions()`. That is a shallow copy: `table_factory`, and through it `filter_policy`, are `shared_ptr`s to the live objects. `DBImpl::CreateOptionsFileForSubsetCheckpoint` then calls `PersistRocksDBOptions` without holding `options_mutex_`. `PersistRocksDBOptions` serializes the file, then re-parses it and checks it against the same in-memory options. - `RibbonFilterPolicy::bloom_before_level_` is a `kMutable` atomic that `SetOptions(table_factory.filter_policy.bloom_before_level=...)` changes in place, and `RibbonFilterPolicy::GetId()` includes it (`ribbonfilter:<bits>:<bloom_before_level>`). A concurrent `SetOptions` between the write and the verify makes the in-memory `filter_policy.id` differ from the file, so verification returns Corruption and the checkpoint fails. - Every other OPTIONS writer (`WriteOptionsFile`, CF create/drop) holds `options_mutex_`. `SetOptions` holds it for its whole mutate-and-persist sequence. The new subset-checkpoint writer was the only one that skipped it. Trigger in the crash test: db_stress randomly sets `table_factory.filter_policy.bloom_before_level` through `SetOptions` (when `keepRibbonFilterPolicyOnly`, i.e. `bloom_before_level` != INT_MAX with ribbon filters; see the OPTIONS dump with `ribbonfilter:18:1`, `ribbonfilter:8.9:2147483647`, `ribbonfilter:5.67:-1` across CFs). It also takes subset checkpoints. The two run concurrently on different threads. Fix: hold `options_mutex_` in `CreateOptionsFileForSubsetCheckpoint` while persisting and verifying, the same as `WriteOptionsFile`. The only caller is `CheckpointImpl::CreateCheckpoint`, which holds no DB locks at that point, so lock ordering (options_mutex_ before mutex_) is kept. Note: the snapshot's scalar fields are still captured at live-file time. Live pointer sub-options such as the filter policy reflect their state at persist time. This was already true of the shallow snapshot. The change only makes the persisted file self-consistent with what is verified. Reviewed By: xingbowang Differential Revision: D124728996 fbshipit-source-id: adb5511c2ddd73ab138fd7fd21109199ff4f5b1e |
||
|
Peter Dillinger
|
76d7c29389 |
Update folly hash (#15375)
Summary: has been neglected for a few months, finally taking care of it. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15375 Test Plan: local folly build (from internal wiki instructions) to verify the fixes from claude, CI Reviewed By: archang19 Differential Revision: D124436669 Pulled By: pdillinger fbshipit-source-id: 0a0e3f6857cc8ba20941281b9dfd618a6997b86f |
||
|
Xingbo Wang
|
1334e4dd52 |
Add offline OPTIONS binding repair (#15310)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15310 Why the change is required: The committed OPTIONS number in MANIFEST gives persisted-options readers a precise source of truth. If that file is missing or corrupt, or an operator must deliberately replace it with a known-good snapshot, persisted-options loading can fail before a normal writable open can publish a replacement. RocksDB therefore needs a supported offline recovery path that obeys the binding and global file-number rules. Previous behavior: ldb update_manifest could rewrite file-temperature metadata but could not repair an OPTIONS binding. Operators could not safely bind a selected snapshot while accounting for the current MANIFEST boundary, every file-number allocation root, recoverable WAL output, and crash durability. Blindly selecting or reusing an existing OPTIONS number could bind the wrong snapshot or collide with another RocksDB file. New behavior: ldb update_manifest --rebind_options_file --options_file=OPTIONS-<number> performs an offline repair from an explicitly selected trusted snapshot whose tracking marker is enabled. Optional --file_number_paths arguments identify external allocation roots that are not serialized in OPTIONS. The command locks the DB and validates that the selected OPTIONS snapshot contains every live MANIFEST column family. Extra OPTIONS CFs are accepted as a safe superset and are filtered by binding-aware persisted-options APIs; a snapshot missing any live CF is rejected. The command raw-copies the selected bytes to a newly allocated canonical OPTIONS number, syncs that file and its directory entry, and commits the new number with one MANIFEST record. It rewrites the current MANIFEST only through the last completely decoded record, appends the commit and next-file boundary to a temporary copy, syncs it, atomically renames it over the original, and syncs the MANIFEST directory. The crash contract has one publication point. Before MANIFEST replacement, the previous commit remains authoritative and the copied tracked OPTIONS file is only an ignorable orphan. After replacement, the new snapshot is authoritative. Implementation: - Parse the selected snapshot for validation but copy it byte-for-byte so unknown or newer option fields are preserved. - Replay MANIFEST to recover live CF membership, the committed number, next file number, and last valid logical-record boundary; discard a torn physical tail during the bounded-memory rewrite. - Scan the DB root, WAL/archive directories, serialized roots, and operator-supplied roots for allocated numbers. - Parse recoverable WALs and reserve conservative headroom for recovery outputs, missing-column-family creation, persistent statistics, asynchronous WAL creation, MANIFEST rollover, and subsequent writable-open allocations. - Reject file-number overflow, missing allocation roots, corrupt or unreadable WALs, invalid input names, missing-live-CF snapshots, and simultaneous temperature rewriting. - Preserve the existing update_temperatures behavior and result text. Safety boundaries and unsupported cases: - This is an explicit offline mutation. It never auto-selects a snapshot and refuses to run while another process holds the DB lock. - The selected input must be a canonical OPTIONS-<number> basename in the DB directory, must parse successfully, must enable MANIFEST tracking, and must contain all live MANIFEST CFs. Extra CFs are allowed but missing live CFs are not. - The command does not repair snapshot contents or alter topology. - OPTIONS does not serialize all DB/CF data roots. Operators must provide every external root through --file_number_paths; the command fails closed when a supplied or inferred root cannot be scanned. - Conservative allocation headroom can reject a repair near kFileNumberMask even when a particular future open would allocate fewer files. - Normal DB open and core publication do not depend on this maintenance command. Validation coverage and known limitations: Coverage includes parsing and incompatible modes, the DB lock, tracking and membership validation, safe-superset acceptance, missing-live-CF rejection, byte-preserving copy, torn MANIFEST tails, one-record commit publication, file-number exhaustion and writable-open headroom, external and missing roots, WAL recovery bounds, asynchronous WAL and persistent-stat allocations, MANIFEST rollover, writable reopen, and later CF creation. The end-to-end test confirms safe-superset rebinding and rejection of a higher tracked snapshot that omits a live CF, verifies the final committed MANIFEST number, and runs the real ldb scan --try_load_options subprocess. Cross-platform CI is not part of local validation. Reviewed By: pdillinger Differential Revision: D123104846 fbshipit-source-id: 8cf7c54c1c7a360411034469614eed8aaffdcf8e |
||
|
Xingbo Wang
|
9fb698e218 |
Synchronize IteratorCloseWALFile1 flushes (#15382)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15382 Why the change is required: IteratorCloseWALFile1 is flaky under suite load. Its WAL-count assertions assume the iterator is the final owner of an obsolete SuperVersion, but the test did not prove that the preceding background flush had released its temporary references or that the next flush was unable to start. Previous behavior: The test occupied one HIGH-priority Env worker with a sleeping task. The shared Env can have multiple HIGH-priority workers, so another worker could run the flush concurrently. Also, Flush() can return before BackgroundCallFlush() has fully exited. Either race can leave two WAL writers open after iterator deletion. A fatal assertion could then destroy the stack-local SpecialEnv before fixture teardown closed the DB, causing a secondary use-after-return or segmentation fault. New behavior: The test waits for DBImpl::BGWorkFlush:done after the iterator-establishing flush, then replaces that dependency with a barrier at DBImpl::BackgroundCallFlush:start for the flush triggered by the final write. The iterator is therefore the final old-SuperVersion owner when it is destroyed, and the test releases the blocked flush only after checking synchronous WAL closure. Implementation: Replace the sleeping background task with existing SyncPoint dependencies. Own the iterator with unique_ptr, disable SyncPoint processing on every exit, and close the DB with a scope guard before the local SpecialEnv is destroyed. Safety, compatibility, and limitations: This changes only test synchronization and failure cleanup. It does not change production code, RocksDB APIs, persisted formats, locking, or runtime behavior. The test continues to cover foreground iterator cleanup with background_close_inactive_wals enabled; background purge behavior remains covered separately by IteratorCloseWALFile2. ___ landed-with-radar-review Differential Revision: D124653963 fbshipit-source-id: 65d381128b5013b4d62ad60e42f0e4566928f000 |
||
|
Xingbo Wang
|
baa4d373a6 |
Commit authoritative OPTIONS file numbers in MANIFEST (#15305)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15305 Why the change is required: Column-family topology is durable in MANIFEST, while its operational configuration snapshot is stored in a separate OPTIONS file. CreateColumnFamily and DropColumnFamily previously committed MANIFEST first and wrote OPTIONS afterward. An OPTIONS I/O failure could therefore make the API report failure after the topology change was already durable. Persisted-options readers also selected the highest file number and could consume an orphan left by a failed publication. Previous behavior: RocksDB had no durable correspondence between a MANIFEST state and the OPTIONS snapshot describing it. LoadLatestOptions and GetLatestOptionsFileName treated the highest-numbered OPTIONS file as authoritative. Column-family creation and deletion returned the later OPTIONS persistence error even when their MANIFEST edit had already succeeded. Multi-column-family operations wrote a complete OPTIONS snapshot after every successful member, producing quadratic OPTIONS bytes. Ordinary DB::Open consumes caller-supplied DB and column-family options rather than loading OPTIONS. This change preserves that contract. New behavior: Add the default-disabled experimental DB option track_options_file_number_in_manifest. Its value is serialized into each OPTIONS file so the selection rule is self-describing and participates in the normal DB-options compatibility checks. When enabled, RocksDB first durably creates and renames OPTIONS-N and then commits N with one safely ignorable MANIFEST field. A normal OPTIONS rotation uses one MANIFEST commit. Column-family creation and deletion prewrite the proposed snapshot and commit its number together with a topology edit in the same VersionEdit. A failure before the OPTIONS file is durable changes no topology. A failure after the file is durable but before MANIFEST commit leaves an orphan that binding-aware readers ignore, while the previous committed snapshot remains authoritative. Each multi-CF create or drop writes one proposed snapshot, independent of the batch size. Create commits the new number with the first successful add; drop commits it with the last successful drop. Existing partial-success semantics can therefore leave a committed snapshot with extra CFs, but never one missing a live MANIFEST CF. Persisted-options APIs filter this safe superset against recovered MANIFEST membership. A snapshot missing a live CF is corruption. DB::Open with create_missing_column_families uses the same batched path. If the newest OPTIONS file has tracking disabled, it retains the legacy highest-numbered-file behavior, including after an old or disabled writer has run. If it has tracking enabled, readers use the recovered MANIFEST commit and reject newer tracked orphans. A missing usable commit falls back to the highest tracked snapshot for RepairDB and legacy-rewrite recovery. DB open logs that fallback through its configured logger; logger-less utility APIs remain silent. Writable open removes rejected tracked orphans when local obsolete-file deletion is enabled. Read-only open never mutates the DB directory. A DB with no recovered commit and tracking disabled takes the legacy fast path without parsing the newest OPTIONS file. When inspection is required, RocksDB reads only the tracking marker. Marker-read failures used for selection do not fail DB open; a recovered commit remains authoritative and ambiguous newer candidates are conservatively retained. The first topology change after enabling tracking first binds the current topology, so failure of the proposed change cannot make its orphan authoritative. MANIFEST failure during an ordinary tracked OPTIONS rotation also enters the existing manifest-write background-error path. Implementation: - Add the safely ignorable CommittedOptionsFileNumber VersionEdit field and carry it through recovery, MANIFEST snapshots, rollover, and subset checkpoints. - Persist the tracking marker through C++, C, and Java options, generated round-trip coverage, compatibility checking, and db_stress configuration. - Centralize binding-aware selection for DB open and public persisted-options APIs. Reuse RecoveryContext's directory listing, use a narrow marker scan, and recover live CF names while replaying MANIFEST. - Filter manifest-selected safe-superset snapshots to live MANIFEST CFs in LoadLatestOptions and compatibility verification without discarding their option maps. - Prevalidate and sanitize CF batches outside the DB mutex, write one complete snapshot while options_mutex_ serializes topology changes, and hold the DB mutex only for the existing individual MANIFEST commits. Batch missing-CF creation during DB open follows the same locking boundary. - Keep the committed snapshot live in cleanup and live-file APIs, and make subset checkpoints write an OPTIONS snapshot and filtered MANIFEST with identical membership in tracked and legacy modes. - Preserve the first partial multi-CF drop error while consuming legacy OPTIONS persistence Status, and sync the checkpoint MANIFEST directory after replacement. - Pass checkpoint helper callbacks by reference and group VersionEdit storage by alignment so the new paths add neither callback copies nor avoidable object padding. Safety boundaries and unsupported cases: - Tracking defaults to false. Disabled writers and never-tracked databases retain legacy write ordering and highest-numbered selection. - Older binaries safely ignore the new MANIFEST tag and continue choosing the highest OPTIONS number. Downgrade is readable but not crash-atomic: an old binary can consume a tracked orphan. That is the same class of OPTIONS/MANIFEST inconsistency as the previous behavior. - A rollback binary must know the serialized option name before the feature is enabled. Exact compatibility checks intentionally report a mismatch when the caller's tracking setting differs from the persisted snapshot. - A newer untracked OPTIONS file deliberately supersedes an older committed number, allowing an old or disabled writer to become authoritative without rewriting MANIFEST. - Reactive secondary and follower instances do not enforce or repair the binding. Ordinary DB::Open still uses caller-supplied options. - Writable open does not delete rejected tracked or ambiguous files when a compaction service owns OPTIONS-file retention. Read-only open performs no repair writes. - Direct consumers that scan OPTIONS filenames themselves are unchanged; the binding is enforced by RocksDB's persisted-options APIs. - If the committed snapshot is missing and only tracked candidates remain, selection intentionally falls back to the highest candidate so RepairDB output remains usable. Validation coverage and known limitations: Focused coverage exercises encoding compatibility, tracked and legacy selection, the never-tracked open fast path, marker-read failures, exact tracking-marker compatibility, activation and disable/re-enable transitions, batched create/drop and create-missing behavior, partial-batch safe supersets, OPTIONS/MANIFEST failure injection, background-error propagation, orphan cleanup and retention, read-only non-mutation, subset-checkpoint behavior, stale OPTIONS recovery, RepairDB option sanitization, and transient read-corruption reconstruction. The assertion-enabled full executable suite passes. The repository check-format and check-sources wrappers cannot run because they invoke Git inside this Sapling checkout; fbsource lint is used instead. Cross-platform CI is not part of local validation. Reviewed By: pdillinger Differential Revision: D123104845 fbshipit-source-id: 77d94c989243c3f3f9fedfa81fa364c166250303 |
||
|
Ranjan Banerjee
|
4b44c2b4ba |
Fix WBM worker retirement and flush test accounting (#15373)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15373 Fix worker retirement when changing `WriteBufferManager` from `kFlushLargestAcrossDBs` to `kFlushOldest`. The coordinator could pass its active-policy check, consume the policy-change notification during refresh, and enter a one-second idle wait. This delayed retirement beyond the 300 ms deadline in `WriteBufferManagerTest.FlushPolicyRetiresAndRestartsHandoffWorkers`. Recheck the policy under `sorter_wait_mu` before waiting so the coordinator promptly enters the retirement path. The setter remains non-blocking, and retirement still waits for executor callbacks to finish. Also fix startup pressure-wait accounting in `BackgroundFlushCycleTest.FastRejectedCandidateDoesNotAddPressureDelay`. Registering the healthy candidate can invalidate the coordinator's initial ranking snapshot and cause a legitimate pressure wait before either handoff runs. The old assertion counted waits from startup and incorrectly attributed that wait to the rejected candidate. Record the counts at rejected and healthy dispatch and require them to match, preserving the check that rejection adds no pressure interval. Include both fixes in this diff so its `write_buffer_manager_test` target covers both corrected behaviors. Replace worker retirement and restart polling with sync-point synchronization. The rejection test deterministically invalidates the first snapshot during healthy-candidate registration, verifies that a startup wait occurred, and checks that advancing to the healthy candidate adds no further pressure wait. Retain the clang-format 21 line wrapping required by CI. Reviewed By: pdillinger Differential Revision: D124377137 fbshipit-source-id: 1420196e47ccba0268d0427c4ed623925aa3ad5b |
||
|
Josh Kang
|
da82001d81 |
Mark overriding methods and destructors override (#15366)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15366 ## Summary RocksDB's `.clang-tidy` enables `modernize-use-override`, and clang reports `-Winconsistent-missing-destructor-override` when a class marks its other overrides `override` but not its destructor. A full scan finds 190 `modernize-use-override`, 100 `inconsistent-missing-destructor-override` and 2 `suggest-override` findings in existing code. Each one comes back whenever a nearby line is edited. They also leave classes where it isn't obvious which methods are overrides and which introduce new virtuals. This diff marks every overriding member function and destructor `override` and drops the redundant `virtual` on them. It also changes `final override` to plain `final`, following the check's default. Methods that introduce a new virtual keep `virtual`. Most edits are clang-tidy's own fix-its, applied mechanically and then clang-formatted. Compatibility: no signatures change. In public headers the only effect is `override` on existing overrides (mostly destructors, plus `Cache::Name()` and `Cache::GetPrintableOptions()`, whose base declarations are unconditional), so user code that subclasses these types is unaffected. Every touched class now marks all of its overrides consistently. This matters because a partially-marked class would make clang emit `-Winconsistent-missing-override` in user builds. Reviewed By: rban1 Differential Revision: D123186308 fbshipit-source-id: 1f489ea32d938e3ec0faf9abc4f7ff49e042d37a |
||
|
Peter Dillinger
|
72beaf8dfb |
Update RocksDB main for 11.12 release (#15372)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15372 Why: keep internal RocksDB main ready for 11.13.0 development while the 11.12 release is cut. Previous behavior: Conveyor opened a GitHub pull request for this update, which could be missed and was not stacked with the internal release diff. New behavior: Conveyor owns an internal Phabricator diff that the release-import diff stacks on top of. The release manager exports this diff manually with ExportIt. Implementation: - Bump version.h from 11.12.0 to 11.13.0 - Add `11.12.fb` to `check_format_compatible.sh` - Sync HISTORY.md `## 11.12.` section from `11.12.fb` - Delete 24 consumed `unreleased_history/` note file(s) from main Safety and unsupported cases: - Only version.h, check_format_compatible.sh, HISTORY.md, and consumed release-note files are allowed. - Open legacy GitHub update-main PRs block this flow; this automation never modifies, exports, or lands them. - Concurrent direct CLI invocations are unsupported; Conveyor serializes the Release Cut node to one active run. - The tool refuses unrelated commits, mismatched release metadata, and non-canonical GitHub repositories. Known limitations: - ExportIt remains manual. Bot create permission for internal_repo_rocksdb/repo is a rollout prerequisite. Reviewed By: archang19 Differential Revision: D124370612 fbshipit-source-id: fcd3864695f118bf8be316878d92be5bf8ace43c |
||
|
Andrew Chang
|
9b4b980c6a |
Check background error Status in DirSyncFailureAfterTrashFileDeleted (#15371)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15371 `DeleteSchedulerTest.DirSyncFailureAfterTrashFileDeleted` aborts with "Failed to check Status" in builds with `ASSERT_STATUS_CHECKED=1`. **Why it matters:** this breaks the `ASSERT_STATUS_CHECKED=1` `make check` CI job. Only the test is wrong; `DeleteScheduler` is unchanged. - **Problem:** the test calls `GetBackgroundErrors().size()`. The returned `std::map<std::string, Status>` holds unchecked `Status` copies, so destroying it aborts. - **Fix:** keep the map, assert its size, and check each error with `ASSERT_NOK`, as the other tests in this file do. ___ landed-with-radar-review Differential Revision: D124369694 fbshipit-source-id: 85429b0bc886f6c87af3836bfe34b5da6eb47459 |
||
|
Josh Kang
|
928527b869 |
Fix bugs and false positives from clang-analyzer/bugprone (#15325)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15325 ## Summary RocksDB's `.clang-tidy` enables the `clang-analyzer-*` and `bugprone-*` checks. A full scan of the repo still reports 78 findings from these checks in existing code. Each time a nearby line is edited, they come back as noise, and a few of them are real defects. This diff resolves 73 of the 78; upstream has since fixed one more, and the four left are listed under "Intentionally not changed". Some fixes repair real bugs. The rest make the code's intent explicit to the analyzer without changing behavior. Real bugs fixed: - `port/mmap.cc`: `MemMapping::operator=(MemMapping&&)` ran the destructor on `*this`, `memcpy`'d over a non-trivially-copyable object, then placement-new'd the source. That is undefined behavior, and `*this` can be the base subobject of a `TypedMemMapping`. It now moves the members one by one, and the previous mapping is released through a local `MemMapping`. - `port/mmap.h`: `TypedMemMapping::operator=(MemMapping&&)` had no `return *this;`, so calling it was undefined behavior. - `cache/typed_cache.h`: `if constexpr (sizeof(TCreateContext) > 0)` is always true, so the branch meant for empty create contexts (for example `BlobContentsCreator`) could never run. The check is now `!std::is_empty_v<TCreateContext>`. Existing instantiations still call the same `Create` function. - `cassandra_row_merge_test.cc`: the test reused a vector after `std::move`-ing it into `RowValue::Merge(std::vector<RowValue>&&)`. That only worked because `Merge` happens not to consume its input. The test now rebuilds the input. Behavior-preserving changes: - Virtual calls in constructors and destructors are now explicitly qualified (for example `DBImpl::CloseImpl()` in `~DBImpl`). During construction and destruction, dispatch already resolved to these functions. - Stale `/*name=*/` argument comments now match the parameter names. - `unique_ptr::release()` after a successful cache `Insert` (where the cache takes ownership) uses `[[maybe_unused]] T* released = x.release();`, the pattern already used in `table_cache.cc`. - Removed `class Mutex;` and `class StaticMeta;` forward declarations that sat in the wrong namespace. - Added default member initializers for the `TablePropertiesCollectorFactory::Context` fields that its default constructor left uninitialized. - Smaller cleanups: a redundant `cond ? 3 : 3` in a test, `sizeof(std::string)` in a memory estimate, and an explicit `Cleanable` base move via `up_cast`. Tests that intentionally use moved-from objects now carry `NOLINT(bugprone-use-after-move)`, matching the existing convention. Intentionally not changed: enum casts in tests that check invalid-value handling; three `clang-analyzer-core.NullDereference` reports the analyzer can't prove safe (the C API timestamp out-parameters in `db/c.cc`, which are only written when the caller asked for a timestamp, and the waiter-list walk in `util/async_file_reader.cc`); bit-flag enums in public headers (`FileOpenContract`, `OptionTypeFlags`), where OR-combined values are valid; ownership transfers the analyzer can't see across translation units (`partitioned_index_reader.cc`, `fs_posix.cc`); and the intentional two-separator check in `file/filename.cc`. Reviewed By: rban1 Differential Revision: D123186307 fbshipit-source-id: 623e871286b1c73263b1309cb194692194199e0f |
||
|
Andrew Chang
|
f9b62a0567 |
Release trash accounting on immediate retry and after dir sync failure (#15352)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15352 DeleteScheduler now releases a trash file's accounting in two more cases where the file is gone but its size stayed counted until restart. **Why it matters:** Leaked bytes stay in the total trash size. Once they pass `max_trash_db_ratio` of the tracked DB size, every later deletion skips rate limiting and is deleted immediately, which can cause I/O stalls. - **Immediate retry:** A failed accounted trash deletion is retried through `DeleteExistingTrashFileImpl`. When the retry deletes immediately, for example with rate limiting disabled, and the file is already gone, `DeleteFileImmediately` skipped `OnDeleteFile` and the retry entry was put back. It now untracks the file and subtracts the entry's size. The NotFound status is still returned. - **Dir sync failure:** If the unlink worked but `NewDirectory` or the dir fsync then failed, `DeleteTrashFile` reported 0 deleted bytes and skipped `OnDeleteFile`, so the accounting stayed. It now records the deleted bytes and untracks the file right after the unlink, and reports `file_gone` so `BackgroundEmptyTrash` releases the rest without parking a retry entry. The sync error is still reported as a background error. - **Details:** Adds an `IsFileGone` helper for NotFound/PathNotFound and a `DeleteScheduler::DeleteTrashFile:BeforeSyncDir` sync point for tests. Both new tests keep one more tracked file, so they check that only the gone file's bytes are released. Reviewed By: anand1976 Differential Revision: D124000703 fbshipit-source-id: 54ed8be66d5670709054a46a258ceaf72b10d899 |
||
|
Peter Dillinger
|
2e689e3c00 |
Fix hang in FlushLargestAcrossDBsPolicy test (#15365)
Summary: DBWriteBufferManagerTest.FlushLargestAcrossDBsPolicy (added in https://github.com/facebook/rocksdb/issues/15047) can hang forever under load, as seen in nightly build-linux-arm-test-full (db_write_buffer_manager_test shard 4). The test's first phase waits for other_db to flush via the OnFlushCompleted listener, then immediately starts a second phase that writes 500KB to other_db and calls TEST_ScheduleFlushOnLargestDB(), expecting other_db to be picked. But OnFlushCompleted fires before the flush thread finishes its post-flush work, including refreshing other_db's WriteBufferManager flushable-memory accounting. If the second phase runs in that window, other_db's accounting is stale and the refresh (which only TryLocks the DB mutex) can leave other_db out of the ranking, so db_ is picked and flushed instead. The test then waits in the unbounded FlushedDBRecorder::WaitForFlush(other_db) forever. The same race occasionally fails ASSERT_TRUE(TEST_ScheduleFlushOnLargestDB()) instead, when other_db still has WBM flush work outstanding. Fix the test by waiting for other_db's background work to finish (including any WBM flush job) after the first-phase flush, before starting the second phase. No production code changes. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15365 Test Plan: Reproduced by running db_write_buffer_manager_test shard 4 of 6 (as generated for a 4-core CI machine) many times concurrently on an oversubscribed machine. In an interleaved A/B of 1500 runs each, the unmodified test hung 71 times (all at the second-phase WaitForFlush, confirmed via stack dumps) and failed the line-995 assertion 16 times; with this change there were no hangs or failures of this test. DB LOG thread IDs from hung runs confirmed the test thread had flushed db_ in the second phase. The hang rate was similar before https://github.com/facebook/rocksdb/issues/15331, so the race dates to https://github.com/facebook/rocksdb/issues/15047. Reviewed By: rban1 Differential Revision: D124196572 Pulled By: pdillinger fbshipit-source-id: d621c5a0b441634fc339283caf9ad7a0d6eea401 |
||
|
Andrew Chang
|
78a9d3c98c |
Release trash accounting when a queued trash file vanishes (#15349)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15349 DeleteScheduler now releases the accounting for a queued trash file that disappears before it is deleted. Before this change, that size leaked until the process restarted. **Why it matters:** The leaked bytes stay in the total trash size. Once they exceed `max_trash_db_ratio` of the tracked DB size, every later deletion bypasses rate limiting and is deleted immediately, which can cause I/O stalls. - **Problem:** If a trash file's directory is renamed or removed while the file waits in the delete queue, `DeleteTrashFile` fails with PathNotFound. `BackgroundEmptyTrash` then keeps the file's remaining accounted bytes in `total_trash_size_` and records it in `failed_accounted_trash_deletions_` for a retry. The retry never happens, because nothing schedules that path again. The file also stays tracked in `SstFileManager`, so the total size is inflated too. - **Fix:** On NotFound/PathNotFound, `DeleteTrashFile` untracks the file via `OnDeleteFile`, and `BackgroundEmptyTrash` subtracts the file's full remaining accounted size and skips the retry map. The error is still reported through `GetBackgroundErrors()`, so that behavior is unchanged. Reviewed By: anand1976 Differential Revision: D123986893 fbshipit-source-id: 4c084241a8100ae9d28fb92edf02a4362d6f60cb |
||
|
Anand Ananthabhotla
|
39d56d5ad7 |
add block read accounting to IODispatcher (#15363)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15363 as title. we were not accurately reporting block read sizes and counts to the thread local perf context given that IODispatcher can coalesce requests, we also count any sub coalesce threshold added bytes to these totals Reviewed By: chaitanyadatye, xingbowang Differential Revision: D123941253 fbshipit-source-id: eddc33d144afa49a8137fb174008030d22f3bddd |
||
|
Ranjan Banerjee
|
dec67990f0 |
Add db_stress coverage for WriteBufferManager flush policies (#15048)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15048 Adds a validated db_stress flag for selecting the WriteBufferManager flush policy and randomizes it in db_crashtest. Crash-test generation covers all three policies; kFlushLargestAcrossDBs automatically uses two DBs sharing one manager so the cross-DB path is exercised. Reviewed By: pdillinger Differential Revision: D114638570 fbshipit-source-id: 00f664445614e083a4c59d4e2bb0896731d4bea7 |
||
|
Ranjan Banerjee
|
9b4c5422f0 |
Avoid WBM pressure delay after fast rejection (#15343)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15343 Problem The cross-DB WriteBufferManager coordinator dispatches ranked flush candidates asynchronously. A candidate can reject its handoff immediately—for example because its DB mutex is busy, writes are stopped, or equivalent WBM work is already scheduled—and finish before the sorter reaches its pressure wait. Before this change, completion called RequestRefresh(), but the sorter’s active-pressure branch did not consume that already-recorded notification. Condition-variable notifications are not retained, so if completion won the race, the sorter still slept for the full 10 ms pressure interval before trying the next ranked DB. With many quickly rejected candidates, that delay accumulated once per candidate; 10,000 rejected DBs ahead of a healthy DB could theoretically delay useful relief by roughly 100 seconds while shared memory remained above its limit. Solution Snapshot the existing refresh-request generation immediately before ProcessFlushHandoffRequest(). After processing, if the handoff is already idle, the current ranked candidate cycle remains active, and the generation advanced, the sorter knows that asynchronous work completed during the dispatch window. It consumes that persistent notification and immediately continues to the next candidate instead of entering the pressure backoff. The generation check is deliberately narrower than checking the refresh Boolean alone: it reacts only to notifications that occurred during this handoff attempt and avoids treating an older unrelated refresh as completion of the current candidate. The existing 10 ms backoff remains after a complete no-progress candidate cycle, and queued or executing handoffs retain their existing lease/wait behavior. Testing A deterministic SyncPoint test pauses the sorter only after a handoff has actually been submitted, lets a rejecting executor callback finish first, and verifies that the healthy next candidate is reached without entering the pressure-wait point. The synchronization wait is bounded so a broken ordering fails the test rather than hanging CI. Reviewed By: xingbowang Differential Revision: D123926506 fbshipit-source-id: b057e5b9a41ee0abd415f72ef2067b0d3f718384 |
||
Davis Rollman
|
7292adcc9a |
AutoHeaders: migrate to explicit headers (batch 116)
Summary: Finish OpenZL, the internal RocksDB source tree, and the released RocksDB copy, removing all three parent AutoHeaders opt-ins. Declare inferred headers while preserving aliases, globs, selectors, and visibility. RocksDB's generator lists library headers and passes test source headers through its template and shared wrapper. Header lists account for the source files compiled by each release, so the older released tree does not gain headers from newer internal test helpers. Add `release_to_fbcode.py internal_repo_rocksdb --buck-only -y` to refresh the four Buck generator files and regenerate BUCK against the existing release's source lists. The command restores its five outputs if copying or generation fails, applies the existing generated-file warning, and rejects release-commit options. It does not copy runtime sources, change the release version, clone, commit, or deploy. Both RocksDB BUCK files are produced by the maintained generators. Graft the OpenZL dev declarations into prod. Align the predictor's Folly include paths and dependencies with prod so promotion is reproducible. Add the IPNext controller transition header explicitly. Authored with Codex. Reviewed By: IanChilds Differential Revision: D123482073 fbshipit-source-id: 05ab47e592d710834da56bbea57f5f93a0aea565 |
||
|
Peter Dillinger
|
6b15f1643e |
Give nightly-candidate workflow a distinct name (#15345)
Summary: The workflow_dispatch-only nightly-candidate.yml shared the exact `name: facebook/rocksdb/nightly` with the scheduled nightly.yml. The name collision confuses GitHub's "list workflow runs by file name" REST query, so release tooling querying actions/workflows/nightly.yml/runs got a stale/partial set missing recent scheduled runs. Rename it to facebook/rocksdb/nightly-candidate, matching the pr-jobs-candidate convention, so every workflow name is unique. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15345 Test Plan: rety the integration after fix Reviewed By: xingbowang Differential Revision: D123973936 Pulled By: pdillinger fbshipit-source-id: cde1c238e6a91444f7112848081e161f54f5bf24 |
||
|
Ranjan Banerjee
|
a38cd3a439 |
Bypass stalled cross-DB WBM flush candidates (#15331)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15331 ## Motivation The parent diff already implements cross-DB flushable-memory accounting and a background sorter that ranks DBs outside the write path. This change does not alter how the best candidate is selected. The remaining liveness problem occurs after the sorter selects a DB and hands it off for execution. Previously, the coordinator tracked one remote handoff as pending and waited for that DB to report completion before advancing. A selected DB could therefore monopolize coordinator progress if its job waited in Env::LOW, blocked on the DB mutex, waited behind an existing write leader or write stall, or otherwise failed to complete promptly. Other ranked DBs could be ready to reclaim memory but would not be attempted while the first handoff remained pending. ## Solution Make cross-DB flush handoff execution bounded and bypassable. Cross-DB attempts now run on a small four-worker executor owned by the WriteBufferManager instead of consuming Env::LOW compaction capacity. The executor is created only for kFlushLargestAcrossDBs and its workers are retired when that policy is disabled. The coordinator distinguishes queued and executing handoffs. A candidate lease begins when an executor worker starts the callback, rather than while the callback is merely waiting in the executor queue. If an executing handoff exceeds its lease, the coordinator releases ownership of that attempt and advances to the next ranked candidate. The original callback is not cancelled and may still complete, so the executor and outstanding-handoff count provide a strict bound on concurrent stuck work. Each registration has a per-DB outstanding-job reservation, preventing the sorter from dispatching the same DB again while an earlier callback remains alive. The coordinator also records the identity of the active candidate, so late completion from an expired attempt cannot clear or corrupt a newer candidate lease. DB mutex acquisition, write-thread joining, memtable switching, and flush dispatch continue to happen outside registry and coordinator locks. The executor invokes the existing BackgroundCallWBMFlush path directly; actual immutable-memtable flushing still uses RocksDB normal flush infrastructure. ## Compatibility This behavior is limited to kFlushLargestAcrossDBs. Existing policies retain their previous local flush behavior, and no WBM executor threads are started for them. ## Validation Unit coverage exercises lease expiry, advancement to the next candidate, late completion, duplicate suppression, bounded worker capacity, policy changes with blocked callbacks, shutdown, and reranking after progress. The end-to-end stack was also compared against the original local policy in one matched memory-pressure run with 1,000 DBs and 16 writers. The complete largest-across-DBs policy delivered 61.0% higher throughput, 55.5% fewer WBM flushes, 37.9% lower mean latency, 47.9% lower P99 latency, 5.17x larger average flushes, and 62.8% lower peak RSS. These measurements validate the full policy stack rather than isolating the performance contribution of this liveness follow-up. Reviewed By: xingbowang Differential Revision: D123461329 fbshipit-source-id: 4ad86102c2a43beb469f7672d722674dfbda9871 |
||
|
Anand Ananthabhotla
|
45203a867f |
Add retryable ExternalTableBuilder contract (#15342)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15342 Add an ExternalTableBuilderBase::TryAdd() contract so external table builders can distinguish accepted rows, real builder errors, and clean requests to cut the current output table and retry the same row. Add external-table-facing result, reason, and context types. Existing external table builders inherit a default TryAdd() implementation that forwards to Add(), so current implementations keep their historical behavior unless they explicitly opt into retryable file cuts. Reviewed By: xingbowang Differential Revision: D123930815 fbshipit-source-id: b5701c704938759db68b7c0bf0ebeba19d176945 |
||
|
Ranjan Banerjee
|
0561153b73 |
Add configurable largest-memtable flush policies to WriteBufferManager (#15047)
Summary: Motivation A WriteBufferManager can be shared by many RocksDB instances and column families. Under the existing policy, the DB whose writer first observes memory pressure flushes its own oldest mutable memtable. That DB might own only a small fraction of the reclaimable memory, so workloads with many DBs or CFs can generate many small flushes while the largest memory consumer remains untouched. Scanning every registered DB while holding process-wide or DB mutexes would improve selection accuracy but would serialize unrelated write paths and is not viable for deployments with thousands of DBs. Previous behavior kFlushOldest was the only behavior: the write-group leader that noticed WBM pressure synchronously selected the oldest eligible mutable memtable in its own DB. The WBM had no per-DB view of reclaimable memory and could not direct reclamation to another DB sharing the same budget. New behavior This change adds configurable WriteBufferFlushPolicy modes: - kFlushOldest preserves the existing behavior and remains the default. - kFlushLargest selects the largest eligible flushable column family in the writing DB. - kFlushLargestAcrossDBs selects the DB advertising the most reclaimable memory across all DBs sharing the WBM, then flushes its largest eligible CF. Atomic-flush DBs advertise and flush the aggregate eligible memory across their CFs. High-level implementation Each writable DB registers a lightweight FlushInitiator with the shared WBM. Per-DB accounting publishes two approximate values using atomics: total mutable memory and the largest flushable CF contribution, including eligible immutable memtables waiting for flush. Allocator reporting is batched, so the allocation hot path avoids registry scans and DB mutexes. A dedicated WBM coordinator thread maintains an approximate ranked snapshot of registered DBs. Refreshes are driven by accounting changes, threshold crossings, and periodic pressure checks. Ranking and O(number of DBs) work happen on that coordinator thread; writers only update counters and signal the coordinator. Registration lifetime is protected independently so DB close can safely drain an in-flight handoff. Threading and flush handoff The coordinator reserves and pins one ranked candidate at a time, without holding a registry lock while invoking the DB. The target DB uses TryLock on its DB mutex and rejects the handoff if it is closing, paused, stopped, stalled, already handling a WBM job, or no longer has flushable memory. Rejected candidates do not block consideration of the next DB. An accepted handoff queues one LOW-priority WBM job. The LOW worker joins the target DB write thread and revalidates eligibility before switching the memtable. The resulting ordinary flush is then scheduled through RocksDB's existing flush machinery on the HIGH-priority flush pool. Keeping the write-thread join off the requesting writer avoids making one DB's write path wait on another DB's mutex or write queue. Only one manager-wide remote handoff is active at a time. Completion or cancellation clears the handoff and requests a rerank. If memory reaches the hard limit while remote coordination is delayed, a rate-limited local fallback lets a writer seal a local memtable to preserve forward progress. Write stalls, shutdown, WaitForCompact, policy changes, and DB deregistration explicitly drain or reject outstanding handoffs. Compatibility The legacy constructor and kFlushOldest behavior remain unchanged unless a new policy is explicitly selected. There are no on-disk format changes. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15047 Reviewed By: xingbowang Differential Revision: D112396106 fbshipit-source-id: 3049adc7034dd4e00d74d7d86e1d19004292d299 |
||
|
Peter Dillinger
|
1c490a4a4d |
Fix "ancester" spelling; deprecate TablePropertiesNames (#15335)
Summary: Follow-up to https://github.com/facebook/rocksdb/issues/15304, which renamed `TableProperties::creation_time` to `oldest_ancestor_time` but left the long-standing "ancester" misspelling everywhere else, so the same concept was spelled two ways (e.g. `TableProperties::oldest_ancestor_time` vs. `FileMetaData::oldest_ancester_time` and `SstFileMetaData::oldest_ancester_time`). This change makes the spelling consistent and addresses remaining review feedback on https://github.com/facebook/rocksdb/issues/15304. Change: * All internal identifiers and comments now use "ancestor": `FileMetaData::oldest_ancestor_time`, `CompactionServiceOutputFile::oldest_ancestor_time`, `kUnknownOldestAncestorTime`, `TryGetOldestAncestorTime()`, `Compaction::MinInputFileOldestAncestorTime()`, the `kOldestAncestorTime` manifest tag enum, and related locals and test sync point names. * Public API: `SstFileMetaData::oldest_ancestor_time` is the new name; `oldest_ancester_time` remains as a deprecated alias sharing storage via an anonymous union (same approach as `TableProperties::creation_time`, so size, layout, and copy semantics are unchanged). * Public API: `TablePropertiesNames::kOldestAncestorTime` is the new name; `kCreationTime` remains as a deprecated `static const std::string&` alias. A static reference member has no per-object cost and binds at constant initialization, so it adds no static-init-order exposure. * `TablePropertiesNames` as a whole is marked DEPRECATED for public use. An fbsource-wide BigGrep found no references outside RocksDB and its vendored copies, and it is only meaningful to table format internals; users should read the corresponding `TableProperties` fields. Fix: * The `table_file_creation` event log entry had the `oldest_key_time` and `newest_key_time` values swapped since https://github.com/facebook/rocksdb/issues/13083 (v10.0). Now each label is paired with its own value. Also: tools/c_api_gen/auto_simple_bindings.py resets the pending anonymous union field list after use and skips unnamed members inside the union (review suggestion); generated C API output is unchanged. Compatibility: names that are persisted or externally parsed intentionally keep the old spelling: the on-disk property name `rocksdb.creation.time`, the `"oldest_ancester_time"` key in remote compaction serialization, and the `oldest_ancester_time` / `OldestAncesterTime` labels in `VersionEdit` DebugString/DebugJSON (ldb manifest_dump output). The manifest tag value is unchanged. HISTORY.md is left as historical record. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15335 Test Plan: DBTablePropertiesTest.CreationTimeAliasesOldestAncestorTime now also verifies `TablePropertiesNames::kCreationTime` and `kOldestAncestorTime` refer to the same object, and new DBTablePropertiesTest.SstFileMetaDataOldestAncesterTimeAlias verifies the two `SstFileMetaData` names share an address and that writes through the old name are visible through the new one. Remote compaction serialization through the unchanged `"oldest_ancester_time"` key is covered by compaction_service_test; manifest encode/decode through the renamed field by version_edit_test. `make check-c-api-gen` confirms the generated C API is up to date and backward compatible. Reviewed By: jaykorean Differential Revision: D123559299 Pulled By: pdillinger fbshipit-source-id: 480106e8d539e654f31db210ef6e1de02325d604 |
||
|
generatedunixname3846135475516776
|
60f090647d |
Rocksdb Crash Test failed: Callstack - SharedState::SafeTerminate -> NonBatchedOpsStres (#15338)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15338 Reviewed By: jaykorean Differential Revision: D122865369 fbshipit-source-id: 65a67955dd79a68fe1e4f15116d40dc669e1519e |
||
|
Josh Kang
|
5b77e34985 |
Add range deletion support to external tables (#15334)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15334 External table formats cannot currently persist range tombstones, so SST ingestion and full-mode DB flushes or compactions fail when they contain `DeleteRange` entries. This change adds an opt-in contract for storing RocksDB’s canonical range-deletion block. RocksDB retains responsibility for sequence-number handling, fragmentation, and visibility through its native range-tombstone iterators. Existing factories remain unsupported by default, and existing files without range tombstones are unaffected. Missing or invalid advertised tombstone blocks are treated as corruption. RocksDB also supplies logical entry counts and sizes when a format does not persist the properties block, while preserving format-specific properties. Reviewed By: xingbowang Differential Revision: D123475827 fbshipit-source-id: de8fec401bdc311282f5d4a48b248875c682f7c9 |
||
|
Andrew Chang
|
3ea1ec56df |
Fix MultiGet reading from disk with kBlockCacheTier (#15321)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15321 `MultiGet` with `read_tier = kBlockCacheTier` now returns `Incomplete` for data blocks that are not in the block cache. Before this fix, it read them from disk. **Why it matters:** Callers use `kBlockCacheTier` to prevent disk I/O. `Get` obeyed it, but `MultiGet` did not. **Problem** - Without mmap reads, `MultiGet` sends cache misses to `RetrieveMultipleBlocks`. - That path called `MultiRead` with no `kBlockCacheTier` check. - `RetrieveBlock` (used by `Get`) has the check. **Fix** - `RetrieveMultipleBlocks`: if `read_tier == kBlockCacheTier`, set `Incomplete("no blocking io")` for each block and do no I/O. - `MultiGet`: when a block is `Incomplete`, return that status for the key, the same as `Get`. Before, the key status stayed `OK` with no value. - Release note in `unreleased_history/bug_fixes`. **Tests** - New `DBTest2.MultiGetBlockCacheTierSkipsColdSstData`: `Get` and `MultiGet` both return `Incomplete` for a key whose data block is not in the cache. - `DBBlobBasicTest.MultiGetBlobsFromCache` relied on the bug. Its inline key passed only because `MultiGet` read the data block from disk. The test now loads the block cache first, so it still checks the blob cache. Reviewed By: joshkang97 Differential Revision: D122431823 fbshipit-source-id: 262913930cb4b570ea0a4d8f4f92364de08a27f5 |
||
|
Jay Huh
|
f16157cfc5 |
Fix ClipColumnFamily dropping the file that ends at begin_key (#15320)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15320 Why the change is required: `DB::ClipColumnFamily(cf, begin_key, end_key)` keeps `[begin_key, end_key)`. It first drops whole non-L0 files outside the range with `DeleteFilesInRanges` on `(-inf, begin_key)` and `[end_key, +inf)`, but called it with the default `include_end = true`. The first range was therefore treated as `(-inf, begin_key]`, so a non-L0 file whose largest key is `begin_key` was deleted even though that key is inside the kept range. Clipping `[b, d)` on L1 files `[a, b]` and `[c, d]` lost `b`. This showed up while smoke-testing `SplitDB` (D120757697): single-key files trivially moved to the bottom level made the split destination miss its first key. New behavior: - Pass `include_end = false`, so `DeleteFilesInRanges` skips files whose largest key equals `begin_key`. The second range has no end, so it is unaffected. The remaining keys below `begin_key` are still removed by the existing `DeleteRange` and compaction steps. Reviewed By: xingbowang Differential Revision: D123306376 fbshipit-source-id: 65cb36ae730a55750ea558a6cdcd48d8fc17f0e9 |
||
|
Peter Dillinger
|
c83a84e98c |
Rename TableProperties::creation_time to oldest_ancestor_time (#15304)
Summary: The `creation_time` table property has long stored the file's oldest ancestor time (the oldest key time among the file's ancestors, falling back to flush/compaction time), not the file's creation time, which is `file_creation_time`. A TODO in table_properties.h acknowledged the misleading name. This change starts the API migration: `TableProperties::oldest_ancestor_time` is the new name, and `creation_time` remains as a deprecated alias sharing storage via an anonymous union, so existing external reads and writes continue to compile and behave identically. An anonymous union is used rather than a `uint64_t&` alias member. A reference member would delete TableProperties copy/move assignment, make the implicit copy constructor bind the copy's alias to the source object (dangling), permit writes through a const TableProperties, add 8 bytes, and break the uint64_t-range walk in TEST_SetRandomTableProperties. The union leaves size, layout, copying, constness, and offsetof unchanged. Reading the other union member is formally UB in ISO C++, but both members are uint64_t and this is supported in practice by GCC, Clang, and MSVC. All in-repo uses of the old name now use `oldest_ancestor_time`. Names that are externally observable or persisted are intentionally unchanged: * On-disk property name `rocksdb.creation.time` (`TablePropertiesNames::kCreationTime`) * The `"creation_time"` key in TableProperties serialization (used by remote compaction) * The event-log JSON key and `TableProperties::ToString()` label * Java `TableProperties.getCreationTime()` and C `rocksdb_table_properties_creation_time()` C API: the auto-binding generator (tools/c_api_gen/auto_simple_bindings.py) only considered direct FieldDecls and failed on the anonymous union's implicit unnamed field. It now treats anonymous union members as direct fields. Regenerating adds a new C getter `rocksdb_table_properties_oldest_ancestor_time()` alongside the existing `rocksdb_table_properties_creation_time()`. Follow-up: remove the deprecated `creation_time` alias in a future release. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15304 Test Plan: New unit test DBTablePropertiesTest.CreationTimeAliasesOldestAncestorTime verifies the two names share an address and that a write through the old name is visible through the new one. options_settable_test and compaction_service_test cover TableProperties serialization round trips through the unchanged "creation_time" key, now mapped to the renamed field. Reviewed By: jaykorean Differential Revision: D123079083 Pulled By: pdillinger fbshipit-source-id: 18b07d91f303a8f6de58da0c730f80d8c371505c |
||
|
Josh Kang
|
c0512c4c34 |
Remove deprecated write_global_seqno ingestion support (#15287)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15287 `IngestExternalFileOptions::write_global_seqno` has been deprecated since RocksDB 8.0 and will soon be removed from the API. It requires ingestion to mutate SST files, sync them, regenerate checksums, and expose file-offset plumbing solely for that mutation. Remove the option and its C, Java, and `ldb` APIs. Assigned sequence numbers now live only in the MANIFEST, leaving newly ingested SST bytes and whole-file checksums unchanged. ## Backward Compatibility New readers continue to support existing SSTs containing a nonzero global sequence number written by legacy ingestion. Because that old write did not update the properties-block checksum, checksum verification retries against a temporary copy with only the global-seqno value restored to its original zero placeholder. The parsed sequence number and file contents remain unchanged. This fallback only accepts mismatches explained by the legacy rewrite. Other checksum failures continue through normal corruption handling, and `verify_checksums=false` continues to bypass verification. Reviewed By: xingbowang Differential Revision: D122405856 fbshipit-source-id: 934e9bc755d58ea8d84e1c085e9769015bd8dfd7 |