From 3005ef7e8cb4107e37b2cebd85e26ec87583524c Mon Sep 17 00:00:00 2001 From: lidezhu <47731263+lidezhu@users.noreply.github.com> Date: Fri, 15 Jul 2022 21:17:06 +0800 Subject: [PATCH] fix pagestorage v2 restore (#5384) close pingcap/tiflash#5385 --- dbms/src/Storages/Page/V2/PageStorage.cpp | 11 ++++--- dbms/src/Storages/Page/V2/PageStorage.h | 1 + .../Page/V2/tests/gtest_page_storage.cpp | 32 +++++++++++++++++++ 3 files changed, 39 insertions(+), 5 deletions(-) diff --git a/dbms/src/Storages/Page/V2/PageStorage.cpp b/dbms/src/Storages/Page/V2/PageStorage.cpp index 6cf454fcf27..fba56017d6f 100644 --- a/dbms/src/Storages/Page/V2/PageStorage.cpp +++ b/dbms/src/Storages/Page/V2/PageStorage.cpp @@ -312,6 +312,7 @@ void PageStorage::restore() } // Fill write_files + PageFileIdAndLevel max_page_file_id_lvl{0, 0}; { const size_t num_delta_paths = delegator->numPaths(); std::vector next_write_fill_idx(num_delta_paths); @@ -319,6 +320,7 @@ void PageStorage::restore() // Only insert location of PageFile when it storing delta data for (const auto & page_file : page_files) { + max_page_file_id_lvl = std::max(max_page_file_id_lvl, page_file.fileIdLevel()); // Checkpoint file is always stored on `delegator`'s default path, so no need to insert it's location here size_t idx_in_delta_paths = delegator->addPageFileUsedSize( page_file.fileIdLevel(), @@ -342,7 +344,7 @@ void PageStorage::restore() std::vector store_paths = delegator->listPaths(); for (size_t i = 0; i < write_files.size(); ++i) { - auto writer = checkAndRenewWriter(write_files[i], /*parent_path_hint=*/store_paths[i % store_paths.size()]); + auto writer = checkAndRenewWriter(write_files[i], max_page_file_id_lvl, /*parent_path_hint=*/store_paths[i % store_paths.size()]); idle_writers.emplace_back(std::move(writer)); } #endif @@ -400,6 +402,7 @@ DB::PageEntry PageStorage::getEntry(NamespaceId /*ns_id*/, PageId page_id, Snaps // The of the new `page_file` is of all `write_files` PageStorage::WriterPtr PageStorage::checkAndRenewWriter( // WritingPageFile & writing_file, + PageFileIdAndLevel max_page_file_id_lvl_hint, const String & parent_path_hint, PageStorage::WriterPtr && old_writer, const String & logging_msg) @@ -440,8 +443,7 @@ PageStorage::WriterPtr PageStorage::checkAndRenewWriter( // // Check whether caller has defined a hint path pf_parent_path = parent_path_hint; } - - PageFileIdAndLevel max_writing_id_lvl{0, 0}; + PageFileIdAndLevel max_writing_id_lvl{max_page_file_id_lvl_hint}; for (const auto & wf : write_files) max_writing_id_lvl = std::max(max_writing_id_lvl, wf.file.fileIdLevel()); delegator->addPageFileUsedSize( // @@ -454,7 +456,6 @@ PageStorage::WriterPtr PageStorage::checkAndRenewWriter( // writing_file.file = PageFile::newPageFile(max_writing_id_lvl.first + 1, 0, pf_parent_path, file_provider, PageFile::Type::Formal, page_file_log); writing_file.persisted.meta_offset = 0; - write_file_writer = writing_file.file.createWriter(config.sync_on_write, true); } return write_file_writer; @@ -557,7 +558,7 @@ void PageStorage::write(DB::WriteBatch && wb, const WriteLimiterPtr & write_limi // Check whether we need to roll to new PageFile and its writer const auto logging_msg = " PageFile_" + DB::toString(writing_file.file.getFileId()) + "_0 is full,"; - file_to_write = checkAndRenewWriter(writing_file, "", std::move(file_to_write), logging_msg); + file_to_write = checkAndRenewWriter(writing_file, {0, 0}, "", std::move(file_to_write), logging_msg); idle_writers.emplace_back(std::move(file_to_write)); diff --git a/dbms/src/Storages/Page/V2/PageStorage.h b/dbms/src/Storages/Page/V2/PageStorage.h index 9f0effeeff8..cd1e4aceb61 100644 --- a/dbms/src/Storages/Page/V2/PageStorage.h +++ b/dbms/src/Storages/Page/V2/PageStorage.h @@ -268,6 +268,7 @@ class PageStorage : public DB::PageStorage private: WriterPtr checkAndRenewWriter( WritingPageFile & writing_file, + PageFileIdAndLevel max_page_file_id_lvl_hint, const String & parent_path_hint, WriterPtr && old_writer = nullptr, const String & logging_msg = ""); diff --git a/dbms/src/Storages/Page/V2/tests/gtest_page_storage.cpp b/dbms/src/Storages/Page/V2/tests/gtest_page_storage.cpp index fc429dde0ac..28421d6781d 100644 --- a/dbms/src/Storages/Page/V2/tests/gtest_page_storage.cpp +++ b/dbms/src/Storages/Page/V2/tests/gtest_page_storage.cpp @@ -443,6 +443,38 @@ try } CATCH +TEST_F(PageStorage_test, RenewWriter) +try +{ + constexpr size_t buf_sz = 100; + char c_buff[buf_sz]; + + { + WriteBatch wb; + memset(c_buff, 0xf, buf_sz); + auto buf = std::make_shared(c_buff, sizeof(c_buff)); + wb.putPage(1, 0, buf, buf_sz); + storage->write(std::move(wb)); + } + + { + PageStorage::ListPageFilesOption opt; + auto page_files = storage->listAllPageFiles(file_provider, storage->delegator, storage->log, opt); + ASSERT_EQ(page_files.size(), 1UL); + } + + PageStorage::Config config; + config.file_roll_size = 10; // make it easy to renew a new page file for write + storage = reopenWithConfig(config); + + { + PageStorage::ListPageFilesOption opt; + auto page_files = storage->listAllPageFiles(file_provider, storage->delegator, storage->log, opt); + ASSERT_EQ(page_files.size(), 2UL); + } +} +CATCH + /// Check if we can correctly do read / write after restore from disk. TEST_F(PageStorage_test, WriteReadRestore) try