Avoid unnecessary work in internal calls to GetSortedWalFiles (#12831)

Summary:
We are seeing a number of crash test failures coming from checkpoint and backup code, likely from WalManager::GetSortedWalFiles -> ... -> WalManager::ReadFirstLine and this code path is not needed, because we don't need to know the sequence numbers of WAL files going into a checkpoint or backup. We can minimize the impact of whatever inconsistency is causing that problem by not relying on it where it's not needed.

Similarly, when we only need a roughly accurate set of current WAL files, we don't need to query all the archived WAL files (and redundantly the live ones again).

So this reduces filesystem queries and DB mutex acquires in creating backups and checkpoints.

Needed follow-up:
Figure out what is causing various failures with an apparent inconsistency where GetSortedWalFiles fails on reading a WAL file. If it's an injected failure, perhaps it's not propagating that injected failure appropriately. It might also be an inconsistency between what the DB knows is flushed and what WalManager reads from the filesystem (which we know is dubious and should be phased out, which this is arguably another step toward). Or completing that phase-out might solve the problem without a full diagnosis.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/12831

Test Plan:
existing tests (easily caught when I went too far in initally developing this change)

Update to BackupUsingDirectIO test so that there's a WAL file in what is backed up. (Was relying on some oddity.)

Reviewed By: cbi42

Differential Revision: D59252649

Pulled By: pdillinger

fbshipit-source-id: 7ad4187a1c70caa59a6d6c1c643ef95232b929f5

db/db_filesnapshot.cc

@@ -93,6 +93,11 @@ Status DBImpl::GetLiveFiles(std::vector<std::string>& ret,
 }
 
 Status DBImpl::GetSortedWalFiles(VectorWalPtr& files) {
+  return GetSortedWalFilesImpl(files,
+                               /*need_seqnos*/ true);
+}
+
+Status DBImpl::GetSortedWalFilesImpl(VectorWalPtr& files, bool need_seqnos) {
   // Record tracked WALs as a (minimum) cross-check for directory scan
   std::vector<uint64_t> required_by_manifest;
 
@@ -118,7 +123,10 @@ Status DBImpl::GetSortedWalFiles(VectorWalPtr& files) {
     }
   }
 
-  Status s = wal_manager_.GetSortedWalFiles(files);
+  // NOTE: need to include archived WALs because needed WALs might have been
+  // archived since getting required_by_manifest set
+  Status s = wal_manager_.GetSortedWalFiles(files, need_seqnos,
+                                            /*include_archived*/ true);
 
   // DisableFileDeletions / EnableFileDeletions not supported in read-only DB
   if (deletions_disabled.ok()) {
@@ -207,7 +215,12 @@ Status DBImpl::GetLiveFilesStorageInfo(
     } else if (opts.wal_size_for_flush > 0) {
       // FIXME: avoid querying the filesystem for current WAL state
       // If the outstanding WAL files are small, we skip the flush.
-      s = GetSortedWalFiles(live_wal_files);
+      // Don't take archived log size into account when calculating wal
+      // size for flush, and don't need to verify consistency with manifest
+      // here & now.
+      s = wal_manager_.GetSortedWalFiles(live_wal_files,
+                                         /* need_seqnos */ false,
+                                         /*include_archived*/ false);
 
       if (!s.ok()) {
         return s;
@@ -218,11 +231,7 @@ Status DBImpl::GetLiveFilesStorageInfo(
       // We may be able to cover 2PC case too.
       uint64_t total_wal_size = 0;
       for (auto& wal : live_wal_files) {
-        // Don't take archived log size into account
+        assert(wal->Type() == kAliveLogFile);
-        // when calculating log size for flush
-        if (wal->Type() == kArchivedLogFile) {
-          continue;
-        }
         total_wal_size += wal->SizeFileBytes();
       }
       if (total_wal_size < opts.wal_size_for_flush) {
@@ -434,7 +443,8 @@ Status DBImpl::GetLiveFilesStorageInfo(
   // WAL files.
   if (s.ok()) {
     // FIXME: avoid querying the filesystem for current WAL state
-    s = GetSortedWalFiles(live_wal_files);
+    s = GetSortedWalFilesImpl(live_wal_files,
+                              /* need_seqnos */ false);
   }
   if (!s.ok()) {
     return s;

db/db_impl/db_impl.h

@@ -508,6 +508,8 @@ class DBImpl : public DB {
   Status GetLiveFiles(std::vector<std::string>&, uint64_t* manifest_file_size,
                       bool flush_memtable = true) override;
   Status GetSortedWalFiles(VectorWalPtr& files) override;
+  Status GetSortedWalFilesImpl(VectorWalPtr& files, bool need_seqnos);
+
   // Get the known flushed sizes of WALs that might still be written to
   // or have pending sync.
   // NOTE: unlike alive_log_files_, this function includes WALs that might

db/wal_manager.cc

@@ -43,16 +43,17 @@ Status WalManager::DeleteFile(const std::string& fname, uint64_t number) {
   }
   return s;
 }
-
+Status WalManager::GetSortedWalFiles(VectorWalPtr& files, bool need_seqnos,
-Status WalManager::GetSortedWalFiles(VectorWalPtr& files) {
+                                     bool include_archived) {
   // First get sorted files in db dir, then get sorted files from archived
   // dir, to avoid a race condition where a log file is moved to archived
   // dir in between.
   Status s;
   // list wal files in main db dir.
   VectorWalPtr logs;
-  s = GetSortedWalsOfType(wal_dir_, logs, kAliveLogFile);
+  s = GetSortedWalsOfType(wal_dir_, logs, kAliveLogFile, need_seqnos);
-  if (!s.ok()) {
+
+  if (!include_archived || !s.ok()) {
     return s;
   }
 
@@ -67,7 +68,7 @@ Status WalManager::GetSortedWalFiles(VectorWalPtr& files) {
   std::string archivedir = ArchivalDirectory(wal_dir_);
   Status exists = env_->FileExists(archivedir);
   if (exists.ok()) {
-    s = GetSortedWalsOfType(archivedir, files, kArchivedLogFile);
+    s = GetSortedWalsOfType(archivedir, files, kArchivedLogFile, need_seqnos);
     if (!s.ok()) {
       return s;
     }
@@ -249,7 +250,8 @@ void WalManager::PurgeObsoleteWALFiles() {
 
   size_t files_del_num = log_files_num - files_keep_num;
   VectorWalPtr archived_logs;
-  s = GetSortedWalsOfType(archival_dir, archived_logs, kArchivedLogFile);
+  s = GetSortedWalsOfType(archival_dir, archived_logs, kArchivedLogFile,
+                          /*need_seqno=*/false);
   if (!s.ok()) {
     ROCKS_LOG_WARN(db_options_.info_log,
                    "Unable to get archived WALs from: %s: %s",
@@ -294,7 +296,7 @@ void WalManager::ArchiveWALFile(const std::string& fname, uint64_t number) {
 
 Status WalManager::GetSortedWalsOfType(const std::string& path,
                                        VectorWalPtr& log_files,
-                                       WalFileType log_type) {
+                                       WalFileType log_type, bool need_seqnos) {
   std::vector<std::string> all_files;
   const Status status = env_->GetChildren(path, &all_files);
   if (!status.ok()) {
@@ -306,6 +308,7 @@ Status WalManager::GetSortedWalsOfType(const std::string& path,
     FileType type;
     if (ParseFileName(f, &number, &type) && type == kWalFile) {
       SequenceNumber sequence;
+      if (need_seqnos) {
         Status s = ReadFirstRecord(log_type, number, &sequence);
         if (!s.ok()) {
           return s;
@@ -314,6 +317,9 @@ Status WalManager::GetSortedWalsOfType(const std::string& path,
           // empty file
           continue;
         }
+      } else {
+        sequence = 0;
+      }
 
       // Reproduce the race condition where a log file is moved
       // to archived dir, between these two sync points, used in
@@ -322,7 +328,7 @@ Status WalManager::GetSortedWalsOfType(const std::string& path,
       TEST_SYNC_POINT("WalManager::GetSortedWalsOfType:2");
 
       uint64_t size_bytes;
-      s = env_->GetFileSize(LogFileName(path, number), &size_bytes);
+      Status s = env_->GetFileSize(LogFileName(path, number), &size_bytes);
       // re-try in case the alive log file has been moved to archive.
       if (!s.ok() && log_type == kAliveLogFile) {
         std::string archived_file = ArchivedLogFileName(path, number);

db/wal_manager.h

@@ -49,7 +49,8 @@ class WalManager {
         wal_in_db_path_(db_options_.IsWalDirSameAsDBPath()),
         io_tracer_(io_tracer) {}
 
-  Status GetSortedWalFiles(VectorWalPtr& files);
+  Status GetSortedWalFiles(VectorWalPtr& files, bool need_seqnos = true,
+                           bool include_archived = true);
 
   // Allow user to tail transaction log to find all recent changes to the
   // database that are newer than `seq_number`.
@@ -78,7 +79,7 @@ class WalManager {
 
  private:
   Status GetSortedWalsOfType(const std::string& path, VectorWalPtr& log_files,
-                             WalFileType type);
+                             WalFileType type, bool need_seqnos);
   // Requires: all_logs should be sorted with earliest log file first
   // Retains all log files in all_logs which contain updates with seq no.
   // Greater Than or Equal to the requested SequenceNumber.

unreleased_history/performance_improvements/sorted_wals.md

@@ -0,0 +1 @@
+* Reduce unnecessary filesystem queries and DB mutex acquires in creating backups and checkpoints.

utilities/backup/backup_engine_test.cc

@@ -3962,7 +3962,7 @@ TEST_P(BackupEngineTestWithParam, BackupUsingDirectIO) {
   OpenDBAndBackupEngine(true /* destroy_old_data */);
   for (int i = 0; i < kNumBackups; ++i) {
     FillDB(db_.get(), i * kNumKeysPerBackup /* from */,
-           (i + 1) * kNumKeysPerBackup /* to */, kFlushAll);
+           (i + 1) * kNumKeysPerBackup /* to */);
 
     // Clear the file open counters and then do a bunch of backup engine ops.
     // For all ops, files should be opened in direct mode.