From dee88fd7a310dcd77f6e27f3a5a66674ee27aa2c Mon Sep 17 00:00:00 2001
From: jbajic <jure.bajic@memgraph.com>
Date: Thu, 26 Jan 2023 14:26:24 +0100
Subject: [PATCH] Fix delta not cloning id
---
src/storage/v3/delta.hpp | 54 ++++++++++++---------------
src/storage/v3/mvcc.hpp | 6 +--
src/storage/v3/splitter.cpp | 9 +++--
src/storage/v3/transaction.hpp | 19 +++++-----
tests/unit/storage_v3_shard_split.cpp | 50 +++++++++++++++++--------
5 files changed, 76 insertions(+), 62 deletions(-)
diff --git a/src/storage/v3/delta.hpp b/src/storage/v3/delta.hpp
index 40bbb521d..c4ffc33e1 100644
--- a/src/storage/v3/delta.hpp
+++ b/src/storage/v3/delta.hpp
@@ -28,6 +28,11 @@ struct Edge;
struct Delta;
struct CommitInfo;
+inline uint64_t GetNextDeltaUUID() noexcept {
+ static uint64_t uuid{0};
+ return ++uuid;
+}
+
// This class stores one of three pointers (`Delta`, `Vertex` and `Edge`)
// without using additional memory for storing the type. The type is stored in
// the pointer itself in the lower bits. All of those structures contain large
@@ -130,11 +135,6 @@ inline bool operator==(const PreviousPtr::Pointer &a, const PreviousPtr::Pointer
inline bool operator!=(const PreviousPtr::Pointer &a, const PreviousPtr::Pointer &b) { return !(a == b); }
-inline uint64_t GetNextDeltaUUID() noexcept {
- static uint64_t uuid{0};
- return ++uuid;
-}
-
struct Delta {
enum class Action : uint8_t {
// Used for both Vertex and Edge
@@ -164,62 +164,54 @@ struct Delta {
struct RemoveInEdgeTag {};
struct RemoveOutEdgeTag {};
- Delta(DeleteObjectTag /*unused*/, CommitInfo *commit_info, uint64_t command_id)
- : action(Action::DELETE_OBJECT), uuid(GetNextDeltaUUID()), commit_info(commit_info), command_id(command_id) {}
+ Delta(DeleteObjectTag /*unused*/, CommitInfo *commit_info, uint64_t delta_id, uint64_t command_id)
+ : action(Action::DELETE_OBJECT), uuid(delta_id), commit_info(commit_info), command_id(command_id) {}
- Delta(RecreateObjectTag /*unused*/, CommitInfo *commit_info, uint64_t command_id)
- : action(Action::RECREATE_OBJECT), uuid(GetNextDeltaUUID()), commit_info(commit_info), command_id(command_id) {}
+ Delta(RecreateObjectTag /*unused*/, CommitInfo *commit_info, uint64_t delta_id, uint64_t command_id)
+ : action(Action::RECREATE_OBJECT), uuid(delta_id), commit_info(commit_info), command_id(command_id) {}
- Delta(AddLabelTag /*unused*/, LabelId label, CommitInfo *commit_info, uint64_t command_id)
- : action(Action::ADD_LABEL),
- uuid(GetNextDeltaUUID()),
- commit_info(commit_info),
- command_id(command_id),
- label(label) {}
+ Delta(AddLabelTag /*unused*/, LabelId label, CommitInfo *commit_info, uint64_t delta_id, uint64_t command_id)
+ : action(Action::ADD_LABEL), uuid(delta_id), commit_info(commit_info), command_id(command_id), label(label) {}
- Delta(RemoveLabelTag /*unused*/, LabelId label, CommitInfo *commit_info, uint64_t command_id)
- : action(Action::REMOVE_LABEL),
- uuid(GetNextDeltaUUID()),
- commit_info(commit_info),
- command_id(command_id),
- label(label) {}
+ Delta(RemoveLabelTag /*unused*/, LabelId label, CommitInfo *commit_info, uint64_t delta_id, uint64_t command_id)
+ : action(Action::REMOVE_LABEL), uuid(delta_id), commit_info(commit_info), command_id(command_id), label(label) {}
Delta(SetPropertyTag /*unused*/, PropertyId key, const PropertyValue &value, CommitInfo *commit_info,
- uint64_t command_id)
+ uint64_t delta_id, uint64_t command_id)
: action(Action::SET_PROPERTY),
- uuid(GetNextDeltaUUID()),
+ uuid(delta_id),
commit_info(commit_info),
command_id(command_id),
property({key, value}) {}
Delta(AddInEdgeTag /*unused*/, EdgeTypeId edge_type, VertexId vertex_id, EdgeRef edge, CommitInfo *commit_info,
- uint64_t command_id)
+ uint64_t delta_id, uint64_t command_id)
: action(Action::ADD_IN_EDGE),
- uuid(GetNextDeltaUUID()),
+ uuid(delta_id),
commit_info(commit_info),
command_id(command_id),
vertex_edge({edge_type, std::move(vertex_id), edge}) {}
Delta(AddOutEdgeTag /*unused*/, EdgeTypeId edge_type, VertexId vertex_id, EdgeRef edge, CommitInfo *commit_info,
- uint64_t command_id)
+ uint64_t delta_id, uint64_t command_id)
: action(Action::ADD_OUT_EDGE),
- uuid(GetNextDeltaUUID()),
+ uuid(delta_id),
commit_info(commit_info),
command_id(command_id),
vertex_edge({edge_type, std::move(vertex_id), edge}) {}
Delta(RemoveInEdgeTag /*unused*/, EdgeTypeId edge_type, VertexId vertex_id, EdgeRef edge, CommitInfo *commit_info,
- uint64_t command_id)
+ uint64_t delta_id, uint64_t command_id)
: action(Action::REMOVE_IN_EDGE),
- uuid(GetNextDeltaUUID()),
+ uuid(delta_id),
commit_info(commit_info),
command_id(command_id),
vertex_edge({edge_type, std::move(vertex_id), edge}) {}
Delta(RemoveOutEdgeTag /*unused*/, EdgeTypeId edge_type, VertexId vertex_id, EdgeRef edge, CommitInfo *commit_info,
- uint64_t command_id)
+ uint64_t delta_id, uint64_t command_id)
: action(Action::REMOVE_OUT_EDGE),
- uuid(GetNextDeltaUUID()),
+ uuid(delta_id),
commit_info(commit_info),
command_id(command_id),
vertex_edge({edge_type, std::move(vertex_id), edge}) {}
diff --git a/src/storage/v3/mvcc.hpp b/src/storage/v3/mvcc.hpp
index 6ce058d62..f4e4cb81e 100644
--- a/src/storage/v3/mvcc.hpp
+++ b/src/storage/v3/mvcc.hpp
@@ -1,4 +1,4 @@
-// Copyright 2022 Memgraph Ltd.
+// Copyright 2023 Memgraph Ltd.
//
// Use of this software is governed by the Business Source License
// included in the file licenses/BSL.txt; by using this file, you agree to be bound by the terms of the Business Source
@@ -108,7 +108,7 @@ inline bool PrepareForWrite(Transaction *transaction, TObj *object) {
/// a `DELETE_OBJECT` delta).
/// @throw std::bad_alloc
inline Delta *CreateDeleteObjectDelta(Transaction *transaction) {
- return &transaction->deltas.emplace_back(Delta::DeleteObjectTag(), transaction->commit_info.get(),
+ return &transaction->deltas.emplace_back(Delta::DeleteObjectTag(), transaction->commit_info.get(), GetNextDeltaUUID(),
transaction->command_id);
}
@@ -119,7 +119,7 @@ template <typename TObj, class... Args>
requires utils::SameAsAnyOf<TObj, Edge, Vertex>
inline void CreateAndLinkDelta(Transaction *transaction, TObj *object, Args &&...args) {
auto delta = &transaction->deltas.emplace_back(std::forward<Args>(args)..., transaction->commit_info.get(),
- transaction->command_id);
+ GetNextDeltaUUID(), transaction->command_id);
auto *delta_holder = GetDeltaHolder(object);
// The operations are written in such order so that both `next` and `prev`
diff --git a/src/storage/v3/splitter.cpp b/src/storage/v3/splitter.cpp
index c57de0f6e..375d3aa60 100644
--- a/src/storage/v3/splitter.cpp
+++ b/src/storage/v3/splitter.cpp
@@ -181,16 +181,19 @@ void Splitter::AdjustClonedTransaction(Transaction &cloned_transaction, const Tr
// NOTE It is important that the order of delta lists is in same order
auto delta_it = transaction.deltas.begin();
auto cloned_delta_it = cloned_transaction.deltas.begin();
- while (delta_it != transaction.deltas.end() && cloned_delta_it != cloned_transaction.deltas.end()) {
+ while (delta_it != transaction.deltas.end()) {
const auto *delta = &*delta_it;
auto *cloned_delta = &*cloned_delta_it;
while (delta != nullptr) {
// Align deltas which belong to cloned transaction, skip others
if (cloned_transactions.contains(delta->commit_info->start_or_commit_timestamp.logical_id)) {
- auto *found_delta_it = &*std::ranges::find_if(
+ const auto end_it =
+ cloned_transactions.at(delta->commit_info->start_or_commit_timestamp.logical_id)->deltas.end();
+ auto found_delta_it = std::ranges::find_if(
cloned_transactions.at(delta->commit_info->start_or_commit_timestamp.logical_id)->deltas,
[delta](const auto &elem) { return elem.uuid == delta->uuid; });
- MG_ASSERT(found_delta_it, "Delta with given uuid must exist!");
+ MG_ASSERT(found_delta_it != end_it, "Delta with given uuid must exist!");
+
cloned_delta->next = &*found_delta_it;
} else {
delta = delta->next;
diff --git a/src/storage/v3/transaction.hpp b/src/storage/v3/transaction.hpp
index 9b13d9c7b..66269e935 100644
--- a/src/storage/v3/transaction.hpp
+++ b/src/storage/v3/transaction.hpp
@@ -69,36 +69,37 @@ struct Transaction {
for (const auto &delta : deltas) {
switch (delta.action) {
case Delta::Action::DELETE_OBJECT:
- copied_deltas.emplace_back(Delta::DeleteObjectTag{}, commit_info, command_id);
+ copied_deltas.emplace_back(Delta::DeleteObjectTag{}, commit_info, delta.uuid, command_id);
break;
case Delta::Action::RECREATE_OBJECT:
- copied_deltas.emplace_back(Delta::RecreateObjectTag{}, commit_info, command_id);
+ copied_deltas.emplace_back(Delta::RecreateObjectTag{}, commit_info, delta.uuid, command_id);
break;
case Delta::Action::ADD_LABEL:
- copied_deltas.emplace_back(Delta::AddLabelTag{}, delta.label, commit_info, command_id);
+ copied_deltas.emplace_back(Delta::AddLabelTag{}, delta.label, commit_info, delta.uuid, command_id);
break;
case Delta::Action::REMOVE_LABEL:
- copied_deltas.emplace_back(Delta::RemoveLabelTag{}, delta.label, commit_info, command_id);
+ copied_deltas.emplace_back(Delta::RemoveLabelTag{}, delta.label, commit_info, delta.uuid, command_id);
break;
case Delta::Action::ADD_IN_EDGE:
copied_deltas.emplace_back(Delta::AddInEdgeTag{}, delta.vertex_edge.edge_type, delta.vertex_edge.vertex_id,
- delta.vertex_edge.edge, commit_info, command_id);
+ delta.vertex_edge.edge, commit_info, delta.uuid, command_id);
break;
case Delta::Action::ADD_OUT_EDGE:
copied_deltas.emplace_back(Delta::AddOutEdgeTag{}, delta.vertex_edge.edge_type, delta.vertex_edge.vertex_id,
- delta.vertex_edge.edge, commit_info, command_id);
+ delta.vertex_edge.edge, commit_info, delta.uuid, command_id);
break;
case Delta::Action::REMOVE_IN_EDGE:
copied_deltas.emplace_back(Delta::RemoveInEdgeTag{}, delta.vertex_edge.edge_type, delta.vertex_edge.vertex_id,
- delta.vertex_edge.edge, commit_info, command_id);
+ delta.vertex_edge.edge, commit_info, delta.uuid, command_id);
break;
case Delta::Action::REMOVE_OUT_EDGE:
copied_deltas.emplace_back(Delta::RemoveOutEdgeTag{}, delta.vertex_edge.edge_type,
- delta.vertex_edge.vertex_id, delta.vertex_edge.edge, commit_info, command_id);
+ delta.vertex_edge.vertex_id, delta.vertex_edge.edge, commit_info, delta.uuid,
+ command_id);
break;
case Delta::Action::SET_PROPERTY:
copied_deltas.emplace_back(Delta::SetPropertyTag{}, delta.property.key, delta.property.value, commit_info,
- command_id);
+ delta.uuid, command_id);
break;
}
}
diff --git a/tests/unit/storage_v3_shard_split.cpp b/tests/unit/storage_v3_shard_split.cpp
index fd32eb0ef..5878ec0ce 100644
--- a/tests/unit/storage_v3_shard_split.cpp
+++ b/tests/unit/storage_v3_shard_split.cpp
@@ -83,7 +83,9 @@ void AssertEqVertexContainer(const VertexContainer &actual, const VertexContaine
auto *expected_delta = expected_it->second.delta;
auto *actual_delta = actual_it->second.delta;
while (expected_delta != nullptr) {
+ // TODO Enable this then fix delta id generator
EXPECT_EQ(actual_delta->action, expected_delta->action);
+ // EXPECT_EQ(actual_delta->uuid, expected_delta->uuid);
switch (expected_delta->action) {
case Delta::Action::ADD_LABEL:
case Delta::Action::REMOVE_LABEL: {
@@ -146,18 +148,19 @@ TEST_F(ShardSplitTest, TestBasicSplitWithVertices) {
EXPECT_EQ(splitted_data.label_property_indices.size(), 0);
CommitInfo commit_info{.start_or_commit_timestamp = current_hlc};
- Delta delta_delete1{Delta::DeleteObjectTag{}, &commit_info, 1};
- Delta delta_delete2{Delta::DeleteObjectTag{}, &commit_info, 2};
- Delta delta_delete3{Delta::DeleteObjectTag{}, &commit_info, 3};
- Delta delta_add_label{Delta::RemoveLabelTag{}, secondary_label, &commit_info, 4};
- Delta delta_add_property{Delta::SetPropertyTag{}, secondary_property, PropertyValue(), &commit_info, 4};
+ Delta delta_delete1{Delta::DeleteObjectTag{}, &commit_info, 5, 1};
+ Delta delta_delete2{Delta::DeleteObjectTag{}, &commit_info, 6, 2};
+ Delta delta_remove_label{Delta::RemoveLabelTag{}, secondary_label, &commit_info, 8, 4};
+ Delta delta_set_property{Delta::SetPropertyTag{}, secondary_property, PropertyValue(), &commit_info, 7, 4};
+ Delta delta_delete3{Delta::DeleteObjectTag{}, &commit_info, 9, 3};
+
VertexContainer expected_vertices;
expected_vertices.emplace(PrimaryKey{PropertyValue{4}}, VertexData(&delta_delete1));
auto [it, inserted] = expected_vertices.emplace(PrimaryKey{PropertyValue{5}}, VertexData(&delta_delete2));
expected_vertices.emplace(PrimaryKey{PropertyValue{6}}, VertexData(&delta_delete3));
it->second.labels.push_back(secondary_label);
- AddDeltaToDeltaChain(&*it, &delta_add_property);
- AddDeltaToDeltaChain(&*it, &delta_add_label);
+ AddDeltaToDeltaChain(&*it, &delta_set_property);
+ AddDeltaToDeltaChain(&*it, &delta_remove_label);
AssertEqVertexContainer(splitted_data.vertices, expected_vertices);
}
@@ -189,15 +192,30 @@ TEST_F(ShardSplitTest, TestBasicSplitVerticesAndEdges) {
EXPECT_EQ(splitted_data.label_property_indices.size(), 0);
CommitInfo commit_info{.start_or_commit_timestamp = current_hlc};
- Delta delta_delete1{Delta::DeleteObjectTag{}, &commit_info, 1};
- Delta delta_delete2{Delta::DeleteObjectTag{}, &commit_info, 1};
- Delta delta_delete3{Delta::DeleteObjectTag{}, &commit_info, 1};
- Delta delta_add_in_edge1{Delta::RemoveInEdgeTag{}, edge_type_id, VertexId{primary_label, {PropertyValue(1)}},
- EdgeRef{Gid::FromUint(1)}, &commit_info, 1};
- Delta delta_add_out_edge2{Delta::RemoveOutEdgeTag{}, edge_type_id, VertexId{primary_label, {PropertyValue(6)}},
- EdgeRef{Gid::FromUint(2)}, &commit_info, 1};
- Delta delta_add_in_edge2{Delta::RemoveInEdgeTag{}, edge_type_id, VertexId{primary_label, {PropertyValue(4)}},
- EdgeRef{Gid::FromUint(2)}, &commit_info, 1};
+ Delta delta_delete1{Delta::DeleteObjectTag{}, &commit_info, 3, 1};
+ Delta delta_delete2{Delta::DeleteObjectTag{}, &commit_info, 4, 1};
+ Delta delta_delete3{Delta::DeleteObjectTag{}, &commit_info, 5, 1};
+ Delta delta_add_in_edge1{Delta::RemoveInEdgeTag{},
+ edge_type_id,
+ VertexId{primary_label, {PropertyValue(1)}},
+ EdgeRef{Gid::FromUint(1)},
+ &commit_info,
+ 13,
+ 1};
+ Delta delta_add_out_edge2{Delta::RemoveOutEdgeTag{},
+ edge_type_id,
+ VertexId{primary_label, {PropertyValue(6)}},
+ EdgeRef{Gid::FromUint(2)},
+ &commit_info,
+ 20,
+ 1};
+ Delta delta_add_in_edge2{Delta::RemoveInEdgeTag{},
+ edge_type_id,
+ VertexId{primary_label, {PropertyValue(4)}},
+ EdgeRef{Gid::FromUint(2)},
+ &commit_info,
+ 15,
+ 1};
VertexContainer expected_vertices;
auto [vtx4, inserted4] = expected_vertices.emplace(PrimaryKey{PropertyValue{4}}, VertexData(&delta_delete1));
auto [vtx5, inserted5] = expected_vertices.emplace(PrimaryKey{PropertyValue{5}}, VertexData(&delta_delete2));