From de86b1c94b8e625f9b7a5e635e0f30c5a556f785 Mon Sep 17 00:00:00 2001 From: jiaqizho Date: Fri, 20 May 2022 15:58:41 +0800 Subject: [PATCH 1/8] Add a global max_id to fix reuse page_id problem --- dbms/src/Storages/DeltaMerge/StoragePool.cpp | 24 ++-- dbms/src/Storages/Page/PageStorage.h | 2 +- dbms/src/Storages/Page/V2/PageStorage.cpp | 2 +- dbms/src/Storages/Page/V2/PageStorage.h | 2 +- dbms/src/Storages/Page/V3/PageDirectory.cpp | 52 ++------ dbms/src/Storages/Page/V3/PageDirectory.h | 3 +- .../Storages/Page/V3/PageDirectoryFactory.cpp | 5 + dbms/src/Storages/Page/V3/PageStorageImpl.cpp | 4 +- dbms/src/Storages/Page/V3/PageStorageImpl.h | 2 +- .../Page/V3/tests/gtest_page_directory.cpp | 111 ------------------ .../Page/V3/tests/gtest_page_storage.cpp | 51 ++++++++ 11 files changed, 86 insertions(+), 172 deletions(-) diff --git a/dbms/src/Storages/DeltaMerge/StoragePool.cpp b/dbms/src/Storages/DeltaMerge/StoragePool.cpp index fbb631064a7..a040c5b6c6a 100644 --- a/dbms/src/Storages/DeltaMerge/StoragePool.cpp +++ b/dbms/src/Storages/DeltaMerge/StoragePool.cpp @@ -378,18 +378,18 @@ PageStorageRunMode StoragePool::restore() data_storage_v2->restore(); meta_storage_v2->restore(); - max_log_page_id = log_storage_v2->getMaxId(ns_id); - max_data_page_id = data_storage_v2->getMaxId(ns_id); - max_meta_page_id = meta_storage_v2->getMaxId(ns_id); + max_log_page_id = log_storage_v2->getMaxId(); + max_data_page_id = data_storage_v2->getMaxId(); + max_meta_page_id = meta_storage_v2->getMaxId(); storage_pool_metrics = CurrentMetrics::Increment{CurrentMetrics::StoragePoolV2Only}; break; } case PageStorageRunMode::ONLY_V3: { - max_log_page_id = log_storage_v3->getMaxId(ns_id); - max_data_page_id = data_storage_v3->getMaxId(ns_id); - max_meta_page_id = meta_storage_v3->getMaxId(ns_id); + max_log_page_id = log_storage_v3->getMaxId(); + max_data_page_id = data_storage_v3->getMaxId(); + max_meta_page_id = meta_storage_v3->getMaxId(); storage_pool_metrics = CurrentMetrics::Increment{CurrentMetrics::StoragePoolV3Only}; break; @@ -456,18 +456,18 @@ PageStorageRunMode StoragePool::restore() data_storage_writer = std::make_shared(PageStorageRunMode::ONLY_V3, /*storage_v2_*/ nullptr, data_storage_v3); meta_storage_writer = std::make_shared(PageStorageRunMode::ONLY_V3, /*storage_v2_*/ nullptr, meta_storage_v3); - max_log_page_id = log_storage_v3->getMaxId(ns_id); - max_data_page_id = data_storage_v3->getMaxId(ns_id); - max_meta_page_id = meta_storage_v3->getMaxId(ns_id); + max_log_page_id = log_storage_v3->getMaxId(); + max_data_page_id = data_storage_v3->getMaxId(); + max_meta_page_id = meta_storage_v3->getMaxId(); run_mode = PageStorageRunMode::ONLY_V3; storage_pool_metrics = CurrentMetrics::Increment{CurrentMetrics::StoragePoolV3Only}; } else // Still running Mix Mode { - max_log_page_id = std::max(log_storage_v2->getMaxId(ns_id), log_storage_v3->getMaxId(ns_id)); - max_data_page_id = std::max(data_storage_v2->getMaxId(ns_id), data_storage_v3->getMaxId(ns_id)); - max_meta_page_id = std::max(meta_storage_v2->getMaxId(ns_id), meta_storage_v3->getMaxId(ns_id)); + max_log_page_id = std::max(log_storage_v2->getMaxId(), log_storage_v3->getMaxId()); + max_data_page_id = std::max(data_storage_v2->getMaxId(), data_storage_v3->getMaxId()); + max_meta_page_id = std::max(meta_storage_v2->getMaxId(), meta_storage_v3->getMaxId()); storage_pool_metrics = CurrentMetrics::Increment{CurrentMetrics::StoragePoolMixMode}; } break; diff --git a/dbms/src/Storages/Page/PageStorage.h b/dbms/src/Storages/Page/PageStorage.h index 481888bdf33..70e598a0e3a 100644 --- a/dbms/src/Storages/Page/PageStorage.h +++ b/dbms/src/Storages/Page/PageStorage.h @@ -233,7 +233,7 @@ class PageStorage : private boost::noncopyable virtual void drop() = 0; - virtual PageId getMaxId(NamespaceId ns_id) = 0; + virtual PageId getMaxId() = 0; virtual SnapshotPtr getSnapshot(const String & tracing_id) = 0; diff --git a/dbms/src/Storages/Page/V2/PageStorage.cpp b/dbms/src/Storages/Page/V2/PageStorage.cpp index 3ab62d55242..304460647de 100644 --- a/dbms/src/Storages/Page/V2/PageStorage.cpp +++ b/dbms/src/Storages/Page/V2/PageStorage.cpp @@ -355,7 +355,7 @@ void PageStorage::restore() LOG_FMT_INFO(log, "{} restore {} pages, write batch sequence: {}, {}", storage_name, num_pages, write_batch_seq, statistics.toString()); } -PageId PageStorage::getMaxId(NamespaceId /*ns_id*/) +PageId PageStorage::getMaxId() { std::lock_guard write_lock(write_mutex); return versioned_page_entries.getSnapshot("")->version()->maxId(); diff --git a/dbms/src/Storages/Page/V2/PageStorage.h b/dbms/src/Storages/Page/V2/PageStorage.h index cb55a769f37..b9e16fd1775 100644 --- a/dbms/src/Storages/Page/V2/PageStorage.h +++ b/dbms/src/Storages/Page/V2/PageStorage.h @@ -95,7 +95,7 @@ class PageStorage : public DB::PageStorage void drop() override; - PageId getMaxId(NamespaceId ns_id) override; + PageId getMaxId() override; PageId getNormalPageIdImpl(NamespaceId ns_id, PageId page_id, SnapshotPtr snapshot, bool throw_on_not_exist) override; diff --git a/dbms/src/Storages/Page/V3/PageDirectory.cpp b/dbms/src/Storages/Page/V3/PageDirectory.cpp index 06b26156529..630683ace7d 100644 --- a/dbms/src/Storages/Page/V3/PageDirectory.cpp +++ b/dbms/src/Storages/Page/V3/PageDirectory.cpp @@ -685,7 +685,8 @@ void VersionedPageEntries::collapseTo(const UInt64 seq, const PageIdV3Internal p *************************/ PageDirectory::PageDirectory(String storage_name, WALStorePtr && wal_, UInt64 max_persisted_log_files_) - : sequence(0) + : max_page_id(0) + , sequence(0) , wal(std::move(wal_)) , max_persisted_log_files(max_persisted_log_files_) , log(Logger::get("PageDirectory", std::move(storage_name))) @@ -923,49 +924,10 @@ PageIdV3Internal PageDirectory::getNormalPageId(PageIdV3Internal page_id, const } } -PageId PageDirectory::getMaxId(NamespaceId ns_id) const +PageId PageDirectory::getMaxId() const { std::shared_lock read_lock(table_rw_mutex); - PageIdV3Internal upper_bound = buildV3Id(ns_id, UINT64_MAX); - - auto iter = mvcc_table_directory.upper_bound(upper_bound); - if (iter == mvcc_table_directory.begin()) - { - // The smallest page id is greater than the target page id or mvcc_table_directory is empty, - // and it means no page id is less than or equal to the target page id, return 0. - return 0; - } - else - { - // iter is not at the beginning and mvcc_table_directory is not empty, - // so iter-- must be a valid iterator, and it's the largest page id which is smaller than the target page id. - iter--; - - do - { - // Can't find any entries in current ns_id - if (iter->first.high != ns_id) - { - break; - } - - // Check and return whether this id is visible, otherwise continue to check the previous one. - if (iter->second->isVisible(UINT64_MAX - 1)) - { - return iter->first.low; - } - - // Current entry/ref/external is deleted and there are no entries before it. - if (iter == mvcc_table_directory.begin()) - { - break; - } - - iter--; - } while (true); - - return 0; - } + return max_page_id; } std::set PageDirectory::getAllPageIds() @@ -1069,6 +1031,12 @@ void PageDirectory::apply(PageEntriesEdit && edit, const WriteLimiterPtr & write // stage 2, create entry version list for page_id. for (const auto & r : edit.getRecords()) { + // Protected in write_lock + if (r.page_id.low > max_page_id) + { + max_page_id = r.page_id.low; + } + auto [iter, created] = mvcc_table_directory.insert(std::make_pair(r.page_id, nullptr)); if (created) { diff --git a/dbms/src/Storages/Page/V3/PageDirectory.h b/dbms/src/Storages/Page/V3/PageDirectory.h index 14833245c7a..2dfeb437a8b 100644 --- a/dbms/src/Storages/Page/V3/PageDirectory.h +++ b/dbms/src/Storages/Page/V3/PageDirectory.h @@ -347,7 +347,7 @@ class PageDirectory } #endif - PageId getMaxId(NamespaceId ns_id) const; + PageId getMaxId() const; std::set getAllPageIds(); @@ -397,6 +397,7 @@ class PageDirectory } private: + UInt64 max_page_id; std::atomic sequence; mutable std::shared_mutex table_rw_mutex; MVCCMapType mvcc_table_directory; diff --git a/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp b/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp index 40b12b64f06..40c65772fb0 100644 --- a/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp +++ b/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp @@ -135,6 +135,11 @@ bool PageDirectoryFactory::applyRecord( iter->second = std::make_shared(); } + if (r.page_id.low > dir->max_page_id) + { + dir->max_page_id = r.page_id.low; + } + const auto & version_list = iter->second; const auto & restored_version = r.version; try diff --git a/dbms/src/Storages/Page/V3/PageStorageImpl.cpp b/dbms/src/Storages/Page/V3/PageStorageImpl.cpp index 8aa9f92675c..58fe4b4dd4c 100644 --- a/dbms/src/Storages/Page/V3/PageStorageImpl.cpp +++ b/dbms/src/Storages/Page/V3/PageStorageImpl.cpp @@ -55,9 +55,9 @@ void PageStorageImpl::restore() .create(storage_name, file_provider, delegator, parseWALConfig(config)); } -PageId PageStorageImpl::getMaxId(NamespaceId ns_id) +PageId PageStorageImpl::getMaxId() { - return page_directory->getMaxId(ns_id); + return page_directory->getMaxId(); } void PageStorageImpl::drop() diff --git a/dbms/src/Storages/Page/V3/PageStorageImpl.h b/dbms/src/Storages/Page/V3/PageStorageImpl.h index f3b696d0351..082adb8df34 100644 --- a/dbms/src/Storages/Page/V3/PageStorageImpl.h +++ b/dbms/src/Storages/Page/V3/PageStorageImpl.h @@ -64,7 +64,7 @@ class PageStorageImpl : public DB::PageStorage void drop() override; - PageId getMaxId(NamespaceId ns_id) override; + PageId getMaxId() override; PageId getNormalPageIdImpl(NamespaceId ns_id, PageId page_id, SnapshotPtr snapshot, bool throw_on_not_exist) override; 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 4511cc8ddd7..dfa33824473 100644 --- a/dbms/src/Storages/Page/V3/tests/gtest_page_directory.cpp +++ b/dbms/src/Storages/Page/V3/tests/gtest_page_directory.cpp @@ -2399,117 +2399,6 @@ try } CATCH -TEST_F(PageDirectoryTest, GetMaxId) -try -{ - NamespaceId small = 20; - NamespaceId medium = 50; - NamespaceId large = 100; - ASSERT_EQ(dir->getMaxId(small), 0); - ASSERT_EQ(dir->getMaxId(medium), 0); - ASSERT_EQ(dir->getMaxId(large), 0); - - PageEntryV3 entry1{.file_id = 1, .size = 1024, .tag = 0, .offset = 0x123, .checksum = 0x4567}; - PageEntryV3 entry2{.file_id = 2, .size = 1024, .tag = 0, .offset = 0x123, .checksum = 0x4567}; - { - PageEntriesEdit edit; - edit.put(buildV3Id(small, 1), entry1); - edit.put(buildV3Id(large, 2), entry2); - dir->apply(std::move(edit)); - ASSERT_EQ(dir->getMaxId(small), 1); - ASSERT_EQ(dir->getMaxId(medium), 0); - ASSERT_EQ(dir->getMaxId(large), 2); - } - - PageEntryV3 entry3{.file_id = 3, .size = 1024, .tag = 0, .offset = 0x123, .checksum = 0x4567}; - PageEntryV3 entry4{.file_id = 4, .size = 1024, .tag = 0, .offset = 0x123, .checksum = 0x4567}; - { - PageEntriesEdit edit; - edit.put(buildV3Id(medium, 300), entry1); - edit.put(buildV3Id(medium, 320), entry2); - dir->apply(std::move(edit)); - ASSERT_EQ(dir->getMaxId(small), 1); - ASSERT_EQ(dir->getMaxId(medium), 320); - ASSERT_EQ(dir->getMaxId(large), 2); - } - - { - PageEntriesEdit edit; - edit.del(buildV3Id(medium, 320)); - dir->apply(std::move(edit)); - ASSERT_EQ(dir->getMaxId(medium), 300); - } - - { - PageEntriesEdit edit; - edit.del(buildV3Id(medium, 300)); - dir->apply(std::move(edit)); - ASSERT_EQ(dir->getMaxId(medium), 0); - } -} -CATCH - -TEST_F(PageDirectoryTest, GetMaxIdAfterDelete) -try -{ - /// test for deleting put - PageEntryV3 entry1{.file_id = 1, .size = 1024, .tag = 0, .offset = 0x123, .checksum = 0x4567}; - PageEntryV3 entry2{.file_id = 2, .size = 1024, .tag = 0, .offset = 0x123, .checksum = 0x4567}; - { - PageEntriesEdit edit; - edit.put(1, entry1); - edit.put(2, entry2); - dir->apply(std::move(edit)); - } - - ASSERT_EQ(dir->getMaxId(TEST_NAMESPACE_ID), 2); - - { - PageEntriesEdit edit; - edit.del(2); - dir->apply(std::move(edit)); - } - ASSERT_EQ(dir->getMaxId(TEST_NAMESPACE_ID), 1); - - { - PageEntriesEdit edit; - edit.del(1); - dir->apply(std::move(edit)); - } - ASSERT_EQ(dir->getMaxId(TEST_NAMESPACE_ID), 0); - - dir->gcInMemEntries(); - ASSERT_EQ(dir->getMaxId(TEST_NAMESPACE_ID), 0); - - /// test for deleting put_ext/ref - - { - PageEntriesEdit edit; - edit.putExternal(1); - edit.ref(2, 1); - dir->apply(std::move(edit)); - } - - { - PageEntriesEdit edit; - edit.del(1); - dir->apply(std::move(edit)); - } - ASSERT_EQ(dir->getMaxId(TEST_NAMESPACE_ID), 2); - dir->gcInMemEntries(); - ASSERT_EQ(dir->getMaxId(TEST_NAMESPACE_ID), 2); - - { - PageEntriesEdit edit; - edit.del(2); - dir->apply(std::move(edit)); - } - ASSERT_EQ(dir->getMaxId(TEST_NAMESPACE_ID), 0); - dir->gcInMemEntries(); - ASSERT_EQ(dir->getMaxId(TEST_NAMESPACE_ID), 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 91dfcaac6a8..498fd4124e5 100644 --- a/dbms/src/Storages/Page/V3/tests/gtest_page_storage.cpp +++ b/dbms/src/Storages/Page/V3/tests/gtest_page_storage.cpp @@ -1329,5 +1329,56 @@ try } CATCH +TEST_F(PageStorageTest, GetMaxId) +try +{ + NamespaceId small = 20; + NamespaceId medium = 50; + NamespaceId large = 100; + + { + WriteBatch batch{small}; + batch.putExternal(1, 0); + batch.putExternal(1999, 0); + batch.putExternal(2000, 0); + page_storage->write(std::move(batch)); + ASSERT_EQ(page_storage->getMaxId(), 2000); + } + + { + page_storage = reopenWithConfig(config); + ASSERT_EQ(page_storage->getMaxId(), 2000); + } + + { + WriteBatch batch{medium}; + batch.putExternal(1, 0); + batch.putExternal(100, 0); + batch.putExternal(200, 0); + page_storage->write(std::move(batch)); + ASSERT_EQ(page_storage->getMaxId(), 2000); + } + + { + page_storage = reopenWithConfig(config); + ASSERT_EQ(page_storage->getMaxId(), 2000); + } + + { + WriteBatch batch{large}; + batch.putExternal(1, 0); + batch.putExternal(20000, 0); + batch.putExternal(20001, 0); + page_storage->write(std::move(batch)); + ASSERT_EQ(page_storage->getMaxId(), 20001); + } + + { + page_storage = reopenWithConfig(config); + ASSERT_EQ(page_storage->getMaxId(), 20001); + } +} +CATCH + } // namespace PS::V3::tests } // namespace DB From 40ee200c92f36b71b8316567c561f5452c72923f Mon Sep 17 00:00:00 2001 From: jiaqizho Date: Fri, 20 May 2022 16:20:20 +0800 Subject: [PATCH 2/8] fix tidy --- dbms/src/Storages/Page/V2/PageStorage.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dbms/src/Storages/Page/V2/PageStorage.cpp b/dbms/src/Storages/Page/V2/PageStorage.cpp index 304460647de..7a23afb11d4 100644 --- a/dbms/src/Storages/Page/V2/PageStorage.cpp +++ b/dbms/src/Storages/Page/V2/PageStorage.cpp @@ -893,9 +893,9 @@ void PageStorage::drop() struct GcContext { PageFileIdAndLevel min_file_id; - PageFile::Type min_file_type; + PageFile::Type min_file_type = PageFile::Type::Invalid; PageFileIdAndLevel max_file_id; - PageFile::Type max_file_type; + PageFile::Type max_file_type = PageFile::Type::Invalid; size_t num_page_files = 0; size_t num_legacy_files = 0; From 15513ca893099e41fbf876f6e432cfdf4bb8e90f Mon Sep 17 00:00:00 2001 From: jiaqizho Date: Fri, 20 May 2022 16:48:39 +0800 Subject: [PATCH 3/8] change segment_id when init delta mergestore --- dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp b/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp index 997db601d1e..17d240d5905 100644 --- a/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp +++ b/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp @@ -245,13 +245,10 @@ DeltaMergeStore::DeltaMergeStore(Context & db_context, !first_segment_entry.isValid()) { // Create the first segment. - auto segment_id = storage_pool->newMetaPageId(); - if (segment_id != DELTA_MERGE_FIRST_SEGMENT_ID) - throw Exception(fmt::format("The first segment id should be {}", DELTA_MERGE_FIRST_SEGMENT_ID), ErrorCodes::LOGICAL_ERROR); auto first_segment - = Segment::newSegment(*dm_context, store_columns, RowKeyRange::newAll(is_common_handle, rowkey_column_size), segment_id, 0); + = Segment::newSegment(*dm_context, store_columns, RowKeyRange::newAll(is_common_handle, rowkey_column_size), DELTA_MERGE_FIRST_SEGMENT_ID, 0); segments.emplace(first_segment->getRowKeyRange().getEnd(), first_segment); - id_to_segment.emplace(segment_id, first_segment); + id_to_segment.emplace(DELTA_MERGE_FIRST_SEGMENT_ID, first_segment); } else { From c6ec0a391dfc79e0bc11f79d8b70c131e00ee648 Mon Sep 17 00:00:00 2001 From: jiaqizho Date: Fri, 20 May 2022 17:33:41 +0800 Subject: [PATCH 4/8] fix --- .../Storages/DeltaMerge/DeltaMergeStore.cpp | 18 +++++++++++++++--- dbms/src/Storages/Page/V3/PageDirectory.h | 2 +- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp b/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp index 17d240d5905..b7da9592e91 100644 --- a/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp +++ b/dbms/src/Storages/DeltaMerge/DeltaMergeStore.cpp @@ -244,11 +244,23 @@ DeltaMergeStore::DeltaMergeStore(Context & db_context, if (const auto first_segment_entry = storage_pool->metaReader()->getPageEntry(DELTA_MERGE_FIRST_SEGMENT_ID); !first_segment_entry.isValid()) { - // Create the first segment. + auto segment_id = storage_pool->newMetaPageId(); + if (segment_id != DELTA_MERGE_FIRST_SEGMENT_ID) + { + if (page_storage_run_mode == PageStorageRunMode::ONLY_V2) + { + throw Exception(fmt::format("The first segment id should be {}", DELTA_MERGE_FIRST_SEGMENT_ID), ErrorCodes::LOGICAL_ERROR); + } + + // In ONLY_V3 or MIX_MODE, If create a new DeltaMergeStore + // Should used fixed DELTA_MERGE_FIRST_SEGMENT_ID to create first segment + segment_id = DELTA_MERGE_FIRST_SEGMENT_ID; + } + auto first_segment - = Segment::newSegment(*dm_context, store_columns, RowKeyRange::newAll(is_common_handle, rowkey_column_size), DELTA_MERGE_FIRST_SEGMENT_ID, 0); + = Segment::newSegment(*dm_context, store_columns, RowKeyRange::newAll(is_common_handle, rowkey_column_size), segment_id, 0); segments.emplace(first_segment->getRowKeyRange().getEnd(), first_segment); - id_to_segment.emplace(DELTA_MERGE_FIRST_SEGMENT_ID, first_segment); + id_to_segment.emplace(segment_id, first_segment); } else { diff --git a/dbms/src/Storages/Page/V3/PageDirectory.h b/dbms/src/Storages/Page/V3/PageDirectory.h index 2dfeb437a8b..a3c6b079fee 100644 --- a/dbms/src/Storages/Page/V3/PageDirectory.h +++ b/dbms/src/Storages/Page/V3/PageDirectory.h @@ -397,7 +397,7 @@ class PageDirectory } private: - UInt64 max_page_id; + PageId max_page_id; std::atomic sequence; mutable std::shared_mutex table_rw_mutex; MVCCMapType mvcc_table_directory; From 23efd70a2b99e5a3adc7af520e409b8ac5b23c4a Mon Sep 17 00:00:00 2001 From: jiaqizho Date: Mon, 23 May 2022 12:45:03 +0800 Subject: [PATCH 5/8] update --- dbms/src/Storages/Page/PageStorage.h | 6 ++++++ dbms/src/Storages/Page/V3/PageDirectory.cpp | 5 +---- dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp | 7 ++----- dbms/src/Storages/Page/V3/PageDirectoryFactory.h | 1 - 4 files changed, 9 insertions(+), 10 deletions(-) diff --git a/dbms/src/Storages/Page/PageStorage.h b/dbms/src/Storages/Page/PageStorage.h index 70e598a0e3a..57c94db46db 100644 --- a/dbms/src/Storages/Page/PageStorage.h +++ b/dbms/src/Storages/Page/PageStorage.h @@ -233,6 +233,12 @@ class PageStorage : private boost::noncopyable virtual void drop() = 0; + // Get the max id from PageStorage + // For V2, every table have three PageStorage(meta/data/log) + // So the Page id returned by calling getMaxId starts from 0 + // For V3, PageStorage is global(distinguish single table with ns_id) + // So the Page id returned is not from 0. + // Page id 1 in each ns_id is special. virtual PageId getMaxId() = 0; virtual SnapshotPtr getSnapshot(const String & tracing_id) = 0; diff --git a/dbms/src/Storages/Page/V3/PageDirectory.cpp b/dbms/src/Storages/Page/V3/PageDirectory.cpp index 630683ace7d..64a3fead674 100644 --- a/dbms/src/Storages/Page/V3/PageDirectory.cpp +++ b/dbms/src/Storages/Page/V3/PageDirectory.cpp @@ -1032,10 +1032,7 @@ void PageDirectory::apply(PageEntriesEdit && edit, const WriteLimiterPtr & write for (const auto & r : edit.getRecords()) { // Protected in write_lock - if (r.page_id.low > max_page_id) - { - max_page_id = r.page_id.low; - } + max_page_id = std::max(max_page_id, r.page_id.low); auto [iter, created] = mvcc_table_directory.insert(std::make_pair(r.page_id, nullptr)); if (created) diff --git a/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp b/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp index 40c65772fb0..deda30dc574 100644 --- a/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp +++ b/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp @@ -40,6 +40,7 @@ PageDirectoryPtr PageDirectoryFactory::create(String storage_name, FileProviderP // After restoring from the disk, we need cleanup all invalid entries in memory, or it will // try to run GC again on some entries that are already marked as invalid in BlobStore. dir->gcInMemEntries(); + LOG_FMT_INFO(DB::Logger::get("PageDirectoryFactory"), "PageDirectory restored. [max_page_id={}]", dir->getMaxId()); if (blob_stats) { @@ -111,7 +112,6 @@ void PageDirectoryFactory::loadEdit(const PageDirectoryPtr & dir, const PageEntr { if (max_applied_ver < r.version) max_applied_ver = r.version; - max_applied_page_id = std::max(r.page_id, max_applied_page_id); // We can not avoid page id from being reused under some corner situation. Try to do gcInMemEntries // and apply again to resolve the error. @@ -135,10 +135,7 @@ bool PageDirectoryFactory::applyRecord( iter->second = std::make_shared(); } - if (r.page_id.low > dir->max_page_id) - { - dir->max_page_id = r.page_id.low; - } + dir->max_page_id = std::max(dir->max_page_id, r.page_id.low); const auto & version_list = iter->second; const auto & restored_version = r.version; diff --git a/dbms/src/Storages/Page/V3/PageDirectoryFactory.h b/dbms/src/Storages/Page/V3/PageDirectoryFactory.h index 11337e4a6cc..e4b76bfba0d 100644 --- a/dbms/src/Storages/Page/V3/PageDirectoryFactory.h +++ b/dbms/src/Storages/Page/V3/PageDirectoryFactory.h @@ -38,7 +38,6 @@ class PageDirectoryFactory { public: PageVersion max_applied_ver; - PageIdV3Internal max_applied_page_id; PageDirectoryFactory & setBlobStore(BlobStore & blob_store) { From 5cc1de924e31d115f3b207222c1ea51c8f70030c Mon Sep 17 00:00:00 2001 From: JaySon Date: Mon, 23 May 2022 13:08:15 +0800 Subject: [PATCH 6/8] Apply suggestions from code review --- dbms/src/Storages/Page/PageStorage.h | 16 ++++++++++------ .../Storages/Page/V3/PageDirectoryFactory.cpp | 2 +- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/dbms/src/Storages/Page/PageStorage.h b/dbms/src/Storages/Page/PageStorage.h index 57c94db46db..569f7dec0ed 100644 --- a/dbms/src/Storages/Page/PageStorage.h +++ b/dbms/src/Storages/Page/PageStorage.h @@ -233,12 +233,16 @@ class PageStorage : private boost::noncopyable virtual void drop() = 0; - // Get the max id from PageStorage - // For V2, every table have three PageStorage(meta/data/log) - // So the Page id returned by calling getMaxId starts from 0 - // For V3, PageStorage is global(distinguish single table with ns_id) - // So the Page id returned is not from 0. - // Page id 1 in each ns_id is special. + // Get the max id from PageStorage. + // + // For V2, every table have its own three PageStorage (meta/data/log). + // So this function return the Page id starts from 0 and is continuously incremented to + // new pages. + // For V3, PageStorage is global(distinguish by ns_id for different table). + // Inorder to avoid Page id from being reused (and cause bugs while restoring WAL from disk), + // this function returns the global max id regardless of ns_id. This causes the ids in a table + // to not be continuously incremented. + // Note that Page id 1 in each ns_id is special. virtual PageId getMaxId() = 0; virtual SnapshotPtr getSnapshot(const String & tracing_id) = 0; diff --git a/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp b/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp index deda30dc574..0592d1ddaa8 100644 --- a/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp +++ b/dbms/src/Storages/Page/V3/PageDirectoryFactory.cpp @@ -40,7 +40,7 @@ PageDirectoryPtr PageDirectoryFactory::create(String storage_name, FileProviderP // After restoring from the disk, we need cleanup all invalid entries in memory, or it will // try to run GC again on some entries that are already marked as invalid in BlobStore. dir->gcInMemEntries(); - LOG_FMT_INFO(DB::Logger::get("PageDirectoryFactory"), "PageDirectory restored. [max_page_id={}]", dir->getMaxId()); + LOG_FMT_INFO(DB::Logger::get("PageDirectoryFactory"), "PageDirectory restored [max_page_id={}] [max_applied_ver={}]", dir->getMaxId(), dir->sequence); if (blob_stats) { From f7b8895a36f97cfec6a12049b69054c6b323ce7f Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Mon, 23 May 2022 13:16:28 +0800 Subject: [PATCH 7/8] Update comment on StoragePool Signed-off-by: JaySon-Huang --- dbms/src/Storages/DeltaMerge/StoragePool.h | 11 +++++++++-- dbms/src/Storages/Page/PageStorage.h | 4 ++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/dbms/src/Storages/DeltaMerge/StoragePool.h b/dbms/src/Storages/DeltaMerge/StoragePool.h index 7ba6dd85995..d05454a5431 100644 --- a/dbms/src/Storages/DeltaMerge/StoragePool.h +++ b/dbms/src/Storages/DeltaMerge/StoragePool.h @@ -149,11 +149,18 @@ class StoragePool : private boost::noncopyable // Caller must cancel gc tasks before drop void drop(); - PageId newDataPageIdForDTFile(StableDiskDelegator & delegator, const char * who); + // For function `newLogPageId`,`newMetaPageId`,`newDataPageIdForDTFile`: + // For PageStorageRunMode::ONLY_V2, every table have its own three PageStorage (meta/data/log). + // So these functions return the Page id starts from 1 and is continuously incremented. + // For PageStorageRunMode::ONLY_V3/MIX_MODE, PageStorage is global(distinguish by ns_id for different table). + // In order to avoid Page id from being reused (and cause troubles while restoring WAL from disk), + // StoragePool will assign the max_log_page_id/max_meta_page_id/max_data_page_id by the global max id + // regardless of ns_id while being restored. This causes the ids in a table to not be continuously incremented. - PageId maxMetaPageId() { return max_meta_page_id; } + PageId newDataPageIdForDTFile(StableDiskDelegator & delegator, const char * who); PageId newLogPageId() { return ++max_log_page_id; } PageId newMetaPageId() { return ++max_meta_page_id; } + #ifndef DBMS_PUBLIC_GTEST private: #endif diff --git a/dbms/src/Storages/Page/PageStorage.h b/dbms/src/Storages/Page/PageStorage.h index 569f7dec0ed..479c368a585 100644 --- a/dbms/src/Storages/Page/PageStorage.h +++ b/dbms/src/Storages/Page/PageStorage.h @@ -234,12 +234,12 @@ class PageStorage : private boost::noncopyable virtual void drop() = 0; // Get the max id from PageStorage. - // + // // For V2, every table have its own three PageStorage (meta/data/log). // So this function return the Page id starts from 0 and is continuously incremented to // new pages. // For V3, PageStorage is global(distinguish by ns_id for different table). - // Inorder to avoid Page id from being reused (and cause bugs while restoring WAL from disk), + // In order to avoid Page id from being reused (and cause troubles while restoring WAL from disk), // this function returns the global max id regardless of ns_id. This causes the ids in a table // to not be continuously incremented. // Note that Page id 1 in each ns_id is special. From f9cc439e714d79f65369a4251a13ad554e0ed39d Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Mon, 23 May 2022 13:50:27 +0800 Subject: [PATCH 8/8] Add exception message Signed-off-by: JaySon-Huang --- dbms/src/Storages/Page/V3/WAL/serialize.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dbms/src/Storages/Page/V3/WAL/serialize.cpp b/dbms/src/Storages/Page/V3/WAL/serialize.cpp index f8e26617499..6b7bc9b8a21 100644 --- a/dbms/src/Storages/Page/V3/WAL/serialize.cpp +++ b/dbms/src/Storages/Page/V3/WAL/serialize.cpp @@ -218,7 +218,7 @@ void deserializeFrom(ReadBuffer & buf, PageEntriesEdit & edit) break; } default: - throw Exception(fmt::format("Unknown record type: {}", record_type)); + throw Exception(fmt::format("Unknown record type: {}", record_type), ErrorCodes::LOGICAL_ERROR); } } } @@ -261,7 +261,7 @@ PageEntriesEdit deserializeFrom(std::string_view record) UInt32 version = 0; readIntBinary(version, buf); if (version != 1) - throw Exception(""); + throw Exception(fmt::format("Unknown version for PageEntriesEdit deser [version={}]", version), ErrorCodes::LOGICAL_ERROR); deserializeFrom(buf, edit); return edit;