diff --git a/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp b/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp index 9d20e0a64ab..149807eab38 100644 --- a/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp +++ b/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp @@ -42,6 +42,22 @@ PageDirectoryPtr PageDirectoryFactory::create(String storage_name, FileProviderP dir->gcInMemEntries(); LOG_FMT_INFO(DB::Logger::get("PageDirectoryFactory"), "PageDirectory restored [max_page_id={}] [max_applied_ver={}]", dir->getMaxId(), dir->sequence); + // MVCC gc must do twice when restore + // Because the first time call MVCC GC may not clean the deleted entries if being reference. + // But it will clean up the deleted reference. + // ex. + // P 1 + // R 2 -> 1 + // D 1 + // D 2 + // First time call gcInMemEntries(): + // - P 1 entry 0 being_ref_count is not zero, won't be cleaned. + // - R 2 entry 0 being_ref_count is zero, deref the P 1, make it being_ref_count is 0. Also R2 will be clean up. + // + // Second time call gcInMemEntries(): + // - P 1 being_ref_count is 0, it will be clean. + dir->gcInMemEntries(); + if (blob_stats) { // After all entries restored to `mvcc_table_directory`, only apply diff --git a/dbms/src/Storages/Page/V3/tests/gtest_page_directory.cpp b/dbms/src/Storages/Page/V3/tests/gtest_page_directory.cpp index 151b3b50657..e98c68fd9c3 100644 --- a/dbms/src/Storages/Page/V3/tests/gtest_page_directory.cpp +++ b/dbms/src/Storages/Page/V3/tests/gtest_page_directory.cpp @@ -2214,6 +2214,35 @@ try } CATCH +TEST_F(PageDirectoryGCTest, testGCTwice) +try +{ + PageEntryV3 entry_50_1{.file_id = 1, .size = 7890, .tag = 0, .offset = 0x123, .checksum = 0x4567}; + PageEntryV3 entry_50_2{.file_id = 2, .size = 7890, .tag = 0, .offset = 0x123, .checksum = 0x4567}; + + auto restore_from_edit = [](const PageEntriesEdit & edit) { + auto ctx = ::DB::tests::TiFlashTestEnv::getContext(); + auto provider = ctx.getFileProvider(); + auto path = getTemporaryPath(); + PSDiskDelegatorPtr delegator = std::make_shared(path); + PageDirectoryFactory factory; + auto d = factory.createFromEdit(getCurrentTestName(), provider, delegator, edit); + return d; + }; + + { + PageEntriesEdit edit; + edit.put(50, entry_50_1); + edit.put(50, entry_50_2); + edit.ref(51, 50); + edit.del(50); + edit.del(51); + auto page_ids = restore_from_edit(edit)->getAllPageIds(); + ASSERT_EQ(page_ids.size(), 0); + } +} +CATCH + #undef INSERT_ENTRY_TO #undef INSERT_ENTRY #undef INSERT_ENTRY_ACQ_SNAP diff --git a/dbms/src/Storages/Page/V3/tests/gtest_page_storage.cpp b/dbms/src/Storages/Page/V3/tests/gtest_page_storage.cpp index 498fd4124e5..049d839afb9 100644 --- a/dbms/src/Storages/Page/V3/tests/gtest_page_storage.cpp +++ b/dbms/src/Storages/Page/V3/tests/gtest_page_storage.cpp @@ -1329,6 +1329,7 @@ try } CATCH + TEST_F(PageStorageTest, GetMaxId) try { @@ -1380,5 +1381,33 @@ try } CATCH +TEST_F(PageStorageTest, testGCTwice) +try +{ + // Make it in log_1_0 + { + WriteBatch batch; + batch.putExternal(1, 0); + page_storage->write(std::move(batch)); + } + + page_storage = reopenWithConfig(config); + + // Make it in log_2_0 + { + WriteBatch batch; + batch.putExternal(1, 0); + batch.putRefPage(2, 1); + batch.delPage(1); + batch.delPage(2); + page_storage->write(std::move(batch)); + } + page_storage = reopenWithConfig(config); + + auto alive_ids = page_storage->getAliveExternalPageIds(TEST_NAMESPACE_ID); + ASSERT_EQ(alive_ids.size(), 0); +} +CATCH + } // namespace PS::V3::tests } // namespace DB