Query::Plan - Reconstruct fails after delete

Reviewers: teon.banek

Reviewed By: teon.banek

Subscribers: pullbot

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

src/database/graph_db_accessor.hpp

@@ -251,12 +251,17 @@ class GraphDbAccessor {
    * The old_ and new_ pointers need to be initialized
    * with appropriate values, and current_ set to old_
    * if it exists and to new_ otherwise.
+   *
+   * @return True if accessor is valid after reconstruction.
+   * This means that at least one record pointer was found
+   * (either new_ or old_), possibly both.
    */
   template <typename TRecord>
-  void Reconstruct(RecordAccessor<TRecord> &accessor) {
+  bool Reconstruct(RecordAccessor<TRecord> &accessor) {
     accessor.vlist_->find_set_new_old(*transaction_, accessor.old_,
                                       accessor.new_);
     accessor.current_ = accessor.old_ ? accessor.old_ : accessor.new_;
+    return accessor.old_ != nullptr || accessor.new_ != nullptr;
     // TODO review: we should never use a record accessor that
     // does not have either old_ or new_ (both are null), but
     // we can't assert that here because we construct such an accessor

src/query/plan/operator.cpp

@@ -899,10 +899,16 @@ namespace {
 void ReconstructTypedValue(TypedValue &value) {
   switch (value.type()) {
     case TypedValue::Type::Vertex:
-      value.Value<VertexAccessor>().Reconstruct();
+      if (!value.Value<VertexAccessor>().Reconstruct())
+        throw QueryRuntimeException(
+            "Vertex invalid after WITH clause, (most likely deleted by a "
+            "preceeding DELETE clause)");
       break;
     case TypedValue::Type::Edge:
-      value.Value<EdgeAccessor>().Reconstruct();
+      if (!value.Value<VertexAccessor>().Reconstruct())
+        throw QueryRuntimeException(
+            "Edge invalid after WITH clause, (most likely deleted by a "
+            "preceeding DELETE clause)");
       break;
     case TypedValue::Type::List:
       for (TypedValue &inner_value : value.Value<std::vector<TypedValue>>())

src/storage/record_accessor.cpp

@@ -1,6 +1,6 @@
+#include "storage/record_accessor.hpp"
 #include "database/graph_db_accessor.hpp"
 #include "storage/edge.hpp"
-#include "storage/record_accessor.hpp"
 #include "storage/vertex.hpp"
 #include "utils/assert.hpp"
 
@@ -59,13 +59,11 @@ RecordAccessor<TRecord> &RecordAccessor<TRecord>::SwitchNew() {
     // we can just Reconstruct the pointers, old_ will get initialized
     // to the same value as it has now, and the amount of work is the
     // same as just looking for a new_ record
-    Reconstruct();
+    if (!Reconstruct())
+      debug_fail(
+          "RecordAccessor::SwitchNew - accessor invalid after Reconstruct");
   }
-  // set new if we have it
-  // if we don't then current_ is old_ and remains there
-  if (new_) current_ = new_;
-  debug_assert(current_ != nullptr,
-               "RecordAccessor::SwitchNew - current_ is nullptr");
+  current_ = new_ ? new_ : old_;
   return *this;
 }
 
@@ -81,8 +79,8 @@ RecordAccessor<TRecord> &RecordAccessor<TRecord>::SwitchOld() {
 }
 
 template <typename TRecord>
-void RecordAccessor<TRecord>::Reconstruct() {
-  db_accessor().Reconstruct(*this);
+bool RecordAccessor<TRecord>::Reconstruct() {
+  return db_accessor().Reconstruct(*this);
 }
 
 template <typename TRecord>

src/storage/record_accessor.hpp

@@ -159,9 +159,11 @@ class RecordAccessor {
    * accessor so it uses the versions appropriate
    * to this transaction+command.
    *
-   * TODO consider what it does after delete+advance_command
+   * @return True if this accessor is valid after reconstruction.
+   * This means that at least one record pointer was found
+   * (either new_ or old_), possibly both.
    */
-  void Reconstruct();
+  bool Reconstruct();
 
  protected:
   /**

src/utils/assert.hpp

@@ -56,6 +56,8 @@
  */
 #ifdef DEBUG_ASSERT_ON
 #define debug_assert(condition, message) permanent_assert(condition, message)
+#define debug_fail(message) permanent_fail(message)
 #else
-#define debug_assert(condition, message)
+#define debug_assert(condition, message) {}
+#define debug_fail(message) {}
 #endif

tests/unit/query_plan_create_set_remove_delete.cpp

@@ -412,6 +412,34 @@ TEST(QueryPlan, DeleteReturn) {
   EXPECT_EQ(0, CountIterable(dba->vertices()));
 }
 
+TEST(QueryPlan, DeleteAdvance) {
+  // test queries on empty DB:
+  // CREATE (n)
+  // MATCH (n) DELETE n WITH n ...
+  // this fails due to us advancing the command
+  // when processing the WITH clause
+  //
+  // note that Neo does not fail when the deleted
+  // record is not used in subsequent clauses, but
+  // we are not yet compatible with that
+  Dbms dbms;
+  auto dba = dbms.active();
+  dba->insert_vertex();
+  dba->advance_command();
+
+  AstTreeStorage storage;
+  SymbolTable symbol_table;
+
+  auto n = MakeScanAll(storage, symbol_table, "n");
+  auto n_get = storage.Create<Identifier>("n");
+  symbol_table[*n_get] = n.sym_;
+  auto delete_op = std::make_shared<plan::Delete>(
+      n.op_, std::vector<Expression *>{n_get}, false);
+  auto advance = std::make_shared<Accumulate>(
+      delete_op, std::vector<Symbol>{n.sym_}, true);
+  EXPECT_THROW(PullAll(advance, *dba, symbol_table), QueryRuntimeException);
+}
+
 TEST(QueryPlan, SetProperty) {
   Dbms dbms;
   auto dba = dbms.active();