From 7d18ffe3163a7fc82cb3032237cd61535e35bc47 Mon Sep 17 00:00:00 2001 From: Anand Ananthabhotla Date: Tue, 18 Aug 2026 17:53:22 -0700 Subject: [PATCH] Ignore OPTIONS files for manifest recovery advance (#15106) Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/15106 When `optimize_manifest_for_recovery` is enabled, `MaybeUpdateNextFileNumber` uses on-disk files to decide whether recovery must append a `SetNextFile` edit. `OPTIONS-*` and temp options files are written as sidecar option snapshots during open, after recovery, so their presence alone should not force MANIFEST growth on a later clean reopen. Exclude `kOptionsFile` and `kTempFile` from the trigger that requires a MANIFEST file-number advance. If any real file requires an advance, the code still advances past the largest observed file number, including OPTIONS files. Also tighten db_stress strict MANIFEST verification so fatal zero-growth checks only run when all fault injection is disabled. Open write/sync fault injection can legitimately make `reuse_manifest_on_open` fall back to a fresh MANIFEST. Reviewed By: xingbowang Differential Revision: D116092705 fbshipit-source-id: 65b57b0b7f57b2cef42efc4df353fb2218688d34 --- db/db_basic_test.cc | 30 +++++++++++++++++++++++++++ db/db_impl/db_impl_files.cc | 4 +++- db_stress_tool/db_stress_test_base.cc | 2 ++ db_stress_tool/db_stress_test_base.h | 3 +-- 4 files changed, 36 insertions(+), 3 deletions(-) diff --git a/db/db_basic_test.cc b/db/db_basic_test.cc index de3aaefca0..0da01c6240 100644 --- a/db/db_basic_test.cc +++ b/db/db_basic_test.cc @@ -355,6 +355,36 @@ TEST_F(DBBasicTest, synthetic_number); } +TEST_F(DBBasicTest, + OptimizeManifestForRecoveryIgnoresOptionsFilesForNextFileNumber) { + Options options = CurrentOptions(); + options.create_if_missing = true; + options.optimize_manifest_for_recovery = true; + DestroyAndReopen(options); + ASSERT_OK(Put("k", "v")); + ASSERT_OK(Flush()); + + const uint64_t next_before_close = + dbfull()->GetVersionSet()->current_next_file_number(); + Close(); + + const uint64_t options_number = next_before_close + 100; + ASSERT_OK(WriteStringToFile(env_, "" /*data*/, + dbname_ + "/" + OptionsFileName(options_number), + /*should_sync=*/true)); + ASSERT_OK(WriteStringToFile(env_, "" /*data*/, + TempOptionsFileName(dbname_, options_number + 1), + /*should_sync=*/true)); + + RecoveryOptimizationCounters counters; + counters.Install(); + Reopen(options); + counters.Uninstall(); + + ASSERT_EQ(1, counters.next_file_number.load()); + ASSERT_EQ("v", Get("k")); +} + // optimize_manifest_for_recovery=true: after a clean Put + Flush + Close, the // next Open's min_log_number_to_keep equals (max_wal_before_close + 1), // proving the close-time MANIFEST write took effect. diff --git a/db/db_impl/db_impl_files.cc b/db/db_impl/db_impl_files.cc index 4e9ea52315..b83ec4ff43 100644 --- a/db/db_impl/db_impl_files.cc +++ b/db/db_impl/db_impl_files.cc @@ -1211,7 +1211,9 @@ Status DBImpl::MaybeUpdateNextFileNumber(RecoveryContext* recovery_ctx) { // Use >= so a crashed-mid-allocation file at exactly // prev_next_file_number triggers the advance -- otherwise the next // NewFileNumber() would collide with it. - if (number >= prev_next_file_number) { + const bool must_reserve_file_number = + type != kOptionsFile && type != kTempFile; + if (must_reserve_file_number && number >= prev_next_file_number) { on_disk_file_advanced_counter = true; } largest_file_number = std::max(largest_file_number, number); diff --git a/db_stress_tool/db_stress_test_base.cc b/db_stress_tool/db_stress_test_base.cc index 9e772d490d..72fa60ffb0 100644 --- a/db_stress_tool/db_stress_test_base.cc +++ b/db_stress_tool/db_stress_test_base.cc @@ -5721,7 +5721,9 @@ void StressTest::RecordManifestStateBeforeReopen() { if (reuse_manifest && optimize_manifest) { // Check if ALL conditions for complete avoidance are met. // If so, use STRICT mode where failures are fatal. + const bool no_fault_injection = !NeedsFaultInjection(); const bool no_manifest_writes_expected = + no_fault_injection && FLAGS_avoid_flush_during_recovery && // No flush during recovery !FLAGS_write_dbid_to_manifest; // No DB_ID write on open // Note: avoid_flush_during_shutdown is NOT required for STRICT mode. diff --git a/db_stress_tool/db_stress_test_base.h b/db_stress_tool/db_stress_test_base.h index 58e1c8cfdc..9035b3f7a2 100644 --- a/db_stress_tool/db_stress_test_base.h +++ b/db_stress_tool/db_stress_test_base.h @@ -429,8 +429,7 @@ class StressTest { // - Not in best_efforts_recovery mode // - avoid_flush_during_recovery=true (no flush during recovery) // - write_dbid_to_manifest=0 (no DB_ID write on open) - // - metadata_write_fault_one_in=0 (no fault injection) - // - open_metadata_write_fault_one_in=0 (no fault injection) + // - all fault injection flags are disabled // - MANIFEST not corrupted, not at size limit // Note: avoid_flush_during_shutdown is NOT required. If it leaves data // in WAL but avoid_flush_during_recovery=true prevents flushing it,