From 3dd393f8eb55188e1990dc105f66da03253439c0 Mon Sep 17 00:00:00 2001 From: Matej Ferencevic Date: Wed, 10 Jun 2020 11:26:13 +0200 Subject: [PATCH] Fix multi-command transaction handling Reviewers: buda Reviewed By: buda Subscribers: pullbot Differential Revision: https://phabricator.memgraph.io/D2784 --- src/query/exceptions.hpp | 17 +++++++ src/query/interpreter.cpp | 46 ++++++++++++++----- tests/unit/interpreter.cpp | 90 +++++++++++++++++++++++++++++++++++--- 3 files changed, 134 insertions(+), 19 deletions(-) diff --git a/src/query/exceptions.hpp b/src/query/exceptions.hpp index fedc997c3..94fc8566b 100644 --- a/src/query/exceptions.hpp +++ b/src/query/exceptions.hpp @@ -84,6 +84,23 @@ class IndexInMulticommandTxException : public QueryException { "Index manipulation not allowed in multicommand transactions.") {} }; +class ConstraintInMulticommandTxException : public QueryException { + public: + using QueryException::QueryException; + ConstraintInMulticommandTxException() + : QueryException( + "Constraint manipulation not allowed in multicommand " + "transactions.") {} +}; + +class InfoInMulticommandTxException : public QueryException { + public: + using QueryException::QueryException; + InfoInMulticommandTxException() + : QueryException( + "Info reporting not allowed in multicommand transactions.") {} +}; + /** * An exception for an illegal operation that can not be detected * before the query starts executing over data. diff --git a/src/query/interpreter.cpp b/src/query/interpreter.cpp index 433abd092..978c05675 100644 --- a/src/query/interpreter.cpp +++ b/src/query/interpreter.cpp @@ -598,6 +598,18 @@ PreparedQuery PrepareProfileQuery( kProfileQueryStart)) << "Expected stripped query to start with '" << kProfileQueryStart << "'"; + // PROFILE isn't allowed inside multi-command (explicit) transactions. This is + // because PROFILE executes each PROFILE'd query and collects additional + // perfomance metadata that it displays to the user instead of the results + // yielded by the query. Because PROFILE has side-effects, each transaction + // that is used to execute a PROFILE query *MUST* be aborted. That isn't + // possible when using multicommand (explicit) transactions (because the user + // controls the lifetime of the transaction) and that is why PROFILE is + // explicitly disabled here in multicommand (explicit) transactions. + // NOTE: Unlike PROFILE, EXPLAIN doesn't have any unwanted side-effects (in + // transaction terms) because it doesn't execute the query, it just prints its + // query plan. That is why EXPLAIN can be used in multicommand (explicit) + // transactions. if (in_explicit_transaction) { throw ProfileInMulticommandTxException(); } @@ -769,10 +781,14 @@ PreparedQuery PrepareAuthQuery( } PreparedQuery PrepareInfoQuery( - ParsedQuery parsed_query, std::map *summary, - InterpreterContext *interpreter_context, - storage::Storage *db, + ParsedQuery parsed_query, bool in_explicit_transaction, + std::map *summary, + InterpreterContext *interpreter_context, storage::Storage *db, utils::MonotonicBufferResource *execution_memory) { + if (in_explicit_transaction) { + throw InfoInMulticommandTxException(); + } + auto *info_query = utils::Downcast(parsed_query.query); std::vector header; std::function< @@ -857,9 +873,14 @@ PreparedQuery PrepareInfoQuery( } PreparedQuery PrepareConstraintQuery( - ParsedQuery parsed_query, std::map *summary, + ParsedQuery parsed_query, bool in_explicit_transaction, + std::map *summary, InterpreterContext *interpreter_context, utils::MonotonicBufferResource *execution_memory) { + if (in_explicit_transaction) { + throw ConstraintInMulticommandTxException(); + } + auto *constraint_query = utils::Downcast(parsed_query.query); std::function handler; @@ -1061,9 +1082,10 @@ Interpreter::Prepare( // Some queries require an active transaction in order to be prepared. if (!in_explicit_transaction_ && - !utils::Downcast(parsed_query.query) && - !utils::Downcast(parsed_query.query) && - !utils::Downcast(parsed_query.query)) { + (utils::Downcast(parsed_query.query) || + utils::Downcast(parsed_query.query) || + utils::Downcast(parsed_query.query) || + utils::Downcast(parsed_query.query))) { db_accessor_.emplace(interpreter_context_->db->Access()); execution_db_accessor_.emplace(&*db_accessor_); } @@ -1097,12 +1119,12 @@ Interpreter::Prepare( interpreter_context_, &*execution_db_accessor_, &execution_memory_); } else if (utils::Downcast(parsed_query.query)) { prepared_query = PrepareInfoQuery( - std::move(parsed_query), &summary_, interpreter_context_, - interpreter_context_->db, &execution_memory_); + std::move(parsed_query), in_explicit_transaction_, &summary_, + interpreter_context_, interpreter_context_->db, &execution_memory_); } else if (utils::Downcast(parsed_query.query)) { - prepared_query = - PrepareConstraintQuery(std::move(parsed_query), &summary_, - interpreter_context_, &execution_memory_); + prepared_query = PrepareConstraintQuery( + std::move(parsed_query), in_explicit_transaction_, &summary_, + interpreter_context_, &execution_memory_); } else { LOG(FATAL) << "Should not get here -- unknown query type!"; } diff --git a/tests/unit/interpreter.cpp b/tests/unit/interpreter.cpp index 954de10c7..c9cc299f6 100644 --- a/tests/unit/interpreter.cpp +++ b/tests/unit/interpreter.cpp @@ -281,13 +281,6 @@ TEST_F(InterpreterTest, Bfs) { } } -TEST_F(InterpreterTest, CreateIndexInMulticommandTransaction) { - Interpret("BEGIN"); - ASSERT_THROW(Interpret("CREATE INDEX ON :X(y)"), - query::IndexInMulticommandTxException); - Interpret("ROLLBACK"); -} - // Test shortest path end to end. TEST_F(InterpreterTest, ShortestPath) { Interpret( @@ -327,6 +320,56 @@ TEST_F(InterpreterTest, ShortestPath) { } } +TEST_F(InterpreterTest, CreateLabelIndexInMulticommandTransaction) { + Interpret("BEGIN"); + ASSERT_THROW(Interpret("CREATE INDEX ON :X"), + query::IndexInMulticommandTxException); + Interpret("ROLLBACK"); +} + +TEST_F(InterpreterTest, CreateLabelPropertyIndexInMulticommandTransaction) { + Interpret("BEGIN"); + ASSERT_THROW(Interpret("CREATE INDEX ON :X(y)"), + query::IndexInMulticommandTxException); + Interpret("ROLLBACK"); +} + +TEST_F(InterpreterTest, CreateExistenceConstraintInMulticommandTransaction) { + Interpret("BEGIN"); + ASSERT_THROW(Interpret("CREATE CONSTRAINT ON (n:A) ASSERT EXISTS (n.a)"), + query::ConstraintInMulticommandTxException); + Interpret("ROLLBACK"); +} + +TEST_F(InterpreterTest, CreateUniqueConstraintInMulticommandTransaction) { + Interpret("BEGIN"); + ASSERT_THROW( + Interpret("CREATE CONSTRAINT ON (n:A) ASSERT n.a, n.b IS UNIQUE"), + query::ConstraintInMulticommandTxException); + Interpret("ROLLBACK"); +} + +TEST_F(InterpreterTest, ShowIndexInfoInMulticommandTransaction) { + Interpret("BEGIN"); + ASSERT_THROW(Interpret("SHOW INDEX INFO"), + query::InfoInMulticommandTxException); + Interpret("ROLLBACK"); +} + +TEST_F(InterpreterTest, ShowConstraintInfoInMulticommandTransaction) { + Interpret("BEGIN"); + ASSERT_THROW(Interpret("SHOW CONSTRAINT INFO"), + query::InfoInMulticommandTxException); + Interpret("ROLLBACK"); +} + +TEST_F(InterpreterTest, ShowStorageInfoInMulticommandTransaction) { + Interpret("BEGIN"); + ASSERT_THROW(Interpret("SHOW STORAGE INFO"), + query::InfoInMulticommandTxException); + Interpret("ROLLBACK"); +} + // NOLINTNEXTLINE(hicpp-special-member-functions) TEST_F(InterpreterTest, ExistenceConstraintTest) { Interpret("CREATE CONSTRAINT ON (n:A) ASSERT EXISTS (n.a);"); @@ -438,6 +481,32 @@ TEST_F(InterpreterTest, ExplainQuery) { EXPECT_EQ(interpreter_context_.ast_cache.size(), 2U); } +TEST_F(InterpreterTest, ExplainQueryInMulticommandTransaction) { + EXPECT_EQ(interpreter_context_.plan_cache.size(), 0U); + EXPECT_EQ(interpreter_context_.ast_cache.size(), 0U); + Interpret("BEGIN"); + auto stream = Interpret("EXPLAIN MATCH (n) RETURN *;"); + Interpret("COMMIT"); + ASSERT_EQ(stream.GetHeader().size(), 1U); + EXPECT_EQ(stream.GetHeader().front(), "QUERY PLAN"); + std::vector expected_rows{" * Produce {n}", " * ScanAll (n)", + " * Once"}; + ASSERT_EQ(stream.GetResults().size(), expected_rows.size()); + auto expected_it = expected_rows.begin(); + for (const auto &row : stream.GetResults()) { + ASSERT_EQ(row.size(), 1U); + EXPECT_EQ(row.front().ValueString(), *expected_it); + ++expected_it; + } + // We should have a plan cache for MATCH ... + EXPECT_EQ(interpreter_context_.plan_cache.size(), 1U); + // We should have AST cache for EXPLAIN ... and for inner MATCH ... + EXPECT_EQ(interpreter_context_.ast_cache.size(), 2U); + Interpret("MATCH (n) RETURN *;"); + EXPECT_EQ(interpreter_context_.plan_cache.size(), 1U); + EXPECT_EQ(interpreter_context_.ast_cache.size(), 2U); +} + TEST_F(InterpreterTest, ExplainQueryWithParams) { EXPECT_EQ(interpreter_context_.plan_cache.size(), 0U); EXPECT_EQ(interpreter_context_.ast_cache.size(), 0U); @@ -488,6 +557,13 @@ TEST_F(InterpreterTest, ProfileQuery) { EXPECT_EQ(interpreter_context_.ast_cache.size(), 2U); } +TEST_F(InterpreterTest, ProfileQueryInMulticommandTransaction) { + Interpret("BEGIN"); + ASSERT_THROW(Interpret("PROFILE MATCH (n) RETURN *;"), + query::ProfileInMulticommandTxException); + Interpret("ROLLBACK"); +} + TEST_F(InterpreterTest, ProfileQueryWithParams) { EXPECT_EQ(interpreter_context_.plan_cache.size(), 0U); EXPECT_EQ(interpreter_context_.ast_cache.size(), 0U);