From 95ce34cc0a600925972dc0586a4b0dceacf6d1a6 Mon Sep 17 00:00:00 2001 From: florijan Date: Thu, 18 May 2017 15:23:08 +0200 Subject: [PATCH] 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 | 41 ++++++++++---------- tests/unit/mvcc_gc.cpp | 78 +++++++++++++++++++++++++-------------- 2 files changed, 73 insertions(+), 46 deletions(-) diff --git a/src/mvcc/version_list.hpp b/src/mvcc/version_list.hpp index 958ab90d4..7ce2a33be 100644 --- a/src/mvcc/version_list.hpp +++ b/src/mvcc/version_list.hpp @@ -85,52 +85,55 @@ class VersionList { * visible anymore. If none exists to_delete will point to nullptr. */ std::pair 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 - // | | Jump backwards until you find a first old deleted + // [v3] <------+ oldest_visible_record + // | | 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)) { - oldest_not_deleted_record = newest_deleted_record; - newest_deleted_record = - newest_deleted_record->next(std::memory_order_seq_cst); + + auto current = head.load(); + T *head_of_deletable_records = current; + T *oldest_visible_record = nullptr; + 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 - // newest_deleted_record is not and will never be visted by the find + // oldest_visible_record might be visible to some transaction but + // 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); } /** diff --git a/tests/unit/mvcc_gc.cpp b/tests/unit/mvcc_gc.cpp index 5835fdf45..aff1d2a96 100644 --- a/tests/unit/mvcc_gc.cpp +++ b/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 ids; auto t1 = engine.begin(); std::atomic 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), - std::make_pair(false, (PropCount *)nullptr)); - EXPECT_EQ(count, 0); - 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); + // deleting with the first transaction does nothing + { + auto ret = version_list.GcDeleted(ids[0], engine); + EXPECT_EQ(ret.first, false); + EXPECT_EQ(ret.second, nullptr); + } - auto tl = engine.begin(); - version_list.remove(*tl); - auto id = tl->id + 1; - tl->abort(); + // deleting with the last transaction + 1 deletes + // everything except the last update + { + 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); - EXPECT_EQ(ret2.first, false); - EXPECT_EQ(ret2.second, nullptr); + // remove and abort, nothing gets deleted + { + 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(); - version_list.remove(*tk); - auto id2 = tk->id + 1; - tk->commit(); + // update and abort, nothing gets deleted + { + auto t = engine.begin(); + 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); - EXPECT_EQ(ret3.first, true); - EXPECT_NE(ret3.second, nullptr); - - delete ret3.second; - - EXPECT_EQ(count, UPDATES + 1); + // remove and commit, everything gets deleted + { + auto t = engine.begin(); + version_list.remove(*t); + auto id = t->id + 1; + t->commit(); + 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); + } } /**