From 78e291b17e4c9c6b777ca4cfc666ade6f69c3c17 Mon Sep 17 00:00:00 2001 From: Levi Tamasi Date: Wed, 3 Jun 2020 11:21:16 -0700 Subject: [PATCH] Improve consistency checks in VersionBuilder (#6901) Summary: The patch cleans up the code and improves the consistency checks around adding/deleting table files in `VersionBuilder`. Namely, it makes the checks stricter and improves them in the following ways: 1) A table file can now only be deleted from the LSM tree using the level it resides on. Earlier, there was some unnecessary wiggle room for trivially moved files (they could be deleted using a lower level number than the actual one). 2) A table file cannot be added to the tree if it is already present in the tree on any level (not just the target level). The earlier code only had an assertion (which is a no-op in release builds) that the newly added file is not already present on the target level. 3) The above consistency checks around state transitions are now mandatory, as opposed to the earlier `CheckConsistencyForDeletes`, which was a no-op in release mode unless `force_consistency_checks` was set to `true`. The rationale here is that assuming that the initial state is consistent, a valid transition leads to a next state that is also consistent; however, an *invalid* transition offers no such guarantee. Hence it makes sense to validate the transitions unconditionally, and save `force_consistency_checks` for the paranoid checks that re-validate the entire state. 4) The new checks build on the mechanism introduced in https://github.com/facebook/rocksdb/pull/6862, which enables us to efficiently look up the location (level and position within level) of files in a `Version` by file number. This makes the consistency checks much more efficient than the earlier `CheckConsistencyForDeletes`, which essentially performed a linear search. Pull Request resolved: https://github.com/facebook/rocksdb/pull/6901 Test Plan: Extended the unit tests and ran: `make check` `make whitebox_crash_test` Reviewed By: ajkr Differential Revision: D21822714 Pulled By: ltamasi fbshipit-source-id: e2b29c8b6da1bf0f59004acc889e4870b2d18215 --- db/version_builder.cc | 232 ++++++++++++++++++++++--------------- db/version_builder_test.cc | 231 +++++++++++++++++++++++++++++++++++- 2 files changed, 366 insertions(+), 97 deletions(-) diff --git a/db/version_builder.cc b/db/version_builder.cc index c8ef9b986..49259f19f 100644 --- a/db/version_builder.cc +++ b/db/version_builder.cc @@ -130,14 +130,16 @@ class VersionBuilder::Rep { VersionSet* version_set_; int num_levels_; LevelState* levels_; - // Store states of levels larger than num_levels_. We do this instead of + // Store sizes of levels larger than num_levels_. We do this instead of // storing them in levels_ to avoid regression in case there are no files // on invalid levels. The version is not consistent if in the end the files // on invalid levels don't cancel out. - std::map> invalid_levels_; + std::unordered_map invalid_level_sizes_; // Whether there are invalid new files or invalid deletion on levels larger // than num_levels_. bool has_invalid_levels_; + // Current levels of table files affected by additions/deletions. + std::unordered_map table_file_levels_; FileComparator level_zero_cmp_; FileComparator level_nonzero_cmp_; @@ -360,65 +362,19 @@ class VersionBuilder::Rep { return ret_s; } - Status CheckConsistencyForDeletes(VersionEdit* /*edit*/, uint64_t number, - int level) { -#ifdef NDEBUG - if (!base_vstorage_->force_consistency_checks()) { - // Dont run consistency checks in release mode except if - // explicitly asked to - return Status::OK(); - } -#endif - // a file to be deleted better exist in the previous version - bool found = false; - for (int l = 0; !found && l < num_levels_; l++) { - const std::vector& base_files = - base_vstorage_->LevelFiles(l); - for (size_t i = 0; i < base_files.size(); i++) { - FileMetaData* f = base_files[i]; - if (f->fd.GetNumber() == number) { - found = true; - break; - } - } - } - // if the file did not exist in the previous version, then it - // is possibly moved from lower level to higher level in current - // version - for (int l = level + 1; !found && l < num_levels_; l++) { - auto& level_added = levels_[l].added_files; - auto got = level_added.find(number); - if (got != level_added.end()) { - found = true; - break; - } - } - - // maybe this file was added in a previous edit that was Applied - if (!found) { - auto& level_added = levels_[level].added_files; - auto got = level_added.find(number); - if (got != level_added.end()) { - found = true; - } - } - if (!found) { - fprintf(stderr, "not found %" PRIu64 "\n", number); - return Status::Corruption("not found " + NumberToString(number)); - } - return Status::OK(); - } - - bool CheckConsistencyForNumLevels() { + bool CheckConsistencyForNumLevels() const { // Make sure there are no files on or beyond num_levels(). if (has_invalid_levels_) { return false; } - for (auto& level : invalid_levels_) { - if (level.second.size() > 0) { + + for (const auto& pair : invalid_level_sizes_) { + const size_t level_size = pair.second; + if (level_size != 0) { return false; } } + return true; } @@ -479,64 +435,148 @@ class VersionBuilder::Rep { return Status::OK(); } + int GetCurrentLevelForTableFile(uint64_t file_number) const { + auto it = table_file_levels_.find(file_number); + if (it != table_file_levels_.end()) { + return it->second; + } + + assert(base_vstorage_); + return base_vstorage_->GetFileLocation(file_number).GetLevel(); + } + + Status ApplyFileDeletion(int level, uint64_t file_number) { + assert(level != VersionStorageInfo::FileLocation::Invalid().GetLevel()); + + const int current_level = GetCurrentLevelForTableFile(file_number); + + if (level != current_level) { + if (level >= num_levels_) { + has_invalid_levels_ = true; + } + + std::ostringstream oss; + oss << "Cannot delete table file #" << file_number << " from level " + << level << " since it is "; + if (current_level == + VersionStorageInfo::FileLocation::Invalid().GetLevel()) { + oss << "not in the LSM tree"; + } else { + oss << "on level " << current_level; + } + + return Status::Corruption("VersionBuilder", oss.str()); + } + + if (level >= num_levels_) { + assert(invalid_level_sizes_[level] > 0); + --invalid_level_sizes_[level]; + + table_file_levels_[file_number] = + VersionStorageInfo::FileLocation::Invalid().GetLevel(); + + return Status::OK(); + } + + auto& level_state = levels_[level]; + + auto& add_files = level_state.added_files; + auto add_it = add_files.find(file_number); + if (add_it != add_files.end()) { + UnrefFile(add_it->second); + add_files.erase(add_it); + } else { + auto& del_files = level_state.deleted_files; + assert(del_files.find(file_number) == del_files.end()); + del_files.emplace(file_number); + } + + table_file_levels_[file_number] = + VersionStorageInfo::FileLocation::Invalid().GetLevel(); + + return Status::OK(); + } + + Status ApplyFileAddition(int level, const FileMetaData& meta) { + assert(level != VersionStorageInfo::FileLocation::Invalid().GetLevel()); + + const uint64_t file_number = meta.fd.GetNumber(); + + const int current_level = GetCurrentLevelForTableFile(file_number); + + if (current_level != + VersionStorageInfo::FileLocation::Invalid().GetLevel()) { + if (level >= num_levels_) { + has_invalid_levels_ = true; + } + + std::ostringstream oss; + oss << "Cannot add table file #" << file_number << " to level " << level + << " since it is already in the LSM tree on level " << current_level; + return Status::Corruption("VersionBuilder", oss.str()); + } + + if (level >= num_levels_) { + ++invalid_level_sizes_[level]; + table_file_levels_[file_number] = level; + + return Status::OK(); + } + + auto& level_state = levels_[level]; + + auto& del_files = level_state.deleted_files; + auto del_it = del_files.find(file_number); + if (del_it != del_files.end()) { + del_files.erase(del_it); + } else { + FileMetaData* const f = new FileMetaData(meta); + f->refs = 1; + + auto& add_files = level_state.added_files; + assert(add_files.find(file_number) == add_files.end()); + add_files.emplace(file_number, f); + } + + table_file_levels_[file_number] = level; + + return Status::OK(); + } + // Apply all of the edits in *edit to the current state. Status Apply(VersionEdit* edit) { - Status s = CheckConsistency(base_vstorage_); - if (!s.ok()) { - return s; + { + const Status s = CheckConsistency(base_vstorage_); + if (!s.ok()) { + return s; + } } // Delete files - const auto& del = edit->GetDeletedFiles(); - for (const auto& del_file : del) { - const auto level = del_file.first; - const auto number = del_file.second; - if (level < num_levels_) { - levels_[level].deleted_files.insert(number); - s = CheckConsistencyForDeletes(edit, number, level); - if (!s.ok()) { - return s; - } + for (const auto& deleted_file : edit->GetDeletedFiles()) { + const int level = deleted_file.first; + const uint64_t file_number = deleted_file.second; - auto exising = levels_[level].added_files.find(number); - if (exising != levels_[level].added_files.end()) { - UnrefFile(exising->second); - levels_[level].added_files.erase(exising); - } - } else { - if (invalid_levels_[level].erase(number) == 0) { - // Deleting an non-existing file on invalid level. - has_invalid_levels_ = true; - } + const Status s = ApplyFileDeletion(level, file_number); + if (!s.ok()) { + return s; } } // Add new files for (const auto& new_file : edit->GetNewFiles()) { const int level = new_file.first; - if (level < num_levels_) { - FileMetaData* f = new FileMetaData(new_file.second); - f->refs = 1; + const FileMetaData& meta = new_file.second; - assert(levels_[level].added_files.find(f->fd.GetNumber()) == - levels_[level].added_files.end()); - levels_[level].deleted_files.erase(f->fd.GetNumber()); - levels_[level].added_files[f->fd.GetNumber()] = f; - } else { - uint64_t number = new_file.second.fd.GetNumber(); - auto& lvls = invalid_levels_[level]; - if (lvls.count(number) == 0) { - lvls.insert(number); - } else { - // Creating an already existing file on invalid level. - has_invalid_levels_ = true; - } + const Status s = ApplyFileAddition(level, meta); + if (!s.ok()) { + return s; } } // Add new blob files for (const auto& blob_file_addition : edit->GetBlobFileAdditions()) { - s = ApplyBlobFileAddition(blob_file_addition); + const Status s = ApplyBlobFileAddition(blob_file_addition); if (!s.ok()) { return s; } @@ -544,13 +584,13 @@ class VersionBuilder::Rep { // Increase the amount of garbage for blob files affected by GC for (const auto& blob_file_garbage : edit->GetBlobFileGarbages()) { - s = ApplyBlobFileGarbage(blob_file_garbage); + const Status s = ApplyBlobFileGarbage(blob_file_garbage); if (!s.ok()) { return s; } } - return s; + return Status::OK(); } static std::shared_ptr CreateMetaDataForNewBlobFile( diff --git a/db/version_builder_test.cc b/db/version_builder_test.cc index 73db31c1b..6e6fd1c51 100644 --- a/db/version_builder_test.cc +++ b/db/version_builder_test.cc @@ -54,7 +54,7 @@ class VersionBuilderTest : public testing::Test { return InternalKey(ukey, smallest_seq, kTypeValue); } - void Add(int level, uint32_t file_number, const char* smallest, + void Add(int level, uint64_t file_number, const char* smallest, const char* largest, uint64_t file_size = 0, uint32_t path_id = 0, SequenceNumber smallest_seq = 100, SequenceNumber largest_seq = 100, uint64_t num_entries = 0, uint64_t num_deletions = 0, @@ -367,6 +367,235 @@ TEST_F(VersionBuilderTest, ApplyDeleteAndSaveTo) { UnrefFilesInVersion(&new_vstorage); } +TEST_F(VersionBuilderTest, ApplyFileDeletionIncorrectLevel) { + constexpr int level = 1; + constexpr uint64_t file_number = 2345; + constexpr char smallest[] = "bar"; + constexpr char largest[] = "foo"; + + Add(level, file_number, smallest, largest); + + EnvOptions env_options; + constexpr TableCache* table_cache = nullptr; + constexpr VersionSet* version_set = nullptr; + + VersionBuilder builder(env_options, &ioptions_, table_cache, &vstorage_, + version_set); + + VersionEdit edit; + + constexpr int incorrect_level = 3; + + edit.DeleteFile(incorrect_level, file_number); + + const Status s = builder.Apply(&edit); + ASSERT_TRUE(s.IsCorruption()); + ASSERT_TRUE(std::strstr(s.getState(), + "Cannot delete table file #2345 from level 3 since " + "it is on level 1")); +} + +TEST_F(VersionBuilderTest, ApplyFileDeletionNotInLSMTree) { + EnvOptions env_options; + constexpr TableCache* table_cache = nullptr; + constexpr VersionSet* version_set = nullptr; + + VersionBuilder builder(env_options, &ioptions_, table_cache, &vstorage_, + version_set); + + VersionEdit edit; + + constexpr int level = 3; + constexpr uint64_t file_number = 1234; + + edit.DeleteFile(level, file_number); + + const Status s = builder.Apply(&edit); + ASSERT_TRUE(s.IsCorruption()); + ASSERT_TRUE(std::strstr(s.getState(), + "Cannot delete table file #1234 from level 3 since " + "it is not in the LSM tree")); +} + +TEST_F(VersionBuilderTest, ApplyFileDeletionAndAddition) { + constexpr int level = 1; + constexpr uint64_t file_number = 2345; + constexpr char smallest[] = "bar"; + constexpr char largest[] = "foo"; + + Add(level, file_number, smallest, largest); + + EnvOptions env_options; + constexpr TableCache* table_cache = nullptr; + constexpr VersionSet* version_set = nullptr; + + VersionBuilder builder(env_options, &ioptions_, table_cache, &vstorage_, + version_set); + + VersionEdit deletion; + + deletion.DeleteFile(level, file_number); + + ASSERT_OK(builder.Apply(&deletion)); + + VersionEdit addition; + + constexpr uint32_t path_id = 0; + constexpr uint64_t file_size = 10000; + constexpr SequenceNumber smallest_seqno = 100; + constexpr SequenceNumber largest_seqno = 1000; + constexpr bool marked_for_compaction = false; + + addition.AddFile(level, file_number, path_id, file_size, + GetInternalKey(smallest), GetInternalKey(largest), + smallest_seqno, largest_seqno, marked_for_compaction, + kInvalidBlobFileNumber, kUnknownOldestAncesterTime, + kUnknownFileCreationTime, kUnknownFileChecksum, + kUnknownFileChecksumFuncName); + + ASSERT_OK(builder.Apply(&addition)); + + constexpr bool force_consistency_checks = false; + VersionStorageInfo new_vstorage(&icmp_, ucmp_, options_.num_levels, + kCompactionStyleLevel, &vstorage_, + force_consistency_checks); + + ASSERT_OK(builder.SaveTo(&new_vstorage)); + ASSERT_EQ(new_vstorage.GetFileLocation(file_number).GetLevel(), level); + + UnrefFilesInVersion(&new_vstorage); +} + +TEST_F(VersionBuilderTest, ApplyFileAdditionAlreadyInBase) { + constexpr int level = 1; + constexpr uint64_t file_number = 2345; + constexpr char smallest[] = "bar"; + constexpr char largest[] = "foo"; + + Add(level, file_number, smallest, largest); + + EnvOptions env_options; + constexpr TableCache* table_cache = nullptr; + constexpr VersionSet* version_set = nullptr; + + VersionBuilder builder(env_options, &ioptions_, table_cache, &vstorage_, + version_set); + + VersionEdit edit; + + constexpr int new_level = 2; + constexpr uint32_t path_id = 0; + constexpr uint64_t file_size = 10000; + constexpr SequenceNumber smallest_seqno = 100; + constexpr SequenceNumber largest_seqno = 1000; + constexpr bool marked_for_compaction = false; + + edit.AddFile(new_level, file_number, path_id, file_size, + GetInternalKey(smallest), GetInternalKey(largest), + smallest_seqno, largest_seqno, marked_for_compaction, + kInvalidBlobFileNumber, kUnknownOldestAncesterTime, + kUnknownFileCreationTime, kUnknownFileChecksum, + kUnknownFileChecksumFuncName); + + const Status s = builder.Apply(&edit); + ASSERT_TRUE(s.IsCorruption()); + ASSERT_TRUE(std::strstr(s.getState(), + "Cannot add table file #2345 to level 2 since it is " + "already in the LSM tree on level 1")); +} + +TEST_F(VersionBuilderTest, ApplyFileAdditionAlreadyApplied) { + EnvOptions env_options; + constexpr TableCache* table_cache = nullptr; + constexpr VersionSet* version_set = nullptr; + + VersionBuilder builder(env_options, &ioptions_, table_cache, &vstorage_, + version_set); + + VersionEdit edit; + + constexpr int level = 3; + constexpr uint64_t file_number = 2345; + constexpr uint32_t path_id = 0; + constexpr uint64_t file_size = 10000; + constexpr char smallest[] = "bar"; + constexpr char largest[] = "foo"; + constexpr SequenceNumber smallest_seqno = 100; + constexpr SequenceNumber largest_seqno = 1000; + constexpr bool marked_for_compaction = false; + + edit.AddFile(level, file_number, path_id, file_size, GetInternalKey(smallest), + GetInternalKey(largest), smallest_seqno, largest_seqno, + marked_for_compaction, kInvalidBlobFileNumber, + kUnknownOldestAncesterTime, kUnknownFileCreationTime, + kUnknownFileChecksum, kUnknownFileChecksumFuncName); + + ASSERT_OK(builder.Apply(&edit)); + + VersionEdit other_edit; + + constexpr int new_level = 2; + + other_edit.AddFile(new_level, file_number, path_id, file_size, + GetInternalKey(smallest), GetInternalKey(largest), + smallest_seqno, largest_seqno, marked_for_compaction, + kInvalidBlobFileNumber, kUnknownOldestAncesterTime, + kUnknownFileCreationTime, kUnknownFileChecksum, + kUnknownFileChecksumFuncName); + + const Status s = builder.Apply(&other_edit); + ASSERT_TRUE(s.IsCorruption()); + ASSERT_TRUE(std::strstr(s.getState(), + "Cannot add table file #2345 to level 2 since it is " + "already in the LSM tree on level 3")); +} + +TEST_F(VersionBuilderTest, ApplyFileAdditionAndDeletion) { + constexpr int level = 1; + constexpr uint64_t file_number = 2345; + constexpr uint32_t path_id = 0; + constexpr uint64_t file_size = 10000; + constexpr char smallest[] = "bar"; + constexpr char largest[] = "foo"; + constexpr SequenceNumber smallest_seqno = 100; + constexpr SequenceNumber largest_seqno = 1000; + constexpr bool marked_for_compaction = false; + + EnvOptions env_options; + constexpr TableCache* table_cache = nullptr; + constexpr VersionSet* version_set = nullptr; + + VersionBuilder builder(env_options, &ioptions_, table_cache, &vstorage_, + version_set); + + VersionEdit addition; + + addition.AddFile(level, file_number, path_id, file_size, + GetInternalKey(smallest), GetInternalKey(largest), + smallest_seqno, largest_seqno, marked_for_compaction, + kInvalidBlobFileNumber, kUnknownOldestAncesterTime, + kUnknownFileCreationTime, kUnknownFileChecksum, + kUnknownFileChecksumFuncName); + + ASSERT_OK(builder.Apply(&addition)); + + VersionEdit deletion; + + deletion.DeleteFile(level, file_number); + + ASSERT_OK(builder.Apply(&deletion)); + + constexpr bool force_consistency_checks = false; + VersionStorageInfo new_vstorage(&icmp_, ucmp_, options_.num_levels, + kCompactionStyleLevel, &vstorage_, + force_consistency_checks); + + ASSERT_OK(builder.SaveTo(&new_vstorage)); + ASSERT_FALSE(new_vstorage.GetFileLocation(file_number).IsValid()); + + UnrefFilesInVersion(&new_vstorage); +} + TEST_F(VersionBuilderTest, ApplyBlobFileAddition) { EnvOptions env_options; constexpr TableCache* table_cache = nullptr;