From 28eef7edf48b65219029f29d95cb6be5c547e2b7 Mon Sep 17 00:00:00 2001 From: Marko Budiselic Date: Sun, 1 May 2022 04:02:27 +0200 Subject: [PATCH] Add CommitError struct --- src/query/db_accessor.hpp | 2 +- src/query/interpreter.cpp | 102 +++++++++++++++----------- src/storage/v2/CMakeLists.txt | 1 + src/storage/v2/commit_error.cpp | 20 +++++ src/storage/v2/commit_error.hpp | 32 ++++++++ src/storage/v2/constraints.hpp | 2 - src/storage/v2/storage.cpp | 14 ++-- src/storage/v2/storage.hpp | 10 ++- tests/unit/storage_v2_constraints.cpp | 57 +++++++++----- 9 files changed, 165 insertions(+), 75 deletions(-) create mode 100644 src/storage/v2/commit_error.cpp create mode 100644 src/storage/v2/commit_error.hpp diff --git a/src/query/db_accessor.hpp b/src/query/db_accessor.hpp index 55514c883..1e6f64f1e 100644 --- a/src/query/db_accessor.hpp +++ b/src/query/db_accessor.hpp @@ -324,7 +324,7 @@ class DbAccessor final { void AdvanceCommand() { accessor_->AdvanceCommand(); } - utils::BasicResult Commit() { return accessor_->Commit(); } + utils::BasicResult Commit() { return accessor_->Commit(); } void Abort() { accessor_->Abort(); } diff --git a/src/query/interpreter.cpp b/src/query/interpreter.cpp index 18236f28f..9ba4bc13e 100644 --- a/src/query/interpreter.cpp +++ b/src/query/interpreter.cpp @@ -2206,29 +2206,36 @@ void RunTriggersIndividually(const utils::SkipList &triggers, Interpret continue; } - auto maybe_constraint_violation = db_accessor.Commit(); - if (maybe_constraint_violation.HasError()) { - const auto &constraint_violation = maybe_constraint_violation.GetError(); - switch (constraint_violation.type) { - case storage::ConstraintViolation::Type::UNABLE_TO_REPLICATE: { - spdlog::warn("Unable to replicate"); + auto maybe_commit_error = db_accessor.Commit(); + if (maybe_commit_error.HasError()) { + const auto &commit_error = maybe_commit_error.GetError(); + switch (commit_error.type) { + case storage::CommitError::Type::UNABLE_TO_REPLICATE: { + spdlog::warn("Unable to replicate to SYNC replica on COMMIT"); break; } - case storage::ConstraintViolation::Type::EXISTENCE: { - const auto &label_name = db_accessor.LabelToName(constraint_violation.label); - MG_ASSERT(constraint_violation.properties.size() == 1U); - const auto &property_name = db_accessor.PropertyToName(*constraint_violation.properties.begin()); - spdlog::warn("Trigger '{}' failed to commit due to existence constraint violation on :{}({})", trigger.Name(), - label_name, property_name); - break; - } - case storage::ConstraintViolation::Type::UNIQUE: { - const auto &label_name = db_accessor.LabelToName(constraint_violation.label); - std::stringstream property_names_stream; - utils::PrintIterable(property_names_stream, constraint_violation.properties, ", ", - [&](auto &stream, const auto &prop) { stream << db_accessor.PropertyToName(prop); }); - spdlog::warn("Trigger '{}' failed to commit due to unique constraint violation on :{}({})", trigger.Name(), - label_name, property_names_stream.str()); + case storage::CommitError::Type::CONSTRAINT_VIOLATION: { + MG_ASSERT(commit_error.maybe_constraint_violation.has_value()); + const auto &constraint_violation = *commit_error.maybe_constraint_violation; + switch (constraint_violation.type) { + case storage::ConstraintViolation::Type::EXISTENCE: { + const auto &label_name = db_accessor.LabelToName(constraint_violation.label); + MG_ASSERT(constraint_violation.properties.size() == 1U); + const auto &property_name = db_accessor.PropertyToName(*constraint_violation.properties.begin()); + spdlog::warn("Trigger '{}' failed to commit due to existence constraint violation on :{}({})", + trigger.Name(), label_name, property_name); + break; + } + case storage::ConstraintViolation::Type::UNIQUE: { + const auto &label_name = db_accessor.LabelToName(constraint_violation.label); + std::stringstream property_names_stream; + utils::PrintIterable(property_names_stream, constraint_violation.properties, ", ", + [&](auto &stream, const auto &prop) { stream << db_accessor.PropertyToName(prop); }); + spdlog::warn("Trigger '{}' failed to commit due to unique constraint violation on :{}({})", + trigger.Name(), label_name, property_names_stream.str()); + break; + } + } break; } } @@ -2273,33 +2280,40 @@ void Interpreter::Commit() { trigger_context_collector_.reset(); }; - auto maybe_constraint_violation = db_accessor_->Commit(); - if (maybe_constraint_violation.HasError()) { - const auto &constraint_violation = maybe_constraint_violation.GetError(); - switch (constraint_violation.type) { - case storage::ConstraintViolation::Type::UNABLE_TO_REPLICATE: { + auto maybe_commit_error = db_accessor_->Commit(); + if (maybe_commit_error.HasError()) { + const auto &commit_error = maybe_commit_error.GetError(); + switch (commit_error.type) { + case storage::CommitError::Type::UNABLE_TO_REPLICATE: { reset_necessary_members(); throw QueryException("Unable to replicate to SYNC replica"); break; } - case storage::ConstraintViolation::Type::EXISTENCE: { - auto label_name = execution_db_accessor_->LabelToName(constraint_violation.label); - MG_ASSERT(constraint_violation.properties.size() == 1U); - auto property_name = execution_db_accessor_->PropertyToName(*constraint_violation.properties.begin()); - reset_necessary_members(); - throw QueryException("Unable to commit due to existence constraint violation on :{}({})", label_name, - property_name); - break; - } - case storage::ConstraintViolation::Type::UNIQUE: { - auto label_name = execution_db_accessor_->LabelToName(constraint_violation.label); - std::stringstream property_names_stream; - utils::PrintIterable( - property_names_stream, constraint_violation.properties, ", ", - [this](auto &stream, const auto &prop) { stream << execution_db_accessor_->PropertyToName(prop); }); - reset_necessary_members(); - throw QueryException("Unable to commit due to unique constraint violation on :{}({})", label_name, - property_names_stream.str()); + case storage::CommitError::Type::CONSTRAINT_VIOLATION: { + MG_ASSERT(commit_error.maybe_constraint_violation.has_value()); + const auto &constraint_violation = *commit_error.maybe_constraint_violation; + switch (constraint_violation.type) { + case storage::ConstraintViolation::Type::EXISTENCE: { + auto label_name = execution_db_accessor_->LabelToName(constraint_violation.label); + MG_ASSERT(constraint_violation.properties.size() == 1U); + auto property_name = execution_db_accessor_->PropertyToName(*constraint_violation.properties.begin()); + reset_necessary_members(); + throw QueryException("Unable to commit due to existence constraint violation on :{}({})", label_name, + property_name); + break; + } + case storage::ConstraintViolation::Type::UNIQUE: { + auto label_name = execution_db_accessor_->LabelToName(constraint_violation.label); + std::stringstream property_names_stream; + utils::PrintIterable( + property_names_stream, constraint_violation.properties, ", ", + [this](auto &stream, const auto &prop) { stream << execution_db_accessor_->PropertyToName(prop); }); + reset_necessary_members(); + throw QueryException("Unable to commit due to unique constraint violation on :{}({})", label_name, + property_names_stream.str()); + break; + } + } break; } } diff --git a/src/storage/v2/CMakeLists.txt b/src/storage/v2/CMakeLists.txt index f33a8553d..a6d6b0127 100644 --- a/src/storage/v2/CMakeLists.txt +++ b/src/storage/v2/CMakeLists.txt @@ -1,5 +1,6 @@ set(storage_v2_src_files commit_log.cpp + commit_error.cpp constraints.cpp temporal.cpp durability/durability.cpp diff --git a/src/storage/v2/commit_error.cpp b/src/storage/v2/commit_error.cpp new file mode 100644 index 000000000..216c42d32 --- /dev/null +++ b/src/storage/v2/commit_error.cpp @@ -0,0 +1,20 @@ +// Copyright 2022 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 +// License, and you may not use this file except in compliance with the Business Source License. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +#include "storage/v2/commit_error.hpp" + +namespace memgraph::storage { + +bool operator==(const CommitError &lhs, const CommitError &rhs) { + return lhs.type == rhs.type && lhs.maybe_constraint_violation == rhs.maybe_constraint_violation; +} + +} // namespace memgraph::storage diff --git a/src/storage/v2/commit_error.hpp b/src/storage/v2/commit_error.hpp new file mode 100644 index 000000000..995afc04f --- /dev/null +++ b/src/storage/v2/commit_error.hpp @@ -0,0 +1,32 @@ +// Copyright 2022 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 +// License, and you may not use this file except in compliance with the Business Source License. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +#pragma once + +#include + +#include "storage/v2/constraints.hpp" + +namespace memgraph::storage { + +struct CommitError { + enum class Type { + CONSTRAINT_VIOLATION, + UNABLE_TO_REPLICATE, + }; + Type type; + + std::optional maybe_constraint_violation; +}; + +bool operator==(const CommitError &lhs, const CommitError &rhs); + +} // namespace memgraph::storage diff --git a/src/storage/v2/constraints.hpp b/src/storage/v2/constraints.hpp index a0d359416..b209437f8 100644 --- a/src/storage/v2/constraints.hpp +++ b/src/storage/v2/constraints.hpp @@ -46,8 +46,6 @@ struct ConstraintViolation { enum class Type { EXISTENCE, UNIQUE, - // TODO(gitbuda): Total workaround, rename/move the ConstraintViloation struct and add empty data state. - UNABLE_TO_REPLICATE, }; Type type; diff --git a/src/storage/v2/storage.cpp b/src/storage/v2/storage.cpp index 2553cdf21..2f67f211c 100644 --- a/src/storage/v2/storage.cpp +++ b/src/storage/v2/storage.cpp @@ -40,6 +40,7 @@ #include "utils/uuid.hpp" /// REPLICATION /// +#include "storage/v2/commit_error.hpp" #include "storage/v2/replication/replication_client.hpp" #include "storage/v2/replication/replication_server.hpp" #include "storage/v2/replication/rpc.hpp" @@ -813,8 +814,7 @@ EdgeTypeId Storage::Accessor::NameToEdgeType(const std::string_view &name) { ret void Storage::Accessor::AdvanceCommand() { ++transaction_.command_id; } -// TODO(gitbuda): Consider adding addional Commit BasicResult because of replication failures. -utils::BasicResult Storage::Accessor::Commit( +utils::BasicResult Storage::Accessor::Commit( const std::optional desired_commit_timestamp) { MG_ASSERT(is_transaction_active_, "The transaction is already terminated!"); MG_ASSERT(!transaction_.must_abort, "The transaction can't be committed!"); @@ -837,7 +837,8 @@ utils::BasicResult Storage::Accessor::Commit( auto validation_result = ValidateExistenceConstraints(*prev.vertex, storage_->constraints_); if (validation_result) { Abort(); - return *validation_result; + return CommitError{.type = CommitError::Type::CONSTRAINT_VIOLATION, + .maybe_constraint_violation = *validation_result}; } } @@ -869,7 +870,7 @@ utils::BasicResult Storage::Accessor::Commit( check_replicas(); if (unable_to_sync_replicate) { Abort(); - return ConstraintViolation{ConstraintViolation::Type::UNABLE_TO_REPLICATE, LabelId(), std::set{}}; + return CommitError{.type = CommitError::Type::UNABLE_TO_REPLICATE}; } { @@ -951,12 +952,13 @@ utils::BasicResult Storage::Accessor::Commit( check_replicas(); if (unable_to_sync_replicate) { Abort(); - return ConstraintViolation{ConstraintViolation::Type::UNABLE_TO_REPLICATE, LabelId(), std::set{}}; + return CommitError{.type = CommitError::Type::UNABLE_TO_REPLICATE}; } if (unique_constraint_violation) { Abort(); - return *unique_constraint_violation; + return CommitError{.type = CommitError::Type::CONSTRAINT_VIOLATION, + .maybe_constraint_violation = *unique_constraint_violation}; } } is_transaction_active_ = false; diff --git a/src/storage/v2/storage.hpp b/src/storage/v2/storage.hpp index 7b20dc20b..29faecf58 100644 --- a/src/storage/v2/storage.hpp +++ b/src/storage/v2/storage.hpp @@ -43,6 +43,7 @@ /// REPLICATION /// #include "rpc/server.hpp" +#include "storage/v2/commit_error.hpp" #include "storage/v2/replication/config.hpp" #include "storage/v2/replication/enums.hpp" #include "storage/v2/replication/rpc.hpp" @@ -308,11 +309,12 @@ class Storage final { void AdvanceCommand(); - /// Commit returns `ConstraintViolation` if the changes made by this - /// transaction violate an existence or unique constraint. In that case the - /// transaction is automatically aborted. Otherwise, void is returned. + /// Commit returns `CommitError` if the changes made by this transaction + /// violate an existence, unique constraint or data could NOT be replicated + /// to SYNC replica. In that case the transaction is automatically aborted. + /// Otherwise, void is returned. /// @throw std::bad_alloc - utils::BasicResult Commit(std::optional desired_commit_timestamp = {}); + utils::BasicResult Commit(std::optional desired_commit_timestamp = {}); /// @throw std::bad_alloc void Abort(); diff --git a/tests/unit/storage_v2_constraints.cpp b/tests/unit/storage_v2_constraints.cpp index 5c0fde426..50e00ebfe 100644 --- a/tests/unit/storage_v2_constraints.cpp +++ b/tests/unit/storage_v2_constraints.cpp @@ -137,8 +137,10 @@ TEST_F(ConstraintsTest, ExistenceConstraintsViolationOnCommit) { auto res = acc.Commit(); ASSERT_TRUE(res.HasError()); - EXPECT_EQ(res.GetError(), - (ConstraintViolation{ConstraintViolation::Type::EXISTENCE, label1, std::set{prop1}})); + EXPECT_EQ( + res.GetError(), + (CommitError{CommitError::Type::CONSTRAINT_VIOLATION, + ConstraintViolation{ConstraintViolation::Type::EXISTENCE, label1, std::set{prop1}}})); } { @@ -157,8 +159,10 @@ TEST_F(ConstraintsTest, ExistenceConstraintsViolationOnCommit) { auto res = acc.Commit(); ASSERT_TRUE(res.HasError()); - EXPECT_EQ(res.GetError(), - (ConstraintViolation{ConstraintViolation::Type::EXISTENCE, label1, std::set{prop1}})); + EXPECT_EQ( + res.GetError(), + (CommitError{CommitError::Type::CONSTRAINT_VIOLATION, + ConstraintViolation{ConstraintViolation::Type::EXISTENCE, label1, std::set{prop1}}})); } { @@ -458,8 +462,10 @@ TEST_F(ConstraintsTest, UniqueConstraintsViolationOnCommit1) { ASSERT_NO_ERROR(vertex2.SetProperty(prop1, PropertyValue(1))); auto res = acc.Commit(); ASSERT_TRUE(res.HasError()); - EXPECT_EQ(res.GetError(), - (ConstraintViolation{ConstraintViolation::Type::UNIQUE, label1, std::set{prop1}})); + EXPECT_EQ( + res.GetError(), + (CommitError{CommitError::Type::CONSTRAINT_VIOLATION, + ConstraintViolation{ConstraintViolation::Type::UNIQUE, label1, std::set{prop1}}})); } } @@ -500,8 +506,10 @@ TEST_F(ConstraintsTest, UniqueConstraintsViolationOnCommit2) { ASSERT_NO_ERROR(acc2.Commit()); auto res = acc3.Commit(); ASSERT_TRUE(res.HasError()); - EXPECT_EQ(res.GetError(), - (ConstraintViolation{ConstraintViolation::Type::UNIQUE, label1, std::set{prop1}})); + EXPECT_EQ( + res.GetError(), + (CommitError{CommitError::Type::CONSTRAINT_VIOLATION, + ConstraintViolation{ConstraintViolation::Type::UNIQUE, label1, std::set{prop1}}})); } } @@ -545,12 +553,16 @@ TEST_F(ConstraintsTest, UniqueConstraintsViolationOnCommit3) { auto res = acc2.Commit(); ASSERT_TRUE(res.HasError()); - EXPECT_EQ(res.GetError(), - (ConstraintViolation{ConstraintViolation::Type::UNIQUE, label1, std::set{prop1}})); + EXPECT_EQ( + res.GetError(), + (CommitError{CommitError::Type::CONSTRAINT_VIOLATION, + ConstraintViolation{ConstraintViolation::Type::UNIQUE, label1, std::set{prop1}}})); res = acc3.Commit(); ASSERT_TRUE(res.HasError()); - EXPECT_EQ(res.GetError(), - (ConstraintViolation{ConstraintViolation::Type::UNIQUE, label1, std::set{prop1}})); + EXPECT_EQ( + res.GetError(), + (CommitError{CommitError::Type::CONSTRAINT_VIOLATION, + ConstraintViolation{ConstraintViolation::Type::UNIQUE, label1, std::set{prop1}}})); } } @@ -620,7 +632,9 @@ TEST_F(ConstraintsTest, UniqueConstraintsLabelAlteration) { auto res = acc.Commit(); ASSERT_TRUE(res.HasError()); - EXPECT_EQ(res.GetError(), (ConstraintViolation{ConstraintViolation::Type::UNIQUE, label1, std::set{prop1}})); + EXPECT_EQ(res.GetError(), + (CommitError{CommitError::Type::CONSTRAINT_VIOLATION, + ConstraintViolation{ConstraintViolation::Type::UNIQUE, label1, std::set{prop1}}})); } { @@ -654,7 +668,9 @@ TEST_F(ConstraintsTest, UniqueConstraintsLabelAlteration) { auto res = acc1.Commit(); ASSERT_TRUE(res.HasError()); - EXPECT_EQ(res.GetError(), (ConstraintViolation{ConstraintViolation::Type::UNIQUE, label1, std::set{prop1}})); + EXPECT_EQ(res.GetError(), + (CommitError{CommitError::Type::CONSTRAINT_VIOLATION, + ConstraintViolation{ConstraintViolation::Type::UNIQUE, label1, std::set{prop1}}})); } } @@ -749,8 +765,9 @@ TEST_F(ConstraintsTest, UniqueConstraintsMultipleProperties) { ASSERT_NO_ERROR(vertex2->SetProperty(prop2, PropertyValue(2))); auto res = acc.Commit(); ASSERT_TRUE(res.HasError()); - EXPECT_EQ(res.GetError(), - (ConstraintViolation{ConstraintViolation::Type::UNIQUE, label1, std::set{prop1, prop2}})); + EXPECT_EQ(res.GetError(), (CommitError{CommitError::Type::CONSTRAINT_VIOLATION, + ConstraintViolation{ConstraintViolation::Type::UNIQUE, label1, + std::set{prop1, prop2}}})); } // Then change the second property of both vertex to null. Property values of @@ -861,7 +878,9 @@ TEST_F(ConstraintsTest, UniqueConstraintsInsertRemoveAbortInsert) { auto res = acc.Commit(); ASSERT_TRUE(res.HasError()); - EXPECT_EQ(res.GetError(), (ConstraintViolation{ConstraintViolation::Type::UNIQUE, label1, std::set{prop1, prop2}})); + EXPECT_EQ(res.GetError(), + (CommitError{CommitError::Type::CONSTRAINT_VIOLATION, + ConstraintViolation{ConstraintViolation::Type::UNIQUE, label1, std::set{prop1, prop2}}})); } } @@ -900,7 +919,9 @@ TEST_F(ConstraintsTest, UniqueConstraintsDeleteVertexSetProperty) { auto res = acc1.Commit(); ASSERT_TRUE(res.HasError()); - EXPECT_EQ(res.GetError(), (ConstraintViolation{ConstraintViolation::Type::UNIQUE, label1, std::set{prop1}})); + EXPECT_EQ(res.GetError(), + (CommitError{CommitError::Type::CONSTRAINT_VIOLATION, + ConstraintViolation{ConstraintViolation::Type::UNIQUE, label1, std::set{prop1}}})); ASSERT_NO_ERROR(acc2.Commit()); }