mirror of
https://github.com/facebook/rocksdb.git
synced 2026-10-23 08:09:02 +03:00
main
122 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
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 |
||
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 |
||
|
Xingbo Wang
|
e6a2ee0bd2 |
Fix subset checkpoints with incomplete MANIFEST tails (#15123)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15123 Subset checkpoint post-processing appends column-family drop records to a copied MANIFEST. Recovery can tolerate an incomplete atomic group at the tail while retaining the reader high-water size, so appending at that size can produce a checkpoint that fails recovery. Track the valid logical append boundary together with its MANIFEST file number, refresh it after recovery and successful writes, reset it across reactive MANIFEST switches, and trim only subset-checkpoint copies before appending drop records. Whole-DB checkpoints and the source MANIFEST remain unchanged. Reviewed By: anand1976 Differential Revision: D116449394 fbshipit-source-id: a162dd6a2558f0f9ec40d878ee83956a89df698b |
||
|
Xingbo Wang
|
bbdcd2825f |
Add remote storage liveness crash test signal (#15102)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15102 Add a dedicated Skycastle signal that invokes liveness through the existing remote-storage test-driver path. For remote DB runs, double default wrapper/no-progress timeouts while preserving explicit overrides. Default remote liveness `expected_values_dir` to local storage so `UniqueIdVerifier` never treats a remote DB path as a local path; local liveness behavior remains unchanged. Normalize both equals and spaced `--env_uri`/`--fs_uri` forms once so remote detection, workload execution, and cleanup share the same arguments. Track the crash harness and internal release scripts as CI source triggers, scoped so release-script changes select only the remote-storage signal. Add command-level regression coverage and failure-safe expected-values cleanup. Use generic remote DB/storage terminology throughout public RocksDB code and comments. Reviewed By: pdillinger Differential Revision: D115936164 fbshipit-source-id: e4997bf19eec18e4914626d479690b78c6cd3ad2 |
||
Darkhan Imangaliyev
|
5fe225357a |
Extend parallel checkpointing and db_stress coverage for column-family-subset checkpoints (#15059)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15059 **What**: 1. Extends the parallel `CheckpointEngine` so it can also checkpoint a subset of column families. 2. Adds `db_stress` and `db_crashtest.py` coverage for subset checkpoints, over both the serial and the parallel API. **How**: - **Engine API.** `CreateCheckpointOptions` gains a `std::vector<ColumnFamilyHandle*> column_families` field (empty, the default, still means all column families). This goes on the existing per-operation options truct rather than adding a second pure virtual to `CheckpointEngineBase`: it avoids overload-resolution hazards with the defaulted `sequence_number_ptr`, and it is the pattern the engine API already uses. `CreateCheckpointOptions` has no users outside the engine API, so adding a field breaks nothing. - **Shared validation.** The handle checks that were inline in `CheckpointImpl::CreateCheckpoint` -- reject null handles, handles belonging to another DB, and handles for already-dropped families; coalesce ids; always add the default CF -- move to `static CheckpointImpl::ResolveIncludeColumnFamilyIds()`, now called by both APIs. Rejecting a foreign handle matters because `handle->GetID()` alone would silently coerce a foreign id onto whichever family in this DB happens to share it. Validation runs before any staging directory is created, so a rejected request leaves nothing behind. - **Almost no new plumbing.** `CheckpointImpl::CreateCheckpointImpl()` already accepted `include_cf_ids` and is shared by the serial and parallel movers, and the MANIFEST `kColumnFamilyDrop` rewrite runs after `mover->Finish()`, so it was already mover-agnostic. The engine change is just resolving the ids and forwarding them. - **db_stress.** New `subset_cf_checkpoint_one_in` knob (mirrors `parallel_checkpoint_one_in`): each checkpoint operation covers a random proper subset with probability 1/N. It is a no-op when the DB has only the default column family, and it is independent of `parallel_checkpoint_one_in`, so subset checkpoints get exercised through both the serial API and the engine. Subset selection includes each non-default family with probability 1/2 and then force-excludes one, so every subset checkpoint exercises the MANIFEST rewrite instead of degenerating into a whole-DB checkpoint; the default handle is passed in only half the time, covering the "default is always included whether or not you pass it" contract on both sides. - **db_stress validation.** `DB::ListColumnFamilies()` on the checkpoint must return exactly the requested set -- the direct check on the MANIFEST post-processing, catching both an excluded family that was not recorded as dropped and a requested family that went missing. The checkpoint is then opened read-only with exactly those descriptors, proving the reduced MANIFEST and reduced file set are self-consistent under the default `paranoid_checks`. The pre-existing per-key comparison against `SharedState::Exists()` still runs, restricted to the families actually present. - **Incompatibility.** `clear_column_family_one_in > 0` is rejected together with the new knob, the same way D113103899 did for `abort_and_resume_cf_compactions_one_in`: a subset checkpoint hands live handles from `column_families_` to the API and validates against `column_family_names_`, and neither is stable while families are dropped and recreated underneath. `db_crashtest.py` already pins `clear_column_family_one_in` to 0. - **Test cleanup.** The subset tests shared a lot of setup and verification, so `PopulateFourCfsForSubsetCheckpoint()`, `VerifyFourCfsIntact()` and `VerifyDefaultAndTwoSubsetCheckpoint()` were extracted into the test file's anonymous namespace and are reused by the pre-existing `CheckpointSubsetOfCF` as well as the four new engine tests. This extends `StressTest::TestCheckpoint` with a mode rather than adding a separate operation, which is how `parallel_checkpoint_one_in` was integrated. Reviewed By: rban1 Differential Revision: D114832267 fbshipit-source-id: f3f843819614477497cc1466910547922cafc51d |
||
Peter Dillinger
|
af0eae4de3 |
Rename TransactionLogIterator to WalIterator and document its behavior (#15056)
Summary:
`TransactionLogIterator` iterates the write-ahead log. It does not return transactions, does not require `TransactionDB`, and in fact returns `Status::NotSupported()` for two of the three `TransactionDB` write policies (WritePrepared and WriteUnprepared). The name predates `TransactionDB` and comes from "transaction log" being an old synonym for WAL. Everything else in this header was already renamed away from that terminology -- `LogFile` -> `WalFile`, `VectorLogPtr` -> `VectorWalPtr` -- and the C API has long used `rocksdb_wal_iterator_t` and `rocksdb_wal_iter_*`. This finishes the job.
The public class is now `WalIterator` in `include/rocksdb/wal_iterator.h`. `include/rocksdb/transaction_log.h` remains as a forwarding header and `TransactionLogIterator` remains as an alias, both marked deprecated by comment only (not `[[deprecated]]`) so existing users compiling with -Werror are unaffected, matching how `LogFile` and `VectorLogPtr` were retired. `DB::GetUpdatesSince()` keeps its name.
More importantly, the documentation was incomplete in ways that could cost a user data, so this states plainly what the API has always done:
- It only sees the WAL. Writes made with `WriteOptions::disableWAL`, and sequence numbers consumed by `IngestExternalFile()`, advance the DB sequence number without producing a WAL record. They leave permanent holes that end iteration. This was not documented anywhere, so it was unreasonable to expect a user to know those features silently break WAL tailing.
- The three iterator states, in particular that `!Valid()` with an OK `status()` means caught up rather than finished, and that calling `Next()` again is the intended way to tail a DB. The old comment on `Next()` said "REQUIRES: Valid() to be true", which forbids the primary use case.
- `Status::TryAgain`, which a caller must handle but which appeared nowhere in the public headers.
- That `GetUpdatesSince(seq)` silently starts later when `seq` is unavailable. Combined with the above this is the sharp edge: recovering from a spent iterator means calling `GetUpdatesSince()` again, so the recovery step is exactly where data can be skipped unnoticed. A consumer applying these batches to a copy of the DB diverges permanently and undetectably if it misses one, so the header now says such consumers must verify continuity themselves using `BatchResult::sequence` and `WriteBatch::Count()`.
Almost all of this diff is mechanical: renaming the type and its files, updating `#include` paths, rewriting comments, and updating build lists (`src.mk`, `CMakeLists.txt`, `BUCK`, `java/Makefile`, `java/CMakeLists.txt`). `db/c.cc` is regenerated from `tools/c_api_gen/c_base.cc`. Reviewers should focus on the four places that are not:
1. `WalIteratorImpl::UpdateCurrentWriteBatch()` in `db/wal_iterator_impl.cc` (renamed from `db/transaction_log_impl.cc`) drops dead code. On a sequence discontinuity it set `current_status_` to `NotFound` and then called `SeekToStartSequence()`, which returns immediately when `current_status_` is not OK. That guard and the early return in `Next()` were both added by
|
||
Ranjan Banerjee
|
cda4a73a19 |
Fsync destination files in parallel CheckpointEngine copies (#15038)
Summary: Parallel checkpoint copies were never fsynced. `CopyEngine::CopyOrCreateFile` gated the destination sync on `use_fsync` (wired to `db_options.use_fsync`) -- but `use_fsync` means "fsync vs fdatasync", not "whether to sync". With the default `use_fsync=false`, the parallel path skipped the sync, so a copied MANIFEST/SST stayed in the unsynced buffer and was dropped by `TestFSWritableFile::Close` under `sync_fault_injection`, leaving a 0-byte file and checkpoint corruption: Corruption: no log_file_number, next_file_number, last_sequence entry in MANIFEST Corruption: Sst file size mismatch between expected N and file system 0 Serial `CopyFile` always syncs (using `use_fsync` only to pick the flavor), so it never hit this. Fix: split `WorkItem` / `CopyOrCreateFile` into `sync` (whether to sync) and `use_fsync` (fsync vs fdatasync). CheckpointEngine now passes `sync=true, use_fsync=db_options.use_fsync`, matching serial. BackupEngine is unchanged -- `sync=options_.sync` with the new `use_fsync=false` default reproduces the prior behavior. Pull Request resolved: https://github.com/facebook/rocksdb/pull/15038 Reviewed By: xingbowang Differential Revision: D114414620 Pulled By: rban1 fbshipit-source-id: e0f1f3dbadcd48c5a5c2a11111a266dbed623f86 |
||
|
Darkhan Imangaliyev
|
f9df618e4d |
Api to checkpoint only selected set of column families (#15019)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15019 RocksDB's Checkpoint API currently copies all column families. For use cases that only need a subset, this is wasteful. This diff adds a new `CreateCheckpoint(dir, column_families, ...)` overload that: - Validates input handles (rejects null/foreign handles, coalesces duplicates, always includes default CF). - Filters GetLiveFilesStorageInfo to capture only the requested CFs' table/blob files. - Post-processes the copied MANIFEST by appending kColumnFamilyDrop records for excluded CFs, so the checkpoint opens without referencing missing files. - An empty column_families vector falls through to the existing whole-DB path. Reviewed By: xingbowang, pdillinger Differential Revision: D113963616 fbshipit-source-id: d1becbbb9528dd13c2dc314165880b76123757f0 |
||
|
Ranjan Banerjee
|
f5c0c3e274 |
Record CheckpointEngine copy bytes under checkpoint tickers (#14997)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14997 `CheckpointEngine`'s shared `CopyEngine` recorded copied bytes under the `BACKUP_READ_BYTES` / `BACKUP_WRITE_BYTES` tickers (the `CopyEngineOptions` defaults), conflating checkpoint I/O with backup I/O in statistics. This adds dedicated `CHECKPOINT_READ_BYTES` / `CHECKPOINT_WRITE_BYTES` tickers and points `CheckpointEngineImpl`'s `CopyEngineOptions` at them, so checkpoint copies report under their own metric names. `BackupEngine` is unchanged (still uses the backup-ticker defaults). Addresses review feedback that the shared `CopyEngine` should not hardcode backup metric names for non-backup callers. The new tickers are intentionally not mirrored in the Java bindings (`TickerType.java` / `portal.h`): the signed-`jbyte` id space there is exhausted, so they remain C++-only, documented inline in `statistics.h`. Reviewed By: xingbowang Differential Revision: D113469742 fbshipit-source-id: 2a6a7ef04c939a1240f1c48c7bcaba8b2e7a1c10 |
||
|
Ranjan Banerjee
|
19482988e3 |
Add parallel CheckpointEngine [4/4] (#14948)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14948 Adds a reusable `CheckpointEngine` (opened once, works for any DB) that creates checkpoints via the shared `CopyEngine` pool, copying and hard-linking data files across `max_background_operations` threads. `CheckpointImpl::CreateCheckpoint` is refactored into a shared `CreateCheckpointImpl`, with the serial/parallel split behind a `CheckpointFileMover`; files the FileSystem cannot hard-link fall back to copies. The legacy `Checkpoint` API stays serial and unchanged. Reviewed By: xingbowang Differential Revision: D111387993 fbshipit-source-id: 036d375da9c9b8f051cfc3da0551b035e29d80c4 |
||
|
Josh Kang
|
dc72a64a5b |
Flush no-WAL live-file data with LockWAL (#14931)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14931 The failing crash-test scenario combines `disable_wal=1`, `LockWAL()`, and backup/checkpoint live-file-copy verification. A WAL-disabled write can leave a key only in the memtable. The old live-file path skipped `FlushForGetLiveFiles()` whenever `LockWAL()` was active, so the copied SST/WAL file set could omit that memtable-only key even though the original DB still served it. Restore verification could then report the key as present in the original DB but missing from the restored backup/checkpoint. Fix live-file capture to flush unpersisted WAL-disabled data even while `LockWAL()` is active. If all data is already persisted, the path still skips the flush while WAL is locked. If unpersisted data exists, backup/checkpoint can now block behind `LockWAL()` until another thread calls `UnlockWAL()` and the flush can safely proceed. To avoid self-deadlock, track `LockWAL()` ownership by thread with recursive counts. `UnlockWAL()` now has to be called by the same thread that successfully called `LockWAL()`, and wait=true flushes called by that same owner return `Aborted("Likely deadlock as the same thread called LockWAL()")` instead of waiting forever. Cross-thread flushes remain allowed and wait for all `LockWAL()` owners to unlock. Also audited MyRocks and can confirm they already conform to the Lock/Unlock on same thread semantics. Reviewed By: pdillinger Differential Revision: D111793089 fbshipit-source-id: 1bbb03b25d00d47440126cc6bb4ce7410302e25b |
||
|
Peter Dillinger
|
795f3bd61f |
Fix check-sources.sh non-ASCII check and remove non-ASCII from sources (#14729)
Summary: The non-ASCII character check in check-sources.sh used git grep -P (Perl regex), which requires git compiled with PCRE support. On systems without it, the command fails with exit code 128, which is != 1 (no match), so the check always reported a violation -- effectively dead. Even in CI where git has PCRE2 support, the check was silently broken: git grep -P uses PCRE2 in UTF mode by default, which interprets [\x80-\xFF] as a Unicode codepoint range (U+0080 to U+00FF). Characters like em-dash (U+2014), arrows (U+2192), and math symbols (U+2248, etc.) fall outside that range and were not detected. Only Latin-1 Supplement characters (U+0080-U+00FF) would have been caught. Replace with LC_ALL=C git grep using bash $'[\x80-\xff]' literal byte range, which works with basic regex in the C locale, and replace all non-ASCII characters in non-excluded source files: - em-dash to -- - arrow to -> - math symbols to ASCII equivalents (~=, <=, >=) - box-drawing characters to ASCII art Also exclude .github/ from the check, as scripts there can use non-ascii without disrupting RocksDB builds on non-UTF-8 systems. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14729 Test Plan: manual / CI (make check-sources passes clean) Reviewed By: hx235 Differential Revision: D104692574 Pulled By: pdillinger fbshipit-source-id: 1d884c21056dcd83558b825a04b867f1c08e3f45 |
||
|
Xingbo Wang
|
af4e32945b |
Blob direct write v1: write-path blob separation with partitioned files (reduced scope) (#14535)
Summary: This PR introduces **blob direct write v1**, a reduced-scope write-path optimization where large values (>= `min_blob_size`) are written directly to blob files during `Put()` and replaced in the memtable with compact `BlobIndex` references. This avoids holding full values in memory until flush time. ### Motivation In the existing BlobDB architecture, values are written to the WAL and memtable in their full form and separated into blob files only at flush time. This means: - Large values are held in memory twice (raw in memtable + blob file at flush) - Blob I/O is serialized through a single flush thread per column family Blob direct write addresses both: values leave the write path as small `BlobIndex` references, and multiple **partitions** (configurable via `blob_direct_write_partitions`) allow concurrent blob writes with independent locks. ### Design (v1 — single-writer, WAL-disabled, reduced scope) The v1 design intentionally keeps scope narrow for correctness and reviewability: - **Single writer thread assumption**: no concurrent writes to the same partition file. One logical writer serializes the batch. - **WAL-disabled**: direct-write blob files are only registered in MANIFEST at flush time. WAL replay cannot recover unregistered blob references, so WAL is disabled for this v1. - **Flush-on-write**: each `AddRecord` call flushes to the OS immediately. - **FIFO generation batching**: each memtable switch creates one generation batch. Direct-write files for that memtable are sealed and registered atomically when the batch is flushed to MANIFEST. - **Round-robin partitions**: blob writes are distributed across `blob_direct_write_partitions` files using an atomic counter. ### New components | Component | Description | |---|---| | `BlobFilePartitionManager` | Owns N partition files per CF. Manages open/seal/register lifecycle tied to memtable generations. | | `BlobWriteBatchTransformer` | A `WriteBatch::Handler` that rewrites qualifying `Put` values as `BlobIndex` entries before the batch enters the write group. | ### Write path integration 1. `DBImpl::WriteImpl` calls `BlobWriteBatchTransformer::TransformBatch` before entering the writer group (for default write path), or before joining the batch group (for pipelined/unordered write). 2. Values >= `min_blob_size` are written to a partition file; the key is stored with a `BlobIndex` in the transformed batch. A rollback guard marks blob bytes as initial garbage if the write fails. 3. On `SwitchMemtable`, `RotateCurrentGeneration` moves active partitions into the next immutable batch. 4. `FlushMemTableToOutputFile` / `AtomicFlushMemTablesToOutputFiles` call `PrepareFlushAdditions` to seal partition files and collect `BlobFileAddition` + `BlobFileGarbage` entries registered to MANIFEST alongside the flush. 5. Shutdown paths (`CancelAllBackgroundWork`, `WaitForCompact` with `close_db=true`) force-flush all CFs with active direct-write managers to ensure blob files are registered before close. ### Read path - **Get/MultiGet**: `MaybeResolveBlobForWritePath` resolves `BlobIndex` references found in memtable or immutable memtable via `BlobFilePartitionManager::ResolveBlobDirectWriteIndex`, which first checks manifest-visible state and falls back to direct blob-file reads via `BlobFileCache`. - **Iterator**: `DBIter::BlobReader` is extended with a `BlobFilePartitionManager*` to resolve direct-write blob indexes during iteration. The unified `ResolveBlobDirectWriteIndex` path handles both manifest-visible and not-yet-flushed files. ### New options | Option | Default | Description | |---|---|---| | `enable_blob_direct_write` | `false` | Enable write-path blob separation for this CF. Requires `enable_blob_files = true`. Not dynamically changeable. | | `blob_direct_write_partitions` | `1` | Number of parallel partition files per CF. Not dynamically changeable. | ### Feature incompatibilities (reduced v1 scope) The following features are *not supported* when `enable_blob_direct_write = true`, and are enforced both in `db_stress_tool` validation and `db_crashtest.py` sanitization: **Write model constraints:** - `threads` must be 1 (single writer assumption) - `allow_concurrent_memtable_write` = 0 - `enable_pipelined_write` = 0 (transformation done before batch group, but pipelined path supported with pre-transform) - `two_write_queues` = 0 - `unordered_write` = 0 (transformation done before batch group, but unordered path supported with pre-transform) **WAL and recovery:** - `disable_wal` = 1 (required — WAL replay of unregistered blob files is out of v1 scope) - `best_efforts_recovery` = 0 - `reopen` = 0 (no crash-restart with WAL replay) - All WAL-related stress features disabled: `manual_wal_flush_one_in`, `sync_wal_one_in`, `lock_wal_one_in`, `get_sorted_wal_files_one_in`, `get_current_wal_file_one_in`, `track_and_verify_wals`, `rate_limit_auto_wal_flush`, `recycle_log_file_num` **Blob GC and dynamic options:** - `use_blob_db` = 0 (stacked BlobDB not supported) - `allow_setting_blob_options_dynamically` = 0 - `enable_blob_garbage_collection` = 0 - `blob_compaction_readahead_size` = 0 - `blob_file_starting_level` = 0 **Unsupported value types and APIs:** - Merge (`use_merge`, `use_full_merge_v1`) — merge values pass through untransformed - Entity APIs (`use_put_entity_one_in`, `use_get_entity`, `use_multi_get_entity`, `use_attribute_group`) - `use_timed_put_one_in` - User-defined timestamps (`user_timestamp_size`, `persist_user_defined_timestamps`, `create_timestamped_snapshot_one_in`) - Transactions (`use_txn`, `use_optimistic_txn`, `test_multi_ops_txns`, `commit_bypass_memtable_one_in`) — though `WriteCommittedTxn::CommitInternal` falls back from bypass-memtable to normal path when BDW is active - `IngestWriteBatchWithIndex` returns `NotSupported` - `inplace_update_support` = 0 **Fault injection:** - All write/read/metadata fault injection disabled (`sync_fault_injection`, `write_fault_one_in`, `metadata_write_fault_one_in`, `read_fault_one_in`, `metadata_read_fault_one_in`, `open_*_fault_one_in`) **Infrastructure/snapshot APIs:** - `remote_compaction_worker_threads` = 0 - `test_secondary` = 0 - `backup_one_in` = 0 - `checkpoint_one_in` = 0 - `get_live_files_apis_one_in` = 0 - `ingest_external_file_one_in` = 0 - `ingest_wbwi_one_in` = 0 ### Tests - `db/blob/db_blob_basic_test.cc`: ~660 lines of new direct-write unit tests covering basic put/get, multi-partition, flush/compaction, recovery, and error injection. - `db/blob/blob_file_cache_test.cc`: ~96 lines of new tests for direct-write blob file cache behavior. - `db/write_batch_test.cc`: ~96 lines of tests for WriteBatch with blob index entries. - `utilities/transactions/transaction_test.cc`: verifies transaction commit path falls back correctly with direct write enabled. - `db_stress_tool/`: full stress test support with `--enable_blob_direct_write` and `--blob_direct_write_partitions` flags, integrated into `db_crashtest.py` with 10% random selection alongside regular blob params. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14535 Test Plan: ``` make -j128 db_blob_basic_test && ./db_blob_basic_test make -j128 blob_file_cache_test && ./blob_file_cache_test make -j128 write_batch_test && ./write_batch_test make -j128 transaction_test && ./transaction_test make -j128 check ``` Stress test: ``` python3 tools/db_crashtest.py blackbox --enable_blob_direct_write=1 \ --enable_blob_files=1 --blob_direct_write_partitions=4 \ --disable_wal=1 --threads=1 ``` Reviewed By: pdillinger Differential Revision: D98766843 Pulled By: xingbowang fbshipit-source-id: 1577653826913a59d05680a87bce5534ac5a5e69 |
||
Andrew Chang
|
24dc8306f8 |
Add atomic_flush to CreateBackupOptions (#14511)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14511 Add `atomic_flush` option to `CreateBackupOptions` that plumbs through `CheckpointImpl::CreateCustomCheckpoint` to `LiveFilesStorageInfoOptions`. When `flush_before_backup=true` and `atomic_flush=true`, the backup engine atomically flushes all column families before creating the backup. This ensures cross-CF consistency without needing WAL files. Combined with `BackupEngineOptions::backup_log_files=false`, this allows safely skipping WAL backup for multi-CF databases, reducing backup size and complexity. Changes: - `backup_engine.h`: Add `bool atomic_flush = false` to `CreateBackupOptions` - `checkpoint_impl.h/.cc`: Add `bool atomic_flush` parameter to `CreateCustomCheckpoint`, plumb to `LiveFilesStorageInfoOptions::atomic_flush` - `backup_engine.cc`: Pass `options.atomic_flush` through to `CreateCustomCheckpoint` - `checkpoint_test.cc`: Add `BackupWithAtomicFlushSkipsWAL` test verifying backup with atomic flush + no WAL creates a valid restorable multi-CF backup Reviewed By: xingbowang Differential Revision: D98208696 fbshipit-source-id: e818ba8669ac52a206e30e9dd56f1d7573cc0175 |
||
|
Xingbo Wang
|
056a0f9664 |
Add atomic_flush option for checkpoint (#14477)
Summary: Add `force_atomic_flush` to `FlushOptions` so any flush caller (Checkpoint, `DB::Flush()`, etc.) can force atomic flush of all column families even when `DBOptions::atomic_flush` is disabled. ## Changes ### Public API - Added `bool force_atomic_flush = false` to `FlushOptions` - Added `bool atomic_flush = false` to `LiveFilesStorageInfoOptions` ### Flush dispatch - `FlushAllColumnFamilies()` — uses `immutable_db_options_.atomic_flush || flush_options.force_atomic_flush` - `DB::Flush()` (single-CF and multi-CF) — respects `force_atomic_flush` - `FlushForGetLiveFiles()` — constructs `FlushOptions` with `force_atomic_flush` from `LiveFilesStorageInfoOptions::atomic_flush` ### Per-request atomic flush in pipeline - `FlushRequest.atomic_flush` and `BGFlushArg.atomic_flush_` carry the per-request flag - `GenerateFlushRequest`, `EnqueuePendingFlush`, `PopFirstFromFlushQueue`, `BackgroundFlush`, `FlushMemTablesToOutputFiles` all use the per-request flag ### Stress test - Added `--checkpoint_atomic_flush` flag to db_stress, randomly enabled in db_crashtest.py ### Unit tests (6 new) - End-to-end checkpoint with atomic flush override - Negative test (default = no atomic flush) - Single-CF edge case - Interaction with `DBOptions::atomic_flush=true` - Mixed non-atomic then atomic flushes in queue - Mixed atomic then non-atomic flushes in queue Pull Request resolved: https://github.com/facebook/rocksdb/pull/14477 Reviewed By: joshkang97 Differential Revision: D97639233 Pulled By: xingbowang fbshipit-source-id: b924c044d6179e68b38c6604eb46c9140516d42a |
||
Omkar Gawde
|
e43171d6ca |
Fix memory leak in ExportColumnFamily for empty column families (#14458)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/14458 In `CheckpointImpl::ExportColumnFamily()`, the assignment `*metadata = result_metadata` is inside the `for` loop over `db_metadata.levels`. If the column family has no levels, the loop body never executes, so the `new ExportImportFilesMetaData()` is leaked and the caller receives a nullptr despite a success status. Fix: Move `*metadata = result_metadata` outside the loop. Reviewed By: anand1976 Differential Revision: D95303457 fbshipit-source-id: 6a9be47bcca257803969eb3daac7b91e95143ebf |
||
|
Peter Dillinger
|
d3817f058d |
Remove deprecated DB::Open raw pointer variants (and more) (#14335)
Summary: and remove deprecated DB::MaxMemCompactionLevel(). In the process of pushing through a relatively clean refactoring of uses of the old functions, some other minor public APIs are also migrated from raw DB pointers to unique_ptr. Claude did pretty much all the work, but requiring dozens of prompts to actually push through relatively clean phase out of raw DB pointers from what needed to be touched, and leaving that code in better shape. (Hundreds of `DB*` still remain all over the place even outside C and Java bindings.) Pull Request resolved: https://github.com/facebook/rocksdb/pull/14335 Test Plan: existing tests; no functional changes intended Reviewed By: xingbowang, mszeszko-meta Differential Revision: D93523820 Pulled By: pdillinger fbshipit-source-id: e4ca22ad81cd2cfe91122d7507d7ca34fe03d043 |
||
|
Peter Dillinger
|
37176a4a44 |
Auto-tune manifest file size (#14076)
Summary:
Adds auto-tuning of manifest file size to avoid the need to scale `max_manifest_file_size` in proportion to things like number of SST files to properly balance (a) manifest file write amp and new file creation, vs. (b) manifest file space amp and replay time, including non-incremental space usage in backups. (Manifest file write amp comes from re-writing a "live" record when the manifest file is re-created, or "compacted"; space amp is usage beyond what would be used by a compacted manifest file.) In more detail,
* Add new option `max_manifest_space_amp_pct` with default value of 500, which defaults to 0.2 write amp and up to roughly 5.0 space amp, except `max_manifest_file_size` is treated as the "minimum" size before re-creating ("compacting") the manifest file.
* `max_manifest_file_size` in a way means the same thing, with the same default of 1GB, but in a way has taken on a new role. What is the same is that we do not re-create the manifest file before reaching this size (except for DB re-open), and so users are very unlikely to see a change in default behavior (auto-tuning only kicking in if auto-tuning would exceed 1GB for effective max size for the current manifest file). The new role is as a file size lower bound before auto-tuning kicks in, to minimize churn in files considered "negligibly small." We recommend a new setting of around 1MB or even smaller like 64KB, and expect something like this to become the default soon.
* These two options along with `manifest_preallocation_size` are now mutable with SetDBOptions. The effect is nearly immediate, affecting the next write to the current manifest file.
Also in this PR:
* Refactoring of VersionSet to allow it to get (more) settings from MutableDBOptions. This touches a number of files in not very interesting ways, but notably we have to be careful about thread-safe access to MutableDBOptions fields, and even fields within VersionSet. I have decided to save copies of relevant fields from MutableDBOptions to simplify testing, etc. by not saving a reference to MutableDBOptions but getting notified of updates.
* Updated some logging in VersionSet to provide some basic data about final and compacted manifest sizes (effects of auto-tuning), making sure to avoid I/O while holding DB mutex.
* Added db_etc3_test.cc which is intended as a successor to db_test and db_test2, but having "test.cc" in its name for easier exclusion of test files when using `git grep`. Intended follow-up: rename db_test2 to db_etc2_test
* Moved+updated `ManifestRollOver` test to the new file to be closer to other manifest file rollover testing.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/14076
Test Plan:
As for correctness, new unit test AutoTuneManifestSize is pretty thorough. Some other unit tests updated appropriately. Manual tests in the performance section were also audited for expected behavior based on the new logging in the DB LOG. Example LOG data with -max_manifest_file_size=2048 -max_manifest_space_amp_pct=500:
```
2025/10/24-11:12:48.979472 2150678 [/version_set.cc:5927] Created manifest 5, compacted+appended from 52 to 116
2025/10/24-11:12:49.626441 2150682 [/version_set.cc:5927] Created manifest 24, compacted+appended from 2169 to 1801
2025/10/24-11:12:52.194592 2150682 [/version_set.cc:5927] Created manifest 91, compacted+appended from 10913 to 8707
2025/10/24-11:13:02.969944 2150682 [/version_set.cc:5927] Created manifest 362, compacted+appended from 52259 to 13321
2025/10/24-11:13:18.815120 2150681 [/version_set.cc:5927] Created manifest 765, compacted+appended from 80064 to 13304
2025/10/24-11:13:35.590905 2150681 [/version_set.cc:5927] Created manifest 1167, compacted+appended from 79863 to 13304
```
As you can see, it only took a few iterations of ramp-up to settle on the auto-tuned max manifest size for tracking ~122 live SST files, around 80KB and compacting down to about 13KB. (13KB * (500 + 100) / 100 = 78KB). With the default large setting for max_manifest_file_size, we end up with a 232KB manifest, which is more than 90% wasted space. (A long-running DB would be much worse.)
As for performance, we don't expect a difference, even with TransactionDB because actual writing of the manifest is done without holding the DB mutex. I was not able to see a performance regression using db_bench with FIFO compaction and >1000 ~10MB SST files, including settings of -max_manifest_file_size=2048 -max_manifest_space_amp_pct={500,10,0}. No "hiccups" visible with -histogram either.
I also tried seeding a 1 second delay in writing new manifest files (other than the first). This had no significant effect at -max_manifest_space_amp_pct=500 but at 100 started causing write stalls in my test. In many ways this is kind of a worst case scenario and out-of-proportion test, but gives me more confidence that a higher number like 500 is probably the best balance in general.
Reviewed By: xingbowang
Differential Revision: D85445178
Pulled By: pdillinger
fbshipit-source-id: 1e6e07e89c586762dd65c65bb7cb2b8b719513f9
|
||
Changyu Bi
|
0119a8c78b |
Fix Checkpoint::ExportColumnFamily() returning staled data (#13654)
Summary: `Checkpoint::ExportColumnFamily()` calls DB::Flush() before getting all SST file metadata through `GetColumnFamilyMetaData()`. `GetColumnFamilyMetaData()` gets metadata through the SuperVersion but Flush() does not guarantee the flush result is reflected in SuperVersion upon return (explained below). This PR updates `GetColumnFamilyMetaData()` to get metadata from version instead. Since `GetColumnFamilyMetaData()` [acquires db mutex]( |
||
anand76
|
f7764cb6b2 |
Remove fail_if_options_file_error DB option (#13504)
Summary: The fail_if_options_file_error has been deprecated for more than a year. This PR removes it from the code base. https://github.com/facebook/rocksdb/issues/12056 fixed a bug that was blocking the option from removal. https://github.com/facebook/rocksdb/issues/12249 marked it as deprecated. Pull Request resolved: https://github.com/facebook/rocksdb/pull/13504 Reviewed By: hx235 Differential Revision: D72194063 Pulled By: anand1976 fbshipit-source-id: 0aa7cf56e60c48c7e7654743d3e64922ce65225d |
||
|
Peter Dillinger
|
c72e79a262 |
Standardize on clang-format version 18 (#13233)
Summary: ... which is the default for CentOS 9 and Ubuntu 24, the latter of which is now available in GitHub Actions. Relevant CI job updated. Re-formatted all cc|c|h files except in third-party/, using ``` clang-format -i `git ls-files | grep -E '[.](cc|c|h)$' | grep -v third-party/` ``` Pull Request resolved: https://github.com/facebook/rocksdb/pull/13233 Test Plan: CI Reviewed By: jaykorean, archang19 Differential Revision: D67461638 Pulled By: pdillinger fbshipit-source-id: 0c9ac21a3f5eea6f5ade68bb6af7b6ba16c8b301 |
||
Hui Xiao
|
16c21afc06 |
Fix failure to clean the temporary directory due to NotFound in crash test checkpoint creation (#12919)
Summary:
**Context/Summary:**
|
||
|
Hui Xiao
|
b26b395e0a |
Fix CreateCheckpoint not handling failed CleanStagingDirectory well (#12894)
Summary: **Context/Summary:** `CleanStagingDirectory()` is called when the temporary .tmp folder we use to create checkpoint is not empty to begin with. Expanded fault injection can make this call fail e.g, `Delete file /dev/shm/rocksdb_test/rocksdb_crashtest_blackbox/.checkpoint17.tmp/012393.sst -- IO error: injected metadata write error`. But The result of `CleanStagingDirectory()` is ignored in `CreateCheckpoint()`. So the injected IO error can't be propagated to db stress test and handled correctly. Hence we see `While mkdir: /dev/shm/rocksdb_test/rocksdb_crashtest_blackbox/.checkpoint17.tmp: File exists` when we try to re-use a non-empty .tmp folder for new snapshots. Pull Request resolved: https://github.com/facebook/rocksdb/pull/12894 Test Plan: Monitor CI Reviewed By: ltamasi Differential Revision: D60422849 Pulled By: hx235 fbshipit-source-id: 6f735c98eaa05d2b97ba4f781e0928357a50377a |
||
Yu Zhang
|
d12aaf23ca |
Fix file deletions in DestroyDB not rate limited (#12891)
Summary: Make `DestroyDB` slowly delete files if it's configured and enabled via `SstFileManager`. It's currently not available mainly because of DeleteScheduler's logic related to tracked total_size_ and total_trash_size_. These accounting and logic should not be applied to `DestroyDB`. This PR adds a `DeleteUnaccountedDBFile` util for this purpose which deletes files without accounting it. This util also supports assigning a file to a specified trash bucket so that user can later wait for a specific trash bucket to be empty. For `DestroyDB`, files with more than 1 hard links will be deleted immediately. Pull Request resolved: https://github.com/facebook/rocksdb/pull/12891 Test Plan: Added unit tests, existing tests. Reviewed By: anand1976 Differential Revision: D60300220 Pulled By: jowlyzhang fbshipit-source-id: 8b18109a177a3a9532f6dc2e40e08310c08ca3c7 |
||
HypenZou
|
5bb7f95ed6 |
Don't take archived log size into account when calculating log size for flush (#12680)
Summary: **Context/Summary:** It seems unreasonable to take the archived log size into account when calculating log size **for flush** in method CreateCheckpoint. If the user sets WAL_ttl_seconds or WAL_size_limit_MB, the argument _log_size_for_flush_ can easily be reached due to the size of the archived dir. As a result, the flush may always be triggered. **Test** corverd by ./checkpoint_test Pull Request resolved: https://github.com/facebook/rocksdb/pull/12680 Reviewed By: jaykorean Differential Revision: D59097904 Pulled By: ajkr fbshipit-source-id: 0ed29c1b078d8f40b85288541b008e00dbc517d3 |
||
|
Hui Xiao
|
1adb935720 |
Inject more errors to more files in stress test (#12713)
Summary: **Context:** We currently have partial error injection: - DB operation: all read, SST write - DB open: all read, SST write, all metadata write. This PR completes the error injection (with some limitations below): - DB operation & open: all read, all write, all metadata write, all metadata read **Summary:** - Inject retryable metadata read, metadata write error concerning directory (e.g, dir sync, ) or file metadata (e.g, name, size, file creation/deletion...) - Inject retryable errors to all major file types: random access file, sequential file, writable file - Allow db stress test operations to handle above injected errors gracefully without crashing - Change all error injection to thread-local implementation for easier disabling and enabling in the same thread. For example, we can control error handling thread to have no error injection. It's also cleaner in code. - Limitation: compared to before, we now don't have write fault injection for backup/restore CopyOrCreateFiles work threads since they use anonymous background threads as well as read injection for db open bg thread - Add a new flag to test error recovery without error injection so we can test the path where error recovery actually succeeds - Some Refactory & fix to db stress test framework (see PR review comments) - Fix some minor bugs surfaced (see PR review comments) - Limitation: had to disable backup restore with metadata read/write injection since it surfaces too many testing issues. Will add it back later to focus on surfacing actual code/internal bugs first. Pull Request resolved: https://github.com/facebook/rocksdb/pull/12713 Test Plan: - Existing UT - CI with no trivial error failure Reviewed By: pdillinger Differential Revision: D58326608 Pulled By: hx235 fbshipit-source-id: 011b5195aaeb6011641ae0a9194f7f2a0e325ad7 |
||
|
Peter Dillinger
|
0646ec6e2d |
Ensure Close() before LinkFile() for WALs in Checkpoint (#12734)
Summary: POSIX semantics for LinkFile (hard links) allow linking a file that is still being written two, with both the source and destination showing any subsequent writes to the source. This may not be practical semantics for some FileSystem implementations such as remote storage. They might only link the flushed or sync-ed file contents at time of LinkFile, or might even have undefined behavior if LinkFile is called on a file still open for write (not yet "sealed"). This change builds on https://github.com/facebook/rocksdb/issues/12731 to bring more hygiene to our handling of WAL files in Checkpoint. Specifically, we now Close WAL files as soon as they are either (a) inactive and fully synced, or (b) inactive and obsolete (so maybe never fully synced), rather than letting Close() happen in handling obsolete files (maybe a background thread). This should not be a performance issue as Close() should be trivial cost relative to other IO ops, but just in case: * We don't Close() while holding a mutex, to avoid blocking, and * The old behavior is available with a new kill switch option `background_close_inactive_wals`. Stacked on https://github.com/facebook/rocksdb/issues/12731 Pull Request resolved: https://github.com/facebook/rocksdb/pull/12734 Test Plan: Extended existing unit test, especially adding a hygiene check to FaultInjectionTestFS to detect LinkFile() on a file still open for writes. FaultInjectionTestFS already has relevant tracking data, and tests can opt out of the new check, as in a smoke test I have left for the old, deprecated functionality `background_close_inactive_wals=true`. Also ran lengthy blackbox_crash_test to ensure the hygiene check is OK with the crash test. (The only place I can find we use LinkFile in production is Checkpoint.) Reviewed By: cbi42 Differential Revision: D58295284 Pulled By: pdillinger fbshipit-source-id: 64d90ed8477e2366c19eaf9c4c5ad60b82cac5c6 |
||
|
Peter Dillinger
|
98393f0139 |
Fix Checkpoint hard link of inactive but unsynced WAL (#12731)
Summary: Background: there is one active WAL file but there can be several more WAL files in various states. Those other WALs are always in a "flushed" state but could be on the `logs_` list not yet fully synced. We currently allow any WAL that is not the active WAL to be hard-linked when creating a Checkpoint, as although it might still be open for write, we are not appending any more data to it. The problem is that a created Checkpoint is supposed to be fully synced on return of that function, and a hard-linked WAL in the state described above might not be fully synced. (Through some prudence in https://github.com/facebook/rocksdb/issues/10083, it would synced if using track_and_verify_wals_in_manifest=true.) The fix is a step toward a long term goal of removing the need to query the filesystem to determine WAL files and their state. (I consider it dubious any time we independently read from or query metadata from a file we have open for writing, as this makes us more susceptible to FileSystem deficiencies or races.) More specifically: * Detect which WALs might not be fully synced, according to our DBImpl metadata, and prevent hard linking those (with `trim_to_size=true` from `GetLiveFilesStorageInfo()`. And while we're at it, use our known flushed sizes for those WALs. * To avoid a race between that and GetSortedWalFiles(), track a maximum needed WAL number for the Checkpoint/GetLiveFilesStorageInfo. * Because of the level of consistency provided by those two, we no longer need to consider syncing as part of the FlushWAL in GetLiveFilesStorageInfo. (We determine the max WAL number consistent with the manifest file size, while holding DB mutex. Should make track_and_verify_wals_in_manifest happy.) This makes the premise of test PutRaceWithCheckpointTrackedWalSync obsolete (sync point callback no longer hit) so the test is removed, with crash test as backstop for related issues. See https://github.com/facebook/rocksdb/issues/10185 Stacked on https://github.com/facebook/rocksdb/issues/12729 Pull Request resolved: https://github.com/facebook/rocksdb/pull/12731 Test Plan: Expanded an existing test, which now fails before fix. Also long runs of blackbox_crash_test with amplified checkpoint frequency. Reviewed By: cbi42 Differential Revision: D58199629 Pulled By: pdillinger fbshipit-source-id: 376e55f4a2b082cd2adb6408a41209de14422382 |
||
|
Peter Dillinger
|
a53ed91691 |
Fix/improve temperature handling for file ingestion (#12402)
Summary: Partly following up on leftovers from https://github.com/facebook/rocksdb/issues/12388 In terms of public API: * Make it clear that IngestExternalFileArg::file_temperature is just a hint for opening the existing file, though it was previously used for both copy-from temp hint and copy-to temp, which was bizarre. * Specify how IngestExternalFile assigns temperature to file ingested into DB. (See details in comments.) This approach is not perfect in terms of matching how the DB assigns temperatures, but was the simplest way to get close. The key complication for matching DB temperature assignments is that ingestion files are copied (to a destination temp) before their target level is determined (in general). * Add a temperature option to SstFileWriter::Open so that files intended for ingestion can be initially written to a chosen temperature. * Note that "fail_if_not_bottommost_level" is obsolete/confusing use of "bottommost" In terms of the implementation, there was a similar bit of oddness with the internal CopyFile API, which only took one temperature, ambiguously applicable to the source, destination, or both. This is also fixed. Eventual suggested follow-up: * Before copying files for ingestion, determine a tentative level assignment to use for destination temperature, and keep that even if final level assignment happens to be different at commit time (rare). * More temperature handling for CreateColumnFamilyWithImport and Checkpoints. Pull Request resolved: https://github.com/facebook/rocksdb/pull/12402 Test Plan: Deeply revamped ExternalSSTFileBasicTest.IngestWithTemperature to test the new changes. Previously this test was insufficient because it was only looking at temperatures according to the DB manifest. Incorporating FileTemperatureTestFS allows us to also test the temperatures in the storage layer. Used macros instead of functions for better tracing to critical source location on test failures. Some enhancements to FileTemperatureTestFS in the process of developing the revamped test. Reviewed By: jowlyzhang Differential Revision: D54442794 Pulled By: pdillinger fbshipit-source-id: 41d9d0afdc073e6a983304c10bbc07c70cc7e995 |
||
|
yuzhangyu@fb.com
|
1cfdece85d |
Run internal cpp modernizer on RocksDB repo (#12398)
Summary: When internal cpp modernizer attempts to format rocksdb code, it will replace macro `ROCKSDB_NAMESPACE` with its default definition `rocksdb` when collapsing nested namespace. We filed a feedback for the tool T180254030 and the team filed a bug for this: https://github.com/llvm/llvm-project/issues/83452. At the same time, they suggested us to run the modernizer tool ourselves so future auto codemod attempts will be smaller. This diff contains: Running `xplat/scripts/codemod_service/cpp_modernizer.sh` in fbcode/internal_repo_rocksdb/repo (excluding some directories in utilities/transactions/lock/range/range_tree/lib that has a non meta copyright comment) without swapping out the namespace macro `ROCKSDB_NAMESPACE` Followed by RocksDB's own `make format` Pull Request resolved: https://github.com/facebook/rocksdb/pull/12398 Test Plan: Auto tests Reviewed By: hx235 Differential Revision: D54382532 Pulled By: jowlyzhang fbshipit-source-id: e7d5b40f9b113b60e5a503558c181f080b9d02fa |
||
|
Yu Zhang
|
4bea83aa44 |
Remove the force mode for EnableFileDeletions API (#12337)
Summary: There is no strong reason for user to need this mode while on the other hand, its behavior is destructive. Pull Request resolved: https://github.com/facebook/rocksdb/pull/12337 Reviewed By: hx235 Differential Revision: D53630393 Pulled By: jowlyzhang fbshipit-source-id: ce94b537258102cd98f89aa4090025663664dd78 |
||
|
Yu Zhang
|
c6c683a0ca |
Remove the default force behavior for EnableFileDeletion API (#12001)
Summary: Disabling file deletion can be critical for operations like making a backup, recovery from manifest IO error (for now). Ideally as long as there is one caller requesting file deletion disabled, it should be kept disabled until all callers agree to re-enable it. So this PR removes the default forcing behavior for the `EnableFileDeletion` API, and users need to explicitly pass the argument if they insisted on doing so knowing the consequence of what can be potentially disrupted. This PR removes the API's default argument value so it will cause breakage for all users that are relying on the default value, regardless of whether the forcing behavior is critical for them. When fixing this breakage, it's good to check if the forcing behavior is indeed needed and potential disruption is OK. This PR also makes unit test that do not need force behavior to do a regular enable file deletion. Pull Request resolved: https://github.com/facebook/rocksdb/pull/12001 Reviewed By: ajkr Differential Revision: D51214683 Pulled By: jowlyzhang fbshipit-source-id: ca7b1ebf15c09eed00f954da2f75c00d2c6a97e4 |
||
|
Changyu Bi
|
76ed9a3990 |
Add missing status check when compiling with ASSERT_STATUS_CHECKED=1 (#11686)
Summary:
It seems the flag `-fno-elide-constructors` is incorrectly overwritten in Makefile by
|
||
Jay Huh
|
87bc929db3 |
Flush option in WaitForCompact() (#11483)
Summary: Context: As mentioned in https://github.com/facebook/rocksdb/issues/11436, introducing `flush` option in `WaitForCompactOptions` to flush before waiting for compactions to finish. Must be set to true to ensure no immediate compactions (except perhaps periodic compactions) after closing and re-opening the DB. 1. `bool flush = false` added to `WaitForCompactOptions` 2. `DBImpl::FlushAllColumnFamilies()` is introduced and `DBImpl::FlushForGetLiveFiles()` is refactored to call it. 3. `DBImpl::FlushAllColumnFamilies()` gets called before waiting in `WaitForCompact()` if `flush` option is `true` 4. Some previous WaitForCompact tests were parameterized to include both cases for `abort_on_pause_` being true/false as well as `flush_` being true/false Pull Request resolved: https://github.com/facebook/rocksdb/pull/11483 Test Plan: - `DBCompactionTest::WaitForCompactWithOptionToFlush` added - Changed existing DBCompactionTest::WaitForCompact tests to `DBCompactionWaitForCompactTest` to include params Reviewed By: pdillinger Differential Revision: D46289770 Pulled By: jaykorean fbshipit-source-id: 70d3f461d96a6e06390be60170dd7c4d0d38f8b0 |
||
|
Changyu Bi
|
9aa3b6f9ae |
Support range deletion tombstones in CreateColumnFamilyWithImport (#11252)
Summary: CreateColumnFamilyWithImport() did not support range tombstones for two reasons: 1. it uses point keys of a input file to determine its boundary (smallest and largest internal key), which means range tombstones outside of the point key range will be effectively dropped. 2. it does not handle files with no point keys. Also included a fix in external_sst_file_ingestion_job.cc where the blocks read in `GetIngestedFileInfo()` can be added to block cache now (issue fixed in https://github.com/facebook/rocksdb/pull/6429). This PR adds support for exporting and importing column family with range tombstones. The main change is to add smallest internal key and largest internal key to `SstFileMetaData` that will be part of the output of `ExportColumnFamily()`. Then during `CreateColumnFamilyWithImport(...,const ExportImportFilesMetaData& metadata,...)`, file boundaries can be set from `metadata` directly. This is needed since when file boundaries are extended by range tombstones, sometimes they cannot be deduced from a file's content alone. Pull Request resolved: https://github.com/facebook/rocksdb/pull/11252 Test Plan: - added unit tests that fails before this change Closes https://github.com/facebook/rocksdb/issues/11245 Reviewed By: ajkr Differential Revision: D43577443 Pulled By: cbi42 fbshipit-source-id: 6bff78e583cc50c44854994dea0a8dd519398f2f |
||
Peter Dillinger
|
94e3beec77 |
Cleanup, improve, stress test LockWAL() (#11143)
Summary: The previous API comments for LockWAL didn't provide much about why you might want to use it, and didn't really meet what one would infer its contract was. Also, LockWAL was not in db_stress / crash test. In this change: * Implement a counting semantics for LockWAL()+UnlockWAL(), so that they can safely be used concurrently across threads or recursively within a thread. This should make the API much less bug-prone and easier to use. * Make sure no UnlockWAL() is needed after non-OK LockWAL() (to match RocksDB conventions) * Make UnlockWAL() reliably return non-OK when there's no matching LockWAL() (for debug-ability) * Clarify API comments on LockWAL(), UnlockWAL(), FlushWAL(), and SyncWAL(). Their exact meanings are not obvious, and I don't think it's appropriate to talk about implementation mutexes in the API comments, but about what operations might block each other. * Add LockWAL()/UnlockWAL() to db_stress and crash test, mostly to check for assertion failures, but also checks that latest seqno doesn't change while WAL is locked. This is simpler to add when LockWAL() is allowed in multiple threads. * Remove unnecessary use of sync points in test DBWALTest::LockWal. There was a bug during development of above changes that caused this test to fail sporadically, with and without this sync point change. Pull Request resolved: https://github.com/facebook/rocksdb/pull/11143 Test Plan: unit tests added / updated, added to stress/crash test Reviewed By: ajkr Differential Revision: D42848627 Pulled By: pdillinger fbshipit-source-id: 6d976c51791941a31fd8fbf28b0f82e888d9f4b4 |
||
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 |
||
|
Hui Xiao
|
98d5db5c2e |
Sort L0 files by newly introduced epoch_num (#10922)
Summary:
**Context:**
Sorting L0 files by `largest_seqno` has at least two inconvenience:
- File ingestion and compaction involving ingested files can create files of overlapping seqno range with the existing files. `force_consistency_check=true` will catch such overlap seqno range even those harmless overlap.
- For example, consider the following sequence of events ("key@n" indicates key at seqno "n")
- insert k1@1 to memtable m1
- ingest file s1 with k2@2, ingest file s2 with k3@3
- insert k4@4 to m1
- compact files s1, s2 and result in new file s3 of seqno range [2, 3]
- flush m1 and result in new file s4 of seqno range [1, 4]. And `force_consistency_check=true` will think s4 and s3 has file reordering corruption that might cause retuning an old value of k1
- However such caught corruption is a false positive since s1, s2 will not have overlapped keys with k1 or whatever inserted into m1 before ingest file s1 by the requirement of file ingestion (otherwise the m1 will be flushed first before any of the file ingestion completes). Therefore there in fact isn't any file reordering corruption.
- Single delete can decrease a file's largest seqno and ordering by `largest_seqno` can introduce a wrong ordering hence file reordering corruption
- For example, consider the following sequence of events ("key@n" indicates key at seqno "n", Credit to ajkr for this example)
- an existing SST s1 contains only k1@1
- insert k1@2 to memtable m1
- ingest file s2 with k3@3, ingest file s3 with k4@4
- insert single delete k5@5 in m1
- flush m1 and result in new file s4 of seqno range [2, 5]
- compact s1, s2, s3 and result in new file s5 of seqno range [1, 4]
- compact s4 and result in new file s6 of seqno range [2] due to single delete
- By the last step, we have file ordering by largest seqno (">" means "newer") : s5 > s6 while s6 contains a newer version of the k1's value (i.e, k1@2) than s5, which is a real reordering corruption. While this can be caught by `force_consistency_check=true`, there isn't a good way to prevent this from happening if ordering by `largest_seqno`
Therefore, we are redesigning the sorting criteria of L0 files and avoid above inconvenience. Credit to ajkr , we now introduce `epoch_num` which describes the order of a file being flushed or ingested/imported (compaction output file will has the minimum `epoch_num` among input files'). This will avoid the above inconvenience in the following ways:
- In the first case above, there will no longer be overlap seqno range check in `force_consistency_check=true` but `epoch_number` ordering check. This will result in file ordering s1 < s2 < s4 (pre-compaction) and s3 < s4 (post-compaction) which won't trigger false positive corruption. See test class `DBCompactionTestL0FilesMisorderCorruption*` for more.
- In the second case above, this will result in file ordering s1 < s2 < s3 < s4 (pre-compacting s1, s2, s3), s5 < s4 (post-compacting s1, s2, s3), s5 < s6 (post-compacting s4), which are correct file ordering without causing any corruption.
**Summary:**
- Introduce `epoch_number` stored per `ColumnFamilyData` and sort CF's L0 files by their assigned `epoch_number` instead of `largest_seqno`.
- `epoch_number` is increased and assigned upon `VersionEdit::AddFile()` for flush (or similarly for WriteLevel0TableForRecovery) and file ingestion (except for allow_behind_true, which will always get assigned as the `kReservedEpochNumberForFileIngestedBehind`)
- Compaction output file is assigned with the minimum `epoch_number` among input files'
- Refit level: reuse refitted file's epoch_number
- Other paths needing `epoch_number` treatment:
- Import column families: reuse file's epoch_number if exists. If not, assign one based on `NewestFirstBySeqNo`
- Repair: reuse file's epoch_number if exists. If not, assign one based on `NewestFirstBySeqNo`.
- Assigning new epoch_number to a file and adding this file to LSM tree should be atomic. This is guaranteed by us assigning epoch_number right upon `VersionEdit::AddFile()` where this version edit will be apply to LSM tree shape right after by holding the db mutex (e.g, flush, file ingestion, import column family) or by there is only 1 ongoing edit per CF (e.g, WriteLevel0TableForRecovery, Repair).
- Assigning the minimum input epoch number to compaction output file won't misorder L0 files (even through later `Refit(target_level=0)`). It's due to for every key "k" in the input range, a legit compaction will cover a continuous epoch number range of that key. As long as we assign the key "k" the minimum input epoch number, it won't become newer or older than the versions of this key that aren't included in this compaction hence no misorder.
- Persist `epoch_number` of each file in manifest and recover `epoch_number` on db recovery
- Backward compatibility with old db without `epoch_number` support is guaranteed by assigning `epoch_number` to recovered files by `NewestFirstBySeqno` order. See `VersionStorageInfo::RecoverEpochNumbers()` for more
- Forward compatibility with manifest is guaranteed by flexibility of `NewFileCustomTag`
- Replace `force_consistent_check` on L0 with `epoch_number` and remove false positive check like case 1 with `largest_seqno` above
- Due to backward compatibility issue, we might encounter files with missing epoch number at the beginning of db recovery. We will still use old L0 sorting mechanism (`NewestFirstBySeqno`) to check/sort them till we infer their epoch number. See usages of `EpochNumberRequirement`.
- Remove fix https://github.com/facebook/rocksdb/pull/5958#issue-511150930 and their outdated tests to file reordering corruption because such fix can be replaced by this PR.
- Misc:
- update existing tests with `epoch_number` so make check will pass
- update https://github.com/facebook/rocksdb/pull/5958#issue-511150930 tests to verify corruption is fixed using `epoch_number` and cover universal/fifo compaction/CompactRange/CompactFile cases
- assert db_mutex is held for a few places before calling ColumnFamilyData::NewEpochNumber()
Pull Request resolved: https://github.com/facebook/rocksdb/pull/10922
Test Plan:
- `make check`
- New unit tests under `db/db_compaction_test.cc`, `db/db_test2.cc`, `db/version_builder_test.cc`, `db/repair_test.cc`
- Updated tests (i.e, `DBCompactionTestL0FilesMisorderCorruption*`) under https://github.com/facebook/rocksdb/pull/5958#issue-511150930
- [Ongoing] Compatibility test: manually run
|
||
Levi Tamasi
|
4d9cb433fa |
Run clang-format on utilities/ (except utilities/transactions/) (#10853)
Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/10853 Test Plan: `make check` Reviewed By: siying Differential Revision: D40651315 Pulled By: ltamasi fbshipit-source-id: 8b270ff4777a06464be86e376c2a680427866a46 |
||
Andrew Kryczka
|
d5d8920f2c |
Fix race condition with WAL tracking and FlushWAL(true /* sync */) (#10185)
Summary: `FlushWAL(true /* sync */)` is used internally and for manual WAL sync. It had a bug when used together with `track_and_verify_wals_in_manifest` where the synced size tracked in MANIFEST was larger than the number of bytes actually synced. The bug could be repro'd almost immediately with the following crash test command: `python3 tools/db_crashtest.py blackbox --simple --write_buffer_size=524288 --max_bytes_for_level_base=2097152 --target_file_size_base=524288 --duration=3600 --interval=10 --sync_fault_injection=1 --disable_wal=0 --checkpoint_one_in=1000 --max_key=10000 --value_size_mult=33`. An example error message produced by the above command is shown below. The error sometimes arose from the checkpoint and other times arose from the main stress test DB. ``` Corruption: Size mismatch: WAL (log number: 119) in MANIFEST is 27938 bytes , but actually is 27859 bytes on disk. ``` Pull Request resolved: https://github.com/facebook/rocksdb/pull/10185 Test Plan: - repro unit test - the above crash test command no longer finds the error. It does find a different error after a while longer such as "Corruption: WAL file 481 required by manifest but not in directory list" Reviewed By: riversand963 Differential Revision: D37200993 Pulled By: ajkr fbshipit-source-id: 98e0071c1a89f4d009888512ed89f9219779ae5f |
||
Jay Zhuang
|
d3a2f284d9 |
Add Temperature info in NewSequentialFile() (#9499)
Summary: Add Temperature hints information from RocksDB in API `NewSequentialFile()`. backup and checkpoint operations need to open the source files with `NewSequentialFile()`, which will have the temperature hints. Other operations are not covered. Pull Request resolved: https://github.com/facebook/rocksdb/pull/9499 Test Plan: Added unittest Reviewed By: pdillinger Differential Revision: D34006115 Pulled By: jay-zhuang fbshipit-source-id: 568b34602b76520e53128672bd07e9d886786a2f |
||
|
Jay Zhuang
|
29102641dd |
Skip directory fsync for filesystem btrfs (#8903)
Summary: Directory fsync might be expensive on btrfs and it may not be needed. Here are 4 directory fsync cases: 1. creating a new file: dir-fsync is not needed on btrfs, as long as the new file itself is synced. 2. renaming a file: dir-fsync is not needed if the renamed file is synced. So an API `FsyncAfterFileRename(filename, ...)` is provided to sync the file on btrfs. By default, it just calls dir-fsync. 3. deleting files: dir-fsync is forced by set `IOOptions.force_dir_fsync = true` 4. renaming multiple files (like backup and checkpoint): dir-fsync is forced, the same as above. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8903 Test Plan: run tests on btrfs and non btrfs Reviewed By: ajkr Differential Revision: D30885059 Pulled By: jay-zhuang fbshipit-source-id: dd2730b31580b0bcaedffc318a762d7dbf25de4a |
||
|
Peter Dillinger
|
3ffb3baa0b |
Add (Live)FileStorageInfo API (#8968)
Summary: New classes FileStorageInfo and LiveFileStorageInfo and 'experimental' function DB::GetLiveFilesStorageInfo, which is intended to largely replace several fragmented DB functions needed to create checkpoints and backups. This function is now used to create checkpoints and backups, because it fixes many (probably not all) of the prior complexities of checkpoint not having atomic access to DB metadata. This also ensures strong functional test coverage of the new API. Specifically, much of the old CheckpointImpl::CreateCustomCheckpoint has been migrated to and updated in DBImpl::GetLiveFilesStorageInfo, with the former now calling the latter. Also, the class FileStorageInfo in metadata.h compatibly replaces BackupFileInfo and serves as a new base class for SstFileMetaData. Some old fields of SstFileMetaData are still provided (for now) but deprecated. Although FileStorageInfo::directory is accurate when using db_paths and/or cf_paths, these have never been supported by Checkpoint nor BackupEngine and still are not. This change does now detect these cases and return NotSupported when appropriate. (More work needed for support.) Somehow this change broke ProgressCallbackDuringBackup, but the progress_callback logic was dubious to begin with because it would call the callback based on copy buffer size, not size actually copied. Logic and test updated to track size actually copied per-thread. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8968 Test Plan: tests updated. DB::GetLiveFilesStorageInfo mostly tested by use in CheckpointImpl. DBTest.SnapshotFiles updated to also test GetLiveFilesStorageInfo, including reading the data after DB close. Added CheckpointTest.CheckpointWithDbPath (NotSupported). Reviewed By: siying Differential Revision: D31242045 Pulled By: pdillinger fbshipit-source-id: b183d1ce9799e220daaefd6b3b5365d98de676c0 |
||
mrambacher
|
13ae16c315 |
Cleanup includes in dbformat.h (#8930)
Summary: This header file was including everything and the kitchen sink when it did not need to. This resulted in many places including this header when they needed other pieces instead. Cleaned up this header to only include what was needed and fixed up the remaining code to include what was now missing. Hopefully, this sort of code hygiene cleanup will speed up the builds... Pull Request resolved: https://github.com/facebook/rocksdb/pull/8930 Reviewed By: pdillinger Differential Revision: D31142788 Pulled By: mrambacher fbshipit-source-id: 6b45de3f300750c79f751f6227dece9cfd44085d |
||
Merlin Mao
|
4811115b3e |
Revert checkpoint fix (#8607)
Summary: PR https://github.com/facebook/rocksdb/pull/8572 looses custom types in the options file. Need more API changes to fix this issue. Revert this PR. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8607 Reviewed By: ajkr Differential Revision: D30058289 Pulled By: autopear fbshipit-source-id: 78f5a154c0bf193e8441bae4a36fa79b95277fd4 |
||
|
mrambacher
|
ab7f7c9e49 |
Allow WAL dir to change with db dir (#8582)
Summary: Prior to this change, the "wal_dir" DBOption would always be set (defaults to dbname) when the DBOptions were sanitized. Because of this setitng in the options file, it was not possible to rename/relocate a database directory after it had been created and use the existing options file. After this change, the "wal_dir" option is only set under specific circumstances. Methods were added to the ImmutableDBOptions class to see if it is set and if it is set to something other than the dbname. Additionally, a method was added to retrieve the effective value of the WAL dir (either the option or the dbname/path). Tests were added to the core and ldb to test that a database could be created and renamed without issue. Additional tests for various permutations of wal_dir were also added. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8582 Reviewed By: pdillinger, autopear Differential Revision: D29881122 Pulled By: mrambacher fbshipit-source-id: 67d3d033dc8813d59917b0a3fba2550c0efd6dfb |
||
|
Merlin Mao
|
55f7ded80d |
Checkpoint dir options fix (#8572)
Summary: Originally the 2 options `db_log_dir` and `wal_dir` will be reused in a snapshot db since the options files are just copied. By default, if `wal_dir` was not set when a db was created, it is set to the db's dir. Therefore, the snapshot db will use the same WAL dir. If both the original db and the snapshot db write to or delete from the WAL dir, one may modify or delete files which belong to the other. The same applies to `db_log_dir` as well, but as info log files are not copied or linked, it is simpler for this option. 2 arguments are added to `Checkpoint::CreateCheckpoint()`, allowing to override these 2 options. `wal_dir`: If the function argument `wal_dir` is empty, or set to the original db location, or the checkpoint location, the snapshot's `wal_dir` option will be updated to the checkpoint location. Otherwise, the absolute path specified in the argument will be used. During checkpointing, live WAL files will be copied or linked the new location, instead of the current WAL dir specified in the original db. `db_log_dir`: Same as `wal_dir`, but no files will be copied or linked. A new unit test was added: `CheckpointTest.CheckpointWithOptionsDirsTest`. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8572 Test Plan: New unit test ``` checkpoint_test --gtest_filter="CheckpointTest.CheckpointWithOptionsDirsTest" ``` Output ``` Note: Google Test filter = CheckpointTest.CheckpointWithOptionsDirsTest [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from CheckpointTest [ RUN ] CheckpointTest.CheckpointWithOptionsDirsTest [ OK ] CheckpointTest.CheckpointWithOptionsDirsTest (11712 ms) [----------] 1 test from CheckpointTest (11712 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (11713 ms total) [ PASSED ] 1 test. ``` This test will fail without this patch. Just modify the code to remove the 2 arguments introduced in this patch in `CreateCheckpoint()`. Reviewed By: zhichao-cao Differential Revision: D29832761 Pulled By: autopear fbshipit-source-id: e6a639b4d674380df82998c0839e79cab695fe29 |
||
Myth
|
bbdc4f2e9a |
Fix a minor issue in checkpoint test case (#8483)
Summary: A very simple change :) Pull Request resolved: https://github.com/facebook/rocksdb/pull/8483 Reviewed By: ajkr Differential Revision: D29558904 Pulled By: mrambacher fbshipit-source-id: bbe68c20c861103726cb6231ca3fb8fbe1e5a546 |
||
|
Andrew Kryczka
|
0f42e50fec |
Fix GetLiveFiles() returning OPTIONS-000000 (#8268)
Summary: See release note in HISTORY.md. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8268 Test Plan: unit test repro Reviewed By: siying Differential Revision: D28227901 Pulled By: ajkr fbshipit-source-id: faf61d13b9e43a761e3d5dcf8203923126b51339 |
||
Akanksha Mahajan
|
f19612970d |
Support retrieving checksums for blob files from the MANIFEST when checkpointing (#8003)
Summary: The checkpointing logic supports passing file level checksums to the copy_file_cb callback function which is used by the backup code for detecting corruption during file copies. However, this is currently implemented only for table files. This PR extends the checksum retrieval to blob files as well. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8003 Test Plan: Add new test units Reviewed By: ltamasi Differential Revision: D26680701 Pulled By: akankshamahajan15 fbshipit-source-id: 1bd1e2464df6e9aa31091d35b8c72786d94cd1c5 |