diff --git a/HISTORY.md b/HISTORY.md index 7f65f86f6..f696d9d8f 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -3,6 +3,7 @@ ### Bug Fixes * If the primary's CURRENT file is missing or inaccessible, the secondary instance should not hang repeatedly trying to switch to a new MANIFEST. It should instead return the error code encountered while accessing the file. * Fixed a race related to the destruction of `ColumnFamilyData` objects. The earlier logic unlocked the DB mutex before destroying the thread-local `SuperVersion` pointers, which could result in a process crash if another thread managed to get a reference to the `ColumnFamilyData` object. +* Removed a call to `RenameFile()` on a non-existent info log file ("LOG") when opening a new DB. Such a call was guaranteed to fail though did not impact applications since we swallowed the error. Now we also stopped swallowing errors in renaming "LOG" file. ### New Features * Made the EventListener extend the Customizable class. diff --git a/db/db_test_util.h b/db/db_test_util.h index f2f56e890..689af0788 100644 --- a/db/db_test_util.h +++ b/db/db_test_util.h @@ -675,6 +675,14 @@ class SpecialEnv : public EnvWrapper { } } + Status RenameFile(const std::string& src, const std::string& dest) override { + rename_count_.fetch_add(1); + if (rename_error_.load(std::memory_order_acquire)) { + return Status::NotSupported("Simulated `RenameFile()` error."); + } + return target()->RenameFile(src, dest); + } + // Something to return when mocking current time const int64_t maybe_starting_time_; @@ -702,6 +710,9 @@ class SpecialEnv : public EnvWrapper { // Force write to log files to fail while this pointer is non-nullptr std::atomic log_write_error_; + // Force `RenameFile()` to fail while this pointer is non-nullptr + std::atomic rename_error_{false}; + // Slow down every log write, in micro-seconds. std::atomic log_write_slowdown_; @@ -745,6 +756,8 @@ class SpecialEnv : public EnvWrapper { std::atomic delete_count_; + std::atomic rename_count_{0}; + std::atomic is_wal_sync_thread_safe_{true}; std::atomic compaction_readahead_size_{}; diff --git a/logging/auto_roll_logger.cc b/logging/auto_roll_logger.cc index 1ff08c1ad..d32645a42 100644 --- a/logging/auto_roll_logger.cc +++ b/logging/auto_roll_logger.cc @@ -296,12 +296,19 @@ Status CreateLoggerFromOptions(const std::string& dbname, } #endif // !ROCKSDB_LITE // Open a log file in the same directory as the db - env->RenameFile( - fname, OldInfoLogFileName(dbname, clock->NowMicros(), db_absolute_path, - options.db_log_dir)) - .PermitUncheckedError(); - s = env->NewLogger(fname, logger); - if (logger->get() != nullptr) { + s = env->FileExists(fname); + if (s.ok()) { + s = env->RenameFile( + fname, OldInfoLogFileName(dbname, clock->NowMicros(), db_absolute_path, + options.db_log_dir)); + } else if (s.IsNotFound()) { + // "LOG" is not required to exist since this could be a new DB. + s = Status::OK(); + } + if (s.ok()) { + s = env->NewLogger(fname, logger); + } + if (s.ok() && logger->get() != nullptr) { (*logger)->SetInfoLogLevel(options.info_log_level); } return s; diff --git a/logging/auto_roll_logger_test.cc b/logging/auto_roll_logger_test.cc index 59e0ebac6..b9e8ed55a 100644 --- a/logging/auto_roll_logger_test.cc +++ b/logging/auto_roll_logger_test.cc @@ -19,6 +19,7 @@ #include #include +#include "db/db_test_util.h" #include "logging/logging.h" #include "port/port.h" #include "rocksdb/db.h" @@ -686,6 +687,50 @@ TEST_F(AutoRollLoggerTest, FileCreateFailure) { ASSERT_NOK(CreateLoggerFromOptions("", options, &logger)); ASSERT_TRUE(!logger); } + +TEST_F(AutoRollLoggerTest, RenameOnlyWhenExists) { + InitTestDb(); + SpecialEnv env(Env::Default()); + Options options; + options.env = &env; + + // Originally no LOG exists. Should not see a rename. + { + std::shared_ptr logger; + ASSERT_OK(CreateLoggerFromOptions(kTestDir, options, &logger)); + ASSERT_EQ(0, env.rename_count_); + } + + // Now a LOG exists. Create a new one should see a rename. + { + std::shared_ptr logger; + ASSERT_OK(CreateLoggerFromOptions(kTestDir, options, &logger)); + ASSERT_EQ(1, env.rename_count_); + } +} + +TEST_F(AutoRollLoggerTest, RenameError) { + InitTestDb(); + SpecialEnv env(Env::Default()); + env.rename_error_ = true; + Options options; + options.env = &env; + + // Originally no LOG exists. Should not be impacted by rename error. + { + std::shared_ptr logger; + ASSERT_OK(CreateLoggerFromOptions(kTestDir, options, &logger)); + ASSERT_TRUE(logger != nullptr); + } + + // Now a LOG exists. Rename error should cause failure. + { + std::shared_ptr logger; + ASSERT_NOK(CreateLoggerFromOptions(kTestDir, options, &logger)); + ASSERT_TRUE(logger == nullptr); + } +} + } // namespace ROCKSDB_NAMESPACE int main(int argc, char** argv) {