Fail fast in paranoid mode when LoadTableHandlers fail during recovering (#6368)

Summary:
Previously, when recovering version set, LoadTableHandlers failures are ignored.
If paranoid_checks is true, this failure should not be ignored, otherwise, the opened db might be in an inconsistent state.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6368

Test Plan: make check

Differential Revision: D19713459

Pulled By: cheng-chang

fbshipit-source-id: 68cb94f4f2cc43f8b024b14755193cd45cfcad55

db/corruption_test.cc

@@ -393,12 +393,16 @@ TEST_F(CorruptionTest, TableFileIndexData) {
 
   // corrupt an index block of an entire file
   Corrupt(kTableFile, -2000, 500);
-  Reopen();
+  options.paranoid_checks = false;
+  Reopen(&options);
   dbi = reinterpret_cast<DBImpl*>(db_);
   // one full file may be readable, since only one was corrupted
   // the other file should be fully non-readable, since index was corrupted
   Check(0, 5000);
   ASSERT_NOK(dbi->VerifyChecksum());
+
+  // In paranoid mode, the db cannot be opened due to the corrupted file.
+  ASSERT_TRUE(TryReopen().IsCorruption());
 }
 
 TEST_F(CorruptionTest, MissingDescriptor) {
@@ -554,13 +558,7 @@ TEST_F(CorruptionTest, RangeDeletionCorrupted) {
 
   ASSERT_OK(TryReopen());
   CorruptFile(filename, static_cast<int>(range_del_handle.offset()), 1);
-  // The test case does not fail on TryReopen because failure to preload table
-  // handlers is not considered critical.
-  ASSERT_OK(TryReopen());
-  std::string val;
-  // However, it does fail on any read involving that file since that file
-  // cannot be opened with a corrupt range deletion meta-block.
-  ASSERT_TRUE(db_->Get(ReadOptions(), "a", &val).IsCorruption());
+  ASSERT_TRUE(TryReopen().IsCorruption());
 }
 
 TEST_F(CorruptionTest, FileSystemStateCorrupted) {
@@ -585,14 +583,15 @@ TEST_F(CorruptionTest, FileSystemStateCorrupted) {
       env_.NewWritableFile(filename, &file, EnvOptions());
       file->Append(Slice("corrupted sst"));
       file.reset();
+      Status x = TryReopen(&options);
+      ASSERT_TRUE(x.IsCorruption());
     } else {  // delete the file
       env_.DeleteFile(filename);
+      Status x = TryReopen(&options);
+      ASSERT_TRUE(x.IsPathNotFound());
     }
 
-    Status x = TryReopen(&options);
-    ASSERT_TRUE(x.IsCorruption());
     DestroyDB(dbname_, options_);
-    Reopen(&options);
   }
 }
 

db/cuckoo_table_db_test.cc

@@ -298,17 +298,25 @@ TEST_F(CuckooTableDBTest, AdaptiveTable) {
   dbfull()->TEST_FlushMemTable();
 
   // Write some keys using plain table.
+  std::shared_ptr<TableFactory> block_based_factory(
+      NewBlockBasedTableFactory());
+  std::shared_ptr<TableFactory> plain_table_factory(
+      NewPlainTableFactory());
+  std::shared_ptr<TableFactory> cuckoo_table_factory(
+      NewCuckooTableFactory());
   options.create_if_missing = false;
-  options.table_factory.reset(NewPlainTableFactory());
+  options.table_factory.reset(NewAdaptiveTableFactory(
+    plain_table_factory, block_based_factory, plain_table_factory,
+    cuckoo_table_factory));
   Reopen(&options);
   ASSERT_OK(Put("key4", "v4"));
   ASSERT_OK(Put("key1", "v5"));
   dbfull()->TEST_FlushMemTable();
 
   // Write some keys using block based table.
-  std::shared_ptr<TableFactory> block_based_factory(
-      NewBlockBasedTableFactory());
-  options.table_factory.reset(NewAdaptiveTableFactory(block_based_factory));
+  options.table_factory.reset(NewAdaptiveTableFactory(
+    block_based_factory, block_based_factory, plain_table_factory,
+    cuckoo_table_factory));
   Reopen(&options);
   ASSERT_OK(Put("key5", "v6"));
   ASSERT_OK(Put("key2", "v7"));

db/plain_table_db_test.cc

@@ -401,19 +401,17 @@ TEST_P(PlainTableDBTest, BadOptions1) {
   // Bad attempt to re-open without a prefix extractor
   Options options = CurrentOptions();
   options.prefix_extractor.reset();
-  Reopen(&options);
   ASSERT_EQ(
       "Invalid argument: Prefix extractor is missing when opening a PlainTable "
       "built using a prefix extractor",
-      Get("1000000000000foo"));
+      TryReopen(&options).ToString());
 
   // Bad attempt to re-open with different prefix extractor
   options.prefix_extractor.reset(NewFixedPrefixTransform(6));
-  Reopen(&options);
   ASSERT_EQ(
       "Invalid argument: Prefix extractor given doesn't match the one used to "
       "build PlainTable",
-      Get("1000000000000foo"));
+      TryReopen(&options).ToString());
 
   // Correct prefix extractor
   options.prefix_extractor.reset(NewFixedPrefixTransform(8));
@@ -1323,11 +1321,13 @@ TEST_P(PlainTableDBTest, AdaptiveTable) {
   dbfull()->TEST_FlushMemTable();
 
   options.create_if_missing = false;
-  std::shared_ptr<TableFactory> dummy_factory;
   std::shared_ptr<TableFactory> block_based_factory(
       NewBlockBasedTableFactory());
+  std::shared_ptr<TableFactory> plain_table_factory(
+      NewPlainTableFactory());
+  std::shared_ptr<TableFactory> dummy_factory;
   options.table_factory.reset(NewAdaptiveTableFactory(
-      block_based_factory, dummy_factory, dummy_factory));
+      block_based_factory, block_based_factory, plain_table_factory));
   Reopen(&options);
   ASSERT_EQ("v3", Get("1000000000000foo"));
   ASSERT_EQ("v2", Get("0000000000000bar"));
@@ -1344,10 +1344,12 @@ TEST_P(PlainTableDBTest, AdaptiveTable) {
   ASSERT_EQ("v4", Get("2000000000000foo"));
   ASSERT_EQ("v5", Get("3000000000000bar"));
 
+  options.paranoid_checks = false;
   options.table_factory.reset(NewBlockBasedTableFactory());
   Reopen(&options);
   ASSERT_NE("v3", Get("1000000000000foo"));
 
+  options.paranoid_checks = false;
   options.table_factory.reset(NewPlainTableFactory());
   Reopen(&options);
   ASSERT_NE("v5", Get("3000000000000bar"));

db/version_set.cc

@@ -3803,17 +3803,23 @@ Status VersionSet::ProcessManifestWrites(
         assert(!mutable_cf_options_ptrs.empty() &&
                builder_guards.size() == versions.size());
         ColumnFamilyData* cfd = versions[i]->cfd_;
-        builder_guards[i]->version_builder()->LoadTableHandlers(
+        s = builder_guards[i]->version_builder()->LoadTableHandlers(
             cfd->internal_stats(), cfd->ioptions()->optimize_filters_for_hits,
             true /* prefetch_index_and_filter_in_cache */,
             false /* is_initial_load */,
             mutable_cf_options_ptrs[i]->prefix_extractor.get());
+        if (!s.ok()) {
+          if (db_options_->paranoid_checks) {
+            break;
+          }
+          s = Status::OK();
+        }
       }
     }
 
-    // This is fine because everything inside of this block is serialized --
-    // only one thread can be here at the same time
-    if (new_descriptor_log) {
+    if (s.ok() && new_descriptor_log) {
+      // This is fine because everything inside of this block is serialized --
+      // only one thread can be here at the same time
       // create new manifest file
       ROCKS_LOG_INFO(db_options_->info_log, "Creating manifest %" PRIu64 "\n",
                      pending_manifest_file_number_);
@@ -3835,14 +3841,14 @@ Status VersionSet::ProcessManifestWrites(
       }
     }
 
-    if (!first_writer.edit_list.front()->IsColumnFamilyManipulation()) {
-      for (int i = 0; i < static_cast<int>(versions.size()); ++i) {
-        versions[i]->PrepareApply(*mutable_cf_options_ptrs[i], true);
+    if (s.ok()) {
+      if (!first_writer.edit_list.front()->IsColumnFamilyManipulation()) {
+        for (int i = 0; i < static_cast<int>(versions.size()); ++i) {
+          versions[i]->PrepareApply(*mutable_cf_options_ptrs[i], true);
+        }
       }
-    }
 
-    // Write new records to MANIFEST log
-    if (s.ok()) {
+      // Write new records to MANIFEST log
 #ifndef NDEBUG
       size_t idx = 0;
 #endif
@@ -4386,8 +4392,8 @@ Status VersionSet::Recover(
     const std::vector<ColumnFamilyDescriptor>& column_families, bool read_only,
     std::string* db_id) {
   std::unordered_map<std::string, ColumnFamilyOptions> cf_name_to_options;
-  for (auto cf : column_families) {
-    cf_name_to_options.insert({cf.name, cf.options});
+  for (const auto& cf : column_families) {
+    cf_name_to_options.emplace(cf.name, cf.options);
   }
   // keeps track of column families in manifest that were not found in
   // column families parameters. if those column families are not dropped
@@ -4518,11 +4524,17 @@ Status VersionSet::Recover(
 
       // unlimited table cache. Pre-load table handle now.
       // Need to do it out of the mutex.
-      builder->LoadTableHandlers(
+      s = builder->LoadTableHandlers(
           cfd->internal_stats(), db_options_->max_file_opening_threads,
           false /* prefetch_index_and_filter_in_cache */,
           true /* is_initial_load */,
           cfd->GetLatestMutableCFOptions()->prefix_extractor.get());
+      if (!s.ok()) {
+        if (db_options_->paranoid_checks) {
+          return s;
+        }
+        s = Status::OK();
+      }
 
       Version* v = new Version(cfd, this, file_options_,
                                *cfd->GetLatestMutableCFOptions(),