From 1333bfeb39f8d2528c1a9acb0900c497f1ddacaa Mon Sep 17 00:00:00 2001 From: Florijan Stamenkovic Date: Fri, 5 May 2017 11:05:56 +0200 Subject: [PATCH] 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 | 2 +- src/transactions/lock_store.hpp | 2 +- tests/unit/mvcc.cpp | 59 ++++++-------- tests/unit/mvcc_find.cpp | 106 +++++++++++++++++++++++++ tests/unit/mvcc_find_update_common.hpp | 78 ++++++++++++++++++ tests/unit/mvcc_parallel_update.cpp | 88 ++++++++++++++++++++ 6 files changed, 299 insertions(+), 36 deletions(-) create mode 100644 tests/unit/mvcc_find.cpp create mode 100644 tests/unit/mvcc_find_update_common.hpp create mode 100644 tests/unit/mvcc_parallel_update.cpp diff --git a/src/storage/locking/record_lock.hpp b/src/storage/locking/record_lock.hpp index 55c14caea..4427236f7 100644 --- a/src/storage/locking/record_lock.hpp +++ b/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: diff --git a/src/transactions/lock_store.hpp b/src/transactions/lock_store.hpp index cc0501b9c..55c474065 100644 --- a/src/transactions/lock_store.hpp +++ b/src/transactions/lock_store.hpp @@ -14,7 +14,7 @@ class LockStore { LockHolder() noexcept = default; template - 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)...); diff --git a/tests/unit/mvcc.cpp b/tests/unit/mvcc.cpp index 6274a0c44..97068f57d 100644 --- a/tests/unit/mvcc.cpp +++ b/tests/unit/mvcc.cpp @@ -1,46 +1,35 @@ -#include "gmock/gmock.h" +#include +#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 {}; -TEST(MVCC, Case1Test3) { +TEST(MVCC, Deadlock) { tx::Engine engine; + + auto t0 = engine.begin(); + mvcc::VersionList version_list1(*t0); + mvcc::VersionList version_list2(*t0); + t0->commit(); + auto t1 = engine.begin(); - mvcc::VersionList 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 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 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 version_list(*t1); + mvcc::VersionList 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? diff --git a/tests/unit/mvcc_find.cpp b/tests/unit/mvcc_find.cpp new file mode 100644 index 000000000..6bbd291aa --- /dev/null +++ b/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)); +} diff --git a/tests/unit/mvcc_find_update_common.hpp b/tests/unit/mvcc_find_update_common.hpp new file mode 100644 index 000000000..5001c478d --- /dev/null +++ b/tests/unit/mvcc_find_update_common.hpp @@ -0,0 +1,78 @@ +#include +#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(left) == static_cast(right); +} + +class TestClass : public mvcc::Record { + 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 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); +} diff --git a/tests/unit/mvcc_parallel_update.cpp b/tests/unit/mvcc_parallel_update.cpp new file mode 100644 index 000000000..600d84e49 --- /dev/null +++ b/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); +}