Mvcc.GcDeleted bugfix and test

Summary: Changed mvcc::VersionList::GcDeleted to look for the oldest visible record.

Reviewers: teon.banek, buda, dgleich

Reviewed By: teon.banek, dgleich

Subscribers: pullbot

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

src/mvcc/version_list.hpp

@@ -85,52 +85,55 @@ class VersionList {
    * visible anymore. If none exists to_delete will point to nullptr.
   */
   std::pair<bool, T *> GcDeleted(const Id &id, tx::Engine &engine) {
-    auto newest_deleted_record = head.load(std::memory_order_seq_cst);
-    T *oldest_not_deleted_record = nullptr;
 
     //    nullptr
     //       |
-    //     [v1]      ...
+    //     [v1]      ...  all of this gets deleted!
     //       |
-    //     [v2] <------+  newest_deleted_record
+    //     [v2] <------+  head_of_deletable_records
     //       |         |
-    //     [v3] <------+  oldest_not_deleted_record
+    //     [v3] <------+  oldest_visible_record
-    //       |         |  Jump backwards until you find a first old deleted
+    //       |         |  Jump backwards until you find the oldest visible
     //   [VerList] ----+  record, or you reach the end of the list
     //
-    while (newest_deleted_record != nullptr &&
+    
-           !newest_deleted_record->is_not_visible_from(id, engine)) {
+    auto current = head.load();
-      oldest_not_deleted_record = newest_deleted_record;
+    T *head_of_deletable_records = current;
-      newest_deleted_record =
+    T *oldest_visible_record = nullptr;
-          newest_deleted_record->next(std::memory_order_seq_cst);
+    while (current) {
+      if (!current->is_not_visible_from(id, engine))
+          oldest_visible_record = current;
+        current = current->next();
     }
+    if (oldest_visible_record)
+      head_of_deletable_records = oldest_visible_record->next();
 
     // This can happen only if the head already points to a deleted record or
     // the version list is empty. This means that the version_list is ready
     // for complete destruction.
-    if (oldest_not_deleted_record == nullptr) {
+    if (oldest_visible_record == nullptr) {
       // Head points to a deleted record.
-      if (newest_deleted_record != nullptr) {
+      if (head_of_deletable_records != nullptr) {
         head.store(nullptr, std::memory_order_seq_cst);
         // This is safe to return as ready for deletion since we unlinked head
         // above and this will only be deleted after the last active transaction
         // ends.
-        return std::make_pair(true, newest_deleted_record);
+        return std::make_pair(true, head_of_deletable_records);
       }
       return std::make_pair(true, nullptr);
     }
-    // oldest_not_deleted_record might be visible to some transaction but
+    // oldest_visible_record might be visible to some transaction but
-    // newest_deleted_record is not and will never be visted by the find
+    // head_of_deletable_records is not and will never be visted by the find
     // function and as such doesn't represent pointer invalidation
     // race-condition risk.
-    oldest_not_deleted_record->next(
+    oldest_visible_record->next(
         nullptr, std::memory_order_seq_cst);  // No transaction will look
                                               // further than this record and
                                               // that's why it's safe to set
                                               // next to nullptr.
-    // Calling destructor  of newest_deleted_record will clean everything older
+    // Calling destructor  of head_of_deletable_records will clean everything older
     // than this record since they are called recursively.
-    return std::make_pair(false, newest_deleted_record);
+    return std::make_pair(false, head_of_deletable_records);
   }
 
   /**

tests/unit/mvcc_gc.cpp

@@ -22,8 +22,9 @@
  * are not longer visible.
  */
 TEST(VersionList, GcDeleted) {
-  const int UPDATES = 10;
   tx::Engine engine;
+
+  // create a version_list with one record
   std::vector<uint64_t> ids;
   auto t1 = engine.begin();
   std::atomic<int> count{0};
@@ -31,6 +32,8 @@ TEST(VersionList, GcDeleted) {
   ids.push_back(t1->id);
   t1->commit();
 
+  // create some updates
+  const int UPDATES = 10;
   for (int i = 0; i < UPDATES; ++i) {
     auto t2 = engine.begin();
     ids.push_back(t2->id);
@@ -38,36 +41,57 @@ TEST(VersionList, GcDeleted) {
     t2->commit();
   }
 
-  EXPECT_EQ(version_list.GcDeleted(ids[0], engine),
+  // deleting with the first transaction does nothing
-            std::make_pair(false, (PropCount *)nullptr));
+  {
-  EXPECT_EQ(count, 0);
+    auto ret = version_list.GcDeleted(ids[0], engine);
-  auto ret = version_list.GcDeleted(ids.back() + 1, engine);
+    EXPECT_EQ(ret.first, false);
-  EXPECT_EQ(ret.first, false);
+    EXPECT_EQ(ret.second, nullptr);
-  EXPECT_NE(ret.second, nullptr);
+  }
-  delete ret.second;
-  EXPECT_EQ(count, UPDATES);
 
-  auto tl = engine.begin();
+  // deleting with the last transaction + 1 deletes
-  version_list.remove(*tl);
+  // everything except the last update
-  auto id = tl->id + 1;
+  {
-  tl->abort();
+    auto ret = version_list.GcDeleted(ids.back() + 1, engine);
+    EXPECT_EQ(ret.first, false);
+    EXPECT_NE(ret.second, nullptr);
+    delete ret.second;
+    EXPECT_EQ(count, UPDATES);
+  }
 
-  auto ret2 = version_list.GcDeleted(id, engine);
+  // remove and abort, nothing gets deleted
-  EXPECT_EQ(ret2.first, false);
+  {
-  EXPECT_EQ(ret2.second, nullptr);
+    auto t = engine.begin();
+    version_list.remove(*t);
+    auto id = t->id + 1;
+    t->abort();
+    auto ret = version_list.GcDeleted(id, engine);
+    EXPECT_EQ(ret.first, false);
+    EXPECT_EQ(ret.second, nullptr);
+  }
 
-  auto tk = engine.begin();
+  // update and abort, nothing gets deleted
-  version_list.remove(*tk);
+  {
-  auto id2 = tk->id + 1;
+    auto t = engine.begin();
-  tk->commit();
+    version_list.update(*t);
+    auto id = t->id + 1;
+    t->abort();
+    auto ret = version_list.GcDeleted(id, engine);
+    EXPECT_EQ(ret.first, false);
+    EXPECT_EQ(ret.second, nullptr);
+  }
 
-  auto ret3 = version_list.GcDeleted(id2, engine);
+  // remove and commit, everything gets deleted
-  EXPECT_EQ(ret3.first, true);
+  {
-  EXPECT_NE(ret3.second, nullptr);
+    auto t = engine.begin();
-
+    version_list.remove(*t);
-  delete ret3.second;
+    auto id = t->id + 1;
-
+    t->commit();
-  EXPECT_EQ(count, UPDATES + 1);
+    auto ret = version_list.GcDeleted(id, engine);
+    EXPECT_EQ(ret.first, true);
+    EXPECT_NE(ret.second, nullptr);
+    delete ret.second;
+    EXPECT_EQ(count, UPDATES + 2);
+  }
 }
 
 /**