From df059cc38dce533ced06dbdccb1d30c030d7d85d Mon Sep 17 00:00:00 2001 From: JaySon Date: Fri, 3 Jun 2022 16:12:28 +0800 Subject: [PATCH] PageStorage: Fix pages are not deleted under some cases (#5069) (#5070) close pingcap/tiflash#5054 --- dbms/src/Storages/DeltaMerge/StoragePool.cpp | 18 +-- dbms/src/Storages/Page/Page.h | 3 +- dbms/src/Storages/Page/V2/PageEntries.h | 26 ++-- .../V3/tests/gtest_page_storage_mix_mode.cpp | 126 ++++++++++++++++++ 4 files changed, 149 insertions(+), 24 deletions(-) diff --git a/dbms/src/Storages/DeltaMerge/StoragePool.cpp b/dbms/src/Storages/DeltaMerge/StoragePool.cpp index fa765cd9b1d..752898f9c75 100644 --- a/dbms/src/Storages/DeltaMerge/StoragePool.cpp +++ b/dbms/src/Storages/DeltaMerge/StoragePool.cpp @@ -515,21 +515,9 @@ void StoragePool::dataRegisterExternalPagesCallbacks(const ExternalPageCallbacks } case PageStorageRunMode::MIX_MODE: { - // When PageStorage run as Mix Mode. - // We need both get alive pages from V2 and V3 which will feedback for the DM. - // But V2 and V3 won't GC in the same time. So V3 need proxy V2 external pages callback. - // When V3 GC happend, scan the external pages from V3, in remover will scanner all of external pages from V2. - ExternalPageCallbacks mix_mode_callbacks; - - mix_mode_callbacks.scanner = callbacks.scanner; - mix_mode_callbacks.remover = [this, callbacks](const ExternalPageCallbacks::PathAndIdsVec & path_and_ids_vec, const std::set & valid_ids) { - // ns_id won't used on V2 - auto v2_valid_page_ids = data_storage_v2->getAliveExternalPageIds(ns_id); - v2_valid_page_ids.insert(valid_ids.begin(), valid_ids.end()); - callbacks.remover(path_and_ids_vec, v2_valid_page_ids); - }; - mix_mode_callbacks.ns_id = ns_id; - data_storage_v3->registerExternalPagesCallbacks(mix_mode_callbacks); + // We have transformed all pages from V2 to V3 in `restore`, so + // only need to register callbacks for V3. + data_storage_v3->registerExternalPagesCallbacks(callbacks); break; } default: diff --git a/dbms/src/Storages/Page/Page.h b/dbms/src/Storages/Page/Page.h index b54b25033dd..5328490e5ad 100644 --- a/dbms/src/Storages/Page/Page.h +++ b/dbms/src/Storages/Page/Page.h @@ -128,12 +128,13 @@ struct PageEntry String toDebugString() const { - return fmt::format("PageEntry{{file: {}, offset: 0x{:X}, size: {}, checksum: 0x{:X}, tag: {}, field_offsets_size: {}}}", + return fmt::format("PageEntry{{file: {}, offset: 0x{:X}, size: {}, checksum: 0x{:X}, tag: {}, ref: {}, field_offsets_size: {}}}", file_id, offset, size, checksum, tag, + ref, field_offsets.size()); } diff --git a/dbms/src/Storages/Page/V2/PageEntries.h b/dbms/src/Storages/Page/V2/PageEntries.h index c99e0dade6b..0a0504b0cb5 100644 --- a/dbms/src/Storages/Page/V2/PageEntries.h +++ b/dbms/src/Storages/Page/V2/PageEntries.h @@ -252,7 +252,7 @@ class PageEntriesMixin protected: template - void decreasePageRef(PageId page_id); + void decreasePageRef(PageId page_id, bool keep_tombstone); void copyEntries(const PageEntriesMixin & rhs) { @@ -370,8 +370,10 @@ void PageEntriesMixin::del(PageId page_id) const size_t num_erase = page_ref.erase(page_id); if (num_erase > 0) { - // decrease origin page's ref counting - decreasePageRef(normal_page_id); + // decrease origin page's ref counting, this method can + // only called by base, so we should remove the entry if + // the ref count down to zero + decreasePageRef(normal_page_id, /*keep_tombstone=*/false); } } @@ -392,7 +394,9 @@ void PageEntriesMixin::ref(const PageId ref_id, const PageId page_id) // if RefPage{ref-id} -> Page{normal_page_id} already exists, just ignore if (ori_ref->second == normal_page_id) return; - decreasePageRef(ori_ref->second); + // this method can only called by base, so we should remove the entry if + // the ref count down to zero + decreasePageRef(ori_ref->second, /*keep_tombstone=*/false); } // build ref page_ref[ref_id] = normal_page_id; @@ -408,7 +412,7 @@ void PageEntriesMixin::ref(const PageId ref_id, const PageId page_id) template template -void PageEntriesMixin::decreasePageRef(const PageId page_id) +void PageEntriesMixin::decreasePageRef(const PageId page_id, bool keep_tombstone) { auto iter = normal_pages.find(page_id); if constexpr (must_exist) @@ -421,8 +425,11 @@ void PageEntriesMixin::decreasePageRef(const PageId page_id) if (iter != normal_pages.end()) { auto & entry = iter->second; - entry.ref -= 1; - if (entry.ref == 0) + if (entry.ref > 0) + { + entry.ref -= 1; + } + if (!keep_tombstone && entry.ref == 0) { normal_pages.erase(iter); } @@ -620,7 +627,10 @@ class PageEntriesForDelta : public PageEntriesMixin { ref_deletions.insert(page_id); } - decreasePageRef(page_id); + // If this is the base version, we should remove the entry if + // the ref count down to zero. Otherwise it is the delta version + // we should keep a tombstone. + decreasePageRef(page_id, /*keep_tombstone=*/!this->isBase()); } for (auto it : rhs.page_ref) { diff --git a/dbms/src/Storages/Page/V3/tests/gtest_page_storage_mix_mode.cpp b/dbms/src/Storages/Page/V3/tests/gtest_page_storage_mix_mode.cpp index 6090f7bbe42..7ae771820a0 100644 --- a/dbms/src/Storages/Page/V3/tests/gtest_page_storage_mix_mode.cpp +++ b/dbms/src/Storages/Page/V3/tests/gtest_page_storage_mix_mode.cpp @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include #include #include #include @@ -19,6 +20,8 @@ #include #include #include +#include +#include #include namespace DB @@ -568,6 +571,129 @@ try } CATCH + +TEST_F(PageStorageMixedTest, RefV2External2) +try +{ + auto logger = DB::Logger::get("PageStorageMixedTest"); + { + WriteBatch batch; + batch.putExternal(100, 0); + batch.putRefPage(101, 100); + batch.delPage(100); + batch.putExternal(102, 0); + page_writer_v2->write(std::move(batch), nullptr); + } + + ASSERT_EQ(reloadMixedStoragePool(), PageStorageRunMode::MIX_MODE); + { + WriteBatch batch; + batch.putExternal(100, 0); + batch.putRefPage(101, 100); + batch.delPage(100); + batch.putExternal(102, 0); + page_writer_mix->writeIntoV3(std::move(batch), nullptr); + } + { + auto snap = storage_pool_mix->log_storage_v2->getSnapshot("zzz"); // must hold + // after transform to v3, delete these from v2 + WriteBatch batch; + batch.delPage(100); + batch.delPage(101); + batch.delPage(102); + page_writer_mix->writeIntoV2(std::move(batch), nullptr); + } + + { + LOG_FMT_INFO(logger, "first check alive id in v2"); + auto alive_dt_ids_in_v2 = storage_pool_mix->log_storage_v2->getAliveExternalPageIds(TEST_NAMESPACE_ID); + EXPECT_EQ(alive_dt_ids_in_v2.size(), 0); + + storage_pool_mix->log_storage_v3->gc(false, nullptr, nullptr); + auto alive_dt_ids_in_v3 = storage_pool_mix->log_storage_v3->getAliveExternalPageIds(TEST_NAMESPACE_ID); + ASSERT_EQ(alive_dt_ids_in_v3.size(), 2); + auto iter = alive_dt_ids_in_v3.begin(); + EXPECT_EQ(*iter, 100); + iter++; + EXPECT_EQ(*iter, 102); + } + + { + LOG_FMT_INFO(logger, "remove 100, create 105"); + StorageSnapshot snap(*storage_pool_mix, nullptr, "xxx", true); // must hold and write + // write delete again + WriteBatch batch; + batch.delPage(100); + batch.putExternal(105, 0); + page_writer_mix->write(std::move(batch), nullptr); + LOG_FMT_INFO(logger, "done"); + } + { + LOG_FMT_INFO(logger, "remove 101, create 106"); + StorageSnapshot snap(*storage_pool_mix, nullptr, "xxx", true); // must hold and write + // write delete again + WriteBatch batch; + batch.delPage(101); + batch.putExternal(106, 0); + page_writer_mix->write(std::move(batch), nullptr); + LOG_FMT_INFO(logger, "done"); + } + { + LOG_FMT_INFO(logger, "remove 102, create 107"); + StorageSnapshot snap(*storage_pool_mix, nullptr, "xxx", true); // must hold and write + // write delete again + WriteBatch batch; + batch.delPage(102); + batch.putExternal(107, 0); + page_writer_mix->write(std::move(batch), nullptr); + LOG_FMT_INFO(logger, "done"); + } + + { + LOG_FMT_INFO(logger, "second check alive id in v2"); + auto alive_dt_ids_in_v2 = storage_pool_mix->log_storage_v2->getAliveExternalPageIds(TEST_NAMESPACE_ID); + EXPECT_EQ(alive_dt_ids_in_v2.size(), 0) << fmt::format("{}", alive_dt_ids_in_v2); + + storage_pool_mix->log_storage_v3->gc(false, nullptr, nullptr); + auto alive_dt_ids_in_v3 = storage_pool_mix->log_storage_v3->getAliveExternalPageIds(TEST_NAMESPACE_ID); + ASSERT_EQ(alive_dt_ids_in_v3.size(), 3) << fmt::format("{}", alive_dt_ids_in_v3); + auto iter = alive_dt_ids_in_v3.begin(); + EXPECT_EQ(*iter, 105); + iter++; + EXPECT_EQ(*iter, 106); + iter++; + EXPECT_EQ(*iter, 107); + } + { + LOG_FMT_INFO(logger, "third check alive id in v2"); + auto alive_dt_ids_in_v2 = storage_pool_mix->log_storage_v2->getAliveExternalPageIds(TEST_NAMESPACE_ID); + EXPECT_EQ(alive_dt_ids_in_v2.size(), 0) << fmt::format("{}", alive_dt_ids_in_v2); + + storage_pool_mix->log_storage_v3->gc(false, nullptr, nullptr); + auto alive_dt_ids_in_v3 = storage_pool_mix->log_storage_v3->getAliveExternalPageIds(TEST_NAMESPACE_ID); + ASSERT_EQ(alive_dt_ids_in_v3.size(), 3) << fmt::format("{}", alive_dt_ids_in_v3); + auto iter = alive_dt_ids_in_v3.begin(); + EXPECT_EQ(*iter, 105); + iter++; + EXPECT_EQ(*iter, 106); + iter++; + EXPECT_EQ(*iter, 107); + } + + { + // cleanup v3 + WriteBatch batch; + batch.delPage(100); + batch.delPage(101); + batch.delPage(102); + batch.delPage(105); + batch.delPage(106); + batch.delPage(107); + page_writer_mix->write(std::move(batch), nullptr); + } +} +CATCH + TEST_F(PageStorageMixedTest, ReadWithSnapshotAfterMergeDelta) try {