Fix mvcc bug.

Summary: Changing mvcc again. This should be double-checked since it's quite a substantial change. The test here tests for the new behaviour - which should be correct.

Reviewers: matej.gradicek, dtomicevic, buda

Reviewed By: matej.gradicek, buda

Subscribers: pullbot

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

src/mvcc/record.hpp

@@ -128,13 +128,19 @@ class Record : public Version<T> {
  protected:
   template <class U>
   bool committed(U &hints, const Id &id, const tx::Transaction &t) {
+    // This whole section below is commented out because even though you can't
+    // see something with greater id you should still able to check if it's
+    // commited - consult the MVCC tests to better understand this behaviour.
     // you certainly can't see the transaction with id greater than yours
     // as that means it started after this transaction and if it committed,
     // it committed after this transaction had started.
-    if (id >= t.id) return false;
+    // if (id >= t.id) return false;
 
+    // This is commented out because something even though was started before
+    // and not ended yet could have ended in the meantime and data should now be
+    // visible if it's commited.
     // The creating transaction is still in progress (examine snapshot)
-    if (t.in_snapshot(id)) return false;
+    // if (t.in_snapshot(id)) return false;
 
     auto hint_bits = hints.load();
 
@@ -147,10 +153,18 @@ class Record : public Version<T> {
     auto info = t.engine.clog.fetch_info(id);
 
     if (info.is_committed()) return hints.set_committed(), true;
+    if (info.is_aborted()) return hints.set_aborted(), false;
 
-    debug_assert(info.is_aborted(),
-                 "Info isn't aborted, but function would return as aborted.");
-    return hints.set_aborted(), false;
+    return false;
+    /*
+      BLAME dgleich and matej.gradicek. This check shouldn't be done here since
+      this function should only check if transaction commited or not, and not
+      actually assume that transaction associated with this record either
+      aborted or commited.
+      debug_assert(info.is_aborted(),
+                  "Info isn't aborted, but function would return as aborted.");
+      return hints.set_aborted(), false;
+    */
   }
 };
 }

tests/unit/mvcc.cpp

@@ -0,0 +1,44 @@
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+#include "mvcc/record.hpp"
+#include "mvcc/version_list.hpp"
+#include "storage/vertex.hpp"
+#include "transactions/engine.hpp"
+
+class Prop : public mvcc::Record<Prop> {};
+
+TEST(MVCC, Case1Test3) {
+  tx::Engine engine;
+  mvcc::VersionList<Prop> version_list;
+  auto t1 = engine.begin();
+  version_list.insert(*t1);
+  t1->commit();
+
+  auto t2 = engine.begin();
+  auto v2 = version_list.update(*t2);
+  t2->commit();
+
+  auto t3 = engine.begin();
+  auto t4 = engine.begin();
+  version_list.update(*t4);
+  t4->commit();
+  EXPECT_THROW(version_list.remove(v2, *t3), SerializationError);
+}
+
+TEST(MVCC, InSnapshot) {
+  tx::Engine engine;
+  mvcc::VersionList<Prop> version_list;
+  auto t1 = engine.begin();
+  auto v1 = version_list.insert(*t1);
+  version_list.update(*t1);  // expire old record and create new
+  auto t2 = engine.begin();  // t1 is in snapshot of t2
+  t1->commit();
+
+  EXPECT_THROW(version_list.update(v1, *t2), SerializationError);
+}
+
+int main(int argc, char **argv) {
+  ::testing::InitGoogleTest(&argc, argv);
+  return RUN_ALL_TESTS();
+}