From 7819b22053fb1d73a2ad6b658e0b37f6c20bd7a7 Mon Sep 17 00:00:00 2001 From: Calvin Neo Date: Mon, 12 Aug 2024 11:08:21 +0800 Subject: [PATCH 01/10] a Signed-off-by: Calvin Neo --- dbms/src/Storages/Page/V3/PageDirectory.cpp | 75 ++++++++++++++------- 1 file changed, 51 insertions(+), 24 deletions(-) diff --git a/dbms/src/Storages/Page/V3/PageDirectory.cpp b/dbms/src/Storages/Page/V3/PageDirectory.cpp index 45a964e0730..9878f9c3542 100644 --- a/dbms/src/Storages/Page/V3/PageDirectory.cpp +++ b/dbms/src/Storages/Page/V3/PageDirectory.cpp @@ -337,7 +337,21 @@ bool VersionedPageEntries::updateLocalCacheForRemotePage(const PageVersio if (type == EditRecordType::VAR_ENTRY) { auto last_iter = MapUtils::findMutLess(entries, PageVersion(ver.sequence + 1, 0)); - RUNTIME_CHECK_MSG(last_iter != entries.end() && last_iter->second.isEntry(), "{}", toDebugString()); + if unlikely (last_iter != entries.end() && last_iter->second.isEntry()) + { + FmtBuffer buf; + for (const auto & e : entries) + { + buf.fmtAppend("{}|", e); + } + throw Exception( + ErrorCodes::LOGICAL_ERROR, + "this={}, entries={}, ver={}, entry={}", + toDebugString(), + buf.toString(), + ver, + entry); + } auto & ori_entry = last_iter->second.entry.value(); RUNTIME_CHECK_MSG(ori_entry.checkpoint_info.has_value(), "{}", toDebugString()); if (!ori_entry.checkpoint_info.is_local_data_reclaimed) @@ -1797,35 +1811,48 @@ typename PageDirectory::PageEntries PageDirectory::updateLocalCach for (const auto & r : edit.getRecords()) { - auto id_to_resolve = r.page_id; - auto sequence_to_resolve = seq; - while (true) + try { - auto iter = mvcc_table_directory.lower_bound(id_to_resolve); - assert(iter != mvcc_table_directory.end()); - auto & version_list = iter->second; - auto [resolve_state, next_id_to_resolve, next_ver_to_resolve] = version_list->resolveToPageId( - sequence_to_resolve, - /*ignore_delete=*/id_to_resolve != r.page_id, - nullptr); - if (resolve_state == ResolveResult::TO_NORMAL) + auto id_to_resolve = r.page_id; + auto sequence_to_resolve = seq; + while (true) { - if (!version_list->updateLocalCacheForRemotePage(PageVersion(sequence_to_resolve, 0), r.entry)) + auto iter = mvcc_table_directory.lower_bound(id_to_resolve); + assert(iter != mvcc_table_directory.end()); + auto & version_list = iter->second; + auto [resolve_state, next_id_to_resolve, next_ver_to_resolve] = version_list->resolveToPageId( + sequence_to_resolve, + /*ignore_delete=*/id_to_resolve != r.page_id, + nullptr); + if (resolve_state == ResolveResult::TO_NORMAL) { - ignored_entries.push_back(r.entry); + if (!version_list->updateLocalCacheForRemotePage(PageVersion(sequence_to_resolve, 0), r.entry)) + { + ignored_entries.push_back(r.entry); + } + break; + } + else if (resolve_state == ResolveResult::TO_REF) + { + id_to_resolve = next_id_to_resolve; + sequence_to_resolve = next_ver_to_resolve.sequence; + } + else + { + RUNTIME_CHECK(false); } - break; - } - else if (resolve_state == ResolveResult::TO_REF) - { - id_to_resolve = next_id_to_resolve; - sequence_to_resolve = next_ver_to_resolve.sequence; - } - else - { - RUNTIME_CHECK(false); } } + catch (DB::Exception & e) + { + e.addMessage(fmt::format( + " type={}, page_id={}, ver={}, seq={}", + magic_enum::enum_name(r.type), + r.page_id, + r.version, + seq)); + throw e; + } } } return ignored_entries; From 4259a45d50334991c9c74bfbf1ea59e9769cca3c Mon Sep 17 00:00:00 2001 From: Calvin Neo Date: Mon, 12 Aug 2024 12:50:09 +0800 Subject: [PATCH 02/10] f Signed-off-by: Calvin Neo --- dbms/src/Storages/Page/V3/PageDirectory.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/src/Storages/Page/V3/PageDirectory.cpp b/dbms/src/Storages/Page/V3/PageDirectory.cpp index 9878f9c3542..1a9ee0d5308 100644 --- a/dbms/src/Storages/Page/V3/PageDirectory.cpp +++ b/dbms/src/Storages/Page/V3/PageDirectory.cpp @@ -337,7 +337,7 @@ bool VersionedPageEntries::updateLocalCacheForRemotePage(const PageVersio if (type == EditRecordType::VAR_ENTRY) { auto last_iter = MapUtils::findMutLess(entries, PageVersion(ver.sequence + 1, 0)); - if unlikely (last_iter != entries.end() && last_iter->second.isEntry()) + if unlikely (last_iter == entries.end() || !last_iter->second.isEntry()) { FmtBuffer buf; for (const auto & e : entries) From 51e7a02d35eb3d2b8e3d08a90606b2ee3b72536a Mon Sep 17 00:00:00 2001 From: Calvin Neo Date: Mon, 12 Aug 2024 13:10:51 +0800 Subject: [PATCH 03/10] a Signed-off-by: Calvin Neo --- dbms/src/Storages/Page/V3/PageDirectory.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/src/Storages/Page/V3/PageDirectory.cpp b/dbms/src/Storages/Page/V3/PageDirectory.cpp index 1a9ee0d5308..20fd326259c 100644 --- a/dbms/src/Storages/Page/V3/PageDirectory.cpp +++ b/dbms/src/Storages/Page/V3/PageDirectory.cpp @@ -86,7 +86,7 @@ namespace PS::V3 template PageLock VersionedPageEntries::acquireLock() const NO_THREAD_SAFETY_ANALYSIS { - return std::lock_guard(m); + return std::lock_guard{m}; } template From 1ecc5b79cc09e00eccec43214c0f769a8cc8cc3b Mon Sep 17 00:00:00 2001 From: Calvin Neo Date: Mon, 12 Aug 2024 14:47:00 +0800 Subject: [PATCH 04/10] a Signed-off-by: Calvin Neo --- libs/libcommon/cmake/find_jemalloc.cmake | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/libs/libcommon/cmake/find_jemalloc.cmake b/libs/libcommon/cmake/find_jemalloc.cmake index b03bca63b37..a1e19099a5c 100644 --- a/libs/libcommon/cmake/find_jemalloc.cmake +++ b/libs/libcommon/cmake/find_jemalloc.cmake @@ -12,7 +12,15 @@ # See the License for the specific language governing permissions and # limitations under the License. -option (ENABLE_JEMALLOC "Set to TRUE to use jemalloc" ON) +if(${CMAKE_SYSTEM_NAME} MATCHES "Linux") + set(ENABLE_JEMALLOC_DEFAULT 1) +elseif(${CMAKE_SYSTEM_NAME} MATCHES "FreeBSD") + set(ENABLE_JEMALLOC_DEFAULT 0) +elseif(${CMAKE_SYSTEM_NAME} MATCHES "Darwin") + set(ENABLE_JEMALLOC_DEFAULT 0) +endif() + +option (ENABLE_JEMALLOC "Set to TRUE to use jemalloc" ${ENABLE_JEMALLOC_DEFAULT}) # 1. The deadlock mentioned in https://github.com/pingcap/tics/issues/3236 is not related to ENABLE_JEMALLOC_PROF. # 2. It is also expected to be eliminated even if the heap profiling is activated, with a newer version of pprof-rs. # TODO: Enable continuous heap profiling after we make sure statement 2. From e72ba19f7f398d36270c736b5b9c399cf88cb97f Mon Sep 17 00:00:00 2001 From: Calvin Neo Date: Mon, 12 Aug 2024 15:34:00 +0800 Subject: [PATCH 05/10] a Signed-off-by: Calvin Neo --- dbms/src/Storages/Page/V3/PageDirectory.cpp | 23 +++++++-------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/dbms/src/Storages/Page/V3/PageDirectory.cpp b/dbms/src/Storages/Page/V3/PageDirectory.cpp index 20fd326259c..f46ba239da4 100644 --- a/dbms/src/Storages/Page/V3/PageDirectory.cpp +++ b/dbms/src/Storages/Page/V3/PageDirectory.cpp @@ -337,21 +337,14 @@ bool VersionedPageEntries::updateLocalCacheForRemotePage(const PageVersio if (type == EditRecordType::VAR_ENTRY) { auto last_iter = MapUtils::findMutLess(entries, PageVersion(ver.sequence + 1, 0)); - if unlikely (last_iter == entries.end() || !last_iter->second.isEntry()) - { - FmtBuffer buf; - for (const auto & e : entries) - { - buf.fmtAppend("{}|", e); - } - throw Exception( - ErrorCodes::LOGICAL_ERROR, - "this={}, entries={}, ver={}, entry={}", - toDebugString(), - buf.toString(), - ver, - entry); - } + RUNTIME_CHECK_MSG( + last_iter != entries.end() && last_iter->second.isEntry(), + "this={}, entries={}, ver={}, entry={}", + toDebugString(), + entries, + ver, + entry); + auto & ori_entry = last_iter->second.entry.value(); RUNTIME_CHECK_MSG(ori_entry.checkpoint_info.has_value(), "{}", toDebugString()); if (!ori_entry.checkpoint_info.is_local_data_reclaimed) From 808e3bd83acc0764ef7e62e045db30e8f1a30cb1 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Mon, 12 Aug 2024 14:57:28 +0800 Subject: [PATCH 06/10] Add test case about remove --- dbms/src/Storages/Page/V3/PageDirectory.cpp | 9 ++ .../V3/Universal/UniversalPageStorage.cpp | 4 + .../V3/Universal/tests/gtest_remote_read.cpp | 85 +++++++++++++++++++ .../tests/gtest_universal_page_storage.cpp | 4 +- 4 files changed, 100 insertions(+), 2 deletions(-) diff --git a/dbms/src/Storages/Page/V3/PageDirectory.cpp b/dbms/src/Storages/Page/V3/PageDirectory.cpp index f46ba239da4..13f12334ffa 100644 --- a/dbms/src/Storages/Page/V3/PageDirectory.cpp +++ b/dbms/src/Storages/Page/V3/PageDirectory.cpp @@ -347,10 +347,14 @@ bool VersionedPageEntries::updateLocalCacheForRemotePage(const PageVersio auto & ori_entry = last_iter->second.entry.value(); RUNTIME_CHECK_MSG(ori_entry.checkpoint_info.has_value(), "{}", toDebugString()); + + // maybe another thread has in-place update the local blob location, ignored if (!ori_entry.checkpoint_info.is_local_data_reclaimed) { return false; } + + // update the entry in-place ori_entry.file_id = entry.file_id; ori_entry.size = entry.size; ori_entry.offset = entry.offset; @@ -1821,6 +1825,11 @@ typename PageDirectory::PageEntries PageDirectory::updateLocalCach { if (!version_list->updateLocalCacheForRemotePage(PageVersion(sequence_to_resolve, 0), r.entry)) { + // The entry is not valid for updating the version_list. + // Caller shuold notice these part of "ignored_entries" and release + // the space allocated for these invalid entries. + // For the information persisted in WAL, it should be ignored when + // restoring from disk. ignored_entries.push_back(r.entry); } break; diff --git a/dbms/src/Storages/Page/V3/Universal/UniversalPageStorage.cpp b/dbms/src/Storages/Page/V3/Universal/UniversalPageStorage.cpp index 5288bb653e1..a21b2b1e797 100644 --- a/dbms/src/Storages/Page/V3/Universal/UniversalPageStorage.cpp +++ b/dbms/src/Storages/Page/V3/Universal/UniversalPageStorage.cpp @@ -450,10 +450,14 @@ void UniversalPageStorage::unregisterUniversalExternalPagesCallbacks(const Strin void UniversalPageStorage::tryUpdateLocalCacheForRemotePages(UniversalWriteBatch & wb, SnapshotPtr snapshot) const { + // store the downloaded page data into local cache and generate new "edit" auto edit = blob_store->write(std::move(wb)); + // Update the entries to the location of BlobFile. auto ignored_entries = page_directory->updateLocalCacheForRemotePages(std::move(edit), snapshot); if (!ignored_entries.empty()) { + // Some entries are not valid for updating the page_directory. BlobStore should + // release the space for new blob data. blob_store->removeEntries(ignored_entries); } } diff --git a/dbms/src/Storages/Page/V3/Universal/tests/gtest_remote_read.cpp b/dbms/src/Storages/Page/V3/Universal/tests/gtest_remote_read.cpp index 1bf93b01b98..396c76ae255 100644 --- a/dbms/src/Storages/Page/V3/Universal/tests/gtest_remote_read.cpp +++ b/dbms/src/Storages/Page/V3/Universal/tests/gtest_remote_read.cpp @@ -279,6 +279,90 @@ try } CATCH +TEST_P(UniPageStorageRemoteReadTest, WriteReadGCWithRestart) +try +{ + const String test_page_id = "aaabbb"; + /// Prepare data on remote store + auto writer = PS::V3::CPFilesWriter::create({ + .data_file_path_pattern = data_file_path_pattern, + .data_file_id_pattern = data_file_id_pattern, + .manifest_file_path = manifest_file_path, + .manifest_file_id = manifest_file_id, + .data_source = PS::V3::CPWriteDataSourceFixture::create({{10, "nahida opened her eyes"}}), + }); + + writer->writePrefix({ + .writer = {}, + .sequence = 5, + .last_sequence = 3, + }); + { + auto edits = PS::V3::universal::PageEntriesEdit{}; + edits.appendRecord( + {.type = PS::V3::EditRecordType::VAR_ENTRY, .page_id = test_page_id, .entry = {.size = 22, .offset = 10}}); + writer->writeEditsAndApplyCheckpointInfo(edits); + } + auto data_paths = writer->writeSuffix(); + writer.reset(); + for (const auto & data_path : data_paths) + { + uploadFile(data_path); + } + uploadFile(manifest_file_path); + + /// Put remote page into local + auto manifest_file = PosixRandomAccessFile::create(manifest_file_path); + auto manifest_reader = PS::V3::CPManifestFileReader::create({ + .plain_file = manifest_file, + }); + manifest_reader->readPrefix(); + PS::V3::CheckpointProto::StringsInternMap im; + { + auto edits_r = manifest_reader->readEdits(im); + auto r = edits_r->getRecords(); + ASSERT_EQ(1, r.size()); + + UniversalWriteBatch wb; + wb.disableRemoteLock(); + wb.putRemotePage( + r[0].page_id, + 0, + r[0].entry.size, + r[0].entry.checkpoint_info.data_location, + std::move(r[0].entry.field_offsets)); + page_storage->write(std::move(wb)); + } + + // generate snapshot for reading + auto snap0 = page_storage->getSnapshot("read"); + + // Delete the page before reading from snapshot + { + UniversalWriteBatch wb; + wb.disableRemoteLock(); + wb.delPage(test_page_id); + page_storage->write(std::move(wb)); + } + + // read with snapshot + { + auto page = page_storage->read(test_page_id, nullptr, snap0); + ASSERT_TRUE(page.isValid()); + ASSERT_EQ("nahida opened her eyes", String(page.data.begin(), page.data.size())); + } + + // Mock restart + reload(); + + { + // ensure the page is deleted as expected + auto page = page_storage->read(test_page_id, nullptr, nullptr, false); + ASSERT_FALSE(page.isValid()); + } +} +CATCH + TEST_P(UniPageStorageRemoteReadTest, WriteReadWithRef) try { @@ -613,6 +697,7 @@ CATCH INSTANTIATE_TEST_CASE_P( UniPageStorageRemote, UniPageStorageRemoteReadTest, + // testing::Values(std::make_pair(false, false), std::make_pair(true, false), std::make_pair(true, true))); } // namespace DB::PS::universal::tests diff --git a/dbms/src/Storages/Page/V3/Universal/tests/gtest_universal_page_storage.cpp b/dbms/src/Storages/Page/V3/Universal/tests/gtest_universal_page_storage.cpp index 1d9f7c197e2..05e056d7fdb 100644 --- a/dbms/src/Storages/Page/V3/Universal/tests/gtest_universal_page_storage.cpp +++ b/dbms/src/Storages/Page/V3/Universal/tests/gtest_universal_page_storage.cpp @@ -499,7 +499,7 @@ TEST_F(UniPageStorageTest, Scan) { auto start = UniversalPageIdFormat::toFullPageId(region_prefix, 15); - auto end = ""; + const auto * end = ""; size_t count = 0; auto checker = [&](const UniversalPageId & page_id, const DB::Page & page) { UNUSED(page); @@ -555,7 +555,7 @@ TEST(UniPageStorageIdTest, UniversalPageId) } { - auto u_id = "z"; + const auto * u_id = "z"; ASSERT_EQ(UniversalPageIdFormat::getU64ID(u_id), 0); ASSERT_EQ(UniversalPageIdFormat::getFullPrefix(u_id), "z"); } From 2b477fe60bf205c51603c6a2293bbe8616fd2f97 Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Mon, 12 Aug 2024 16:17:33 +0800 Subject: [PATCH 07/10] Try multi read test case --- dbms/src/Common/FailPoint.cpp | 23 ++-- dbms/src/Storages/Page/V3/PageDirectory.cpp | 3 + .../V3/Universal/tests/gtest_remote_read.cpp | 104 ++++++++++++++++++ 3 files changed, 119 insertions(+), 11 deletions(-) diff --git a/dbms/src/Common/FailPoint.cpp b/dbms/src/Common/FailPoint.cpp index 94e76cfea09..1c909b9b36c 100644 --- a/dbms/src/Common/FailPoint.cpp +++ b/dbms/src/Common/FailPoint.cpp @@ -117,17 +117,18 @@ namespace DB M(force_agg_two_level_hash_table_before_merge) \ M(force_thread_0_no_agg_spill) -#define APPLY_FOR_PAUSEABLE_FAILPOINTS_ONCE(M) \ - M(pause_with_alter_locks_acquired) \ - M(hang_in_execution) \ - M(pause_before_dt_background_delta_merge) \ - M(pause_until_dt_background_delta_merge) \ - M(pause_before_apply_raft_cmd) \ - M(pause_before_apply_raft_snapshot) \ - M(pause_until_apply_raft_snapshot) \ - M(pause_after_copr_streams_acquired_once) \ - M(pause_before_register_non_root_mpp_task) \ - M(pause_before_make_non_root_mpp_task_active) +#define APPLY_FOR_PAUSEABLE_FAILPOINTS_ONCE(M) \ + M(pause_with_alter_locks_acquired) \ + M(hang_in_execution) \ + M(pause_before_dt_background_delta_merge) \ + M(pause_until_dt_background_delta_merge) \ + M(pause_before_apply_raft_cmd) \ + M(pause_before_apply_raft_snapshot) \ + M(pause_until_apply_raft_snapshot) \ + M(pause_after_copr_streams_acquired_once) \ + M(pause_before_register_non_root_mpp_task) \ + M(pause_before_make_non_root_mpp_task_active) \ + M(pause_before_page_dir_update_local_cache) #define APPLY_FOR_PAUSEABLE_FAILPOINTS(M) \ M(pause_when_reading_from_dt_stream) \ diff --git a/dbms/src/Storages/Page/V3/PageDirectory.cpp b/dbms/src/Storages/Page/V3/PageDirectory.cpp index 13f12334ffa..2078aae6c7e 100644 --- a/dbms/src/Storages/Page/V3/PageDirectory.cpp +++ b/dbms/src/Storages/Page/V3/PageDirectory.cpp @@ -67,6 +67,7 @@ namespace FailPoints { extern const char random_slow_page_storage_remove_expired_snapshots[]; extern const char pause_before_full_gc_prepare[]; +extern const char pause_before_page_dir_update_local_cache[]; } // namespace FailPoints namespace ErrorCodes @@ -1795,6 +1796,7 @@ typename PageDirectory::PageEntries PageDirectory::updateLocalCach const DB::PageStorageSnapshotPtr & snap_, const WriteLimiterPtr & write_limiter) { + FAIL_POINT_PAUSE(FailPoints::pause_before_page_dir_update_local_cache); std::unique_lock apply_lock(apply_mutex); auto seq = toConcreteSnapshot(snap_)->sequence; for (auto & r : edit.getMutRecords()) @@ -1802,6 +1804,7 @@ typename PageDirectory::PageEntries PageDirectory::updateLocalCach r.version = PageVersion(seq, 0); } wal->apply(Trait::Serializer::serializeTo(edit), write_limiter); + SYNC_FOR("after_PageDirectory::updateLocalCacheForRemotePages_persist_wal"); typename PageDirectory::PageEntries ignored_entries; { std::unique_lock table_lock(table_rw_mutex); diff --git a/dbms/src/Storages/Page/V3/Universal/tests/gtest_remote_read.cpp b/dbms/src/Storages/Page/V3/Universal/tests/gtest_remote_read.cpp index 396c76ae255..836a464776d 100644 --- a/dbms/src/Storages/Page/V3/Universal/tests/gtest_remote_read.cpp +++ b/dbms/src/Storages/Page/V3/Universal/tests/gtest_remote_read.cpp @@ -13,6 +13,7 @@ // limitations under the License. #include +#include #include #include #include @@ -38,6 +39,10 @@ #include +namespace DB::FailPoints +{ +extern const char pause_before_page_dir_update_local_cache[]; +} // namespace DB::FailPoints namespace DB::PS::universal::tests { @@ -363,6 +368,105 @@ try } CATCH +TEST_P(UniPageStorageRemoteReadTest, MultiThreadReadUpdateRmotePage) +try +{ + const String test_page_id = "aaabbb"; + /// Prepare data on remote store + auto writer = PS::V3::CPFilesWriter::create({ + .data_file_path_pattern = data_file_path_pattern, + .data_file_id_pattern = data_file_id_pattern, + .manifest_file_path = manifest_file_path, + .manifest_file_id = manifest_file_id, + .data_source = PS::V3::CPWriteDataSourceFixture::create({{10, "nahida opened her eyes"}}), + }); + + writer->writePrefix({ + .writer = {}, + .sequence = 5, + .last_sequence = 3, + }); + { + auto edits = PS::V3::universal::PageEntriesEdit{}; + edits.appendRecord( + {.type = PS::V3::EditRecordType::VAR_ENTRY, .page_id = test_page_id, .entry = {.size = 22, .offset = 10}}); + writer->writeEditsAndApplyCheckpointInfo(edits); + } + auto data_paths = writer->writeSuffix(); + writer.reset(); + for (const auto & data_path : data_paths) + { + uploadFile(data_path); + } + uploadFile(manifest_file_path); + + /// Put remote page into local + auto manifest_file = PosixRandomAccessFile::create(manifest_file_path); + auto manifest_reader = PS::V3::CPManifestFileReader::create({ + .plain_file = manifest_file, + }); + manifest_reader->readPrefix(); + PS::V3::CheckpointProto::StringsInternMap im; + { + auto edits_r = manifest_reader->readEdits(im); + auto r = edits_r->getRecords(); + ASSERT_EQ(1, r.size()); + + UniversalWriteBatch wb; + wb.disableRemoteLock(); + wb.putRemotePage( + r[0].page_id, + 0, + r[0].entry.size, + r[0].entry.checkpoint_info.data_location, + std::move(r[0].entry.field_offsets)); + page_storage->write(std::move(wb)); + } + + // read with snapshot + // auto sp = SyncPointCtl::enableInScope("after_PageDirectory::updateLocalCacheForRemotePages_persist_wal"); + FAIL_POINT_PAUSE(FailPoints::pause_before_page_dir_update_local_cache); + auto th_read0 = std::async([&]() { + auto snap0 = page_storage->getSnapshot("read0"); + auto page = page_storage->read(test_page_id, nullptr, snap0); + ASSERT_TRUE(page.isValid()); + ASSERT_EQ("nahida opened her eyes", String(page.data.begin(), page.data.size())); + LOG_DEBUG(log, "th_read0 finished"); + }); + auto th_read1 = std::async([&]() { + auto snap1 = page_storage->getSnapshot("read1"); + auto page = page_storage->read(test_page_id, nullptr, snap1); + ASSERT_TRUE(page.isValid()); + ASSERT_EQ("nahida opened her eyes", String(page.data.begin(), page.data.size())); + LOG_DEBUG(log, "th_read1 finished"); + }); + // sp.waitAndPause(); + LOG_DEBUG(log, "concurrent read block before update"); + + FailPointHelper::disableFailPoint(FailPoints::pause_before_page_dir_update_local_cache); + // sp.next(); + th_read0.get(); + // sp.next(); + th_read1.get(); + LOG_DEBUG(log, "there must be one read thread update fail"); + + { + auto page = page_storage->read(test_page_id); + ASSERT_TRUE(page.isValid()); + ASSERT_EQ("nahida opened her eyes", String(page.data.begin(), page.data.size())); + } + + // Mock restart + reload(); + + { + auto page = page_storage->read(test_page_id); + ASSERT_TRUE(page.isValid()); + ASSERT_EQ("nahida opened her eyes", String(page.data.begin(), page.data.size())); + } +} +CATCH + TEST_P(UniPageStorageRemoteReadTest, WriteReadWithRef) try { From 2af2cafbc6acc545f3fdf3e835bb5e00f207b03d Mon Sep 17 00:00:00 2001 From: JaySon-Huang Date: Mon, 12 Aug 2024 16:18:30 +0800 Subject: [PATCH 08/10] cleanup --- dbms/src/Storages/Page/V3/PageDirectory.cpp | 1 - .../Storages/Page/V3/Universal/tests/gtest_remote_read.cpp | 4 ---- 2 files changed, 5 deletions(-) diff --git a/dbms/src/Storages/Page/V3/PageDirectory.cpp b/dbms/src/Storages/Page/V3/PageDirectory.cpp index 2078aae6c7e..a610d55c017 100644 --- a/dbms/src/Storages/Page/V3/PageDirectory.cpp +++ b/dbms/src/Storages/Page/V3/PageDirectory.cpp @@ -1804,7 +1804,6 @@ typename PageDirectory::PageEntries PageDirectory::updateLocalCach r.version = PageVersion(seq, 0); } wal->apply(Trait::Serializer::serializeTo(edit), write_limiter); - SYNC_FOR("after_PageDirectory::updateLocalCacheForRemotePages_persist_wal"); typename PageDirectory::PageEntries ignored_entries; { std::unique_lock table_lock(table_rw_mutex); diff --git a/dbms/src/Storages/Page/V3/Universal/tests/gtest_remote_read.cpp b/dbms/src/Storages/Page/V3/Universal/tests/gtest_remote_read.cpp index 836a464776d..814bd12b1f1 100644 --- a/dbms/src/Storages/Page/V3/Universal/tests/gtest_remote_read.cpp +++ b/dbms/src/Storages/Page/V3/Universal/tests/gtest_remote_read.cpp @@ -424,7 +424,6 @@ try } // read with snapshot - // auto sp = SyncPointCtl::enableInScope("after_PageDirectory::updateLocalCacheForRemotePages_persist_wal"); FAIL_POINT_PAUSE(FailPoints::pause_before_page_dir_update_local_cache); auto th_read0 = std::async([&]() { auto snap0 = page_storage->getSnapshot("read0"); @@ -440,13 +439,10 @@ try ASSERT_EQ("nahida opened her eyes", String(page.data.begin(), page.data.size())); LOG_DEBUG(log, "th_read1 finished"); }); - // sp.waitAndPause(); LOG_DEBUG(log, "concurrent read block before update"); FailPointHelper::disableFailPoint(FailPoints::pause_before_page_dir_update_local_cache); - // sp.next(); th_read0.get(); - // sp.next(); th_read1.get(); LOG_DEBUG(log, "there must be one read thread update fail"); From 84f251fb2012e4c6e87681d1d0e6d7cafb2df121 Mon Sep 17 00:00:00 2001 From: Calvin Neo Date: Mon, 12 Aug 2024 16:25:08 +0800 Subject: [PATCH 09/10] Update libs/libcommon/cmake/find_jemalloc.cmake Co-authored-by: JaySon --- libs/libcommon/cmake/find_jemalloc.cmake | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/libs/libcommon/cmake/find_jemalloc.cmake b/libs/libcommon/cmake/find_jemalloc.cmake index a1e19099a5c..b03bca63b37 100644 --- a/libs/libcommon/cmake/find_jemalloc.cmake +++ b/libs/libcommon/cmake/find_jemalloc.cmake @@ -12,15 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. -if(${CMAKE_SYSTEM_NAME} MATCHES "Linux") - set(ENABLE_JEMALLOC_DEFAULT 1) -elseif(${CMAKE_SYSTEM_NAME} MATCHES "FreeBSD") - set(ENABLE_JEMALLOC_DEFAULT 0) -elseif(${CMAKE_SYSTEM_NAME} MATCHES "Darwin") - set(ENABLE_JEMALLOC_DEFAULT 0) -endif() - -option (ENABLE_JEMALLOC "Set to TRUE to use jemalloc" ${ENABLE_JEMALLOC_DEFAULT}) +option (ENABLE_JEMALLOC "Set to TRUE to use jemalloc" ON) # 1. The deadlock mentioned in https://github.com/pingcap/tics/issues/3236 is not related to ENABLE_JEMALLOC_PROF. # 2. It is also expected to be eliminated even if the heap profiling is activated, with a newer version of pprof-rs. # TODO: Enable continuous heap profiling after we make sure statement 2. From 975e768f0f4a672b9dc0d2f8de1a0aa7fd5317ad Mon Sep 17 00:00:00 2001 From: Calvin Neo Date: Mon, 12 Aug 2024 16:50:14 +0800 Subject: [PATCH 10/10] Update dbms/src/Storages/Page/V3/PageDirectory.cpp --- dbms/src/Storages/Page/V3/PageDirectory.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/src/Storages/Page/V3/PageDirectory.cpp b/dbms/src/Storages/Page/V3/PageDirectory.cpp index a610d55c017..c53dc0f2d8d 100644 --- a/dbms/src/Storages/Page/V3/PageDirectory.cpp +++ b/dbms/src/Storages/Page/V3/PageDirectory.cpp @@ -1828,7 +1828,7 @@ typename PageDirectory::PageEntries PageDirectory::updateLocalCach if (!version_list->updateLocalCacheForRemotePage(PageVersion(sequence_to_resolve, 0), r.entry)) { // The entry is not valid for updating the version_list. - // Caller shuold notice these part of "ignored_entries" and release + // Caller should notice these part of "ignored_entries" and release // the space allocated for these invalid entries. // For the information persisted in WAL, it should be ignored when // restoring from disk.