Mvcc - unit test infrastructure setup

Summary:
Gradicek's Mvcc test have seen the following changes:
- provided a test infrastructure (fixture and macros) to facilitate testing and increase readability
- split tests into multi-transaction update, VersionList::find and general Mvcc testing
- multi-transaction update tests have been refactored (i *think* nothing got deleted, but it was a mess so I don't guarantee)
- changed all multithreaded tests to be single-threaded because multiple threads were not necessary
- changed transaction naming from T5, T8, T10 to T1, T2... for consistency with actual transaction indices

What still needs to be done:
- Gleich and Gradicek need to review the infrastructure (possible improvements)
- multi-transaction update tests need to be addressed by Gradicek (see "TODO gradicek" in code, discuss with Flor)
- the wiki/draw.io documentation needs to be updated. it is not imperative that all the tests be drawn in draw.io, only the general infrastructure explained. perhaps only a few examples drawn. Gradicek discuss with Flor
- Gleich see the "TODO Gleich" lines in the diff and discuss with flor

Suggested workflow:
- review this diff, hopefully land (before resolving all the TODOs)
- discard D169
- Gradicek and Gleich address the TODOs
- Flor reviews the results (in following diffs)

Reviewers: dgleich, matej.gradicek, buda

Reviewed By: matej.gradicek, buda

Subscribers: pullbot

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

src/storage/locking/record_lock.hpp

@@ -5,7 +5,7 @@
 #include "threading/sync/futex.hpp"
 
 class RecordLock {
-  static constexpr struct timespec timeout { 20, 0 };
+  static constexpr struct timespec timeout { 2, 0 };
   static constexpr Id INVALID = Id();
 
  public:

src/transactions/lock_store.hpp

@@ -14,7 +14,7 @@ class LockStore {
     LockHolder() noexcept = default;
 
     template <class... Args>
-    LockHolder(T *lock, Args &&... args) noexcept : lock(lock) {
+    LockHolder(T *lock, Args &&... args) : lock(lock) {
       debug_assert(lock != nullptr, "Lock is nullptr.");
       auto status = lock->lock(std::forward<Args>(args)...);
 

tests/unit/mvcc.cpp

@@ -1,46 +1,35 @@
-#include "gmock/gmock.h"
+#include <vector>
+#include "gc_common.hpp"
 #include "gtest/gtest.h"
 
 #include "mvcc/id.hpp"
 #include "mvcc/record.hpp"
+#include "mvcc/version.hpp"
 #include "mvcc/version_list.hpp"
-#include "storage/vertex.hpp"
+#include "threading/sync/lock_timeout_exception.hpp"
 #include "transactions/engine.hpp"
+#include "transactions/transaction.cpp"
 
-#include "gc_common.hpp"
+class TestClass : public mvcc::Record<TestClass> {};
 
-TEST(MVCC, Case1Test3) {
+TEST(MVCC, Deadlock) {
   tx::Engine engine;
+
+  auto t0 = engine.begin();
+  mvcc::VersionList<TestClass> version_list1(*t0);
+  mvcc::VersionList<TestClass> version_list2(*t0);
+  t0->commit();
+
   auto t1 = engine.begin();
-  mvcc::VersionList<Prop> version_list(*t1);
-  t1->commit();
-
   auto t2 = engine.begin();
-  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(*t3), mvcc::SerializationError);
+  version_list1.update(*t1);
+  version_list2.update(*t2);
+  EXPECT_THROW(version_list1.update(*t2), LockTimeoutException);
 }
 
-TEST(MVCC, InSnapshotSerializationError) {
-  tx::Engine engine;
-  auto t1 = engine.begin();
-  mvcc::VersionList<Prop> version_list(*t1);
-  t1->commit();
-
-  auto t2 = engine.begin();
-  version_list.update(*t2);
-  auto t3 = engine.begin();  // t2 is in snapshot of t3
-  t2->commit();
-
-  EXPECT_THROW(version_list.update(*t3), mvcc::SerializationError);
-}
-
-// Check that we don't delete records when we re-link.
+// TODO Gleich: move this test to mvcc_gc???
+// check that we don't delete records when we re-link
 TEST(MVCC, UpdateDontDelete) {
   std::atomic<int> count{0};
   {
@@ -60,6 +49,7 @@ TEST(MVCC, UpdateDontDelete) {
     version_list.update(*t3);
     EXPECT_EQ(count, 0);
 
+    // TODO Gleich: why don't we also test that remove doesn't delete?
     t3->commit();
   }
   EXPECT_EQ(count, 3);
@@ -69,17 +59,18 @@ TEST(MVCC, UpdateDontDelete) {
 TEST(MVCC, Oldest) {
   tx::Engine engine;
   auto t1 = engine.begin();
-  mvcc::VersionList<Prop> version_list(*t1);
+  mvcc::VersionList<TestClass> version_list(*t1);
   auto first = version_list.Oldest();
   EXPECT_NE(first, nullptr);
+  // TODO Gleich: no need to do 10 checks of the same thing
   for (int i = 0; i < 10; ++i) {
     engine.advance(t1->id);
     version_list.update(*t1);
     EXPECT_EQ(version_list.Oldest(), first);
   }
+  // TODO Gleich: what about remove?
+  // TODO Gleich: here it might make sense to write a concurrent test
+  //    since these ops rely heavily on linkage atomicity?
 }
 
-int main(int argc, char **argv) {
-  ::testing::InitGoogleTest(&argc, argv);
-  return RUN_ALL_TESTS();
-}
+// TODO Gleich: perhaps some concurrent VersionList::find tests?

tests/unit/mvcc_find.cpp

@@ -0,0 +1,106 @@
+#include "mvcc_find_update_common.hpp"
+
+TEST_F(Mvcc, FindUncommittedHigherTXUpdate) {
+  T2_FIND;
+  T3_BEGIN;
+  T3_UPDATE;
+  EXPECT_EQ(v2, version_list.find(*t2));
+}
+
+TEST_F(Mvcc, FindCommittedHigherTXUpdate) {
+  T2_FIND;
+  T3_BEGIN;
+  T3_UPDATE;
+  T3_COMMIT;
+  EXPECT_EQ(v2, version_list.find(*t2));
+}
+
+TEST_F(Mvcc, FindAbortedHigherTXUpdate) {
+  T2_FIND;
+  T3_BEGIN;
+  T3_UPDATE;
+  T3_ABORT;
+  EXPECT_EQ(v2, version_list.find(*t2));
+}
+
+TEST_F(Mvcc, FindCommittedLowerTXUpdate) {
+  T2_UPDATE;
+  T3_BEGIN;
+  T3_FIND;
+  T2_COMMIT;
+  EXPECT_EQ(v3, version_list.find(*t3));
+}
+
+TEST_F(Mvcc, FindAbortedLowerTXUpdate) {
+  T2_UPDATE;
+  T3_BEGIN;
+  T3_FIND;
+  T2_ABORT;
+  EXPECT_EQ(v3, version_list.find(*t3));
+}
+
+TEST_F(Mvcc, FindUncommittedHigherTXRemove) {
+  T2_FIND;
+  T3_BEGIN;
+  T3_REMOVE;
+  EXPECT_EQ(v2, version_list.find(*t2));
+}
+
+TEST_F(Mvcc, FindCommittedHigherTXRemove) {
+  T2_FIND;
+  T3_BEGIN;
+  T3_REMOVE;
+  T3_COMMIT;
+  EXPECT_EQ(v2, version_list.find(*t2));
+}
+
+TEST_F(Mvcc, FindAbortedHigherTXRemove) {
+  T2_FIND;
+  T3_BEGIN;
+  T3_REMOVE;
+  T3_ABORT;
+  EXPECT_EQ(v2, version_list.find(*t2));
+}
+
+TEST_F(Mvcc, FindCommittedLowerTXRemove) {
+  T2_REMOVE;
+  T3_BEGIN;
+  T3_FIND;
+  EXPECT_EQ(v3, version_list.find(*t3));
+}
+
+TEST_F(Mvcc, FindAbortedLowerTXRemove) {
+  T2_REMOVE;
+  T3_BEGIN;
+  T3_FIND;
+  T2_ABORT;
+  EXPECT_EQ(v3, version_list.find(*t3));
+}
+
+TEST_F(Mvcc, ReadUncommitedUpdateFromSameTXSameCommand) {
+  T2_UPDATE;
+  EXPECT_NE(v2, version_list.find(*t2));
+}
+
+TEST_F(Mvcc, ReadUncommitedUpdateFromSameTXNotSameCommand) {
+  T2_UPDATE;
+  engine.advance(t2->id);
+  EXPECT_EQ(v2, version_list.find(*t2));
+}
+
+TEST_F(Mvcc, ReadUncommitedRemoveFromSameTXSameCommand) {
+  T2_UPDATE;
+  T2_COMMIT;
+  T3_BEGIN;
+  T3_REMOVE;
+  EXPECT_EQ(v2, version_list.find(*t3));
+}
+
+TEST_F(Mvcc, ReadUncommitedRemoveFromSameTXNotSameCommand) {
+  T2_UPDATE;
+  T2_COMMIT;
+  T3_BEGIN;
+  T3_REMOVE;
+  engine.advance(t3->id);
+  EXPECT_NE(v2, version_list.find(*t3));
+}

tests/unit/mvcc_find_update_common.hpp

@@ -0,0 +1,78 @@
+#include <vector>
+#include "gtest/gtest.h"
+
+#include "mvcc/id.hpp"
+#include "mvcc/record.hpp"
+#include "mvcc/version.hpp"
+#include "mvcc/version_list.hpp"
+#include "threading/sync/lock_timeout_exception.hpp"
+#include "transactions/engine.hpp"
+#include "transactions/transaction.cpp"
+
+// make it easy to compare Id with int
+bool operator==(const Id &left, const int right) {
+  return static_cast<uint64_t>(left) == static_cast<uint64_t>(right);
+}
+
+class TestClass : public mvcc::Record<TestClass> {
+  friend std::ostream &operator<<(std::ostream &stream, TestClass &test_class) {
+    stream << test_class.tx.cre() << " " << test_class.tx.exp();
+    return stream;
+  }
+};
+
+/**
+ * Testing mvcc::VersionList::find behavior in
+ * different situations (preceeding update/remove ops
+ * in different transactions).
+ *
+ * The setup for each case is:
+ *  - transaction t1 has created a new version_list v1 and commited
+ *  - transaction t2 has strated
+ *  - *********************
+ *  - here the test fixture ends and custom test behavior should be added
+ *  - *********************
+ *  - tests should check every legal sequence of the following ops
+ *      - creation of transaction t3
+ *      - [commit/abort] of [t2/t3]
+ *      - [removal/update] on version_list by [t2/t3]
+ *  - illegal sequences (for example double commit) don't have to be checked
+ */
+class Mvcc : public ::testing::Test {
+ protected:
+  virtual void SetUp() {
+    t1 = &engine.advance(t1->id);
+    v1 = version_list.find(*t1);
+    t1->commit();
+    t2 = engine.begin();
+  }
+
+  tx::Engine engine;
+  tx::Transaction *t1 = engine.begin();
+  mvcc::VersionList<TestClass> version_list{*t1};
+  TestClass *v1 = nullptr;
+  tx::Transaction *t2 = nullptr;
+};
+
+// helper macros. important:
+//  - TX_FIND and TX_UPDATE set the record variable vX
+//  - TX_BEGIN sets the transaction variable tX
+#define T2_FIND __attribute__((unused)) auto v2 = version_list.find(*t2)
+#define T3_FIND __attribute__((unused)) auto v3 = version_list.find(*t3)
+#define T2_UPDATE __attribute__((unused)) auto v2 = version_list.update(*t2)
+#define T3_UPDATE __attribute__((unused)) auto v3 = version_list.update(*t3)
+#define T2_COMMIT t2->commit();
+#define T3_COMMIT t3->commit();
+#define T2_ABORT t2->abort();
+#define T3_ABORT t3->abort();
+#define T3_BEGIN auto t3 = engine.begin();
+#define T2_REMOVE version_list.remove(*t2)
+#define T3_REMOVE version_list.remove(*t3)
+#define EXPECT_CRE(record, expected) EXPECT_EQ(record->tx.cre(), expected)
+#define EXPECT_EXP(record, expected) EXPECT_EQ(record->tx.exp(), expected)
+
+// test the fixture
+TEST_F(Mvcc, Fixture) {
+  EXPECT_CRE(v1, 1);
+  EXPECT_EXP(v1, 0);
+}

tests/unit/mvcc_parallel_update.cpp

@@ -0,0 +1,88 @@
+#include "mvcc_find_update_common.hpp"
+
+// TODO Gradicek: rename all existing cases
+// TODO Gradicek: check validity of all existing cases
+// TODO Gradicek: add all other cases (48 in total, discuss with Flor)
+// TODO Gradicek: what about command advance testing,
+//                as opposed to transaction commit/abort?
+
+TEST_F(Mvcc, Case1_InsertWithUpdates) {
+  T2_UPDATE;
+  T2_COMMIT;
+  T3_BEGIN;
+  T3_REMOVE;
+  T3_COMMIT;
+  EXPECT_EXP(v2, 3);
+}
+
+TEST_F(Mvcc, RemoveUpdatedRecord) {
+  T3_BEGIN;
+  T3_UPDATE;
+  T3_COMMIT;
+  EXPECT_THROW(T2_REMOVE, mvcc::SerializationError);
+}
+
+TEST_F(Mvcc, UpdateUpdatedRecord) {
+  T3_BEGIN;
+  T3_UPDATE;
+  T3_COMMIT;
+  EXPECT_THROW(version_list.update(*t2), mvcc::SerializationError);
+}
+
+TEST_F(Mvcc, Case2_AbortUpdate_Remove_T10) {
+  T2_UPDATE;
+  T2_ABORT;
+  T3_BEGIN;
+  T3_REMOVE;
+  T3_COMMIT;
+
+  EXPECT_CRE(v1, 1);
+  EXPECT_EXP(v1, 3);
+  EXPECT_CRE(v2, 2);
+  EXPECT_EXP(v2, 0);
+}
+
+TEST_F(Mvcc, Case2_AbortUpdate_Remove_T7) {
+  T3_BEGIN;
+  T3_UPDATE;
+  T3_ABORT;
+  T2_REMOVE;
+  T2_COMMIT;
+  EXPECT_CRE(v1, 1);
+  EXPECT_EXP(v1, 2);
+  EXPECT_CRE(v3, 3);
+  EXPECT_EXP(v3, 0);
+}
+
+TEST_F(Mvcc, Case2_AbortUpdate_Update_T10) {
+  T2_UPDATE;
+  T2_ABORT;
+  T3_BEGIN;
+  T3_UPDATE;
+  T3_COMMIT;
+
+  EXPECT_CRE(v1, 1);
+  EXPECT_EXP(v1, 3);
+  EXPECT_CRE(v3, 3);
+  EXPECT_EXP(v3, 0);
+}
+
+TEST_F(Mvcc, Case2_AbortUpdate_Update_T7) {
+  T3_BEGIN;
+  T3_UPDATE;
+  T3_ABORT;
+  T2_UPDATE;
+  T2_COMMIT;
+
+  EXPECT_CRE(v3, 3);
+  EXPECT_EXP(v3, 0);
+  EXPECT_CRE(v2, 2);
+  EXPECT_EXP(v2, 0);
+}
+
+TEST_F(Mvcc, Case1Test3) {
+  T3_BEGIN;
+  T3_UPDATE;
+  T3_COMMIT;
+  EXPECT_THROW(T2_REMOVE, mvcc::SerializationError);
+}