mirror of
https://github.com/facebook/rocksdb.git
synced 2026-10-24 19:59:35 +03:00
main
65 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
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
|
866e8e9f06 |
Make full error-recovery flush atomic (#15293)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15293 Why the change is required: Full background-error recovery flushes all column families because WAL durability can be uncertain. A later recovery-flush failure or process crash must not persist different WAL replay boundaries for different column families. Otherwise reopen can recover only part of a write batch spanning those column families. The risk includes non-file-scoped WAL write or sync errors. A failed write can have reached the WAL without reaching the memtables. An earlier unsynced write can instead remain in the memtables while its WAL bytes are not durable; retrying the fsync after resetting the writer error does not by itself prove those bytes survived. Sequential recovery can make either case visible in only some column families after reopen. Previous behavior: File-scoped WAL-error recovery already set FlushOptions::force_atomic_flush. Other full kErrorRecovery paths flushed column families sequentially when DBOptions::atomic_flush was false. Each successful per-column-family flush independently advanced its MANIFEST log number, so a later flush failure or process crash could leave mismatched replay boundaries. New behavior: Every full kErrorRecovery atomically commits the flush results and WAL replay boundaries for all participating column families. A successful recovery commits every column family; a failed recovery commits none. If an atomic recovery attempt fails, all of its flush output is discarded and recreated by a later attempt instead of retaining output from column families that finished first. Implementation: Set FlushOptions::force_atomic_flush for every full error-recovery FlushAllColumnFamilies call. Remove the now-unreachable recovery-only behavior from FlushMemTable and assert that recovery does not enter the non-atomic path. Keep the two-write-queue sequence fence at the atomic memtable-switch point. Add an end-to-end fault-injection test for a retryable, non-file-scoped WAL sync error after a multi-column-family record is appended. It covers successful and failed recovery flushes, including a column family with no other memtable data, and verifies all-or-none behavior after reopen. Update the manual_wal_flush comment and add a release note. Safety, compatibility, and limitations: Only full kErrorRecovery changes. Normal, manual, shutdown, and kErrorRecoveryRetryFlush behavior is unchanged. File-scoped WAL-error recovery was already atomic. There are no API, ABI, option-default, or file-format changes. The additional work is limited to the cold recovery path. A failed atomic recovery can redo more flush work than the previous sequential path, but this avoids partial multi-column-family recovery. Automatic recovery with manual_wal_flush remains disabled as fatal pending separate analysis and testing; this change does not remove that workaround. Validation covers local Linux builds and focused recovery tests; platform CI for this version is pending. Reviewed By: pdillinger, rban1 Differential Revision: D122469648 fbshipit-source-id: f72845903d9b6bff70f5d881e02a4cb2e920ed72 |
||
|
Xingbo Wang
|
19b6f85266 |
Support async file open for SecondaryDB (#15294)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15294 Why the change is required: `open_files_async` reduces startup latency by moving SST validation and table-reader opening to background work, but `DB::OpenAsSecondary()` still performed that work synchronously. Previous behavior: A secondary using `open_files_async=true` replayed its MANIFEST through `ReactiveVersionSet`, synchronously verified and opened every current SST, and only then returned. The option therefore provided no startup benefit for secondary instances. New behavior: `DB::OpenAsSecondary()` can now return after installing its initial read view and start initial SST validation and table-reader pinning through the existing asynchronous opener. With `max_open_files=-1`, the opener processes all current SSTs in the background; a finite table cache can stop the eager pass at capacity and validate remaining files on demand. Later `TryCatchUpWithPrimary()` rounds remain synchronous. Async-open failures notify listeners and are latched according to the existing severity and `paranoid_checks` policy. If an error is not latched, a later read retries the file access and reports a persistent failure. A latched error requires fixing the underlying problem and reopening the secondary. Read-only instances do not start write-oriented recovery. Implementation: Initial reactive MANIFEST replay can defer SST verification and handler loading. The deferral is immutable constructor state on `ReactiveVersionSet`, is cleared before catch-up mode, and is restricted to normal secondary recovery. `VersionBuilder::GetAndClearIntermediateFiles()` now moves out and clears its own historical add-then-delete bookkeeping so callers cannot accidentally retain stale state. Secondary open schedules the background opener after publishing the read view. Its context pins the captured Versions and column families, including retained secondary handles whose column family was subsequently dropped by the primary. Existing table-reader opening performs SST size and unique-ID validation, avoiding a separate serial metadata pass. Closing the DB or canceling background work safely cancels a queued opener or waits for a running opener. Read-only async-open errors use an ErrorHandler path that notifies listeners without automatic recovery. Secondary async open explicitly rejects FIFO compaction. Safety, compatibility, and limitations: The default `open_files_async=false` path and existing primary async-open behavior are unchanged. WAL and blob recovery, as well as later secondary catch-up, remain synchronous. Follower DB and `OpenAndCompact()` do not use this deferral. There are no file-format, ABI, or public API signature changes. Secondary async open requires `skip_stats_update_on_db_open=true` and does not support FIFO compaction. Applications must keep files accessible by coordinating deletion with the primary or by using an Env/FileSystem that delays physical deletion until every instance releases a file. With asynchronous opening, `max_open_files=-1` cannot guarantee every SST is pinned before `OpenAsSecondary()` returns; the table-files-only `max_open_files=-1` workaround applies only when asynchronous opening is disabled. No completion API is added. Validation covers initial deferral and pinning, synchronous catch-up, table-reader detection of physical-size mismatch, FIFO rejection, listener-adjusted errors and synchronous read retry, retained handles after primary column-family deletion, close and explicit cancellation with no HIGH-priority worker, stopping new table opens during shutdown, primary async open, follower isolation, and remote compaction. Linux Buck builds and focused tests passed. Cross-platform CI is pending, and `make check-sources` cannot run successfully in this Sapling checkout because its script requires Git. Reviewed By: pdillinger Differential Revision: D122417547 fbshipit-source-id: 91d408469c35c4ea21165c5e7db75068e65c2389 |
||
|
Xingbo Wang
|
31675ca6d0 |
Recover from poisoned WAL handles on remote file systems (#15193)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15193 Keep `DB::Write`'s existing immediate `IOError` contract while allowing RocksDB to recover safely when a remote file system reports that a WAL handle is permanently unusable. The public API documentation makes the persisted-state boundary, reopen behavior, and safe post-resumption retry point explicit. Normal operation remains unchanged; the additional protocol is entered only after a supported file-scoped WAL error. ### Motivation A WAL write crosses an acknowledgement boundary: storage can accept the complete record and then fail before the caller receives confirmation. The resulting `IOError` therefore cannot tell RocksDB or the application whether the record is absent, partial, or complete. At the same time, the remote file handle can become permanently poisoned, so retrying that same handle can never succeed. The previous recovery path retried the poisoned handle. Simply replacing it is not sufficient: RocksDB must also prevent reuse of an ambiguous sequence range, keep every column family on one durable boundary, and prevent WAL tailers from publishing a physically complete record that successful live resumption deliberately abandoned. The new protocol quarantines the affected WALs, reserves the ambiguous sequence range, creates a replacement WAL, atomically persists acknowledged state, retires the quarantined handles, and only then resumes writes. ### Behavior changes #### 1. Recovering from a poisoned remote WAL handle - **Why required:** A remote file system can permanently poison a WAL handle after an I/O failure. Retrying that handle cannot succeed and leaves the database write-stopped. - **Previous behavior:** RocksDB retried the same WAL handle during recovery. - **New behavior:** The remote file system marks the failure as file-scoped. RocksDB creates a replacement WAL, atomically persists all state that could depend on the failed WAL, and then removes the quarantined WAL handles from active use. Physical file deletion remains part of normal cleanup. #### 2. Preserving an indeterminate WAL write outcome - **Why required:** A WAL record can reach storage before the append returns `IOError`; neither RocksDB nor the application can know the outcome from that error alone. - **Previous behavior:** Recovery could reuse a file-scoped failed write group's sequence numbers, allowing a later acknowledged write to overlap a record that might already be in the WAL. The public write API did not explain the acknowledgement ambiguity or distinguish live resumption from close/reopen recovery. - **New behavior:** `DB::Write` still returns the original `IOError` immediately and does not retry the logical batch. Normal DB reads can continue, but the failed write is not visible in that DB session. The persisted DB remains indeterminate with respect to the failed write until supported file-scoped live resumption succeeds without closing or reopening the DB. On reopen, or for any other WAL error without the durable-abandonment guarantee, applications must resolve consistency within RocksDB's documented guarantees, including `WALRecoveryMode`, through idempotency, deduplication, reconciliation, or an equivalent mechanism. After successful supported file-scoped live resumption, RocksDB has durably abandoned the failed batch and reserved its sequence range; retrying the same logical write is then recommended to preserve one linear logical history. #### 3. Keeping TransactionLogIterator consistent with live resumption - **Why required:** A physically complete error-returned record can remain readable from its WAL even though supported file-scoped recovery excludes that batch from the database. Returning it from `GetUpdatesSince()` would make replication or change-data-capture consumers diverge from the recovered DB. - **Previous behavior:** An iterator opened before recovery could return the abandoned batch. A new iterator could also return it while the physical WAL remained visible. - **New behavior:** For file-scoped failures, RocksDB records the exact sequence range and WAL number of each indeterminate write. For the lifetime of that open DB, existing and new WAL iterators stop with `Status::NotFound` before the range, whether the physical record is complete or absent. A caller can begin a new iterator after the reserved range. Iterator creation snapshots the recovery-gap generation together with its WAL-file view so a failure on a later, unseen WAL returns `TryAgain` for the old iterator instead of skipping its valid prefix. #### 4. Keeping all column families on one recovery boundary - **Why required:** A write batch can update multiple column families. Retiring its WAL after independently persisting only some column families can expose a partially applied batch after reopen. - **Previous behavior:** With `atomic_flush=false`, recovery could persist column families independently before abandoning the WAL. - **New behavior:** File-scoped WAL recovery force-flushes every live column family atomically and verifies that each one advanced beyond the failed WAL before writers are retired or writes resume. #### 5. Merging concurrent WAL failures - **Why required:** Another WAL write can fail while an earlier recovery attempt temporarily releases the DB mutex. - **Previous behavior:** A later failed WAL cutoff or ambiguous sequence could be overwritten or cleared by the earlier recovery attempt. - **New behavior:** Each error episode monotonically merges the largest failed WAL cutoff and the sequence range of the latest ambiguous write group. Recovery clears the quarantine cutoff only if no concurrent failure extended it; otherwise the newer failure remains write-stopping for the next attempt. Iterator fencing tracks the actual ambiguous write range separately, so an older failed `SyncWAL()` and a newer ambiguous write are not incorrectly paired. #### 6. Retaining file scope through `NoSpace` recovery - **Why required:** `NoSpace` can still identify one permanently unusable WAL handle. - **Previous behavior:** The `SstFileManager` recovery path entered generic recovery without carrying the failed WAL context. - **New behavior:** File-scoped `NoSpace` carries the same cutoff and sequence evidence through the `SstFileManager` path as other file-scoped WAL errors. #### 7. Scoping recovery context to one error episode - **Why required:** A completed recovery must not affect a later unrelated background error. - **Previous behavior:** WAL recovery context could remain latched after `ClearBGError` succeeded. - **New behavior:** Successful background-error clearing resets the context. A failed recovery retains it so a later retry has the same safety information. The separate iterator gap record remains for the lifetime of the open DB because later WAL readers must continue to reject the abandoned physical record. #### 8. Preserving successful `SyncWAL` metadata - **Why required:** A healthy WAL can finish syncing while a separate recovery error already exists. - **Previous behavior:** `SyncWAL` could return the existing background error before applying the successful WAL's `VersionEdit`, dropping its MANIFEST update. - **New behavior:** `SyncWAL` skips quarantined writers, applies metadata for successfully synced healthy WALs, and only then returns any existing recovery error. #### 9. Classifying only the intended remote-file-system failure - **Why required:** File-scoped recovery is valid only for an actual WAL `IOError` from a handle whose ownership is still valid. - **Previous behavior:** Poison state could be queried before ownership fencing was resolved, and file scope could be stamped onto a non-I/O status. - **New behavior:** Ownership fencing is resolved first, poison state is queried afterward, and only an actual `IOError` can be classified as file-scoped. `IOFenced` remains fatal. #### 10. Preserving `paranoid_checks=false` retry behavior - **Why required:** Non-paranoid mode historically cleared a writer's sticky error so a later operation could retry after storage recovered. - **Previous behavior:** Avoiding reclassification of a cached synthetic writer error also bypassed `WALIOStatusCheck`, so later `SyncWAL` calls kept returning the stale error. - **New behavior:** A cached error still fails the current `SyncWAL` call, but the non-paranoid path resets the writer for a later retry without setting a background error or entering file-scoped recovery. #### 11. Preserving async WAL precreation across recovery - **Why required:** A precreated WAL is an unpublished replacement and is not the active WAL. Recovery can also consume the prepared slot while background work is stopped. - **Previous behavior:** A file-scoped failure while creating or starting the unpublished replacement was attributed to the active WAL, unnecessarily quarantining it. After successful recovery consumed a prepared WAL, the next slot was not replenished, so the following rotation created its WAL synchronously. - **New behavior:** Only an active WAL `WriteBuffer()` failure attaches the active-WAL cutoff. A failed unpublished candidate is discarded while the current WAL remains active, and successful recovery schedules the next async precreation after `ClearBGError()` re-enables background work. ### Implementation - Carry `DBRecoverContext` through generic, `NoSpace`, and `SstFileManager` recovery, merging WAL cutoffs and failed write-group sequence ranges independently of error severity. Generic errors retain the range only so a later retry that establishes file scope can activate the stronger protocol; they do not persist a reservation or install an iterator gap on their own. - Document the WAL acknowledgement ambiguity, persisted-state boundary, reopen behavior, application consistency requirements, and post-resumption retry guidance on the public `DB::Write` API. - Publish a lock-free quarantine cutoff used by write and sync paths; replace even an empty failed current WAL; persist file-scoped ambiguous sequence reservations; force and verify an atomic all-column-family flush; and detach quarantined writers only after that durable boundary. - Record each file-scoped ambiguous write group's exact WAL number and sequence range in a DB-lifetime tracker shared by `WalManager` and all `WalIteratorImpl` instances. Stop an iterator before a batch that contains or crosses the range, and use a generation snapshot to preserve the existing `TryAgain` behavior when an iterator's fixed WAL-file view predates the failure. - Keep successful healthy-WAL tracking edits in `SyncWAL`, reset completed recovery context, and preserve legacy non-paranoid writer reset behavior. - Distinguish failures on an unpublished replacement WAL from failures flushing the active WAL. Log the failed replacement identity, keep the active WAL out of quarantine, and replenish the async-precreation slot after successful recovery. - Classify poisoned remote-file-system handles after ownership fencing and only for `IOError`. - Add lifecycle logging and a state-machine comment covering quarantine, sequence reservation, iterator fencing, replacement, atomic persistence, retirement, and concurrent-error handoff. - Add an opt-in `db_stress` mode that injects file-scoped faults only into WAL writes, disables WAL tracking in that mode, validates unsupported direct invocations, and sanitizes crash-test parameters to the supported recovery envelope. A small fraction of ordinary non-specialized crash-test runs select this mode automatically; existing stress behavior is unchanged when the flag is off. - Make the concurrent-error regression test hold the initial background flush until `SyncWAL` records the intended older-WAL error, preventing scheduler-dependent retirement of the test WAL before fault injection. ### Safety boundaries and unsupported cases - Automatic file-scoped WAL recovery fails closed with `NotSupported` for manual WAL flush, 2PC/transactions, two write queues, pipelined or unordered writes, WAL recycling, WAL TTL or size retention, either WAL-tracking mode, or blob direct writes. - Unsupported recovery leaves the DB write-stopped. `DB::Resume()` returns `NotSupported`; after correcting the I/O condition, operational recovery requires close and reopen. - The failed logical batch is never retried. Only supported file-scoped recovery durably reserves its sequence range and fences `GetUpdatesSince()`. Generic WAL errors retain the existing indeterminate outcome and can replay after close/reopen; applications must use idempotency or reconciliation unless successful file-scoped live resumption has provided the stronger guarantee. - Failed recovery attempts retain their context. Only a successful `ClearBGError` resets it. - Physical WAL deletion is not part of the recovery correctness boundary; normal obsolete-file cleanup removes files later. - The TransactionLogIterator ambiguity fence is kept for the lifetime of the DB instance that observed the file-scoped error and is not persisted as new MANIFEST metadata. After a later close/reopen, normal WAL recovery and obsolete-WAL retention/deletion semantics apply. - The in-memory iterator-gap tracker can grow with the number of recovered file-scoped failures during one DB lifetime. Safe pruning must account for retained WALs and existing iterator snapshots; it is left for follow-up. - Async WAL precreation remains disabled with WAL recycling. A prepared WAL is unpublished until its start records succeed; failure before installation does not make the active WAL file-scoped. - The new stress mode is test-only and defaults off, so normal `db_stress` and database operation without an error are unchanged. - Automatic stress-profile selection is limited to fully parsed, supported configurations. Explicit targeted runs reject options files and other unsupported combinations. ### Tests and known limitations - The affected recovery, WAL-manager, WAL-iterator, fault-injection, async-WAL-precreation, and secondary-close binaries build successfully. - Checked-status suites pass for all 70 `error_handler_fs_test` cases, all 7 `wal_manager_test` cases, all 11 `db_log_iter_test` cases, all 8 `fault_injection_fs_test` cases, and all 5 `DBWALTest.AsyncWalPrecreate*` cases. `DBSecondaryTest.SecondaryCloseFiles` passes 20 consecutive repetitions. - Focused regressions cover file-scoped gap fencing after an iterator snapshot, generic errors not installing a file-scoped sequence gap, and later recovery discovering that a generic error is file-scoped. - Earlier validation on this diff also passed crash-test unit coverage, remote-file-system adapter tests, repeated concurrency tests, two bounded multi-column-family stress runs, and bounded black-box crash/reopen dry runs. - The exact internal RocksDB format check and `arc f --take-formatters --enforce-lint-clean` report no formatting or lint issues. - The full crash/stress matrix and TSAN were not run. Reviewed By: pdillinger Differential Revision: D119324616 fbshipit-source-id: 05779d2bff7018f0ae69c4cbbe5b6455a45675d6 |
||
Josh Kang
|
03b8d1afa0 |
Prevent normal flushes from starving error recovery (#15132)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15132 This fixes a liveness bug in background-error recovery. Retryable soft errors allow foreground writes to continue, but recovery must wait for every scheduled background flush worker to exit before it can submit the recovery flush. While recovery was active, normal flush requests could still enter `flush_queue_`. Atomic flush requests bypass per-column-family deduplication, so writes could accumulate a backlog of normal requests. For each request, the background worker: 1. Pops and drops the normal request because BG work is stopped 2. Sleeps for the one-second background-error backoff 3. Decrements `bg_flush_scheduled_`: 1 -> 0 4. Calls `MaybeScheduleFlushOrCompaction()` 5. Schedules the next queued request: 0 -> 1 6. Only then signals the recovery thread The rescheduling happens under the DB mutex before recovery is signaled: Queued requests: R1, R2, R3, ... Flush worker Recovery thread ------------ --------------- drop R1 sleep 1 second waiting for scheduled == 0 scheduled: 1 -> 0 schedule R2: 0 -> 1 signal -------------------------> wake, observe scheduled == 1 wait again drop R2 sleep 1 second scheduled: 1 -> 0 schedule R3: 0 -> 1 signal -------------------------> wake, observe scheduled == 1 wait again The dropped request is not requeued. Instead, the worker exit path guarantees that the next queued request is scheduled while `unscheduled_flushes_ > 0`. A finite backlog therefore delays recovery by roughly one second per request. If writes keep producing requests as quickly as workers discard them, the queue may never drain and recovery is starved indefinitely. Recovery can complete after writes become quiescent, so this is a liveness failure rather than a lock deadlock or data-corruption bug. There is also a queue-accounting edge. Scheduling consumes `unscheduled_flushes_`, but a worker does not own its `FlushRequest` until it pops `flush_queue_`. A worker can exit on the background error before popping, leaving: flush_queue_.size() > 0 unscheduled_flushes_ == 0 Clearing only unscheduled requests therefore misses scheduled-but-unclaimed requests. The fix: - Reject new non-recovery requests while background work is stopped. - Allow error-recovery flush reasons to bypass that guard. - Clear the entire flush queue before recovery waits for scheduled workers. - Retain the worker-side reason check for requests queued before the error. - Rebuild recovery flushes and schedule a catch-up flush for writes made during recovery. Reviewed By: pdillinger Differential Revision: D116708546 fbshipit-source-id: 6e29fd3653ef72d047c64ca89d311a7bee51ab84 |
||
|
Josh Kang
|
9475798205 |
Refresh recovery plan after error escalation (#15111)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15111 T284514290 exposed an online-recovery race between the automatic recovery coordinator and a foreground synchronous write. A retryable no-WAL flush error selected lightweight retry-flush recovery. While that recovery attempt waited for the failed background flush, `Delete(sync=true)` appended and synced sequence N to the active WAL, then failed while persisting WAL-tracking metadata to the MANIFEST. Because memtable insertion and sequence publication happen after that MANIFEST step, the record was durable but logically unpublished. OLD: the concurrent error triggered another attempt, but the attempt reused a stale recovery plan. That preserved both ingredients needed for a duplicate: the first `Delete(key, N)` remained in the active WAL, while the published sequence remained `N-1`. ``` Recovery coordinator Foreground sync-write thread ------------------------------------ -------------------------------- [db_mutex held] plan = RetryFlush (copied once before the retry loop) attempt 1: ResumeImpl(plan) WaitForBackgroundWork() bg_cv_.Wait() [atomically releases db_mutex] ---------> assign sequence N append + sync Delete(key, N) [db_mutex held] MANIFEST WAL edit fails no memtable insert no LastSequence publication write returns I/O error recovery_error_ = Hard (requests another attempt) bg_error_ = Hard recover_context_ eventually becomes FullRecovery [db_mutex released] [db_mutex reacquired] <--------- ClearBGError() sees recovery_error_ attempt 1 cannot clear the error attempt 2: recovery_error_ = OK ResumeImpl(plan) ^ +-- still the cached RetryFlush plan RetryFlushesForErrorRecovery() retries the failed immutable flush only does not call SwitchMemtable() does not switch the active WAL ClearBGError() succeeds caller retries Delete(key) LastSequence is still N-1 => assigns sequence N again appends Delete(key, N) to same WAL memtable insert succeeds publishes LastSequence = N Reopen / WAL replay: first (key, N, deletion) -> memtable insert succeeds second (key, N, deletion) -> TryAgain("key+seq exists") normal WAL recovery has seq_per_batch_ == false assert(seq_per_batch_) fails ``` The cached argument to `ResumeImpl()` is therefore the direct bridge to the assertion: lightweight retry-flush recovery can report success without retiring the WAL that contains the unpublished record. Since it also does not publish sequence N, the successful caller retry writes the identical internal key into that same WAL. NEW: the same recovery coordinator consumes the concurrent error through a mutex-ordered handoff. ``` Recovery coordinator Foreground sync-write thread ------------------------------------ -------------------------------- [db_mutex held] attempt 1: plan = recover_context_ // RetryFlush ResumeImpl(plan) WaitForBackgroundWork() bg_cv_.Wait() [atomically releases db_mutex] ---------> append + sync Delete(key, seq=N) [db_mutex held] MANIFEST WAL edit fails CheckAndSet...(Hard, FullRecovery): recovery_error_ = Hard bg_error_ = Hard recover_context_ = FullRecovery [db_mutex released] [db_mutex reacquired] <--------- ClearBGError() sees recovery_error_ current attempt cannot clear the error next retry, with db_mutex held: recovery_error_ = OK plan = recover_context_ // FullRecovery ResumeImpl(plan) SwitchMemtable() switch to a new WAL clear the background error ``` The foreground unlock of `db_mutex_` happens before the recovery coordinator reacquires it and snapshots the next plan. Publishing `bg_error_` and `recover_context_` together under that mutex prevents the next attempt from observing a new severity with an old recovery requirement. `recovery_error_` is the per-attempt latch: a concurrent I/O error cannot be cleared as part of the in-flight attempt; it either stops recovery or forces another attempt to consume the updated context. If another error arrives while `ResumeImpl()` releases the mutex again, the same handoff repeats. No second recovery coordinator is needed. Manual `Resume()` now uses the retained recovery context instead of reconstructing one from the background-work scheduling flag, and the soft-error clear-only shortcut is allowed only when that flag shows there is no pending recovery work. The write ordering and `assert(seq_per_batch_)` detector are unchanged. Reviewed By: anand1976, xingbowang Differential Revision: D116106177 fbshipit-source-id: a78450c0924a326b5302ae6709e367f07bd3d5a6 |
||
Anusha Mukka
|
f56112abb7 |
Plumb Env::IOActivity into DBImpl::ResumeImpl (#14952)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14952 `DBImpl::ResumeImpl()` constructed its local `ReadOptions`/`WriteOptions` with defaults and carried a `// TODO: plumb Env::IOActivity, Env::IOPriority` comment. Those options flow into `VersionSet::LogAndApply()` (the MANIFEST write during error recovery), so the rate limiter could not attribute or prioritize that I/O correctly. This resolves the TODO for `ResumeImpl` (1 of 91 identical TODOs across the codebase) by accepting an `Env::IOActivity` parameter and constructing the options with it, following the pattern already established in `Get()`, `MultiGet()`, and `NewIterator()`, and in the `kDBOpen` paths in `db_impl_open.cc`. Error recovery is driven by flush operations (as the existing comment in `ErrorHandler::RecoverFromRetryableBGIOError` notes), so both production callers and the `TEST_ResumeImpl` hook pass `Env::IOActivity::kFlush`. Setting `io_activity` on these options is safe: `LogAndApply` does not assert `io_activity == kUnknown`. Reviewed By: pdillinger Differential Revision: D112537885 fbshipit-source-id: 59045fd2f0a6e5bbacef6e0381e03ff95cd28c51 |
||
|
Josh Kang
|
f25fb41da6 |
Add option to validate sst files in the background on DB open (#14322)
Summary: Add `open_files_async` option for faster DB startup. When enabled, SST file opening and validation is deferred to a background thread after `DB::Open` returns, reducing startup latency for databases with many SST files. WAL recovery remains synchronous. To support this, `FindTable` is extended with a pinning mechanism that stores the cache handle directly on `FileMetaData` via a new `PinnedTableReader` class, and sets the table reader atomically so subsequent reads skip cache lookups. `FileDescriptor::table_reader` is replaced with `PinnedTableReader pinned_reader` which wraps a `std::atomic<TableReader*>` with acquire/release ordering to safely handle concurrent access between the background opener and read threads. Should validations fail, the background opener sets a `kAsyncFileOpen` background error. Future read requests will look up the table reader again via the cache, and if any validations fail there it will get propagated to the user (existing behavior when `max_open_files > 0`). This feature is most useful when `max_open_files=-1`, because otherwise file opening is already capped at 16 files and DB open should be fast. ## Restrictions - This feature also is incompatible with fifo compaction because fifo compaction requires reading table properties under DB mutex. When table reader is unpinned, this may cause a DB hang. - This feature is also incompatible with `skip_stats_update_on_db_open=false` because it will result in even longer DB open ## Key changes - New `open_files_async` DB option with C, Java, and `db_bench` bindings - `BGWorkAsyncFileOpen` background worker that opens all SST files post-`DB::Open`, with shutdown awareness via `shutting_down_` flag - New `PinnedTableReader` class in `version_edit.h` — thread-safe wrapper holding `std::atomic<TableReader*>` and `Cache::Handle*` with proper acquire/release ordering. Replaces the old `FileDescriptor::table_reader` raw pointer and `FileMetaData::table_reader_handle` - Extract `LoadTableHandlersHelper` into `db/version_util.cc` — shared between `VersionBuilder::LoadTableHandlers` (for version edits during recovery) and `BGWorkAsyncFileOpen` (for base storage post-open) - `FindTable` extended with `pin_table_handle` and `out_table_reader` params — when pinning is enabled, the table reader is stored on `FileMetaData` so Get/MultiGet/Iterator skip redundant cache lookups. `FindTable` now performs the pinned-reader fast-path check internally instead of requiring callers to check `fd.table_reader` beforehand - Note: pinning is explicit (not default) because some callers create temporary `FileMetaData`s that would need to properly clean up table handles - `CompactedDBImpl` updated to use `FindTable` + pinning instead of raw `fd.table_reader` access for Get/MultiGet - New `kAsyncFileOpen` background error reason in `listener.h` and `error_handler.cc` - Add a check in ~DBImpl to ensure async file open task has not been forgotten to be scheduled in (future) subclasses of DBImpl. Certain subclasses that never use it will need to explicitly mark it. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14322 Test Plan: - `OpenFilesAsyncTest` parameterized over `num_flushes` (1, 20), `ReadType` (Get, MultiGet, Iterator), `max_open_files` (-1, 10), and `read_only` (true, false) - **ConcurrentFileAccess**: concurrent reads and compactions race with async opener - **AfterRead**: reads happen before async opener, verifying lazy open and that the opener sees already-pinned readers - **BeforeRead**: async opener completes first, verifying reads use pre-loaded table readers - **Shutdown**: DB closes before async opener starts, verifying clean cancellation with 0 file opens - **Error**: corrupted SST files, verifying `kAsyncFileOpen` background error is set and reads return corruption - **DropColumnFamily**: CF dropped before async opener runs, verifying the opener gracefully skips dropped CFs - Added to crash test ### Benchmark To simulate a high-latency remote filesystem, I set up a virtual filesystem with dm-delay using 10ms reads, 0 ms writes. ``` # Generate a DB with many L0 files TEST_TMPDIR=/data/users/jkangs/dm-delay-test/mnt ./db_bench -benchmarks=fillseq -disable_auto_compactions=true -write_buffer_size=1000 -num=1000000 ``` ``` ./db_bench -use_existing_db=true -db=/data/users/jkangs/dm-delay-test/mnt/dbbench -benchmarks=readrandom -reads=1 -report_open_timing=true -open_files_async=true -use_direct_reads -file_opening_threads=1 -skip_stats_update_on_db_open OpenDb: 25.1419 milliseconds ``` ``` ./db_bench -use_existing_db=true -db=/data/users/jkangs/dm-delay-test/mnt/dbbench -benchmarks=readrandom -reads=1 -report_open_timing=true -open_files_async=false -use_direct_reads -file_opening_threads=1 -skip_stats_update_on_db_open OpenDb: 23109.4 milliseconds ``` ### No read regressions On main branch ``` ./db_bench -use_existing_db=true -db=/dev/shm/dbbench -benchmarks=readrandom -seed=1 -threads=8 -duration=30 readrandom : 4.827 micros/op 1657100 ops/sec 30.005 seconds 49720992 operations; 183.3 MB/s (6198999 of 6198999 found) ``` On this branch ``` ./db_bench -use_existing_db=true -db=/dev/shm/dbbench -benchmarks=readrandom -seed=1 -threads=8 -duration=30 readrandom : 4.863 micros/op 1644808 ops/sec 30.007 seconds 49354992 operations; 182.0 MB/s (6099999 of 6099999 found) ./db_bench -use_existing_db=true -db=/dev/shm/dbbench -benchmarks=readrandom -seed=1 -threads=8 -duration=30 -open_files_async=true readrandom : 4.803 micros/op 1665392 ops/sec 30.004 seconds 49968992 operations; 184.2 MB/s (6222999 of 6222999 found) ``` Reviewed By: pdillinger, xingbowang Differential Revision: D93538033 Pulled By: joshkang97 fbshipit-source-id: 32ac70c112cd733b7c1e1c1e2e7ce6422318a5ae |
||
Peter Dillinger
|
b7a9d414c8 |
Fix WriteBatch atomicity and WAL recovery for some failures (#13489)
Summary: Essentially fix https://github.com/facebook/rocksdb/issues/13429 by * Avoiding publishing to readers a partial write batch written to memtable. Also clarify in DB::Write that WriteBatch is applied atomically, and improve some logging. * When we know we have written a bad write batch to WAL due to memtable insert failure, make a good effort to roll it back to make the DB recoverable. (Not compatible with all options.) Fixes https://github.com/facebook/rocksdb/issues/13429 Follow-up items: * More rigorously test and fix the code paths and option combinations where these features could be useful. * Allow default CF with disallow_memtable_writes (with caveat that violation stops writes on your open DB) Pull Request resolved: https://github.com/facebook/rocksdb/pull/13489 Test Plan: Updated existing test, manually verified the DB went into a "stopped" state at least in this example. Reviewed By: jaykorean Differential Revision: D71917670 Pulled By: pdillinger fbshipit-source-id: c9b9dfc102817fc4c160a6c7170c04011c228aaf |
||
Changyu Bi
|
159fa7741d |
Fix an assertion failure in error handler (#13251)
Summary:
we saw this [assertion](
|
||
|
Changyu Bi
|
cc30226b1f |
Deflake unit test DBErrorHandlingFSTest.AtomicFlushNoSpaceError (#13234)
Summary:
`DBErrorHandlingFSTest.AtomicFlushNoSpaceError` is flaky due to seg fault during error recovery:
```
...
frame https://github.com/facebook/rocksdb/issues/5: 0x00007f0b3ea0a9d6 librocksdb.so.9.10`rocksdb::VersionSet::GetObsoleteFiles(std::vector<rocksdb::ObsoleteFileInfo, std::allocator<rocksdb::ObsoleteFileInfo>>*, std::vector<rocksdb::ObsoleteBlobFileInfo, std::allocator<rocksdb::ObsoleteBlobFileInfo>>*, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>>*, unsigned long) [inlined] std::vector<rocksdb::ObsoleteFileInfo, std::allocator<rocksdb::ObsoleteFileInfo>>::begin(this=<unavailable>) at stl_vector.h:812:16
frame https://github.com/facebook/rocksdb/issues/6: 0x00007f0b3ea0a9d6 librocksdb.so.9.10`rocksdb::VersionSet::GetObsoleteFiles(this=0x0000000000000000, files=size=0, blob_files=size=0, manifest_filenames=size=0, min_pending_output=18446744073709551615) at version_set.cc:7258:18
frame https://github.com/facebook/rocksdb/issues/7: 0x00007f0b3e8ccbc0 librocksdb.so.9.10`rocksdb::DBImpl::FindObsoleteFiles(this=<unavailable>, job_context=<unavailable>, force=<unavailable>, no_full_scan=<unavailable>) at db_impl_files.cc:162:30
frame https://github.com/facebook/rocksdb/issues/8: 0x00007f0b3e85e698 librocksdb.so.9.10`rocksdb::DBImpl::ResumeImpl(this=<unavailable>, context=<unavailable>) at db_impl.cc:434:20
frame https://github.com/facebook/rocksdb/issues/9: 0x00007f0b3e921516 librocksdb.so.9.10`rocksdb::ErrorHandler::RecoverFromBGError(this=<unavailable>, is_manual=<unavailable>) at error_handler.cc:632:46
```
I suspect this is due to DB being destructed and reopened during recovery. Specifically, the [ClearBGError() call](
|
||
|
Changyu Bi
|
f97e33454f |
Fix a bug with auto recovery on WAL write error (#12995)
Summary:
A recent crash test failure shows that auto recovery from WAL write failure can cause CFs to be inconsistent. A unit test repro in P1569398553. The following is an example sequence of events:
```
0. manual_wal_flush is true. There are multiple CFs in a DB.
1. Submit a write batch with updates to multiple CF
2. A FlushWAL or a memtable swtich that will try to write the buffered WAL data. Fail this write so that buffered WAL data is dropped:
|
||
Hui Xiao
|
4ff35afb42 |
Fix a bug where OnErrorRecoveryBegin() is not called before auto-recovery (#12860)
Summary:
**Context/Summary:**
`*auto_recovery` needs to be set true in order for `OnErrorRecoveryBegin()` to be called before auto-recovery
|
||
Changyu Bi
|
a31fe52173 |
Remove the return value of SetBGError() (#12792)
Summary: the return value for `ErrorHandler::SetBGError(error)` seems to be not well-defined, it can be `bg_error_` (no matter if the `bg_error_` is set to the input error), ok status or [`recovery_error_`]( |
||
|
Peter Dillinger
|
54cb9c77d9 |
Prefer static_cast in place of most reinterpret_cast (#12308)
Summary: The following are risks associated with pointer-to-pointer reinterpret_cast: * Can produce the "wrong result" (crash or memory corruption). IIRC, in theory this can happen for any up-cast or down-cast for a non-standard-layout type, though in practice would only happen for multiple inheritance cases (where the base class pointer might be "inside" the derived object). We don't use multiple inheritance a lot, but we do. * Can mask useful compiler errors upon code change, including converting between unrelated pointer types that you are expecting to be related, and converting between pointer and scalar types unintentionally. I can only think of some obscure cases where static_cast could be troublesome when it compiles as a replacement: * Going through `void*` could plausibly cause unnecessary or broken pointer arithmetic. Suppose we have `struct Derived: public Base1, public Base2`. If we have `Derived*` -> `void*` -> `Base2*` -> `Derived*` through reinterpret casts, this could plausibly work (though technical UB) assuming the `Base2*` is not dereferenced. Changing to static cast could introduce breaking pointer arithmetic. * Unnecessary (but safe) pointer arithmetic could arise in a case like `Derived*` -> `Base2*` -> `Derived*` where before the Base2 pointer might not have been dereferenced. This could potentially affect performance. With some light scripting, I tried replacing pointer-to-pointer reinterpret_casts with static_cast and kept the cases that still compile. Most occurrences of reinterpret_cast have successfully been changed (except for java/ and third-party/). 294 changed, 257 remain. A couple of related interventions included here: * Previously Cache::Handle was not actually derived from in the implementations and just used as a `void*` stand-in with reinterpret_cast. Now there is a relationship to allow static_cast. In theory, this could introduce pointer arithmetic (as described above) but is unlikely without multiple inheritance AND non-empty Cache::Handle. * Remove some unnecessary casts to void* as this is allowed to be implicit (for better or worse). Most of the remaining reinterpret_casts are for converting to/from raw bytes of objects. We could consider better idioms for these patterns in follow-up work. I wish there were a way to implement a template variant of static_cast that would only compile if no pointer arithmetic is generated, but best I can tell, this is not possible. AFAIK the best you could do is a dynamic check that the void* conversion after the static cast is unchanged. Pull Request resolved: https://github.com/facebook/rocksdb/pull/12308 Test Plan: existing tests, CI Reviewed By: ltamasi Differential Revision: D53204947 Pulled By: pdillinger fbshipit-source-id: 9de23e618263b0d5b9820f4e15966876888a16e2 |
||
|
Peter Dillinger
|
61ed0de600 |
Add more detail to some statuses (#12307)
Summary: and also fix comment/label on some MacOS CI jobs. Motivated by a crash test failure missing a definitive indicator of the genesis of the status: ``` file ingestion error: Operation failed. Try again.: ``` Pull Request resolved: https://github.com/facebook/rocksdb/pull/12307 Test Plan: just cosmetic changes. These statuses should not arise frequently enough to be a performance issue (copying messages). Reviewed By: jaykorean Differential Revision: D53199529 Pulled By: pdillinger fbshipit-source-id: ad83daaa5d80f75c9f81158e90fb6d9ecca33fe3 |
||
Yu Zhang
|
17042a3fb7 |
Remove misspelled tickers used in error handler (#12302)
Summary: As titled, the replacement tickers have been introduced in https://github.com/facebook/rocksdb/issues/11509 and in use since release 8.4. This PR completely removes the misspelled ones. Pull Request resolved: https://github.com/facebook/rocksdb/pull/12302 Test Plan: CI tests Reviewed By: jaykorean Differential Revision: D53196935 Pulled By: jowlyzhang fbshipit-source-id: 9c9d0d321247690db5edfdc52b4fecb2f1218979 |
||
|
Yu Zhang
|
ef342246dc |
Consolidate stats recording in error handler (#11992)
Summary: This is a non functional refactor, mostly for deduplicating the stats recording logic in error handler. Plus some documentation update and simple code dedupe. Pull Request resolved: https://github.com/facebook/rocksdb/pull/11992 Test Plan: existing tests Reviewed By: hx235 Differential Revision: D52967713 Pulled By: jowlyzhang fbshipit-source-id: d584eae1a06410438f5a4c59c2cb67666ea7de1a |
||
Andrew Kryczka
|
5a9ecf6614 |
Automated modernization (#12210)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/12210 Reviewed By: hx235 Differential Revision: D52559771 Pulled By: ajkr fbshipit-source-id: 1ccdd3a0180cc02bc0441f20b0e4a1db50841b03 |
||
|
Yu Zhang
|
509947ce2c |
Quarantine files in a limbo state after a manifest error (#12030)
Summary: Part of the procedures to handle manifest IO error is to disable file deletion in case some files in limbo state get deleted prematurely. This is not ideal because: 1) not all the VersionEdits whose commit encounter such an error contain updates for files, disabling file deletion sometimes are not necessary. 2) `EnableFileDeletion` has a force mode that could make other threads accidentally disrupt this procedure in recovery. 3) Disabling file deletion as a whole is also not as efficient as more precisely tracking impacted files from being prematurely deleted. This PR replaces this mechanism with tracking such files and quarantine them from being deleted in `ErrorHandler`. These are the types of files being actively tracked in quarantine in this PR: 1) new table files and blob files from a background job 2) old manifest file whose immediately following new manifest file's CURRENT file creation gets into unclear state. Current handling is not sufficient to make sure the old manifest file is kept in case it's needed. Note that WAL logs are not part of the quarantine because `min_log_number_to_keep` is a safe mechanism and it's only updated after successful manifest commits so it can prevent this premature deletion issue from happening. We track these files' file numbers because they share the same file number space. Pull Request resolved: https://github.com/facebook/rocksdb/pull/12030 Test Plan: Modified existing unit tests Reviewed By: ajkr Differential Revision: D51036774 Pulled By: jowlyzhang fbshipit-source-id: 84ef26271fbbc888ef70da5c40fe843bd7038716 |
||
Jay Huh
|
04225a2cfa |
Fix for RecoverFromRetryableBGIOError starting with recovery_in_prog_ false (#11991)
Summary: cbi42 helped investigation and found a potential scenario where `RecoverFromRetryableBGIOError()` may start with `recovery_in_prog_ ` set as false. (and other booleans like `bg_error_` and `soft_error_no_bg_work_`) **Thread 1** - `StartRecoverFromRetryableBGIOError()`): (mutex held) sets `recovery_in_prog_ = true` **Thread 1's `recovery_thread_`** - (waits for mutex and acquires it) - `RecoverFromRetryableBGIOError()` -> `ResumeImpl()` -> `ClearBGError()`: sets `recovery_in_prog_ = false` - `ClearBGError()` -> `NotifyOnErrorRecoveryEnd()`: releases `mutex` **Thread 2** - `StartRecoverFromRetryableBGIOError()`): (mutex held) sets `recovery_in_prog_ = true` - Waits for Thread 1 (`recovery_thread_`) to finish **Thread 1's `recovery_thread_`** - re-lock mutex in `NotifyOnErrorRecoveryEnd()` - Still inside `RecoverFromRetryableBGIOError()`: sets `recovery_in_prog_ = false` - Done **Thread 2's `recovery_thread_`** - recovery thread started with `recovery_in_prog_` set as `false` # Fix - Remove double-clearing `bg_error_`, `recovery_in_prog_` and other fields after `ResumeImpl()` already returned `OK()`. - Minor typo and linter fixes in `DBErrorHandlingFSTest` Pull Request resolved: https://github.com/facebook/rocksdb/pull/11991 Test Plan: - `DBErrorHandlingFSTest::MultipleRecoveryThreads` added to reproduce the scenario. - Adding `assert(recovery_in_prog_);` at the start of `ErrorHandler::RecoverFromRetryableBGIOError()` fails the test without the fix and succeeds with the fix as expected. Reviewed By: cbi42 Differential Revision: D50506113 Pulled By: jaykorean fbshipit-source-id: 6dabe01e9ecd3fc50bbe9019587f2f4858bed9c6 |
||
|
Hui Xiao
|
0f141352d8 |
Fix race between flush error recovery and db destruction (#12002)
Summary: **Context:** DB destruction will wait for ongoing error recovery through `EndAutoRecovery()` and join the recovery thread: |
||
|
Yu Zhang
|
933ee295f4 |
Fix a race condition between recovery and backup (#11955)
Summary: A race condition between recovery and backup can happen with error messages like this: ```Failure in BackupEngine::CreateNewBackup with: IO error: No such file or directory: While opening a file for sequentially reading: /dev/shm/rocksdb_test/rocksdb_crashtest_whitebox/002653.log: No such file or directory``` PR https://github.com/facebook/rocksdb/issues/6949 introduced disabling file deletion during error handling of manifest IO errors. Aformentioned race condition is caused by this chain of event: [Backup engine] disable file deletion [Recovery] disable file deletion <= this is optional for the race condition, it may or may not get called [Backup engine] get list of file to copy/link [Recovery] force enable file deletion .... some files refered by backup engine get deleted [Backup engine] copy/link file <= error no file found This PR fixes this with: 1) Recovery thread is currently forcing enabling file deletion as long as file deletion is disabled. Regardless of whether the previous error handling is for manifest IO error and that disabled it in the first place. This means it could incorrectly enabling file deletions intended by other threads like backup threads, file snapshotting threads. This PR does this check explicitly before making the call. 2) `disable_delete_obsolete_files_` is designed as a counter to allow different threads to enable and disable file deletion separately. The recovery thread currently does a force enable file deletion, because `ErrorHandler::SetBGError()` can be called multiple times by different threads when they receive a manifest IO error(details per PR https://github.com/facebook/rocksdb/issues/6949), resulting in `DBImpl::DisableFileDeletions` to be called multiple times too. Making a force enable file deletion call that resets the counter `disable_delete_obsolete_files_` to zero is a workaround for this. However, as it shows in the race condition, it can incorrectly suppress other threads like a backup thread's intention to keep the file deletion disabled. <strike>This PR adds a `std::atomic<int> disable_file_deletion_count_` to the error handler to track the needed counter decrease more precisely</strike>. This PR tracks and caps file deletion enabling/disabling in error handler. 3) for recovery, the section to find obsolete files and purge them was moved to be done after the attempt to enable file deletion. The actual finding and purging is more likely to happen if file deletion was previously disabled and get re-enabled now. An internal function `DBImpl::EnableFileDeletionsWithLock` was added to support change 2) and 3). Some useful logging was explicitly added to keep those log messages around. Pull Request resolved: https://github.com/facebook/rocksdb/pull/11955 Test Plan: existing unit tests Reviewed By: anand1976 Differential Revision: D50290592 Pulled By: jowlyzhang fbshipit-source-id: 73aa8331ca4d636955a5b0324b1e104a26e00c9b |
||
|
Andrew Kryczka
|
4bd5aa4f55 |
Fix two ErrorHandler race conditions (#11939)
Summary: 1. Prevent a double join on a `port::Thread` 2. Ensure `recovery_in_prog_` and `bg_error_` are both set under same lock hold. This is useful for writers who see a non-OK `bg_error_` and are deciding whether to stall based on whether the error will be auto-recovered. Pull Request resolved: https://github.com/facebook/rocksdb/pull/11939 Reviewed By: cbi42 Differential Revision: D50155484 Pulled By: ajkr fbshipit-source-id: fbc1f85c50e7eaee27ee0e376aee688d8a06c93b |
||
|
Andrew Kryczka
|
77d160ef47 |
Consolidate ErrorHandler's recovery status variables (#11937)
Summary: cbi42 pointed out a race condition in which `recovery_io_error_` and `recovery_error_` could be updated inconsistently due to releasing the DB mutex in `EventHelpers::NotifyOnBackgroundError()`. There doesn't seem to be a point to having two status objects, so this PR consolidates them. Pull Request resolved: https://github.com/facebook/rocksdb/pull/11937 Reviewed By: cbi42 Differential Revision: D50105793 Pulled By: ajkr fbshipit-source-id: 3de95baccfa44351a49a5c2aa0986c9bc81baa8f |
||
Yu Zhang
|
7ea6e724fa |
Mark recovery_in_prog_ to false whenever recovery thread joins (#11890)
Summary:
Make the `RecoverFromRetryableBGIOError` function always mark `recovery_in_prog_` to false when it returns.
Otherwise, in below code snippet, when db closes and the `error_handler_.CancelErrorRecovery()` call successfully joined the recovery thread, the immediately following while loop will incorrectly think the error recovery is still in progress and loops in `bg_cv_.Wait()`.
|
||
|
Changyu Bi
|
1c871a4d86 |
Only flush after recovery for retryable IOError (#11880)
Summary: https://github.com/facebook/rocksdb/issues/11872 causes a unit test to start failing with the error message below. The cause is that the additional call to `FlushAllColumnFamilies()` in `DBImpl::ResumeImpl()` can run while DB is closing. More detailed explanation: there are two places where we call `ResumeImpl()`: 1. in `ErrorHandler::RecoverFromBGError`, for manual resume or recovery from errors like OutOfSpace through sst file manager, and 2. in `Errorhandler::RecoverFromRetryableBGIOError`, for error recovery from errors like flush failure due to retryable IOError. This is tracked by `ErrorHandler::recovery_thread_`. Here is how DB close waits for error recovery: |
||
|
Changyu Bi
|
0086809601 |
Fix a bug with atomic_flush that causes DB to stuck after a flush failure (#11872)
Summary:
With atomic_flush=true, a flush job with younger memtables wait for older memtables to be installed before install its memtables. If the flush for older memtables failed, auto-recovery starts a resume thread which can becomes stuck waiting for all background work to finish (including the flush for younger memtables). If a non-recovery flush starts now and tries to flush, it can make the situation worse since it will fail due to background error but never rollback its memtable:
|
||
|
Changyu Bi
|
b927ba5936 |
Rollback other pending memtable flushes when a flush fails (#11865)
Summary:
when atomic_flush=false, there are certain cases where we try to install memtable results with already deleted SST files. This can happen when the following sequence events happen:
```
Start Flush0 for memtable M0 to SST0
Start Flush1 for memtable M1 to SST1
Flush 1 returns OK, but don't install to MANIFEST and let whoever flushes M0 to take care of it
Flush0 finishes with a retryable IOError, it rollbacks M0, (incorrectly) does not rollback M1, and deletes SST0 and SST1
Starts Flush2 for M0, it does not pick up M1 since it thought M1 is flushed
Flush2 writes SST2 and finishes OK, tries to install SST2 and SST1
Error opening SST1 since it's already deleted with an error message like the following:
IO error: No such file or directory: While open a file for random read: /tmp/rocksdbtest-501/db_flush_test_3577_4230653031040984171/000011.sst: No such file or directory
```
This happens since:
1. We currently only rollback the memtables that we are flushing in a flush job when atomic_flush=false.
2. Pending output SSTs from previous flushes are deleted since a pending file number is released whenever a flush job is finished no matter of flush status:
|
||
|
Changyu Bi
|
458acf8169 |
Add some unit tests when file read returns error during compaction/scanning (#11788)
Summary: Some repro unit tests for the bug fixed in https://github.com/facebook/rocksdb/pull/11782. Ran on main without https://github.com/facebook/rocksdb/pull/11782: ``` ./db_compaction_test --gtest_filter='*ErrorWhenReadFileHead' Note: Google Test filter = *ErrorWhenReadFileHead [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from DBCompactionTest [ RUN ] DBCompactionTest.ErrorWhenReadFileHead db/db_compaction_test.cc:10105: Failure Value of: s.IsIOError() Actual: false Expected: true [ FAILED ] DBCompactionTest.ErrorWhenReadFileHead (3960 ms) ./db_iterator_test --gtest_filter="*ErrorWhenReadFile*" Note: Google Test filter = *ErrorWhenReadFile* [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from DBIteratorTest [ RUN ] DBIteratorTest.ErrorWhenReadFile db/db_iterator_test.cc:3399: Failure Value of: (iter->status()).ok() Actual: true Expected: false [ FAILED ] DBIteratorTest.ErrorWhenReadFile (280 ms) [----------] 1 test from DBIteratorTest (280 ms total) ``` Pull Request resolved: https://github.com/facebook/rocksdb/pull/11788 Reviewed By: ajkr Differential Revision: D48940284 Pulled By: cbi42 fbshipit-source-id: 06f3c5963f576db3f85d305ffb2745ee13d209bb |
||
Ignat Loskutov
|
7c67aee4a0 |
statistics.cc: fix mistype (#11509)
Summary:
Add new tickers: `rocksdb.error.handler.bg.error.count`, `rocksdb.error.handler.bg.io.error.count`, `rocksdb.error.handler.bg.retryable.io.error.count` to replace the misspelled ones: `rocksdb.error.handler.bg.errro.count`, `rocksdb.error.handler.bg.io.errro.count`, `rocksdb.error.handler.bg.retryable.io.errro.count` ('error' instead of 'errro'). Users should switch to use the new tickers before 9.0 release as the misspelled old tickers will be completely removed then.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/11509
Reviewed By: ltamasi
Differential Revision: D46542809
Pulled By: jowlyzhang
fbshipit-source-id: a2a6d8354af46a060de81d40ef6f5336a80bd32e
|
||
sdong
|
4720ba4391 |
Remove RocksDB LITE (#11147)
Summary: We haven't been actively mantaining RocksDB LITE recently and the size must have been gone up significantly. We are removing the support. Most of changes were done through following comments: unifdef -m -UROCKSDB_LITE `git grep -l ROCKSDB_LITE | egrep '[.](cc|h)'` by Peter Dillinger. Others changes were manually applied to build scripts, CircleCI manifests, ROCKSDB_LITE is used in an expression and file db_stress_test_base.cc. Pull Request resolved: https://github.com/facebook/rocksdb/pull/11147 Test Plan: See CI Reviewed By: pdillinger Differential Revision: D42796341 fbshipit-source-id: 4920e15fc2060c2cd2221330a6d0e5e65d4b7fe2 |
||
Andrew Kryczka
|
5cf6ab6f31 |
Ran clang-format on db/ directory (#10910)
Summary: Ran `find ./db/ -type f | xargs clang-format -i`. Excluded minor changes it tried to make on db/db_impl/. Everything else it changed was directly under db/ directory. Included minor manual touchups mentioned in PR commit history. Pull Request resolved: https://github.com/facebook/rocksdb/pull/10910 Reviewed By: riversand963 Differential Revision: D40880683 Pulled By: ajkr fbshipit-source-id: cfe26cda05b3fb9a72e3cb82c286e21d8c5c4174 |
||
Wallace
|
1e9bf25f61 |
Do not hold mutex when write keys if not necessary (#7516)
Summary: ## Problem Summary RocksDB will acquire the global mutex of db instance for every time when user calls `Write`. When RocksDB schedules a lot of compaction jobs, it will compete the mutex with write thread and it will hurt the write performance. ## Problem Solution: I want to use log_write_mutex to replace the global mutex in most case so that we do not acquire it in write-thread unless there is a write-stall event or a write-buffer-full event occur. Pull Request resolved: https://github.com/facebook/rocksdb/pull/7516 Test Plan: 1. make check 2. CI 3. COMPILE_WITH_TSAN=1 make db_stress make crash_test make crash_test_with_multiops_wp_txn make crash_test_with_multiops_wc_txn make crash_test_with_atomic_flush Reviewed By: siying Differential Revision: D36908702 Pulled By: riversand963 fbshipit-source-id: 59b13881f4f5c0a58fd3ca79128a396d9cd98efe |
||
anand76
|
a88d8795ec |
Expand auto recovery to background read errors (#9679)
Summary: Fix and enhance the background error recovery logic to handle the following situations - 1. Background read errors during flush/compaction (previously was resulting in unrecoverable state) 2. Fix auto recovery failure on read/write errors during atomic flush. It was failing due to a bug in setting the resuming_from_bg_err variable in AtomicFlushMemTablesToOutputFiles. Pull Request resolved: https://github.com/facebook/rocksdb/pull/9679 Test Plan: Add new unit tests in error_handler_fs_test Reviewed By: riversand963 Differential Revision: D34770097 Pulled By: anand1976 fbshipit-source-id: 136da973a28d684b9c74bdf668519b0cbbbe1742 |
||
Peter Dillinger
|
5cb137a860 |
Work around some new clang-analyze failures (#9515)
Summary: ... seen only in internal clang-analyze runs after https://github.com/facebook/rocksdb/issues/9481 * Mostly, this works around falsely reported leaks by using std::unique_ptr in some places where clang-analyze was getting confused. (I didn't see any changes in C++17 that could make our Status implementation leak memory.) * Also fixed SetBGError returning address of a stack variable. * Also fixed another false null deref report by adding an assert. Also, use SKIP_LINK=1 to speed up `make analyze` Pull Request resolved: https://github.com/facebook/rocksdb/pull/9515 Test Plan: Was able to reproduce the reported errors locally and verify they're fixed (except SetBGError). Otherwise, existing tests Reviewed By: hx235 Differential Revision: D34054630 Pulled By: pdillinger fbshipit-source-id: 38600ef3da75ddca307dff96b7a1a523c2885c2e |
||
|
anand76
|
ecf2bec613 |
Add a listener callback for end of auto error recovery (#9244)
Summary: Previously, the OnErrorRecoveryCompleted callback was called when RocksDB was able to successfully recover from a retryable error. However, if the recovery failed and was eventually stopped, there was no indication of the status. To fix that, a new OnErrorRecoveryEnd callback is introduced that deprecates the OnErrorRecoveryCompleted callback. The new callback is called with the original error and the new error status. Pull Request resolved: https://github.com/facebook/rocksdb/pull/9244 Test Plan: Add a new unit test in error_handler_fs_test Reviewed By: zhichao-cao Differential Revision: D32922303 Pulled By: anand1976 fbshipit-source-id: f04e77a9cb92c5ea6385590682d3fcf559971b99 |
||
Zhichao Cao
|
efaef9b40a |
cleanup error_handler related code (#9098)
Summary: Remove code not in use, add comments, remove redundant code. Pull Request resolved: https://github.com/facebook/rocksdb/pull/9098 Test Plan: make check Reviewed By: anand1976 Differential Revision: D32027219 Pulled By: zhichao-cao fbshipit-source-id: 253aae926c87726268af6c027bf805dc9156c8a8 |
||
|
sdong
|
e8f218cb68 |
DB::GetSortedWalFiles() to ensure file deletion is disabled (#8591)
Summary: If DB::GetSortedWalFiles() runs without file deletion disbled, file might get deleted in the middle and error is returned to users. It makes the function hard to use. Fix it by disabling file deletion if it is not done. Fix another minor issue of logging within DB mutex, which should not be done unless a major failure happens. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8591 Test Plan: Run all existing tests Reviewed By: pdillinger Differential Revision: D29969412 fbshipit-source-id: d5f42b5271608a35b9b07687ce18157d7447b0de |
||
|
Zhichao Cao
|
87e82a41a9 |
Fix incorrect Status::NoSpace() status check (#8504)
Summary: If we want to check whether a Status s is NoSpace() or not, we should check the subcode instread of using s==Status::NoSpace(). Fix some of the incorrect check in the ErrorHandler. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8504 Test Plan: make check Reviewed By: anand1976 Differential Revision: D29601764 Pulled By: zhichao-cao fbshipit-source-id: cdab56a827891c23746bba9cbb53f169fe35f086 |
||
|
Zhichao Cao
|
58162835d1 |
All the NoSpace() errors will be handled by regular SetBGError and RecoverFromNoSpace() (#8376)
Summary: In the current logic, any IO Error with retryable flag == true will be handled by the special logic and in most cases, StartRecoverFromRetryableBGIOError will be called to do the auto resume. If the NoSpace error with retryable flag is set during WAL write, it is mapped as a hard error, which will trigger the auto recovery. During the recover process, if write continues and append to the WAL, the write process sees that bg_error is set to HardError and it calls WriteStatusCheck(), which calls SetBGError() with Status (not IOStatus). This will redirect to the regular SetBGError interface, in which recovery_error_ will be set to the corresponding error. With the recovery_error_ set, the auto resume thread created in StartRecoverFromRetryableBGIOError will keep failing as long as user keeps trying to write. To fix this issue. All the NoSpace error (no matter retryable flag is set or not) will be redirect to the regular SetBGError, and RecoverFromNoSpace() will do the recovery job which calls SstFileManager::StartErrorRecovery(). Pull Request resolved: https://github.com/facebook/rocksdb/pull/8376 Test Plan: make check and added the new testing case Reviewed By: anand1976 Differential Revision: D29071828 Pulled By: zhichao-cao fbshipit-source-id: 7171d7e14cc4620fdab49b7eff7a2fe9a89942c2 |
||
|
Zhichao Cao
|
08ec5e7321 |
Add the statistics and info log for Error handler (#8050)
Summary: Add statistics and info log for error handler: counters for bg error, bg io error, bg retryable io error, auto resume, auto resume total retry, and auto resume sucess; Histogram for auto resume retry count in each recovery call. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8050 Test Plan: make check and add test to error_handler_fs_test Reviewed By: anand1976 Differential Revision: D26990565 Pulled By: zhichao-cao fbshipit-source-id: 49f71e8ea4e9db8b189943976404205b56ab883f |
||
mrambacher
|
3dff28cf9b |
Use SystemClock* instead of std::shared_ptr<SystemClock> in lower level routines (#8033)
Summary: For performance purposes, the lower level routines were changed to use a SystemClock* instead of a std::shared_ptr<SystemClock>. The shared ptr has some performance degradation on certain hardware classes. For most of the system, there is no risk of the pointer being deleted/invalid because the shared_ptr will be stored elsewhere. For example, the ImmutableDBOptions stores the Env which has a std::shared_ptr<SystemClock> in it. The SystemClock* within the ImmutableDBOptions is essentially a "short cut" to gain access to this constant resource. There were a few classes (PeriodicWorkScheduler?) where the "short cut" property did not hold. In those cases, the shared pointer was preserved. Using db_bench readrandom perf_level=3 on my EC2 box, this change performed as well or better than 6.17: 6.17: readrandom : 28.046 micros/op 854902 ops/sec; 61.3 MB/s (355999 of 355999 found) 6.18: readrandom : 32.615 micros/op 735306 ops/sec; 52.7 MB/s (290999 of 290999 found) PR: readrandom : 27.500 micros/op 871909 ops/sec; 62.5 MB/s (367999 of 367999 found) (Note that the times for 6.18 are prior to revert of the SystemClock). Pull Request resolved: https://github.com/facebook/rocksdb/pull/8033 Reviewed By: pdillinger Differential Revision: D27014563 Pulled By: mrambacher fbshipit-source-id: ad0459eba03182e454391b5926bf5cdd45657b67 |
||
|
Zhichao Cao
|
95013df278 |
Do not set bg error for compaction in retryable IO Error case (#7899)
Summary: When retryable IO error occurs during compaction, it is mapped to soft error and set the BG error. However, auto resume is not called to clean the soft error since compaction will reschedule by itself. In this change, When retryable IO error occurs during compaction, BG error is not set. User will be informed the error via EventHelper. Pull Request resolved: https://github.com/facebook/rocksdb/pull/7899 Test Plan: tested with error_handler_fs_test Reviewed By: anand1976 Differential Revision: D26094097 Pulled By: zhichao-cao fbshipit-source-id: c53424f11d237405592cd762f43cbbdf8da8234f |
||
|
mrambacher
|
12f1137355 |
Add a SystemClock class to capture the time functions of an Env (#7858)
Summary: Introduces and uses a SystemClock class to RocksDB. This class contains the time-related functions of an Env and these functions can be redirected from the Env to the SystemClock. Many of the places that used an Env (Timer, PerfStepTimer, RepeatableThread, RateLimiter, WriteController) for time-related functions have been changed to use SystemClock instead. There are likely more places that can be changed, but this is a start to show what can/should be done. Over time it would be nice to migrate most (if not all) of the uses of the time functions from the Env to the SystemClock. There are several Env classes that implement these functions. Most of these have not been converted yet to SystemClock implementations; that will come in a subsequent PR. It would be good to unify many of the Mock Timer implementations, so that they behave similarly and be tested similarly (some override Sleep, some use a MockSleep, etc). Additionally, this change will allow new methods to be introduced to the SystemClock (like https://github.com/facebook/rocksdb/issues/7101 WaitFor) in a consistent manner across a smaller number of classes. Pull Request resolved: https://github.com/facebook/rocksdb/pull/7858 Reviewed By: pdillinger Differential Revision: D26006406 Pulled By: mrambacher fbshipit-source-id: ed10a8abbdab7ff2e23d69d85bd25b3e7e899e90 |
||
|
Zhichao Cao
|
48c0843e69 |
Treat File Scope Write IO Error the same as Retryable IO Error (#7840)
Summary: In RocksDB, when IO error happens, the flags of IOStatus can be set. If the IOStatus is set as "File Scope IO Error", it indicate that the error is constrained in the file level. Since RocksDB does not continues write data to a file when any IO Error happens, File Scope IO Error can be treated the same as Retryable IO Error. Adding the logic to ErrorHandler::SetBGError to include the file scope IO Error in its error handling logic, which is the same as retryable IO Error. Pull Request resolved: https://github.com/facebook/rocksdb/pull/7840 Test Plan: added new unit tests in error_handler_fs_test. make check Reviewed By: anand1976 Differential Revision: D25820481 Pulled By: zhichao-cao fbshipit-source-id: 69cabd3d010073e064d6142ce1cabf341b8a6806 |
||
|
mrambacher
|
db03172d08 |
Change ErrorHandler methods to return const Status& (#7539)
Summary: This change eliminates the need for a lot of the PermitUncheckedError calls on return from ErrorHandler methods. The calls are no longer needed as the status is returned as a reference rather than a copy. Additionally, this means that the originating status (recovery_error_, bg_error_) is not cleared implicitly as a result of calling one of these methods. For this class, I do not know if the proper behavior should be to call PermitUncheckedError in the destructor or if the checked state should be cleared when the status is cleared. I did tests both ways. Without the code in the destructor, the status will need to be cleared in at least some of the places where it is set to OK. When running tests, I found no instances where this class was destructed with a non-OK, non-checked Status. Pull Request resolved: https://github.com/facebook/rocksdb/pull/7539 Reviewed By: anand1976 Differential Revision: D25340565 Pulled By: pdillinger fbshipit-source-id: 1730c035c81a475875ea745226112030ec25136c |
||
|
Zhichao Cao
|
eb5a8c06dd |
Fix the thread wait case in error_handler (#7700)
Summary: In error_handler auto recovery case, if recovery_in_prog_ is false, the recover is finished or failed. In this case, the auto recovery thread should finish its execution so recovery_thread_ should be null. However, in some cases, it is not null, the caller should not directly returned. Instead, it should wait for a while and create a new thread to execute the new recovery. Pull Request resolved: https://github.com/facebook/rocksdb/pull/7700 Test Plan: make check, error_handler_fs_test Reviewed By: anand1976 Differential Revision: D25098233 Pulled By: zhichao-cao fbshipit-source-id: 5a1cba234ca18f6dd5d1be88e02d66e1d5ce931b |
||
|
Zhichao Cao
|
29e8f6a698 |
Add kManifestWriteNoWAL to BackgroundErrorReason to handle Flush IO Error when WAL is disabled (#7693)
Summary: In the current code base, all the manifest writes with IO error will be set with reason: BackgroundErrorReason::kManifestWrite, which will be mapped to the kHardError if the IO Error is retryable. However, if the system does not use the WAL, all the retryable IO error should be mapped to kSoftError. Create this PR to handle is special case by adding kManifestWriteNoWAL to BackgroundErrorReason. Pull Request resolved: https://github.com/facebook/rocksdb/pull/7693 Test Plan: make check, add new testing cases to error_handler_fs_test Reviewed By: anand1976 Differential Revision: D25066204 Pulled By: zhichao-cao fbshipit-source-id: d59553896c2eac3fb37c05238544d2b265379462 |
||
Jay Zhuang
|
1bdaef7a06 |
Status check enforcement for timestamp_basic_test (#7454)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/7454 Reviewed By: riversand963 Differential Revision: D23981719 Pulled By: jay-zhuang fbshipit-source-id: 01073f73e54c17067b886c4a2f179b2804198399 |