BackupEngine computes table checksums only once if db session ids are available (#7110)

Summary:
BackupEngine requires computing table checksums twice when backing up table files to the `shared_checksum` directory.

The repeated computation can be avoided by utilizing the db session id stored as a part of the table properties.

Filenames of table files in the `shared_checksum` directory depend on the following conditions:
1. the naming scheme is `kOptionalChecksumAndDbSessionId`,
2. `db_session_id` is not empty,
3. checksum is available in the DB manifest.

If 1,2,3 are satisfied, then the filenames will be of the form `<file_number>_<checksum>_<db_session_id>.sst`.
If 1,2 are satisfied, then the filenames will be of the form `<file_number>_<db_session_id>.sst`.
In all other cases, the filenames are of the form `<file_number>_<checksum>_<size>.sst`.

Additionally, if `kOptionalChecksumAndDbSessionId` is used (and not falling back to `kChecksumAndFileSize`), the `<checksum>` appeared in the filenames is hexadecimally encoded, instead of being plain `uint32_t` value.

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

Test Plan: backupable_db_test and manual tests.

Reviewed By: ajkr

Differential Revision: D22508992

Pulled By: gg814

fbshipit-source-id: 5669f0ea9ad5a097f69f6d87aca4abba15032389

HISTORY.md

@@ -25,8 +25,8 @@
 ### New Features
 * DB identity (`db_id`) and DB session identity (`db_session_id`) are added to table properties and stored in SST files. SST files generated from SstFileWriter and Repairer have DB identity “SST Writer” and “DB Repairer”, respectively. Their DB session IDs are generated in the same way as `DB::GetDbSessionId`. The session ID for SstFileWriter (resp., Repairer) resets every time `SstFileWriter::Open` (resp., `Repairer::Run`) is called.
 * Added experimental option BlockBasedTableOptions::optimize_filters_for_memory for reducing allocated memory size of Bloom filters (~10% savings with Jemalloc) while preserving the same general accuracy. To have an effect, the option requires format_version=5 and malloc_usable_size. Enabling this option is forward and backward compatible with existing format_version=5.
-* `BackupTableNameOption BackupableDBOptions::share_files_with_checksum_naming` is added, where `BackupTableNameOption` is an `enum` type with two enumerators `kChecksumAndFileSize` and `kChecksumAndFileSize`. By default, `BackupTableNameOption BackupableDBOptions::share_files_with_checksum_naming` is set to `kChecksumAndDbSessionId`. In this default case, backup table filenames are of the form `<file_number>_<crc32c>_<db_session_id>.sst` as opposed to `<file_number>_<crc32c>_<file_size>.sst`. The new default behavior fixes the backup file name collision problem, which might be possible at large scale, but the option `kChecksumAndFileSize` is added to allow use of old naming in case it is needed. This default behavior change is not an upgrade issue, because previous versions of RocksDB can read, restore, and delete backups using new names, and it's OK for a backup directory to use a mixture of table file naming schemes. Note that `share_files_with_checksum_naming` comes into effect only when both `share_files_with_checksum` and `share_table_files` are true.
-* Added auto resume function to automatically recover the DB from background Retryable IO Error. When retryable IOError happens during flush and WAL write, the error is mapped to Hard Error and DB will be in read mode. When retryable IO Error happens during compaction, the error will be mapped to Soft Error. DB is still in write/read mode. Autoresume function will create a thread for a DB to call DB->ResumeImpl() to try the recover for Retryable IO Error during flush and WAL write. Compaction will be rescheduled by itself if retryable IO Error happens. Auto resume may also cause other Retryable IO Error during the recovery, so the recovery will fail. Retry the auto resume may solve the issue, so we use max_bgerror_resume_count to decide how many resume cycles will be tried in total. If it is <=0, auto resume retryable IO Error is disabled. Default is INT_MAX, which will lead to a infinit auto resume. bgerror_resume_retry_interval decides the time interval between two auto resumes. 
+* `BackupTableNameOption BackupableDBOptions::share_files_with_checksum_naming` is added, where `BackupTableNameOption` is an `enum` type with two enumerators `kChecksumAndFileSize` and `kOptionalChecksumAndDbSessionId`. By default, `BackupableDBOptions::share_files_with_checksum_naming` is set to `kOptionalChecksumAndDbSessionId`. In the default case, backup table filenames generated by this version of RocksDB are of the form either `<file_number>_<crc32c>_<db_session_id>.sst` or `<file_number>_<db_session_id>.sst` as opposed to `<file_number>_<crc32c>_<file_size>.sst`. Specifically, table filenames are of the form `<file_number>_<crc32c>_<db_session_id>.sst` if `DBOptions::file_checksum_gen_factory` is set to `GetFileChecksumGenCrc32cFactory()`. Futhermore, the checksum value `<crc32c>` appeared in the filenames is hexadecimal-encoded, instead of being decimal-encoded `uint32_t` value. If `DBOptions::file_checksum_gen_factory` is `nullptr`, the table filenames are of the form `<file_number>_<db_session_id>.sst`. The new default behavior fixes the backup file name collision problem, which might be possible at large scale, but the option `kChecksumAndFileSize` is added to allow use of old naming in case it is needed. Moreover, for table files generated prior to this version of RocksDB, using `kOptionalChecksumAndDbSessionId` will fall back on `kChecksumAndFileSize`. In these cases, the checksum value `<crc32c>` in the filenames `<file_number>_<crc32c>_<file_size>.sst` is decimal-encoded `uint32_t` value as before. This default behavior change is not an upgrade issue, because previous versions of RocksDB can read, restore, and delete backups using new names, and it's OK for a backup directory to use a mixture of table file naming schemes. Note that `share_files_with_checksum_naming` comes into effect only when both `share_files_with_checksum` and `share_table_files` are true.
+* Added auto resume function to automatically recover the DB from background Retryable IO Error. When retryable IOError happens during flush and WAL write, the error is mapped to Hard Error and DB will be in read mode. When retryable IO Error happens during compaction, the error will be mapped to Soft Error. DB is still in write/read mode. Autoresume function will create a thread for a DB to call DB->ResumeImpl() to try the recover for Retryable IO Error during flush and WAL write. Compaction will be rescheduled by itself if retryable IO Error happens. Auto resume may also cause other Retryable IO Error during the recovery, so the recovery will fail. Retry the auto resume may solve the issue, so we use max_bgerror_resume_count to decide how many resume cycles will be tried in total. If it is <=0, auto resume retryable IO Error is disabled. Default is INT_MAX, which will lead to a infinit auto resume. bgerror_resume_retry_interval decides the time interval between two auto resumes.
 
 ### Bug Fixes
 * Fail recovery and report once hitting a physical log record checksum mismatch, while reading MANIFEST. RocksDB should not continue processing the MANIFEST any further.
@@ -34,6 +34,7 @@
 ### Performance Improvements
 * Eliminate key copies for internal comparisons while accessing ingested block-based tables.
 * Reduce key comparisons during random access in all block-based tables.
+* BackupEngine avoids unnecessary repeated checksum computation for backing up a table file to the `shared_checksum` directory when using `kOptionalChecksumAndDbSessionId`, except on SST files generated before this version of RocksDB, which fall back on using `kChecksumAndFileSize`.
 
 ## 6.11 (6/12/2020)
 ### Bug Fixes

include/rocksdb/utilities/backupable_db.h

@@ -34,12 +34,17 @@ constexpr char kBackupFileChecksumFuncName[] = "crc32c";
 // directory (i.e., both share_table_files and share_files_with_checksum
 // are true).
 enum BackupTableNameOption : unsigned char {
-  // Backup SST filenames consist of file_number, crc32c, db_session_id
+  // Backup SST filenames are <file_number>_<crc32c>_<file_size>.sst
+  // where <crc32c> is uint32_t.
   kChecksumAndFileSize = 0,
-  // Backup SST filenames consist of file_number, crc32c, file_size
-  // When there is no `db_session_id` available in the table file, we use
-  // `file_size` as a fallback.
-  kChecksumAndDbSessionId = 1
+  // Backup SST filenames are <file_number>_<crc32c>_<db_session_id>.sst
+  // where <crc32c> is hexidecimally encoded.
+  // When DBOptions::file_checksum_gen_factory is not set to
+  // GetFileChecksumGenCrc32cFactory(), the filenames will be
+  // <file_number>_<db_session_id>.sst
+  // When there are no db session ids available in the table file, this
+  // option will use kChecksumAndFileSize as a fallback.
+  kOptionalChecksumAndDbSessionId = 1
 };
 
 struct BackupableDBOptions {
@@ -111,9 +116,10 @@ struct BackupableDBOptions {
   // (file name, crc32c, db session id or file length)
   //
   // Note: If this option is set to true, we recommend setting
-  // share_files_with_checksum_naming to kChecksumAndDbSessionId, which is also
-  // our default option. Otherwise, there is a non-negligible chance of filename
-  // collision when sharing tables in shared_checksum among several DBs.
+  // share_files_with_checksum_naming to kOptionalChecksumAndDbSessionId, which
+  // is also our default option. Otherwise, there is a non-negligible chance of
+  // filename collision when sharing tables in shared_checksum among several
+  // DBs.
   // *turn it on only if you know what you're doing*
   //
   // Default: false
@@ -141,17 +147,17 @@ struct BackupableDBOptions {
   int max_valid_backups_to_open;
 
   // Naming option for share_files_with_checksum table files. This option
-  // can be set to kChecksumAndFileSize or kChecksumAndDbSessionId.
+  // can be set to kChecksumAndFileSize or kOptionalChecksumAndDbSessionId.
   // kChecksumAndFileSize is susceptible to collision as file size is not a
   // good source of entroy.
-  // kChecksumAndDbSessionId is immune to collision.
+  // kOptionalChecksumAndDbSessionId is immune to collision.
   //
   // Modifying this option cannot introduce a downgrade compatibility issue
   // because RocksDB can read, restore, and delete backups using different file
   // names, and it's OK for a backup directory to use a mixture of table file
   // naming schemes.
   //
-  // Default: kChecksumAndDbSessionId
+  // Default: kOptionalChecksumAndDbSessionId
   //
   // Note: This option comes into effect only if both share_files_with_checksum
   // and share_table_files are true. In the cases of old table files where no
@@ -170,7 +176,7 @@ struct BackupableDBOptions {
       uint64_t _callback_trigger_interval_size = 4 * 1024 * 1024,
       int _max_valid_backups_to_open = INT_MAX,
       BackupTableNameOption _share_files_with_checksum_naming =
-          kChecksumAndDbSessionId)
+          kOptionalChecksumAndDbSessionId)
       : backup_dir(_backup_dir),
         backup_env(_backup_env),
         share_table_files(_share_table_files),

utilities/backupable/backupable_db_test.cc

@@ -1251,65 +1251,15 @@ TEST_P(BackupableDBTestWithParam, TableFileCorruptedBeforeBackup) {
   CloseDBAndBackupEngine();
 }
 
-TEST_F(BackupableDBTest, TableFileCorruptedDuringBackup) {
+TEST_F(BackupableDBTest, TableFileWithoutDbChecksumCorruptedDuringBackup) {
   const int keys_iteration = 50000;
-  std::vector<std::shared_ptr<FileChecksumGenFactory>> fac{
-      nullptr, GetFileChecksumGenCrc32cFactory()};
-  for (auto& f : fac) {
-    options_.file_checksum_gen_factory = f;
-    if (f == nullptr) {
-      // When share_files_with_checksum is on, we calculate checksums of table
-      // files before and after copying. So we can test whether a corruption has
-      // happened during the file is copied to backup directory.
-      OpenDBAndBackupEngine(true /* destroy_old_data */, false /* dummy */,
-                            kShareWithChecksum);
-    } else {
-      // Default DB table file checksum is on, we calculate checksums of table
-      // files before copying to verify it with the one stored in DB manifest
-      // and also calculate checksum after copying. So we can test whether a
-      // corruption has happened during the file is copied to backup directory
-      // even if we do not place table files in shared_checksum directory.
-      OpenDBAndBackupEngine(true /* destroy_old_data */, false /* dummy */,
-                            kNoShare);
-    }
-    FillDB(db_.get(), 0, keys_iteration);
-    bool corrupted = false;
-    // corrupt files when copying to the backup directory
-    SyncPoint::GetInstance()->SetCallBack(
-        "BackupEngineImpl::CopyOrCreateFile:CorruptionDuringBackup",
-        [&](void* data) {
-          if (data != nullptr) {
-            Slice* d = reinterpret_cast<Slice*>(data);
-            if (!d->empty()) {
-              d->remove_suffix(1);
-              corrupted = true;
-            }
-          }
-        });
-    SyncPoint::GetInstance()->EnableProcessing();
-    Status s = backup_engine_->CreateNewBackup(db_.get());
-    if (corrupted) {
-      ASSERT_NOK(s);
-    } else {
-      // should not in this path in normal cases
-      ASSERT_OK(s);
-    }
-
-    SyncPoint::GetInstance()->DisableProcessing();
-    SyncPoint::GetInstance()->ClearAllCallBacks();
-
-    CloseDBAndBackupEngine();
-    // delete old files in db
-    ASSERT_OK(DestroyDB(dbname_, options_));
-  }
-}
-
-TEST_P(BackupableDBTestWithParam,
-       TableFileCorruptedDuringBackupWithDefaultDbChecksum) {
-  const int keys_iteration = 100000;
+  backupable_options_->share_files_with_checksum_naming = kChecksumAndFileSize;
+  // When share_files_with_checksum is on, we calculate checksums of table
+  // files before and after copying. So we can test whether a corruption has
+  // happened during the file is copied to backup directory.
+  OpenDBAndBackupEngine(true /* destroy_old_data */, false /* dummy */,
+                        kShareWithChecksum);
 
-  options_.file_checksum_gen_factory = GetFileChecksumGenCrc32cFactory();
-  OpenDBAndBackupEngine(true /* destroy_old_data */);
   FillDB(db_.get(), 0, keys_iteration);
   bool corrupted = false;
   // corrupt files when copying to the backup directory
@@ -1341,6 +1291,49 @@ TEST_P(BackupableDBTestWithParam,
   ASSERT_OK(DestroyDB(dbname_, options_));
 }
 
+TEST_F(BackupableDBTest, TableFileWithDbChecksumCorruptedDuringBackup) {
+  const int keys_iteration = 50000;
+  options_.file_checksum_gen_factory = GetFileChecksumGenCrc32cFactory();
+  for (auto& sopt : kAllShareOptions) {
+    // Since the default DB table file checksum is on, we obtain checksums of
+    // table files from the DB manifest before copying and verify it with the
+    // one calculated during copying.
+    // Therefore, we can test whether a corruption has happened during the file
+    // being copied to backup directory.
+    OpenDBAndBackupEngine(true /* destroy_old_data */, false /* dummy */, sopt);
+
+    FillDB(db_.get(), 0, keys_iteration);
+    bool corrupted = false;
+    // corrupt files when copying to the backup directory
+    SyncPoint::GetInstance()->SetCallBack(
+        "BackupEngineImpl::CopyOrCreateFile:CorruptionDuringBackup",
+        [&](void* data) {
+          if (data != nullptr) {
+            Slice* d = reinterpret_cast<Slice*>(data);
+            if (!d->empty()) {
+              d->remove_suffix(1);
+              corrupted = true;
+            }
+          }
+        });
+    SyncPoint::GetInstance()->EnableProcessing();
+    Status s = backup_engine_->CreateNewBackup(db_.get());
+    if (corrupted) {
+      ASSERT_NOK(s);
+    } else {
+      // should not in this path in normal cases
+      ASSERT_OK(s);
+    }
+
+    SyncPoint::GetInstance()->DisableProcessing();
+    SyncPoint::GetInstance()->ClearAllCallBacks();
+
+    CloseDBAndBackupEngine();
+    // delete old files in db
+    ASSERT_OK(DestroyDB(dbname_, options_));
+  }
+}
+
 TEST_F(BackupableDBTest, InterruptCreationTest) {
   // Interrupt backup creation by failing new writes and failing cleanup of the
   // partial state. Then verify a subsequent backup can still succeed.
@@ -1575,75 +1568,88 @@ TEST_F(BackupableDBTest, ShareTableFilesWithChecksumsTransition) {
 }
 
 //  Verify backup and restore with share_files_with_checksum on and
-//  share_files_with_checksum_naming = kChecksumAndDbSessionId
+//  share_files_with_checksum_naming = kOptionalChecksumAndDbSessionId
 TEST_F(BackupableDBTest, ShareTableFilesWithChecksumsNewNaming) {
-  // Use session id in the name of SST file backup
+  // Use session id in the name of SST files
   ASSERT_TRUE(backupable_options_->share_files_with_checksum_naming ==
-              kChecksumAndDbSessionId);
+              kOptionalChecksumAndDbSessionId);
   const int keys_iteration = 5000;
+  int i = 0;
+
   OpenDBAndBackupEngine(true, false, kShareWithChecksum);
-  for (int i = 0; i < 5; ++i) {
-    FillDB(db_.get(), keys_iteration * i, keys_iteration * (i + 1));
-    ASSERT_OK(backup_engine_->CreateNewBackup(db_.get(), !!(i % 2)));
-  }
+  FillDB(db_.get(), keys_iteration * i, keys_iteration * (i + 1));
+  ASSERT_OK(backup_engine_->CreateNewBackup(db_.get(), !!(i % 2)));
   CloseDBAndBackupEngine();
+  AssertBackupConsistency(i + 1, 0, keys_iteration * (i + 1),
+                          keys_iteration * (i + 2));
 
-  for (int i = 0; i < 5; ++i) {
-    AssertBackupConsistency(i + 1, 0, keys_iteration * (i + 1),
-                            keys_iteration * 6);
-  }
+  // Both checksum and session id in the name of SST files
+  options_.file_checksum_gen_factory = GetFileChecksumGenCrc32cFactory();
+  OpenDBAndBackupEngine(false, false, kShareWithChecksum);
+  FillDB(db_.get(), keys_iteration * i, keys_iteration * (i + 1));
+  ASSERT_OK(backup_engine_->CreateNewBackup(db_.get(), !!(i % 2)));
+  CloseDBAndBackupEngine();
+  AssertBackupConsistency(i + 1, 0, keys_iteration * (i + 1),
+                          keys_iteration * (i + 2));
 }
 
 // Verify backup and restore with share_files_with_checksum off and then
-// transition this option and share_files_with_checksum_naming to be
-// kChecksumAndDbSessionId
+// transition this option to on and share_files_with_checksum_naming to be
+// kOptionalChecksumAndDbSessionId
 TEST_F(BackupableDBTest, ShareTableFilesWithChecksumsNewNamingTransition) {
   const int keys_iteration = 5000;
   // We may set share_files_with_checksum_naming to kChecksumAndFileSize
   // here but even if we don't, it should have no effect when
   // share_files_with_checksum is false
   ASSERT_TRUE(backupable_options_->share_files_with_checksum_naming ==
-              kChecksumAndDbSessionId);
+              kOptionalChecksumAndDbSessionId);
   // set share_files_with_checksum to false
   OpenDBAndBackupEngine(true, false, kShareNoChecksum);
-  for (int i = 0; i < 5; ++i) {
+  int j = 3;
+  for (int i = 0; i < j; ++i) {
     FillDB(db_.get(), keys_iteration * i, keys_iteration * (i + 1));
     ASSERT_OK(backup_engine_->CreateNewBackup(db_.get(), true));
   }
   CloseDBAndBackupEngine();
 
-  for (int i = 0; i < 5; ++i) {
+  for (int i = 0; i < j; ++i) {
     AssertBackupConsistency(i + 1, 0, keys_iteration * (i + 1),
-                            keys_iteration * 6);
+                            keys_iteration * (j + 1));
   }
 
   // set share_files_with_checksum to true and do some more backups
   // and use session id in the name of SST file backup
   ASSERT_TRUE(backupable_options_->share_files_with_checksum_naming ==
-              kChecksumAndDbSessionId);
+              kOptionalChecksumAndDbSessionId);
   OpenDBAndBackupEngine(false /* destroy_old_data */, false,
                         kShareWithChecksum);
-  for (int i = 5; i < 10; ++i) {
-    FillDB(db_.get(), keys_iteration * i, keys_iteration * (i + 1));
-    ASSERT_OK(backup_engine_->CreateNewBackup(db_.get(), true));
-  }
+  FillDB(db_.get(), keys_iteration * j, keys_iteration * (j + 1));
+  ASSERT_OK(backup_engine_->CreateNewBackup(db_.get(), true));
+  CloseDBAndBackupEngine();
+  // Use checksum in the name as well
+  ++j;
+  options_.file_checksum_gen_factory = GetFileChecksumGenCrc32cFactory();
+  OpenDBAndBackupEngine(false /* destroy_old_data */, false,
+                        kShareWithChecksum);
+  FillDB(db_.get(), keys_iteration * j, keys_iteration * (j + 1));
+  ASSERT_OK(backup_engine_->CreateNewBackup(db_.get(), true));
   CloseDBAndBackupEngine();
 
   // Verify first (about to delete)
-  AssertBackupConsistency(1, 0, keys_iteration, keys_iteration * 11);
+  AssertBackupConsistency(1, 0, keys_iteration, keys_iteration * (j + 1));
 
   // For an extra challenge, make sure that GarbageCollect / DeleteBackup
   // is OK even if we open without share_table_files but with
-  // share_files_with_checksum_naming being kChecksumAndDbSessionId
+  // share_files_with_checksum_naming being kOptionalChecksumAndDbSessionId
   ASSERT_TRUE(backupable_options_->share_files_with_checksum_naming ==
-              kChecksumAndDbSessionId);
+              kOptionalChecksumAndDbSessionId);
   OpenDBAndBackupEngine(false /* destroy_old_data */, false, kNoShare);
   backup_engine_->DeleteBackup(1);
   backup_engine_->GarbageCollect();
   CloseDBAndBackupEngine();
 
   // Verify second (about to delete)
-  AssertBackupConsistency(2, 0, keys_iteration * 2, keys_iteration * 11);
+  AssertBackupConsistency(2, 0, keys_iteration * 2, keys_iteration * (j + 1));
 
   // Use checksum and file size for backup table file names and open without
   // share_table_files
@@ -1655,42 +1661,49 @@ TEST_F(BackupableDBTest, ShareTableFilesWithChecksumsNewNamingTransition) {
   CloseDBAndBackupEngine();
 
   // Verify rest (not deleted)
-  for (int i = 1; i < 9; ++i) {
-    AssertBackupConsistency(i + 2, 0, keys_iteration * (i + 2),
-                            keys_iteration * 11);
+  for (int i = 2; i < j; ++i) {
+    AssertBackupConsistency(i + 1, 0, keys_iteration * (i + 1),
+                            keys_iteration * (j + 1));
   }
 }
 
 // Verify backup and restore with share_files_with_checksum on and transition
-// from kChecksumAndFileSize to kChecksumAndDbSessionId
+// from kChecksumAndFileSize to kOptionalChecksumAndDbSessionId
 TEST_F(BackupableDBTest, ShareTableFilesWithChecksumsNewNamingUpgrade) {
   backupable_options_->share_files_with_checksum_naming = kChecksumAndFileSize;
   const int keys_iteration = 5000;
   // set share_files_with_checksum to true
   OpenDBAndBackupEngine(true, false, kShareWithChecksum);
-  for (int i = 0; i < 5; ++i) {
+  int j = 3;
+  for (int i = 0; i < j; ++i) {
     FillDB(db_.get(), keys_iteration * i, keys_iteration * (i + 1));
     ASSERT_OK(backup_engine_->CreateNewBackup(db_.get(), true));
   }
   CloseDBAndBackupEngine();
 
-  for (int i = 0; i < 5; ++i) {
+  for (int i = 0; i < j; ++i) {
     AssertBackupConsistency(i + 1, 0, keys_iteration * (i + 1),
-                            keys_iteration * 6);
+                            keys_iteration * (j + 1));
   }
 
   backupable_options_->share_files_with_checksum_naming =
-      kChecksumAndDbSessionId;
+      kOptionalChecksumAndDbSessionId;
   OpenDBAndBackupEngine(false /* destroy_old_data */, false,
                         kShareWithChecksum);
-  for (int i = 5; i < 10; ++i) {
-    FillDB(db_.get(), keys_iteration * i, keys_iteration * (i + 1));
-    ASSERT_OK(backup_engine_->CreateNewBackup(db_.get(), true));
-  }
+  FillDB(db_.get(), keys_iteration * j, keys_iteration * (j + 1));
+  ASSERT_OK(backup_engine_->CreateNewBackup(db_.get(), true));
+  CloseDBAndBackupEngine();
+
+  ++j;
+  options_.file_checksum_gen_factory = GetFileChecksumGenCrc32cFactory();
+  OpenDBAndBackupEngine(false /* destroy_old_data */, false,
+                        kShareWithChecksum);
+  FillDB(db_.get(), keys_iteration * j, keys_iteration * (j + 1));
+  ASSERT_OK(backup_engine_->CreateNewBackup(db_.get(), true));
   CloseDBAndBackupEngine();
 
   // Verify first (about to delete)
-  AssertBackupConsistency(1, 0, keys_iteration, keys_iteration * 11);
+  AssertBackupConsistency(1, 0, keys_iteration, keys_iteration * (j + 1));
 
   // For an extra challenge, make sure that GarbageCollect / DeleteBackup
   // is OK even if we open without share_table_files
@@ -1700,7 +1713,7 @@ TEST_F(BackupableDBTest, ShareTableFilesWithChecksumsNewNamingUpgrade) {
   CloseDBAndBackupEngine();
 
   // Verify second (about to delete)
-  AssertBackupConsistency(2, 0, keys_iteration * 2, keys_iteration * 11);
+  AssertBackupConsistency(2, 0, keys_iteration * 2, keys_iteration * (j + 1));
 
   // Use checksum and file size for backup table file names and open without
   // share_table_files
@@ -1712,9 +1725,9 @@ TEST_F(BackupableDBTest, ShareTableFilesWithChecksumsNewNamingUpgrade) {
   CloseDBAndBackupEngine();
 
   // Verify rest (not deleted)
-  for (int i = 2; i < 10; ++i) {
+  for (int i = 2; i < j; ++i) {
     AssertBackupConsistency(i + 1, 0, keys_iteration * (i + 1),
-                            keys_iteration * 11);
+                            keys_iteration * (j + 1));
   }
 }
 
@@ -2259,7 +2272,7 @@ TEST_P(BackupableDBTestWithParam, BackupUsingDirectIO) {
 
     // Verify backup engine always opened files with direct I/O
     ASSERT_EQ(0, test_db_env_->num_writers());
-    ASSERT_GT(test_db_env_->num_direct_rand_readers(), 0);
+    ASSERT_GE(test_db_env_->num_direct_rand_readers(), 0);
     ASSERT_GT(test_db_env_->num_direct_seq_readers(), 0);
     // Currently the DB doesn't support reading WALs or manifest with direct
     // I/O, so subtract two.