From b51f4ff3c9089d4fa5a06dd9afae63d122da74fb Mon Sep 17 00:00:00 2001 From: lidezhu <47731263+lidezhu@users.noreply.github.com> Date: Mon, 31 May 2021 12:09:30 +0800 Subject: [PATCH 1/7] try fix index bug --- dbms/src/Storages/DeltaMerge/DeltaIndex.h | 27 +++++++++++++---------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/dbms/src/Storages/DeltaMerge/DeltaIndex.h b/dbms/src/Storages/DeltaMerge/DeltaIndex.h index f2e84182a60..18ad115f9fa 100644 --- a/dbms/src/Storages/DeltaMerge/DeltaIndex.h +++ b/dbms/src/Storages/DeltaMerge/DeltaIndex.h @@ -156,11 +156,12 @@ class DeltaIndex { std::scoped_lock lock(mutex); - if (placed_deletes > deletes) - return std::make_shared(); + // clone it + if (placed_deletes <= deletes) + return std::make_shared(*this); } - // Otherwise, clone it. - return std::make_shared(*this); + // Otherwise, create a new DeltaIndex. + return std::make_shared(); } DeltaIndexPtr cloneWithUpdates(const Updates & updates) @@ -170,15 +171,17 @@ class DeltaIndex { std::scoped_lock lock(mutex); - // If inserts shuffled before delete range, the old index cannot used any more. - if (placed_deletes > updates.front().delete_ranges_offset) - return std::make_shared(); + // Otherwise clone a new index, and do some updates. + if (placed_deletes <= updates.front().delete_ranges_offset) + { + auto new_index = std::make_shared(*this); + new_index->applyUpdates(updates); + return new_index; + } } - - // Otherwise clone a new index, and do some updates. - auto new_index = std::make_shared(*this); - new_index->applyUpdates(updates); - return new_index; + + // If inserts shuffled before delete range, the old index cannot used any more. + return std::make_shared(); } }; From 1d372dbf5c948050bef1e202f81025a5a78fcd65 Mon Sep 17 00:00:00 2001 From: lidezhu <47731263+lidezhu@users.noreply.github.com> Date: Mon, 31 May 2021 14:41:33 +0800 Subject: [PATCH 2/7] refine comment --- dbms/src/Storages/DeltaMerge/DeltaIndex.h | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/dbms/src/Storages/DeltaMerge/DeltaIndex.h b/dbms/src/Storages/DeltaMerge/DeltaIndex.h index 18ad115f9fa..0c846c20d2d 100644 --- a/dbms/src/Storages/DeltaMerge/DeltaIndex.h +++ b/dbms/src/Storages/DeltaMerge/DeltaIndex.h @@ -152,15 +152,16 @@ class DeltaIndex DeltaIndexPtr tryClone(size_t /*rows*/, size_t deletes) { - // Delete ranges can break MVCC view. + // Make sure `placed_deletes` is smaller than the required `deletes`, + // Because delete ranges can break MVCC view. { std::scoped_lock lock(mutex); - // clone it + // Safe to reuse the copy of the existing DeltaIndex if (placed_deletes <= deletes) return std::make_shared(*this); } - // Otherwise, create a new DeltaIndex. + // Otherwise, create an empty new DeltaIndex. return std::make_shared(); } @@ -169,18 +170,21 @@ class DeltaIndex if (unlikely(updates.empty())) throw Exception("Unexpected empty updates"); + // Make sure the delta index has only placed deletes in front of the `updates` { std::scoped_lock lock(mutex); - // Otherwise clone a new index, and do some updates. + // Safe to reuse the copy of the existing DeltaIndex if (placed_deletes <= updates.front().delete_ranges_offset) { auto new_index = std::make_shared(*this); + // try to do some updates before return it new_index->applyUpdates(updates); return new_index; } } - - // If inserts shuffled before delete range, the old index cannot used any more. + + // Otherwise, the `updates` shuffled before placed delete range, + // so we need create an empty new DeltaIndex. return std::make_shared(); } }; From 4958e7533b06461e77cd5ea514d6f4822f1a48da Mon Sep 17 00:00:00 2001 From: lidezhu <47731263+lidezhu@users.noreply.github.com> Date: Mon, 31 May 2021 14:47:14 +0800 Subject: [PATCH 3/7] refine some log --- dbms/src/Storages/DeltaMerge/Delta/Snapshot.cpp | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/dbms/src/Storages/DeltaMerge/Delta/Snapshot.cpp b/dbms/src/Storages/DeltaMerge/Delta/Snapshot.cpp index fbc6bb3857b..1c24645423b 100644 --- a/dbms/src/Storages/DeltaMerge/Delta/Snapshot.cpp +++ b/dbms/src/Storages/DeltaMerge/Delta/Snapshot.cpp @@ -25,7 +25,10 @@ std::pair findPack(const DeltaPacks & packs, size_t rows_offset, if (deletes_count == deletes_offset) { if (unlikely(rows_count != rows_offset)) - throw Exception("deletes_offset and rows_offset are not matched"); + throw Exception("rows_count and rows_offset are expected to be equal. pack_index: " + DB::toString(pack_index) + + ", pack_size: " + DB::toString(packs.size()) + ", rows_count: " + DB::toString(rows_count) + + ", rows_offset: " + DB::toString(rows_offset) + ", deletes_count: " + DB::toString(deletes_count) + + ", deletes_offset: " + DB::toString(deletes_offset)); return {pack_index, 0}; } ++deletes_count; @@ -36,14 +39,19 @@ std::pair findPack(const DeltaPacks & packs, size_t rows_offset, if (rows_count > rows_offset) { if (unlikely(deletes_count != deletes_offset)) - throw Exception("deletes_offset and rows_offset are not matched"); + throw Exception("deletes_count and deletes_offset are expected to be equal. pack_index: " + DB::toString(pack_index) + + ", pack_size: " + DB::toString(packs.size()) + ", rows_count: " + DB::toString(rows_count) + + ", rows_offset: " + DB::toString(rows_offset) + ", deletes_count: " + DB::toString(deletes_count) + + ", deletes_offset: " + DB::toString(deletes_offset)); return {pack_index, pack->getRows() - (rows_count - rows_offset)}; } } } if (rows_count != rows_offset || deletes_count != deletes_offset) - throw Exception("illegal rows_offset(" + DB::toString(rows_offset) + "), deletes_count(" + DB::toString(deletes_count) + ")"); + throw Exception("illegal rows_offset and deletes_offset. pack_size: " + DB::toString(packs.size()) + + ", rows_count: " + DB::toString(rows_count) + ", rows_offset: " + DB::toString(rows_offset) + + ", deletes_count: " + DB::toString(deletes_count) + ", deletes_offset: " + DB::toString(deletes_offset)); return {pack_index, 0}; } From b411a7175af71eb5cfbe24684dac1b681648c773 Mon Sep 17 00:00:00 2001 From: lidezhu <47731263+lidezhu@users.noreply.github.com> Date: Mon, 31 May 2021 16:24:10 +0800 Subject: [PATCH 4/7] small fix --- dbms/src/Storages/DeltaMerge/DeltaIndex.h | 63 ++++++++++++++--------- 1 file changed, 40 insertions(+), 23 deletions(-) diff --git a/dbms/src/Storages/DeltaMerge/DeltaIndex.h b/dbms/src/Storages/DeltaMerge/DeltaIndex.h index 0c846c20d2d..848a0b573d6 100644 --- a/dbms/src/Storages/DeltaMerge/DeltaIndex.h +++ b/dbms/src/Storages/DeltaMerge/DeltaIndex.h @@ -72,19 +72,6 @@ class DeltaIndex public: DeltaIndex() : id(++NEXT_DELTA_INDEX_ID), delta_tree(std::make_shared()), placed_rows(0), placed_deletes(0) {} - DeltaIndex(const DeltaIndex & o) : id(++NEXT_DELTA_INDEX_ID) - { - DeltaTreePtr delta_tree_copy; - { - std::scoped_lock lock(o.mutex); - delta_tree_copy = o.delta_tree; - placed_rows = o.placed_rows; - placed_deletes = o.placed_deletes; - } - delta_tree = std::make_shared(*delta_tree_copy); - } - - // For test cases. DeltaIndex(const DeltaTreePtr & delta_tree_, size_t placed_rows_, size_t placed_deletes_) : id(++NEXT_DELTA_INDEX_ID), delta_tree(delta_tree_), placed_rows(placed_rows_), placed_deletes(placed_deletes_) { @@ -152,6 +139,10 @@ class DeltaIndex DeltaIndexPtr tryClone(size_t /*rows*/, size_t deletes) { + bool safe_to_copy = false; + DeltaTreePtr delta_tree_copy; + size_t placed_rows_copy; + size_t placed_deletes_copy; // Make sure `placed_deletes` is smaller than the required `deletes`, // Because delete ranges can break MVCC view. { @@ -159,10 +150,24 @@ class DeltaIndex // Safe to reuse the copy of the existing DeltaIndex if (placed_deletes <= deletes) - return std::make_shared(*this); + { + safe_to_copy = true; + delta_tree_copy = delta_tree; + placed_rows_copy = placed_rows; + placed_deletes_copy = placed_deletes; + } + } + + if (safe_to_copy) + { + auto new_delta_tree = std::make_shared(*delta_tree_copy); + return std::make_shared(new_delta_tree, placed_rows_copy, placed_deletes_copy); + } + else + { + // Otherwise, create an empty new DeltaIndex. + return std::make_shared(); } - // Otherwise, create an empty new DeltaIndex. - return std::make_shared(); } DeltaIndexPtr cloneWithUpdates(const Updates & updates) @@ -170,22 +175,34 @@ class DeltaIndex if (unlikely(updates.empty())) throw Exception("Unexpected empty updates"); + bool safe_to_copy = false; + DeltaTreePtr delta_tree_copy; + size_t placed_rows_copy; + size_t placed_deletes_copy; // Make sure the delta index has only placed deletes in front of the `updates` { std::scoped_lock lock(mutex); // Safe to reuse the copy of the existing DeltaIndex if (placed_deletes <= updates.front().delete_ranges_offset) { - auto new_index = std::make_shared(*this); - // try to do some updates before return it - new_index->applyUpdates(updates); - return new_index; + safe_to_copy = true; + delta_tree_copy = delta_tree; + placed_rows_copy = placed_rows; + placed_deletes_copy = placed_deletes; } } - // Otherwise, the `updates` shuffled before placed delete range, - // so we need create an empty new DeltaIndex. - return std::make_shared(); + if (safe_to_copy) + { + auto new_delta_tree = std::make_shared(*delta_tree_copy); + return std::make_shared(new_delta_tree, placed_rows_copy, placed_deletes_copy); + } + else + { + // Otherwise, the `updates` shuffled before placed delete range, + // so we need create an empty new DeltaIndex. + return std::make_shared(); + } } }; From c3f45f672ed740de01a91cd1e70e8ad8a4ec69bd Mon Sep 17 00:00:00 2001 From: lidezhu <47731263+lidezhu@users.noreply.github.com> Date: Mon, 31 May 2021 16:27:38 +0800 Subject: [PATCH 5/7] small fix --- dbms/src/Storages/DeltaMerge/DeltaIndex.h | 29 +++++++++++++---------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/dbms/src/Storages/DeltaMerge/DeltaIndex.h b/dbms/src/Storages/DeltaMerge/DeltaIndex.h index 848a0b573d6..f26ab5f4447 100644 --- a/dbms/src/Storages/DeltaMerge/DeltaIndex.h +++ b/dbms/src/Storages/DeltaMerge/DeltaIndex.h @@ -139,10 +139,10 @@ class DeltaIndex DeltaIndexPtr tryClone(size_t /*rows*/, size_t deletes) { - bool safe_to_copy = false; + bool safe_to_copy = false; DeltaTreePtr delta_tree_copy; - size_t placed_rows_copy; - size_t placed_deletes_copy; + size_t placed_rows_copy; + size_t placed_deletes_copy; // Make sure `placed_deletes` is smaller than the required `deletes`, // Because delete ranges can break MVCC view. { @@ -151,10 +151,10 @@ class DeltaIndex // Safe to reuse the copy of the existing DeltaIndex if (placed_deletes <= deletes) { - safe_to_copy = true; - delta_tree_copy = delta_tree; + safe_to_copy = true; + delta_tree_copy = delta_tree; placed_rows_copy = placed_rows; - placed_deletes_copy = placed_deletes; + placed_deletes_copy = placed_deletes; } } @@ -175,27 +175,30 @@ class DeltaIndex if (unlikely(updates.empty())) throw Exception("Unexpected empty updates"); - bool safe_to_copy = false; + bool safe_to_copy = false; DeltaTreePtr delta_tree_copy; - size_t placed_rows_copy; - size_t placed_deletes_copy; + size_t placed_rows_copy; + size_t placed_deletes_copy; // Make sure the delta index has only placed deletes in front of the `updates` { std::scoped_lock lock(mutex); // Safe to reuse the copy of the existing DeltaIndex if (placed_deletes <= updates.front().delete_ranges_offset) { - safe_to_copy = true; - delta_tree_copy = delta_tree; + safe_to_copy = true; + delta_tree_copy = delta_tree; placed_rows_copy = placed_rows; - placed_deletes_copy = placed_deletes; + placed_deletes_copy = placed_deletes; } } if (safe_to_copy) { auto new_delta_tree = std::make_shared(*delta_tree_copy); - return std::make_shared(new_delta_tree, placed_rows_copy, placed_deletes_copy); + auto new_index = std::make_shared(new_delta_tree, placed_rows_copy, placed_deletes_copy); + // try to do some updates before return it + new_index->applyUpdates(updates); + return new_index; } else { From 4af186d89b1e8148d922221706cbcf7fc7507208 Mon Sep 17 00:00:00 2001 From: lidezhu <47731263+lidezhu@users.noreply.github.com> Date: Mon, 31 May 2021 17:19:10 +0800 Subject: [PATCH 6/7] fix comment --- dbms/src/Storages/DeltaMerge/DeltaIndex.h | 96 +++++++++-------------- 1 file changed, 36 insertions(+), 60 deletions(-) diff --git a/dbms/src/Storages/DeltaMerge/DeltaIndex.h b/dbms/src/Storages/DeltaMerge/DeltaIndex.h index f26ab5f4447..4fa7ed92169 100644 --- a/dbms/src/Storages/DeltaMerge/DeltaIndex.h +++ b/dbms/src/Storages/DeltaMerge/DeltaIndex.h @@ -69,6 +69,40 @@ class DeltaIndex } } + DeltaIndexPtr tryCloneInner(size_t placed_deletes_limit, const Updates * updates = nullptr) + { + DeltaTreePtr delta_tree_copy; + size_t placed_rows_copy; + size_t placed_deletes_copy; + // Make sure the delta index do not place more deletes than `placed_deletes_limit`. + // Because delete ranges can break MVCC view. + { + std::scoped_lock lock(mutex); + // Safe to reuse the copy of the existing DeltaIndex + if (placed_deletes <= placed_deletes_limit) + { + delta_tree_copy = delta_tree; + placed_rows_copy = placed_rows; + placed_deletes_copy = placed_deletes; + } + } + + if (delta_tree_copy) + { + auto new_delta_tree = std::make_shared(*delta_tree_copy); + auto new_index = std::make_shared(new_delta_tree, placed_rows_copy, placed_deletes_copy); + // try to do some updates before return it if need + if (updates) + new_index->applyUpdates(*updates); + return new_index; + } + else + { + // Otherwise, create an empty new DeltaIndex. + return std::make_shared(); + } + } + public: DeltaIndex() : id(++NEXT_DELTA_INDEX_ID), delta_tree(std::make_shared()), placed_rows(0), placed_deletes(0) {} @@ -139,35 +173,7 @@ class DeltaIndex DeltaIndexPtr tryClone(size_t /*rows*/, size_t deletes) { - bool safe_to_copy = false; - DeltaTreePtr delta_tree_copy; - size_t placed_rows_copy; - size_t placed_deletes_copy; - // Make sure `placed_deletes` is smaller than the required `deletes`, - // Because delete ranges can break MVCC view. - { - std::scoped_lock lock(mutex); - - // Safe to reuse the copy of the existing DeltaIndex - if (placed_deletes <= deletes) - { - safe_to_copy = true; - delta_tree_copy = delta_tree; - placed_rows_copy = placed_rows; - placed_deletes_copy = placed_deletes; - } - } - - if (safe_to_copy) - { - auto new_delta_tree = std::make_shared(*delta_tree_copy); - return std::make_shared(new_delta_tree, placed_rows_copy, placed_deletes_copy); - } - else - { - // Otherwise, create an empty new DeltaIndex. - return std::make_shared(); - } + return tryCloneInner(deletes); } DeltaIndexPtr cloneWithUpdates(const Updates & updates) @@ -175,37 +181,7 @@ class DeltaIndex if (unlikely(updates.empty())) throw Exception("Unexpected empty updates"); - bool safe_to_copy = false; - DeltaTreePtr delta_tree_copy; - size_t placed_rows_copy; - size_t placed_deletes_copy; - // Make sure the delta index has only placed deletes in front of the `updates` - { - std::scoped_lock lock(mutex); - // Safe to reuse the copy of the existing DeltaIndex - if (placed_deletes <= updates.front().delete_ranges_offset) - { - safe_to_copy = true; - delta_tree_copy = delta_tree; - placed_rows_copy = placed_rows; - placed_deletes_copy = placed_deletes; - } - } - - if (safe_to_copy) - { - auto new_delta_tree = std::make_shared(*delta_tree_copy); - auto new_index = std::make_shared(new_delta_tree, placed_rows_copy, placed_deletes_copy); - // try to do some updates before return it - new_index->applyUpdates(updates); - return new_index; - } - else - { - // Otherwise, the `updates` shuffled before placed delete range, - // so we need create an empty new DeltaIndex. - return std::make_shared(); - } + return tryCloneInner(updates.front().delete_ranges_offset, &updates); } }; From c68ac4d8988e9c2bbe23b02ff37b303b5d575c61 Mon Sep 17 00:00:00 2001 From: lidezhu <47731263+lidezhu@users.noreply.github.com> Date: Mon, 31 May 2021 17:23:42 +0800 Subject: [PATCH 7/7] format code --- dbms/src/Storages/DeltaMerge/DeltaIndex.h | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/dbms/src/Storages/DeltaMerge/DeltaIndex.h b/dbms/src/Storages/DeltaMerge/DeltaIndex.h index 4fa7ed92169..94b5806d896 100644 --- a/dbms/src/Storages/DeltaMerge/DeltaIndex.h +++ b/dbms/src/Storages/DeltaMerge/DeltaIndex.h @@ -171,10 +171,7 @@ class DeltaIndex return false; } - DeltaIndexPtr tryClone(size_t /*rows*/, size_t deletes) - { - return tryCloneInner(deletes); - } + DeltaIndexPtr tryClone(size_t /*rows*/, size_t deletes) { return tryCloneInner(deletes); } DeltaIndexPtr cloneWithUpdates(const Updates & updates) {