From fef24aa0db145ac354d9d8ffe934be308150db0c Mon Sep 17 00:00:00 2001 From: jiaqizho Date: Thu, 20 Jan 2022 13:26:17 +0800 Subject: [PATCH 1/9] fix some bug in mvcc and blobstore --- dbms/src/Storages/Page/PageDefines.h | 2 +- dbms/src/Storages/Page/V3/BlobStore.cpp | 35 ++++++++- dbms/src/Storages/Page/V3/BlobStore.h | 2 +- dbms/src/Storages/Page/V3/PageEntry.h | 1 + .../Page/V3/tests/gtest_blob_store.cpp | 71 ++++++++++--------- 5 files changed, 74 insertions(+), 37 deletions(-) diff --git a/dbms/src/Storages/Page/PageDefines.h b/dbms/src/Storages/Page/PageDefines.h index 789e71d29ac..a75934d6f93 100644 --- a/dbms/src/Storages/Page/PageDefines.h +++ b/dbms/src/Storages/Page/PageDefines.h @@ -50,7 +50,7 @@ using PageSize = UInt64; using BlobFileId = UInt32; using BlobFileOffset = UInt64; -static constexpr BlobFileId INVALID_BLOBFILE_ID = std::numeric_limits::max(); +static constexpr BlobFileId INVALID_BLOBFILE_ID = 0; static constexpr BlobFileOffset INVALID_BLOBFILE_OFFSET = std::numeric_limits::max(); struct ByteBuffer diff --git a/dbms/src/Storages/Page/V3/BlobStore.cpp b/dbms/src/Storages/Page/V3/BlobStore.cpp index 14d87f2c870..b82b2604c35 100644 --- a/dbms/src/Storages/Page/V3/BlobStore.cpp +++ b/dbms/src/Storages/Page/V3/BlobStore.cpp @@ -70,6 +70,14 @@ PageEntriesEdit BlobStore::write(DB::WriteBatch & wb, const WriteLimiterPtr & wr edit.ref(write.page_id, write.ori_page_id); break; } + case WriteBatch::WriteType::PUT: + { // Only putExternal won't have data. + PageEntryV3 entry; + entry.tag = write.tag; + + edit.put(write.page_id, entry); + break; + } default: throw Exception("write batch have a invalid total size.", ErrorCodes::LOGICAL_ERROR); @@ -100,6 +108,7 @@ PageEntriesEdit BlobStore::write(DB::WriteBatch & wb, const WriteLimiterPtr & wr entry.file_id = blob_id; entry.size = write.size; + entry.tag = write.tag; entry.offset = offset_in_file + offset_in_allocated; offset_in_allocated += write.size; @@ -193,23 +202,33 @@ std::pair BlobStore::getPosFromStats(size_t size) { stat = blob_stats.createStat(blob_file_id, lock_stats); } + + // We must get the lock from BlobStat under the BlobStats lock. + // It will ensure that BlobStat updates are in order. + // Also it won't incur more overhead. + // If BlobStat can updates are not order. Then + + stat->sm_lock.lock(); } // Get Postion from single stat - auto lock_stat = stat->lock(); + auto old_max_cap = stat->sm_max_caps; BlobFileOffset offset = stat->getPosFromStat(size); // Can't insert into this spacemap if (offset == INVALID_BLOBFILE_OFFSET) { + stat->sm_lock.unlock(); stat->smap->logStats(); - throw Exception(fmt::format("Get postion from BlobStat failed, it may caused by `sm_max_caps` is no corrent. [size={}, max_caps={}, BlobFileId={}]", + throw Exception(fmt::format("Get postion from BlobStat failed, it may caused by `sm_max_caps` is no corrent. [size={}, old_max_caps={}, max_caps(updated)={}, BlobFileId={}]", size, + old_max_cap, stat->sm_max_caps, stat->id), ErrorCodes::LOGICAL_ERROR); } + stat->sm_lock.unlock(); return std::make_pair(stat->id, offset); } @@ -328,7 +347,11 @@ std::vector BlobStore::getGCStats() auto right_margin = stat->smap->getRightMargin(); stat->sm_valid_rate = stat->sm_valid_size * 1.0 / right_margin; - assert(stat->sm_valid_rate <= 1.0); + if (stat->sm_valid_rate > 1.0) + { + LOG_FMT_ERROR(log, "Current blob got a invalid rate {:.2f}, total size is {} , valid size is {} , right margin is {}", stat->sm_valid_rate, stat->sm_total_size, stat->sm_valid_size, right_margin); + assert(false); + } // Check if GC is required if (stat->sm_valid_rate <= config.heavy_gc_valid_rate) @@ -573,6 +596,12 @@ std::pair BlobStore::BlobStats::chooseStat(size_t buf_s return std::make_pair(nullptr, chooseNewStat()); } + // We need to assume that this insert will reduce max_cap. + // Because other threads may also be waiting for BlobStats to chooseStat during this time. + // If max_cap is not reduced, it may cause the same BlobStat to accept multiple buffers and exceed its max_cap. + // After the BlobStore records the buffer size, max_caps will also get an accurate update. + // So there won't get problem in reducing max_caps here. + stat_ptr->sm_max_caps -= buf_size; return std::make_pair(stat_ptr, INVALID_BLOBFILE_ID); } diff --git a/dbms/src/Storages/Page/V3/BlobStore.h b/dbms/src/Storages/Page/V3/BlobStore.h index c5305189165..0cd9ff753ba 100644 --- a/dbms/src/Storages/Page/V3/BlobStore.h +++ b/dbms/src/Storages/Page/V3/BlobStore.h @@ -137,7 +137,7 @@ class BlobStore : public Allocator Poco::Logger * log; BlobStore::Config config; - BlobFileId roll_id = 0; + BlobFileId roll_id = 1; std::list old_ids; std::list stats_map; mutable std::mutex lock_stats; diff --git a/dbms/src/Storages/Page/V3/PageEntry.h b/dbms/src/Storages/Page/V3/PageEntry.h index a090cb0bb8c..844cd0ae863 100644 --- a/dbms/src/Storages/Page/V3/PageEntry.h +++ b/dbms/src/Storages/Page/V3/PageEntry.h @@ -9,6 +9,7 @@ struct PageEntryV3 public: BlobFileId file_id = 0; // The id of page data persisted in PageSize size = 0; // The size of page data + UInt64 tag = 0; BlobFileOffset offset = 0; // The offset of page data in file UInt64 checksum = 0; // The checksum of whole page data diff --git a/dbms/src/Storages/Page/V3/tests/gtest_blob_store.cpp b/dbms/src/Storages/Page/V3/tests/gtest_blob_store.cpp index d48cf668960..27921a1acdd 100644 --- a/dbms/src/Storages/Page/V3/tests/gtest_blob_store.cpp +++ b/dbms/src/Storages/Page/V3/tests/gtest_blob_store.cpp @@ -67,12 +67,12 @@ TEST_F(BlobStoreTest, testStat) BlobStats stats(&Poco::Logger::get("BlobStoreTest"), config); std::tie(stat, blob_file_id) = stats.chooseStat(10, BLOBFILE_LIMIT_SIZE, stats.lock()); - ASSERT_EQ(blob_file_id, 0); + ASSERT_EQ(blob_file_id, 1); ASSERT_FALSE(stat); // still 0 std::tie(stat, blob_file_id) = stats.chooseStat(10, BLOBFILE_LIMIT_SIZE, stats.lock()); - ASSERT_EQ(blob_file_id, 0); + ASSERT_EQ(blob_file_id, 1); ASSERT_FALSE(stat); stats.createStat(0, stats.lock()); @@ -139,7 +139,7 @@ TEST_F(BlobStoreTest, testFullStats) BlobStats stats(&Poco::Logger::get("BlobStoreTest"), config); - stat = stats.createStat(0, stats.lock()); + stat = stats.createStat(1, stats.lock()); offset = stat->getPosFromStat(BLOBFILE_LIMIT_SIZE - 1); ASSERT_EQ(offset, 0); @@ -154,7 +154,7 @@ TEST_F(BlobStoreTest, testFullStats) // Won't choose full one std::tie(stat, blob_file_id) = stats.chooseStat(100, BLOBFILE_LIMIT_SIZE, stats.lock()); - ASSERT_EQ(blob_file_id, 1); + ASSERT_EQ(blob_file_id, 2); ASSERT_FALSE(stat); // A new stat can use @@ -163,17 +163,17 @@ TEST_F(BlobStoreTest, testFullStats) ASSERT_EQ(offset, 0); // Remove the stat which id is 0 , now remain the stat which id is 1 - stats.eraseStat(0, stats.lock()); + stats.eraseStat(1, stats.lock()); - // Then full the stat which id 1 + // Then full the stat which id 2 offset = stat->getPosFromStat(BLOBFILE_LIMIT_SIZE - 100); ASSERT_EQ(offset, 100); - // Then choose stat , it should return the stat id 0 + // Then choose stat , it should return the stat id 1 // cause in this time , stat which id is 1 have been earsed, - // and stat which id is 1 is full. + // and stat which id is 2 is full. std::tie(stat, blob_file_id) = stats.chooseStat(100, BLOBFILE_LIMIT_SIZE, stats.lock()); - ASSERT_EQ(blob_file_id, 0); + ASSERT_EQ(blob_file_id, 1); ASSERT_FALSE(stat); } @@ -213,7 +213,7 @@ TEST_F(BlobStoreTest, testWriteRead) ASSERT_EQ(record.type, WriteBatch::WriteType::PUT); ASSERT_EQ(record.entry.offset, index * buff_size); ASSERT_EQ(record.entry.size, buff_size); - ASSERT_EQ(record.entry.file_id, 0); + ASSERT_EQ(record.entry.file_id, 1); // Read directly from the file blob_store.read(record.entry.file_id, @@ -238,7 +238,7 @@ TEST_F(BlobStoreTest, testWriteRead) // Test `PageMap` read page_id = 50; index = 0; - auto page_map = blob_store.read(entries, /* ReadLimiterPtr */ nullptr); + auto page_map = blob_store.read(entries); for (auto & [id, page] : page_map) { ASSERT_EQ(id, page_id++); @@ -252,7 +252,7 @@ TEST_F(BlobStoreTest, testWriteRead) index = 0; for (auto & entry : entries) { - auto page = blob_store.read(entry, /* ReadLimiterPtr */ nullptr); + auto page = blob_store.read(entry); ASSERT_EQ(page.data.size(), buff_size); ASSERT_EQ(strncmp(c_buff + index * buff_size, page.data.begin(), page.data.size()), 0); index++; @@ -305,7 +305,7 @@ TEST_F(BlobStoreTest, testFeildOffsetWriteRead) ASSERT_EQ(record.type, WriteBatch::WriteType::PUT); ASSERT_EQ(record.entry.offset, index * buff_size); ASSERT_EQ(record.entry.size, buff_size); - ASSERT_EQ(record.entry.file_id, 0); + ASSERT_EQ(record.entry.file_id, 1); PageFieldSizes check_field_sizes; for (const auto & [field_offset, crc] : record.entry.field_offsets) @@ -364,14 +364,14 @@ try ASSERT_EQ(record.page_id, page_id); ASSERT_EQ(record.entry.offset, 0); ASSERT_EQ(record.entry.size, buff_size); - ASSERT_EQ(record.entry.file_id, 0); + ASSERT_EQ(record.entry.file_id, 1); record = records[1]; ASSERT_EQ(record.type, WriteBatch::WriteType::PUT); ASSERT_EQ(record.page_id, page_id); ASSERT_EQ(record.entry.offset, buff_size); ASSERT_EQ(record.entry.size, buff_size); - ASSERT_EQ(record.entry.file_id, 0); + ASSERT_EQ(record.entry.file_id, 1); } @@ -422,7 +422,7 @@ try ASSERT_EQ(record.page_id, page_id); ASSERT_EQ(record.entry.offset, buff_size * 2); ASSERT_EQ(record.entry.size, buff_size); - ASSERT_EQ(record.entry.file_id, 0); + ASSERT_EQ(record.entry.file_id, 1); record = records[1]; ASSERT_EQ(record.type, WriteBatch::WriteType::REF); @@ -487,7 +487,7 @@ TEST_F(BlobStoreTest, testWriteOutOfLimitSize) ASSERT_EQ(record.page_id, 50); ASSERT_EQ(record.entry.offset, 0); ASSERT_EQ(record.entry.size, buf_size); - ASSERT_EQ(record.entry.file_id, 0); + ASSERT_EQ(record.entry.file_id, 1); wb.clear(); wb.putPage(51, /*tag*/ 0, buff2, buf_size); @@ -500,7 +500,7 @@ TEST_F(BlobStoreTest, testWriteOutOfLimitSize) ASSERT_EQ(record.page_id, 51); ASSERT_EQ(record.entry.offset, 0); ASSERT_EQ(record.entry.size, buf_size); - ASSERT_EQ(record.entry.file_id, 1); + ASSERT_EQ(record.entry.file_id, 2); } } @@ -519,7 +519,10 @@ TEST_F(BlobStoreTest, testBlobStoreGcStats) char c_buff[buff_size * buff_nums]; for (size_t i = 0; i < buff_nums; ++i) { - c_buff[i * buff_size] = static_cast((0xff) + i); + for (size_t j = 0; j < buff_size; ++j) + { + c_buff[j + i * buff_size] = static_cast((j & 0xff) + i); + } ReadBufferPtr buff = std::make_shared(const_cast(c_buff + i * buff_size), buff_size); wb.putPage(page_id, /* tag */ 0, buff, buff_size); } @@ -555,9 +558,9 @@ TEST_F(BlobStoreTest, testBlobStoreGcStats) // After remove `entries_del1`. // Remain entries index [0, 2, 5, 6, 8] blob_store.remove(entries_del1); - ASSERT_EQ(entries_del1.begin()->file_id, 0); + ASSERT_EQ(entries_del1.begin()->file_id, 1); - auto stat = blob_store.blob_stats.fileIdToStat(0); + auto stat = blob_store.blob_stats.fileIdToStat(1); ASSERT_EQ(stat->sm_valid_rate, 0.5); ASSERT_EQ(stat->sm_total_size, buff_size * buff_nums); @@ -580,7 +583,7 @@ TEST_F(BlobStoreTest, testBlobStoreGcStats) ASSERT_EQ(stat->sm_valid_size, buff_size * 3); // Check disk file have been truncate to right margin - String path = blob_store.getBlobFilePath(0); + String path = blob_store.getBlobFilePath(1); Poco::File blob_file_in_disk(path); ASSERT_EQ(blob_file_in_disk.getSize(), stat->sm_total_size); } @@ -599,7 +602,10 @@ TEST_F(BlobStoreTest, testBlobStoreGcStats2) char c_buff[buff_size * buff_nums]; for (size_t i = 0; i < buff_nums; ++i) { - c_buff[i * buff_size] = static_cast((0xff) + i); + for (size_t j = 0; j < buff_size; ++j) + { + c_buff[j + i * buff_size] = static_cast((j & 0xff) + i); + } ReadBufferPtr buff = std::make_shared(const_cast(c_buff + i * buff_size), buff_size); wb.putPage(page_id, /* tag */ 0, buff, buff_size); } @@ -627,7 +633,7 @@ TEST_F(BlobStoreTest, testBlobStoreGcStats2) // Remain entries index [8, 9]. blob_store.remove(entries_del); - auto stat = blob_store.blob_stats.fileIdToStat(0); + auto stat = blob_store.blob_stats.fileIdToStat(1); const auto & gc_stats = blob_store.getGCStats(); ASSERT_FALSE(gc_stats.empty()); @@ -637,7 +643,7 @@ TEST_F(BlobStoreTest, testBlobStoreGcStats2) ASSERT_EQ(stat->sm_valid_size, buff_size * 2); // Then we must do heavy GC - ASSERT_EQ(*gc_stats.begin(), 0); + ASSERT_EQ(*gc_stats.begin(), 1); } @@ -677,10 +683,10 @@ TEST_F(BlobStoreTest, GC) PageIdAndVersionedEntries versioned_pageid_entries; versioned_pageid_entries.emplace_back(std::make_pair(page_id, versioned_entries)); std::map gc_context; - gc_context[0] = versioned_pageid_entries; + gc_context[1] = versioned_pageid_entries; // Before we do BlobStore we need change BlobFile0 to Read-Only - auto stat = blob_store.blob_stats.fileIdToStat(0); + auto stat = blob_store.blob_stats.fileIdToStat(1); stat->changeToReadOnly(); const auto & gc_edit = blob_store.gc(gc_context, static_cast(buff_size * buff_nums)); @@ -691,18 +697,19 @@ TEST_F(BlobStoreTest, GC) for (const auto & record : gc_edit.getRecords()) { ASSERT_EQ(record.page_id, page_id); - ASSERT_EQ(record.entry.file_id, 1); + ASSERT_EQ(record.entry.file_id, 2); ASSERT_EQ(record.entry.checksum, it->second.checksum); ASSERT_EQ(record.entry.size, it->second.size); it++; } // Check blobfile1 - Poco::File file0(blob_store.getBlobFilePath(0)); Poco::File file1(blob_store.getBlobFilePath(1)); - ASSERT_TRUE(file0.exists()); + Poco::File file2(blob_store.getBlobFilePath(2)); ASSERT_TRUE(file1.exists()); - ASSERT_EQ(file0.getSize(), file1.getSize()); + ASSERT_TRUE(file2.exists()); + ASSERT_EQ(file1.getSize(), file2.getSize()); } -} // namespace DB::PS::V3::tests + +} // namespace DB::PS::V3::tests \ No newline at end of file From 55341e44acbd6566febac75c2a060d289f4a83ee Mon Sep 17 00:00:00 2001 From: jiaqizho Date: Thu, 20 Jan 2022 17:02:22 +0800 Subject: [PATCH 2/9] update --- dbms/src/Storages/Page/V3/BlobStore.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/dbms/src/Storages/Page/V3/BlobStore.cpp b/dbms/src/Storages/Page/V3/BlobStore.cpp index b82b2604c35..9aa076354a2 100644 --- a/dbms/src/Storages/Page/V3/BlobStore.cpp +++ b/dbms/src/Storages/Page/V3/BlobStore.cpp @@ -192,7 +192,7 @@ std::pair BlobStore::getPosFromStats(size_t size) { BlobStatPtr stat; - { + auto lock_stat = [size, this, &stat]() -> std::lock_guard { auto lock_stats = blob_stats.lock(); BlobFileId blob_file_id = INVALID_BLOBFILE_ID; std::tie(stat, blob_file_id) = blob_stats.chooseStat(size, config.file_limit_size, lock_stats); @@ -206,10 +206,11 @@ std::pair BlobStore::getPosFromStats(size_t size) // We must get the lock from BlobStat under the BlobStats lock. // It will ensure that BlobStat updates are in order. // Also it won't incur more overhead. - // If BlobStat can updates are not order. Then + // If BlobStat can updates are not order. + // Then it may cause stat to fail to get the span and write failure. - stat->sm_lock.lock(); - } + return stat->lock(); + }(); // Get Postion from single stat auto old_max_cap = stat->sm_max_caps; @@ -218,7 +219,6 @@ std::pair BlobStore::getPosFromStats(size_t size) // Can't insert into this spacemap if (offset == INVALID_BLOBFILE_OFFSET) { - stat->sm_lock.unlock(); stat->smap->logStats(); throw Exception(fmt::format("Get postion from BlobStat failed, it may caused by `sm_max_caps` is no corrent. [size={}, old_max_caps={}, max_caps(updated)={}, BlobFileId={}]", size, @@ -228,7 +228,6 @@ std::pair BlobStore::getPosFromStats(size_t size) ErrorCodes::LOGICAL_ERROR); } - stat->sm_lock.unlock(); return std::make_pair(stat->id, offset); } @@ -349,7 +348,7 @@ std::vector BlobStore::getGCStats() stat->sm_valid_rate = stat->sm_valid_size * 1.0 / right_margin; if (stat->sm_valid_rate > 1.0) { - LOG_FMT_ERROR(log, "Current blob got a invalid rate {:.2f}, total size is {} , valid size is {} , right margin is {}", stat->sm_valid_rate, stat->sm_total_size, stat->sm_valid_size, right_margin); + LOG_FMT_ERROR(log, "Current blob got an invalid rate {:.2f}, total size is {}, valid size is {}, right margin is {}", stat->sm_valid_rate, stat->sm_total_size, stat->sm_valid_size, right_margin); assert(false); } From 2b08f9b3743b396628d86959039dd59770800c6c Mon Sep 17 00:00:00 2001 From: jiaqizho Date: Thu, 20 Jan 2022 19:51:25 +0800 Subject: [PATCH 3/9] Update dbms/src/Storages/Page/V3/BlobStore.cpp Co-authored-by: JaySon --- dbms/src/Storages/Page/V3/BlobStore.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/dbms/src/Storages/Page/V3/BlobStore.cpp b/dbms/src/Storages/Page/V3/BlobStore.cpp index 9aa076354a2..f5ed48dc538 100644 --- a/dbms/src/Storages/Page/V3/BlobStore.cpp +++ b/dbms/src/Storages/Page/V3/BlobStore.cpp @@ -203,11 +203,10 @@ std::pair BlobStore::getPosFromStats(size_t size) stat = blob_stats.createStat(blob_file_id, lock_stats); } - // We must get the lock from BlobStat under the BlobStats lock. - // It will ensure that BlobStat updates are in order. - // Also it won't incur more overhead. - // If BlobStat can updates are not order. - // Then it may cause stat to fail to get the span and write failure. + // We must get the lock from BlobStat under the BlobStats lock + // to ensure that BlobStat updates are serialized. + // Otherwise it may cause stat to fail to get the span for writing + // and throwing exception. return stat->lock(); }(); From 7c29f039b5925a8fce8b293dd3e44a2eb96ebbf3 Mon Sep 17 00:00:00 2001 From: jiaqizho Date: Thu, 20 Jan 2022 19:51:30 +0800 Subject: [PATCH 4/9] Update dbms/src/Storages/Page/V3/BlobStore.cpp Co-authored-by: JaySon --- dbms/src/Storages/Page/V3/BlobStore.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/src/Storages/Page/V3/BlobStore.cpp b/dbms/src/Storages/Page/V3/BlobStore.cpp index f5ed48dc538..0b06279cc3e 100644 --- a/dbms/src/Storages/Page/V3/BlobStore.cpp +++ b/dbms/src/Storages/Page/V3/BlobStore.cpp @@ -219,7 +219,7 @@ std::pair BlobStore::getPosFromStats(size_t size) if (offset == INVALID_BLOBFILE_OFFSET) { stat->smap->logStats(); - throw Exception(fmt::format("Get postion from BlobStat failed, it may caused by `sm_max_caps` is no corrent. [size={}, old_max_caps={}, max_caps(updated)={}, BlobFileId={}]", + throw Exception(fmt::format("Get postion from BlobStat failed, it may caused by `sm_max_caps` is no correct. [size={}, old_max_caps={}, max_caps={}, BlobFileId={}]", size, old_max_cap, stat->sm_max_caps, From 1b3ef7eb56c9a36f3909563cbf288f80ec473714 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Fri, 21 Jan 2022 00:33:54 +0800 Subject: [PATCH 5/9] Fix bug Signed-off-by: JaySon-Huang --- .../Page/V3/tests/gtest_page_directory.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) 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 b1a0a977c68..f2c5bc1d1b5 100644 --- a/dbms/src/Storages/Page/V3/tests/gtest_page_directory.cpp +++ b/dbms/src/Storages/Page/V3/tests/gtest_page_directory.cpp @@ -425,8 +425,8 @@ TEST_F(PageDirectoryTest, TestRefWontDeadLock) dir.apply(std::move(edit2)); } -#define INSERT_BLOBID_ENTRY(BLOBID, VERSION) \ - PageEntryV3 entry_v##VERSION{.file_id = (BLOBID), .size = (VERSION), .offset = 0x123, .checksum = 0x4567}; \ +#define INSERT_BLOBID_ENTRY(BLOBID, VERSION) \ + PageEntryV3 entry_v##VERSION{.file_id = (BLOBID), .size = (VERSION), .tag = 0, .offset = 0x123, .checksum = 0x4567}; \ entries.createNewVersion((VERSION), entry_v##VERSION); #define INSERT_ENTRY(VERSION) INSERT_BLOBID_ENTRY(1, VERSION) #define INSERT_GC_ENTRY(VERSION, EPOCH) \ @@ -637,12 +637,12 @@ class PageDirectoryGCTest : public PageDirectoryTest { }; -#define INSERT_ENTRY_TO(PAGE_ID, VERSION, BLOB_FILE_ID) \ - PageEntryV3 entry_v##VERSION{.file_id = (BLOB_FILE_ID), .size = (VERSION), .offset = 0x123, .checksum = 0x4567}; \ - { \ - PageEntriesEdit edit; \ - edit.put((PAGE_ID), entry_v##VERSION); \ - dir.apply(std::move(edit)); \ +#define INSERT_ENTRY_TO(PAGE_ID, VERSION, BLOB_FILE_ID) \ + PageEntryV3 entry_v##VERSION{.file_id = (BLOB_FILE_ID), .size = (VERSION), .tag = 0, .offset = 0x123, .checksum = 0x4567}; \ + { \ + PageEntriesEdit edit; \ + edit.put((PAGE_ID), entry_v##VERSION); \ + dir.apply(std::move(edit)); \ } // Insert an entry into mvcc directory #define INSERT_ENTRY(PAGE_ID, VERSION) INSERT_ENTRY_TO(PAGE_ID, VERSION, 1) From 7be644223516af890b619ac6a8e8d0889b1516c1 Mon Sep 17 00:00:00 2001 From: jiaqizho Date: Fri, 21 Jan 2022 10:20:18 +0800 Subject: [PATCH 6/9] fix build --- .../Page/V3/tests/gtest_page_directory.cpp | 64 +++++++++---------- 1 file changed, 32 insertions(+), 32 deletions(-) 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 76443a14a03..84c06027ec6 100644 --- a/dbms/src/Storages/Page/V3/tests/gtest_page_directory.cpp +++ b/dbms/src/Storages/Page/V3/tests/gtest_page_directory.cpp @@ -29,7 +29,7 @@ try auto snap0 = dir.createSnapshot(); EXPECT_ENTRY_NOT_EXIST(dir, 1, snap0); - PageEntryV3 entry1{.file_id = 1, .size = 1024, .offset = 0x123, .checksum = 0x4567}; + PageEntryV3 entry1{.file_id = 1, .size = 1024, .tag = 0, .offset = 0x123, .checksum = 0x4567}; { PageEntriesEdit edit; edit.put(1, entry1); @@ -39,7 +39,7 @@ try auto snap1 = dir.createSnapshot(); EXPECT_ENTRY_EQ(entry1, dir, 1, snap1); - PageEntryV3 entry2{.file_id = 2, .size = 1024, .offset = 0x123, .checksum = 0x4567}; + PageEntryV3 entry2{.file_id = 2, .size = 1024, .tag = 0, .offset = 0x123, .checksum = 0x4567}; { PageEntriesEdit edit; edit.put(2, entry2); @@ -67,7 +67,7 @@ try auto snap0 = dir.createSnapshot(); EXPECT_ENTRY_NOT_EXIST(dir, page_id, snap0); - PageEntryV3 entry1{.file_id = 1, .size = 1024, .offset = 0x123, .checksum = 0x4567}; + PageEntryV3 entry1{.file_id = 1, .size = 1024, .tag = 0, .offset = 0x123, .checksum = 0x4567}; { PageEntriesEdit edit; edit.put(page_id, entry1); @@ -77,7 +77,7 @@ try auto snap1 = dir.createSnapshot(); EXPECT_ENTRY_EQ(entry1, dir, page_id, snap1); - PageEntryV3 entry2{.file_id = 1, .size = 1024, .offset = 0x1234, .checksum = 0x4567}; + PageEntryV3 entry2{.file_id = 1, .size = 1024, .tag = 0, .offset = 0x1234, .checksum = 0x4567}; { PageEntriesEdit edit; edit.put(page_id, entry2); @@ -95,7 +95,7 @@ try // Put identical page within one `edit` page_id++; - PageEntryV3 entry3{.file_id = 1, .size = 1024, .offset = 0x12345, .checksum = 0x4567}; + PageEntryV3 entry3{.file_id = 1, .size = 1024, .tag = 0, .offset = 0x12345, .checksum = 0x4567}; { PageEntriesEdit edit; edit.put(page_id, entry1); @@ -116,8 +116,8 @@ CATCH TEST_F(PageDirectoryTest, ApplyPutDelRead) try { - PageEntryV3 entry1{.file_id = 1, .size = 1024, .offset = 0x123, .checksum = 0x4567}; - PageEntryV3 entry2{.file_id = 2, .size = 1024, .offset = 0x123, .checksum = 0x4567}; + 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); @@ -129,8 +129,8 @@ try EXPECT_ENTRY_EQ(entry1, dir, 1, snap1); EXPECT_ENTRY_EQ(entry2, dir, 2, snap1); - PageEntryV3 entry3{.file_id = 3, .size = 1024, .offset = 0x123, .checksum = 0x4567}; - PageEntryV3 entry4{.file_id = 4, .size = 1024, .offset = 0x123, .checksum = 0x4567}; + 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.del(2); @@ -161,8 +161,8 @@ CATCH TEST_F(PageDirectoryTest, ApplyUpdateOnRefEntries) try { - PageEntryV3 entry1{.file_id = 1, .size = 1024, .offset = 0x123, .checksum = 0x4567}; - PageEntryV3 entry2{.file_id = 2, .size = 1024, .offset = 0x123, .checksum = 0x4567}; + 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); @@ -212,8 +212,8 @@ CATCH TEST_F(PageDirectoryTest, ApplyDeleteOnRefEntries) try { - PageEntryV3 entry1{.file_id = 1, .size = 1024, .offset = 0x123, .checksum = 0x4567}; - PageEntryV3 entry2{.file_id = 2, .size = 1024, .offset = 0x123, .checksum = 0x4567}; + 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); @@ -262,8 +262,8 @@ CATCH TEST_F(PageDirectoryTest, ApplyRefOnRefEntries) try { - PageEntryV3 entry1{.file_id = 1, .size = 1024, .offset = 0x123, .checksum = 0x4567}; - PageEntryV3 entry2{.file_id = 2, .size = 1024, .offset = 0x123, .checksum = 0x4567}; + 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); @@ -300,8 +300,8 @@ CATCH TEST_F(PageDirectoryTest, ApplyDuplicatedRefEntries) try { - PageEntryV3 entry1{.file_id = 1, .size = 1024, .offset = 0x123, .checksum = 0x4567}; - PageEntryV3 entry2{.file_id = 2, .size = 1024, .offset = 0x123, .checksum = 0x4567}; + 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); @@ -336,8 +336,8 @@ CATCH TEST_F(PageDirectoryTest, ApplyCollapseDuplicatedRefEntries) try { - PageEntryV3 entry1{.file_id = 1, .size = 1024, .offset = 0x123, .checksum = 0x4567}; - PageEntryV3 entry2{.file_id = 2, .size = 1024, .offset = 0x123, .checksum = 0x4567}; + 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); @@ -373,9 +373,9 @@ CATCH TEST_F(PageDirectoryTest, ApplyRefToNotExistEntry) try { - PageEntryV3 entry1{.file_id = 1, .size = 1024, .offset = 0x123, .checksum = 0x4567}; - PageEntryV3 entry2{.file_id = 2, .size = 1024, .offset = 0x123, .checksum = 0x4567}; - PageEntryV3 entry3{.file_id = 3, .size = 1024, .offset = 0x123, .checksum = 0x4567}; + 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}; + PageEntryV3 entry3{.file_id = 3, .size = 1024, .tag = 0, .offset = 0x123, .checksum = 0x4567}; { PageEntriesEdit edit; edit.put(1, entry1); @@ -399,12 +399,12 @@ try } CATCH -#define INSERT_BLOBID_ENTRY(BLOBID, VERSION) \ - PageEntryV3 entry_v##VERSION{.file_id = (BLOBID), .size = (VERSION), .offset = 0x123, .checksum = 0x4567}; \ +#define INSERT_BLOBID_ENTRY(BLOBID, VERSION) \ + PageEntryV3 entry_v##VERSION{.file_id = (BLOBID), .size = (VERSION), .tag = 0, .offset = 0x123, .checksum = 0x4567}; \ entries.createNewVersion((VERSION), entry_v##VERSION); #define INSERT_ENTRY(VERSION) INSERT_BLOBID_ENTRY(1, VERSION) -#define INSERT_GC_ENTRY(VERSION, EPOCH) \ - PageEntryV3 entry_gc_v##VERSION##_##EPOCH{.file_id = 2, .size = (VERSION), .offset = 0x234, .checksum = 0x5678}; \ +#define INSERT_GC_ENTRY(VERSION, EPOCH) \ + PageEntryV3 entry_gc_v##VERSION##_##EPOCH{.file_id = 2, .size = (VERSION), .tag = 0, .offset = 0x234, .checksum = 0x5678}; \ entries.createNewVersion((VERSION), (EPOCH), entry_gc_v##VERSION##_##EPOCH); TEST(VersionedEntriesTest, InsertGet) @@ -611,12 +611,12 @@ class PageDirectoryGCTest : public PageDirectoryTest { }; -#define INSERT_ENTRY_TO(PAGE_ID, VERSION, BLOB_FILE_ID) \ - PageEntryV3 entry_v##VERSION{.file_id = (BLOB_FILE_ID), .size = (VERSION), .offset = 0x123, .checksum = 0x4567}; \ - { \ - PageEntriesEdit edit; \ - edit.put((PAGE_ID), entry_v##VERSION); \ - dir.apply(std::move(edit)); \ +#define INSERT_ENTRY_TO(PAGE_ID, VERSION, BLOB_FILE_ID) \ + PageEntryV3 entry_v##VERSION{.file_id = (BLOB_FILE_ID), .size = (VERSION), .tag = 0, .offset = 0x123, .checksum = 0x4567}; \ + { \ + PageEntriesEdit edit; \ + edit.put((PAGE_ID), entry_v##VERSION); \ + dir.apply(std::move(edit)); \ } // Insert an entry into mvcc directory #define INSERT_ENTRY(PAGE_ID, VERSION) INSERT_ENTRY_TO(PAGE_ID, VERSION, 1) From 9efb9587c266921eada0a85086493714f32ffd50 Mon Sep 17 00:00:00 2001 From: jiaqizho Date: Fri, 21 Jan 2022 10:50:19 +0800 Subject: [PATCH 7/9] fix build --- dbms/src/Storages/Page/V3/tests/gtest_page_directory.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 6eb17f7ad0c..261ce72a388 100644 --- a/dbms/src/Storages/Page/V3/tests/gtest_page_directory.cpp +++ b/dbms/src/Storages/Page/V3/tests/gtest_page_directory.cpp @@ -181,7 +181,7 @@ try // Update 3, 2 won't get updated. Update 2, 3 won't get updated. // Note that users should not rely on this behavior - PageEntryV3 entry_updated{.file_id = 999, .size = 16, .offset = 0x123, .checksum = 0x123}; + PageEntryV3 entry_updated{.file_id = 999, .size = 16, .tag = 0, .offset = 0x123, .checksum = 0x123}; { PageEntriesEdit edit; edit.put(3, entry_updated); @@ -193,7 +193,7 @@ try EXPECT_ENTRY_EQ(entry2, dir, 2, snap2); EXPECT_ENTRY_EQ(entry_updated, dir, 3, snap2); - PageEntryV3 entry_updated2{.file_id = 777, .size = 16, .offset = 0x123, .checksum = 0x123}; + PageEntryV3 entry_updated2{.file_id = 777, .size = 16, .tag = 0, .offset = 0x123, .checksum = 0x123}; { PageEntriesEdit edit; edit.put(2, entry_updated2); @@ -887,7 +887,7 @@ try INSERT_ENTRY_ACQ_SNAP(page_id, 5); INSERT_ENTRY(another_page_id, 6); INSERT_ENTRY(another_page_id, 7); - PageEntryV3 entry_v8{.file_id = 1, .size = 8, .offset = 0x123, .checksum = 0x4567}; + PageEntryV3 entry_v8{.file_id = 1, .size = 8, .tag = 0, .offset = 0x123, .checksum = 0x4567}; { PageEntriesEdit edit; edit.del(page_id); From 4956e7201ea76688c83b23db8ae4694f768c2aa2 Mon Sep 17 00:00:00 2001 From: jiaqizho Date: Fri, 21 Jan 2022 11:11:38 +0800 Subject: [PATCH 8/9] update --- dbms/src/Storages/Page/V3/BlobStore.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/dbms/src/Storages/Page/V3/BlobStore.cpp b/dbms/src/Storages/Page/V3/BlobStore.cpp index 0b06279cc3e..b108de9a821 100644 --- a/dbms/src/Storages/Page/V3/BlobStore.cpp +++ b/dbms/src/Storages/Page/V3/BlobStore.cpp @@ -203,6 +203,13 @@ std::pair BlobStore::getPosFromStats(size_t size) stat = blob_stats.createStat(blob_file_id, lock_stats); } + // We need to assume that this insert will reduce max_cap. + // Because other threads may also be waiting for BlobStats to chooseStat during this time. + // If max_cap is not reduced, it may cause the same BlobStat to accept multiple buffers and exceed its max_cap. + // After the BlobStore records the buffer size, max_caps will also get an accurate update. + // So there won't get problem in reducing max_caps here. + stat_ptr->sm_max_caps -= buf_size; + // We must get the lock from BlobStat under the BlobStats lock // to ensure that BlobStat updates are serialized. // Otherwise it may cause stat to fail to get the span for writing @@ -594,12 +601,6 @@ std::pair BlobStore::BlobStats::chooseStat(size_t buf_s return std::make_pair(nullptr, chooseNewStat()); } - // We need to assume that this insert will reduce max_cap. - // Because other threads may also be waiting for BlobStats to chooseStat during this time. - // If max_cap is not reduced, it may cause the same BlobStat to accept multiple buffers and exceed its max_cap. - // After the BlobStore records the buffer size, max_caps will also get an accurate update. - // So there won't get problem in reducing max_caps here. - stat_ptr->sm_max_caps -= buf_size; return std::make_pair(stat_ptr, INVALID_BLOBFILE_ID); } From 2c2e18c86601a194c85a6865b96c614eef9558d1 Mon Sep 17 00:00:00 2001 From: jiaqizho Date: Fri, 21 Jan 2022 11:15:02 +0800 Subject: [PATCH 9/9] update --- dbms/src/Storages/Page/V3/BlobStore.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/src/Storages/Page/V3/BlobStore.cpp b/dbms/src/Storages/Page/V3/BlobStore.cpp index b108de9a821..509f5281913 100644 --- a/dbms/src/Storages/Page/V3/BlobStore.cpp +++ b/dbms/src/Storages/Page/V3/BlobStore.cpp @@ -208,7 +208,7 @@ std::pair BlobStore::getPosFromStats(size_t size) // If max_cap is not reduced, it may cause the same BlobStat to accept multiple buffers and exceed its max_cap. // After the BlobStore records the buffer size, max_caps will also get an accurate update. // So there won't get problem in reducing max_caps here. - stat_ptr->sm_max_caps -= buf_size; + stat->sm_max_caps -= size; // We must get the lock from BlobStat under the BlobStats lock // to ensure that BlobStat updates are serialized.