From 509183e98569c043c81ce810ab53ee67c371e31c Mon Sep 17 00:00:00 2001 From: Antonio Filipovic <61245998+antoniofilipovic@users.noreply.github.com> Date: Thu, 10 Aug 2023 09:06:44 +0200 Subject: [PATCH] Improve performance on set properties (#1115) --- src/query/common.hpp | 65 ++++++++++++------- src/query/db_accessor.hpp | 10 +++ src/query/plan/operator.cpp | 54 +++++++-------- src/storage/v2/delta.hpp | 4 ++ src/storage/v2/edge_accessor.cpp | 22 +++++++ src/storage/v2/edge_accessor.hpp | 3 + src/storage/v2/property_store.cpp | 26 ++++++++ src/storage/v2/property_store.hpp | 8 +++ src/storage/v2/vertex_accessor.cpp | 22 +++++++ src/storage/v2/vertex_accessor.hpp | 3 + .../query_plan_create_set_remove_delete.cpp | 35 ++++++++++ 11 files changed, 199 insertions(+), 53 deletions(-) diff --git a/src/query/common.hpp b/src/query/common.hpp index 6154900b5..8f1b0a94c 100644 --- a/src/query/common.hpp +++ b/src/query/common.hpp @@ -24,6 +24,7 @@ #include "query/typed_value.hpp" #include "storage/v2/id_types.hpp" #include "storage/v2/property_value.hpp" +#include "storage/v2/result.hpp" #include "storage/v2/view.hpp" #include "utils/logging.hpp" @@ -75,6 +76,20 @@ inline void ExpectType(const Symbol &symbol, const TypedValue &value, TypedValue throw QueryRuntimeException("Expected a {} for '{}', but got {}.", expected, symbol.name(), value.type()); } +inline void ProcessError(const storage::Error error) { + switch (error) { + case storage::Error::SERIALIZATION_ERROR: + throw TransactionSerializationException(); + case storage::Error::DELETED_OBJECT: + throw QueryRuntimeException("Trying to set properties on a deleted object."); + case storage::Error::PROPERTIES_DISABLED: + throw QueryRuntimeException("Can't set property because properties on edges are disabled."); + case storage::Error::VERTEX_HAS_EDGES: + case storage::Error::NONEXISTENT_OBJECT: + throw QueryRuntimeException("Unexpected error when setting a property."); + } +} + template concept AccessorWithSetProperty = requires(T accessor, const storage::PropertyId key, const storage::PropertyValue new_value) { @@ -89,17 +104,7 @@ storage::PropertyValue PropsSetChecked(T *record, const storage::PropertyId &key try { auto maybe_old_value = record->SetProperty(key, storage::PropertyValue(value)); if (maybe_old_value.HasError()) { - switch (maybe_old_value.GetError()) { - case storage::Error::SERIALIZATION_ERROR: - throw TransactionSerializationException(); - case storage::Error::DELETED_OBJECT: - throw QueryRuntimeException("Trying to set properties on a deleted object."); - case storage::Error::PROPERTIES_DISABLED: - throw QueryRuntimeException("Can't set property because properties on edges are disabled."); - case storage::Error::VERTEX_HAS_EDGES: - case storage::Error::NONEXISTENT_OBJECT: - throw QueryRuntimeException("Unexpected error when setting a property."); - } + ProcessError(maybe_old_value.GetError()); } return std::move(*maybe_old_value); } catch (const TypedValueException &) { @@ -121,17 +126,7 @@ bool MultiPropsInitChecked(T *record, std::mapInitProperties(properties); if (maybe_values.HasError()) { - switch (maybe_values.GetError()) { - case storage::Error::SERIALIZATION_ERROR: - throw TransactionSerializationException(); - case storage::Error::DELETED_OBJECT: - throw QueryRuntimeException("Trying to set properties on a deleted object."); - case storage::Error::PROPERTIES_DISABLED: - throw QueryRuntimeException("Can't set property because properties on edges are disabled."); - case storage::Error::VERTEX_HAS_EDGES: - case storage::Error::NONEXISTENT_OBJECT: - throw QueryRuntimeException("Unexpected error when setting a property."); - } + ProcessError(maybe_values.GetError()); } return std::move(*maybe_values); } catch (const TypedValueException &) { @@ -139,5 +134,31 @@ bool MultiPropsInitChecked(T *record, std::map +concept AccessorWithUpdateProperties = requires(T accessor, + std::map &properties) { + { + accessor.UpdateProperties(properties) + } -> std::same_as< + storage::Result>>>; +}; + +/// Set property `values` mapped with given `key` on a `record`. +/// +/// @throw QueryRuntimeException if value cannot be set as a property value +template +auto UpdatePropertiesChecked(T *record, std::map &properties) -> + typename std::remove_referenceUpdateProperties(properties).GetValue())>::type { + try { + auto maybe_values = record->UpdateProperties(properties); + if (maybe_values.HasError()) { + ProcessError(maybe_values.GetError()); + } + return std::move(*maybe_values); + } catch (const TypedValueException &) { + throw QueryRuntimeException("Cannot update properties."); + } +} + int64_t QueryTimestamp(); } // namespace memgraph::query diff --git a/src/query/db_accessor.hpp b/src/query/db_accessor.hpp index b14f9ac0b..120f9abac 100644 --- a/src/query/db_accessor.hpp +++ b/src/query/db_accessor.hpp @@ -77,6 +77,11 @@ class EdgeAccessor final { return impl_.InitProperties(properties); } + storage::Result>> + UpdateProperties(std::map &properties) const { + return impl_.UpdateProperties(properties); + } + storage::Result RemoveProperty(storage::PropertyId key) { return SetProperty(key, storage::PropertyValue()); } @@ -135,6 +140,11 @@ class VertexAccessor final { return impl_.InitProperties(properties); } + storage::Result>> + UpdateProperties(std::map &properties) const { + return impl_.UpdateProperties(properties); + } + storage::Result RemoveProperty(storage::PropertyId key) { return SetProperty(key, storage::PropertyValue()); } diff --git a/src/query/plan/operator.cpp b/src/query/plan/operator.cpp index 93bad291e..227acc2df 100644 --- a/src/query/plan/operator.cpp +++ b/src/query/plan/operator.cpp @@ -2711,9 +2711,11 @@ namespace { template concept AccessorWithProperties = requires(T value, storage::PropertyId property_id, - storage::PropertyValue property_value) { + storage::PropertyValue property_value, + std::map properties) { { value.ClearProperties() } -> std::same_as>>; {value.SetProperty(property_id, property_value)}; + {value.UpdateProperties(properties)}; }; /// Helper function that sets the given values on either a Vertex or an Edge. @@ -2723,7 +2725,8 @@ concept AccessorWithProperties = requires(T value, storage::PropertyId property_ template void SetPropertiesOnRecord(TRecordAccessor *record, const TypedValue &rhs, SetProperties::Op op, ExecutionContext *context) { - std::optional> old_values; + using PropertiesMap = std::map; + std::optional old_values; const bool should_register_change = context->trigger_context_collector && context->trigger_context_collector->ShouldRegisterObjectPropertyChange(); @@ -2782,44 +2785,34 @@ void SetPropertiesOnRecord(TRecordAccessor *record, const TypedValue &rhs, SetPr *record, key, TypedValue(std::move(old_value)), TypedValue(std::forward(new_value))); }; - auto set_props = [&, record](auto properties) { - for (auto &kv : properties) { - auto maybe_error = record->SetProperty(kv.first, kv.second); - if (maybe_error.HasError()) { - switch (maybe_error.GetError()) { - case storage::Error::DELETED_OBJECT: - throw QueryRuntimeException("Trying to set properties on a deleted graph element."); - case storage::Error::SERIALIZATION_ERROR: - throw TransactionSerializationException(); - case storage::Error::PROPERTIES_DISABLED: - throw QueryRuntimeException("Can't set property because properties on edges are disabled."); - case storage::Error::VERTEX_HAS_EDGES: - case storage::Error::NONEXISTENT_OBJECT: - throw QueryRuntimeException("Unexpected error when setting properties."); - } - } + auto update_props = [&, record](PropertiesMap &new_properties) { + auto updated_properties = UpdatePropertiesChecked(record, new_properties); - if (should_register_change) { - register_set_property(std::move(*maybe_error), kv.first, std::move(kv.second)); + if (should_register_change) { + for (const auto &[id, old_value, new_value] : updated_properties) { + register_set_property(std::move(old_value), id, std::move(new_value)); } } }; switch (rhs.type()) { - case TypedValue::Type::Edge: - set_props(get_props(rhs.ValueEdge())); + case TypedValue::Type::Edge: { + PropertiesMap new_properties = get_props(rhs.ValueEdge()); + update_props(new_properties); break; - case TypedValue::Type::Vertex: - set_props(get_props(rhs.ValueVertex())); + } + case TypedValue::Type::Vertex: { + PropertiesMap new_properties = get_props(rhs.ValueVertex()); + update_props(new_properties); break; + } case TypedValue::Type::Map: { - for (const auto &kv : rhs.ValueMap()) { - auto key = context->db_accessor->NameToProperty(kv.first); - auto old_value = PropsSetChecked(record, key, kv.second); - if (should_register_change) { - register_set_property(std::move(old_value), key, kv.second); - } + PropertiesMap new_properties; + for (const auto &[prop_id, prop_value] : rhs.ValueMap()) { + auto key = context->db_accessor->NameToProperty(prop_id); + new_properties.emplace(key, prop_value); } + update_props(new_properties); break; } default: @@ -2860,7 +2853,6 @@ bool SetProperties::SetPropertiesCursor::Pull(Frame &frame, ExecutionContext &co throw QueryRuntimeException("Vertex properties not set due to not having enough permission!"); } #endif - SetPropertiesOnRecord(&lhs.ValueVertex(), rhs, self_.op_, &context); break; case TypedValue::Type::Edge: diff --git a/src/storage/v2/delta.hpp b/src/storage/v2/delta.hpp index c23f06298..7af335c31 100644 --- a/src/storage/v2/delta.hpp +++ b/src/storage/v2/delta.hpp @@ -174,6 +174,10 @@ struct Delta { uint64_t command_id) : action(Action::SET_PROPERTY), timestamp(timestamp), command_id(command_id), property({key, value}) {} + Delta(SetPropertyTag /*tag*/, PropertyId key, PropertyValue &&value, std::atomic *timestamp, + uint64_t command_id) + : action(Action::SET_PROPERTY), timestamp(timestamp), command_id(command_id), property({key, std::move(value)}) {} + Delta(AddInEdgeTag /*tag*/, EdgeTypeId edge_type, Vertex *vertex, EdgeRef edge, std::atomic *timestamp, uint64_t command_id) : action(Action::ADD_IN_EDGE), diff --git a/src/storage/v2/edge_accessor.cpp b/src/storage/v2/edge_accessor.cpp index 1c47e7102..3c77f837f 100644 --- a/src/storage/v2/edge_accessor.cpp +++ b/src/storage/v2/edge_accessor.cpp @@ -12,11 +12,13 @@ #include "storage/v2/edge_accessor.hpp" #include +#include #include #include "storage/v2/delta.hpp" #include "storage/v2/mvcc.hpp" #include "storage/v2/property_value.hpp" +#include "storage/v2/result.hpp" #include "storage/v2/vertex_accessor.hpp" #include "utils/memory_tracker.hpp" @@ -145,6 +147,26 @@ Result EdgeAccessor::InitProperties(const std::map>> EdgeAccessor::UpdateProperties( + std::map &properties) const { + utils::MemoryTracker::OutOfMemoryExceptionEnabler oom_exception; + if (!config_.properties_on_edges) return Error::PROPERTIES_DISABLED; + + std::lock_guard guard(edge_.ptr->lock); + + if (!PrepareForWrite(transaction_, edge_.ptr)) return Error::SERIALIZATION_ERROR; + + if (edge_.ptr->deleted) return Error::DELETED_OBJECT; + + auto id_old_new_change = edge_.ptr->properties.UpdateProperties(properties); + + for (auto &[property, old_value, new_value] : id_old_new_change) { + CreateAndLinkDelta(transaction_, edge_.ptr, Delta::SetPropertyTag(), property, std::move(old_value)); + } + + return id_old_new_change; +} + Result> EdgeAccessor::ClearProperties() { if (!config_.properties_on_edges) return Error::PROPERTIES_DISABLED; diff --git a/src/storage/v2/edge_accessor.hpp b/src/storage/v2/edge_accessor.hpp index 93e995a05..2b87b9838 100644 --- a/src/storage/v2/edge_accessor.hpp +++ b/src/storage/v2/edge_accessor.hpp @@ -63,6 +63,9 @@ class EdgeAccessor final { /// @throw std::bad_alloc Result InitProperties(const std::map &properties); + Result>> UpdateProperties( + std::map &properties) const; + /// Remove all properties and return old values for each removed property. /// @throw std::bad_alloc Result> ClearProperties(); diff --git a/src/storage/v2/property_store.cpp b/src/storage/v2/property_store.cpp index 9451789b9..cb23ea551 100644 --- a/src/storage/v2/property_store.cpp +++ b/src/storage/v2/property_store.cpp @@ -1238,6 +1238,32 @@ bool PropertyStore::DoInitProperties(const TContainer &properties) { return true; } + +std::vector> PropertyStore::UpdateProperties( + std::map &properties) { + auto old_properties = Properties(); + ClearProperties(); + + std::vector> id_old_new_change; + id_old_new_change.reserve(properties.size() + old_properties.size()); + for (const auto &[prop_id, new_value] : properties) { + if (!old_properties.contains(prop_id)) { + id_old_new_change.emplace_back(std::make_tuple(prop_id, PropertyValue(), new_value)); + } + } + + for (const auto &[old_key, old_value] : old_properties) { + auto [it, inserted] = properties.emplace(old_key, old_value); + if (!inserted) { + auto &new_value = it->second; + id_old_new_change.emplace_back(std::make_tuple(it->first, old_value, new_value)); + } + } + + MG_ASSERT(InitProperties(properties)); + return id_old_new_change; +} + template bool PropertyStore::DoInitProperties>( const std::map &); template bool PropertyStore::DoInitProperties>>( diff --git a/src/storage/v2/property_store.hpp b/src/storage/v2/property_store.hpp index 2cf785a7b..6a458641b 100644 --- a/src/storage/v2/property_store.hpp +++ b/src/storage/v2/property_store.hpp @@ -91,6 +91,14 @@ class PropertyStore { /// @throw std::bad_alloc bool InitProperties(std::vector> properties); + /// Update property values in property store with sent properties. Returns vector of changed + /// properties. Each tuple inside vector consists of PropertyId of inserted property, together with old + /// property (if existed or empty PropertyValue if didn't exist) and new property which was inserted. + /// The time complexity of this function is O(n*log(n)): + /// @throw std::bad_alloc + std::vector> UpdateProperties( + std::map &properties); + /// Remove all properties and return `true` if any removal took place. /// `false` is returned if there were no properties to remove. The time /// complexity of this function is O(1). diff --git a/src/storage/v2/vertex_accessor.cpp b/src/storage/v2/vertex_accessor.cpp index 9bf3c8fa9..c25c8ef8e 100644 --- a/src/storage/v2/vertex_accessor.cpp +++ b/src/storage/v2/vertex_accessor.cpp @@ -12,12 +12,15 @@ #include "storage/v2/vertex_accessor.hpp" #include +#include +#include #include "storage/v2/edge_accessor.hpp" #include "storage/v2/id_types.hpp" #include "storage/v2/indices/indices.hpp" #include "storage/v2/mvcc.hpp" #include "storage/v2/property_value.hpp" +#include "storage/v2/result.hpp" #include "utils/logging.hpp" #include "utils/memory_tracker.hpp" @@ -254,6 +257,25 @@ Result VertexAccessor::InitProperties(const std::map>> VertexAccessor::UpdateProperties( + std::map &properties) const { + utils::MemoryTracker::OutOfMemoryExceptionEnabler oom_exception; + std::lock_guard guard(vertex_->lock); + + if (!PrepareForWrite(transaction_, vertex_)) return Error::SERIALIZATION_ERROR; + + if (vertex_->deleted) return Error::DELETED_OBJECT; + + auto id_old_new_change = vertex_->properties.UpdateProperties(properties); + + for (auto &[id, old_value, new_value] : id_old_new_change) { + indices_->UpdateOnSetProperty(id, new_value, vertex_, *transaction_); + CreateAndLinkDelta(transaction_, vertex_, Delta::SetPropertyTag(), id, std::move(old_value)); + } + + return id_old_new_change; +} + Result> VertexAccessor::ClearProperties() { std::lock_guard guard(vertex_->lock); diff --git a/src/storage/v2/vertex_accessor.hpp b/src/storage/v2/vertex_accessor.hpp index db1a6a6ef..148abfb28 100644 --- a/src/storage/v2/vertex_accessor.hpp +++ b/src/storage/v2/vertex_accessor.hpp @@ -73,6 +73,9 @@ class VertexAccessor final { /// @throw std::bad_alloc Result InitProperties(const std::map &properties); + Result>> UpdateProperties( + std::map &properties) const; + /// Remove all properties and return the values of the removed properties. /// @throw std::bad_alloc Result> ClearProperties(); diff --git a/tests/unit/query_plan_create_set_remove_delete.cpp b/tests/unit/query_plan_create_set_remove_delete.cpp index 088f6f8cc..eef708dcf 100644 --- a/tests/unit/query_plan_create_set_remove_delete.cpp +++ b/tests/unit/query_plan_create_set_remove_delete.cpp @@ -1678,6 +1678,41 @@ TYPED_TEST(QueryPlanTest, SetPropertiesOnNull) { EXPECT_EQ(1, PullAll(*set_op, &context)); } +TYPED_TEST(QueryPlanTest, UpdateSetPropertiesFromMap) { + auto storage_dba = this->db->Access(); + memgraph::query::DbAccessor dba(storage_dba.get()); + // Add a single vertex. ( {property: 43}) + auto vertex_accessor = dba.InsertVertex(); + auto old_value = vertex_accessor.SetProperty(dba.NameToProperty("property"), memgraph::storage::PropertyValue{43}); + EXPECT_EQ(old_value.HasError(), false); + EXPECT_EQ(*old_value, memgraph::storage::PropertyValue()); + dba.AdvanceCommand(); + EXPECT_EQ(1, CountIterable(dba.Vertices(memgraph::storage::View::OLD))); + SymbolTable symbol_table; + // MATCH (n) SET n += {property: "updated", new_property:"a"} + auto n = MakeScanAll(this->storage, symbol_table, "n"); + + auto prop_property = PROPERTY_PAIR(dba, "property"); + auto prop_new_property = PROPERTY_PAIR(dba, "new_property"); + + std::unordered_map prop_map; + prop_map.emplace(this->storage.GetPropertyIx(prop_property.first), LITERAL("updated")); + prop_map.emplace(this->storage.GetPropertyIx(prop_new_property.first), LITERAL("a")); + auto *rhs = this->storage.template Create(prop_map); + + auto op_type{plan::SetProperties::Op::UPDATE}; + auto set_op = std::make_shared(n.op_, n.sym_, rhs, op_type); + auto context = MakeContext(this->storage, symbol_table, &dba); + PullAll(*set_op, &context); + dba.AdvanceCommand(); + auto new_properties = vertex_accessor.Properties(memgraph::storage::View::OLD); + std::map expected_properties; + expected_properties.emplace(dba.NameToProperty("property"), memgraph::storage::PropertyValue("updated")); + expected_properties.emplace(dba.NameToProperty("new_property"), memgraph::storage::PropertyValue("a")); + EXPECT_EQ(new_properties.HasError(), false); + EXPECT_EQ(*new_properties, expected_properties); +} + TYPED_TEST(QueryPlanTest, SetLabelsOnNull) { // OPTIONAL MATCH (n) SET n :label auto storage_dba = this->db->Access();