Make DUMP DATABASE work correctly in explicit transactions

Summary:
`DUMP DATABASE` used a separate transaction to read database data. That
wouldn't be an issue if the query was correctly disallowed in multicommand
transactions. Because it was allowed the output wasn't transactionally correct.
Instead of disabling `DUMP DATABASE` in multicommand transactions this change
fixes it so that it works properly in multicommand transactions.

Reviewers: buda

Reviewed By: buda

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D2781

src/query/interpreter.cpp

@@ -651,17 +651,13 @@ PreparedQuery PrepareProfileQuery(
 
 PreparedQuery PrepareDumpQuery(
     ParsedQuery parsed_query, std::map<std::string, TypedValue> *summary,
-    InterpreterContext *interpreter_context,
-    utils::MonotonicBufferResource *execution_memory) {
-  return PreparedQuery{
-      {"QUERY"},
-      std::move(parsed_query.required_privileges),
-      [interpreter_context](AnyStream *stream) {
-        auto dba = interpreter_context->db->Access();
-        query::DbAccessor query_dba{&dba};
-        DumpDatabaseToCypherQueries(&query_dba, stream);
-        return QueryHandlerResult::NOTHING;
-      }};
+    DbAccessor *dba, utils::MonotonicBufferResource *execution_memory) {
+  return PreparedQuery{{"QUERY"},
+                       std::move(parsed_query.required_privileges),
+                       [dba](AnyStream *stream) {
+                         DumpDatabaseToCypherQueries(dba, stream);
+                         return QueryHandlerResult::COMMIT;
+                       }};
 }
 
 PreparedQuery PrepareIndexQuery(
@@ -1066,7 +1062,6 @@ Interpreter::Prepare(
     // Some queries require an active transaction in order to be prepared.
     if (!in_explicit_transaction_ &&
         !utils::Downcast<IndexQuery>(parsed_query.query) &&
-        !utils::Downcast<DumpQuery>(parsed_query.query) &&
         !utils::Downcast<ConstraintQuery>(parsed_query.query) &&
         !utils::Downcast<InfoQuery>(parsed_query.query)) {
       db_accessor_.emplace(interpreter_context_->db->Access());
@@ -1091,7 +1086,7 @@ Interpreter::Prepare(
     } else if (utils::Downcast<DumpQuery>(parsed_query.query)) {
       prepared_query =
           PrepareDumpQuery(std::move(parsed_query), &summary_,
-                           interpreter_context_, &execution_memory_);
+                           &*execution_db_accessor_, &execution_memory_);
     } else if (utils::Downcast<IndexQuery>(parsed_query.query)) {
       prepared_query = PrepareIndexQuery(
           std::move(parsed_query), in_explicit_transaction_, &summary_,

tests/unit/query_dump.cpp

@@ -783,3 +783,92 @@ TEST(DumpTest, ExecuteDumpDatabase) {
               "MATCH (u) REMOVE u:__mg_vertex__, u.__mg_id__;");
   }
 }
+
+class StatefulInterpreter {
+ public:
+  explicit StatefulInterpreter(storage::Storage *db)
+      : db_(db), context_(db_), interpreter_(&context_) {}
+
+  auto Execute(const std::string &query) {
+    ResultStreamFaker stream(db_);
+
+    auto [header, _] = interpreter_.Prepare(query, {});
+    stream.Header(header);
+    auto summary = interpreter_.PullAll(&stream);
+    stream.Summary(summary);
+
+    return stream;
+  }
+
+ private:
+  storage::Storage *db_;
+  query::InterpreterContext context_;
+  query::Interpreter interpreter_;
+};
+
+// NOLINTNEXTLINE(hicpp-special-member-functions)
+TEST(DumpTest, ExecuteDumpDatabaseInMulticommandTransaction) {
+  storage::Storage db;
+  StatefulInterpreter interpreter(&db);
+
+  // Begin the transaction before the vertex is created.
+  interpreter.Execute("BEGIN");
+
+  // Verify that nothing is dumped.
+  {
+    auto stream = interpreter.Execute("DUMP DATABASE");
+    const auto &header = stream.GetHeader();
+    const auto &results = stream.GetResults();
+    ASSERT_EQ(header.size(), 1U);
+    ASSERT_EQ(header[0], "QUERY");
+    ASSERT_EQ(results.size(), 0U);
+  }
+
+  // Create the vertex.
+  {
+    auto dba = db.Access();
+    CreateVertex(&dba, {}, {}, false);
+    ASSERT_FALSE(dba.Commit().HasError());
+  }
+
+  // Verify that nothing is dumped.
+  {
+    auto stream = interpreter.Execute("DUMP DATABASE");
+    const auto &header = stream.GetHeader();
+    const auto &results = stream.GetResults();
+    ASSERT_EQ(header.size(), 1U);
+    ASSERT_EQ(header[0], "QUERY");
+    ASSERT_EQ(results.size(), 0U);
+  }
+
+  // Rollback the transaction.
+  interpreter.Execute("ROLLBACK");
+
+  // Start a new transaction, this transaction should see the vertex.
+  interpreter.Execute("BEGIN");
+
+  // Verify that the vertex is dumped.
+  {
+    auto stream = interpreter.Execute("DUMP DATABASE");
+    const auto &header = stream.GetHeader();
+    const auto &results = stream.GetResults();
+    ASSERT_EQ(header.size(), 1U);
+    EXPECT_EQ(header[0], "QUERY");
+    EXPECT_EQ(results.size(), 4U);
+    for (const auto &item : results) {
+      EXPECT_EQ(item.size(), 1);
+      EXPECT_TRUE(item[0].IsString());
+    }
+    EXPECT_EQ(results[0][0].ValueString(),
+              "CREATE INDEX ON :__mg_vertex__(__mg_id__);");
+    EXPECT_EQ(results[1][0].ValueString(),
+              "CREATE (:__mg_vertex__ {__mg_id__: 0});");
+    EXPECT_EQ(results[2][0].ValueString(),
+              "DROP INDEX ON :__mg_vertex__(__mg_id__);");
+    EXPECT_EQ(results[3][0].ValueString(),
+              "MATCH (u) REMOVE u:__mg_vertex__, u.__mg_id__;");
+  }
+
+  // Rollback the transaction.
+  interpreter.Execute("ROLLBACK");
+}