From 3bd5a882cfde2f2bbdc282fc58461e4209e2b5bb Mon Sep 17 00:00:00 2001 From: florijan Date: Wed, 29 Nov 2017 09:34:18 +0100 Subject: [PATCH] Remove Update and Reconstruct from GraphDbAccessor Summary: Code simplification made possible by making `locks_` `mutable` in `tx::Transaction`. Reviewers: dgleich, buda Reviewed By: buda Differential Revision: https://phabricator.memgraph.io/D1015 --- src/database/graph_db_accessor.hpp | 50 ------------------------------ src/mvcc/version_list.hpp | 11 +++---- src/storage/record_accessor.cpp | 21 +++++++++++-- src/storage/record_accessor.hpp | 29 ++++++++--------- src/transactions/transaction.cpp | 2 +- src/transactions/transaction.hpp | 4 +-- 6 files changed, 40 insertions(+), 77 deletions(-) diff --git a/src/database/graph_db_accessor.hpp b/src/database/graph_db_accessor.hpp index dca118039..9129ea8fb 100644 --- a/src/database/graph_db_accessor.hpp +++ b/src/database/graph_db_accessor.hpp @@ -519,56 +519,6 @@ class GraphDbAccessor { /** Return's the database's write-ahead log */ durability::WriteAheadLog &wal(); - /** - * Initializes the record pointers in the given accessor. - * The old_ and new_ pointers need to be initialized - * with appropriate values, and current_ set to old_ - * if it exists and to new_ otherwise. - * - * @return True if accessor is valid after reconstruction. - * This means that at least one record pointer was found - * (either new_ or old_), possibly both. - */ - template - bool Reconstruct(RecordAccessor &accessor) { - DCHECK(!commited_ && !aborted_) << "Accessor committed or aborted"; - accessor.vlist_->find_set_old_new(*transaction_, accessor.old_, - accessor.new_); - accessor.current_ = accessor.old_ ? accessor.old_ : accessor.new_; - return accessor.old_ != nullptr || accessor.new_ != nullptr; - // TODO review: we should never use a record accessor that - // does not have either old_ or new_ (both are null), but - // we can't assert that here because we construct such an accessor - // and filter it out in GraphDbAccessor::[Vertices|Edges] - // any ideas? - } - - /** - * Update accessor record with vlist. - * - * It is not legal - * to call this function on a Vertex/Edge that has - * been deleted in the current transaction+command. - * - * @args accessor whose record to update if possible. - */ - template - void Update(RecordAccessor &accessor) { - DCHECK(!commited_ && !aborted_) << "Accessor committed or aborted"; - // can't update a deleted record if: - // - we only have old_ and it hasn't been deleted - // - we have new_ and it hasn't been deleted - if (!accessor.new_) { - DCHECK(!accessor.old_->is_expired_by(*transaction_)) - << "Can't update a record deleted in the current transaction+commad"; - } else { - DCHECK(!accessor.new_->is_expired_by(*transaction_)) - << "Can't update a record deleted in the current transaction+command"; - } - - if (!accessor.new_) accessor.new_ = accessor.vlist_->update(*transaction_); - } - /** * Returns the current value of the counter with the given name, and * increments that counter. If the counter with the given name does not exist, diff --git a/src/mvcc/version_list.hpp b/src/mvcc/version_list.hpp index 1b4d0b7e6..d4975ac45 100644 --- a/src/mvcc/version_list.hpp +++ b/src/mvcc/version_list.hpp @@ -28,7 +28,7 @@ class VersionList { * creating the first Record (Version) in this VersionList. */ template - VersionList(tx::Transaction &t, int64_t id, Args &&... args) : id_(id) { + VersionList(const tx::Transaction &t, int64_t id, Args &&... args) : id_(id) { // TODO replace 'new' with something better auto *v1 = new T(std::forward(args)...); v1->mark_created(t); @@ -191,7 +191,7 @@ class VersionList { * * @param t The transaction */ - T *update(tx::Transaction &t) { + T *update(const tx::Transaction &t) { DCHECK(head_ != nullptr) << "Head is nullptr on update."; T *old_record = nullptr; T *new_record = nullptr; @@ -208,7 +208,7 @@ class VersionList { } /** Makes the given record as being expired by the given transaction. */ - void remove(T *record, tx::Transaction &t) { + void remove(T *record, const tx::Transaction &t) { DCHECK(record != nullptr) << "Record is nullptr on removal."; lock_and_validate(record, t); record->mark_expired(t); @@ -217,7 +217,7 @@ class VersionList { const int64_t id_; private: - void lock_and_validate(T *record, tx::Transaction &t) { + void lock_and_validate(T *record, const tx::Transaction &t) { DCHECK(record != nullptr) << "Record is nullptr on lock and validation."; // take a lock on this node @@ -231,7 +231,7 @@ class VersionList { throw SerializationError(); } - T *update(T *record, tx::Transaction &t) { + T *update(T *record, const tx::Transaction &t) { DCHECK(record != nullptr) << "Record is nullptr on update."; lock_and_validate(record, t); @@ -257,4 +257,3 @@ class VersionList { RecordLock lock_; }; } // namespace mvcc - diff --git a/src/storage/record_accessor.cpp b/src/storage/record_accessor.cpp index 401622c44..4de9c334b 100644 --- a/src/storage/record_accessor.cpp +++ b/src/storage/record_accessor.cpp @@ -108,12 +108,29 @@ RecordAccessor &RecordAccessor::SwitchOld() { template bool RecordAccessor::Reconstruct() { - return db_accessor().Reconstruct(*this); + vlist_->find_set_old_new(db_accessor_->transaction(), old_, new_); + current_ = old_ ? old_ : new_; + return old_ != nullptr || new_ != nullptr; + // We should never use a record accessor that does not have either old_ or + // new_ (both are null), but we can't assert that here because we construct + // such an accessor and filter it out in GraphDbAccessor::[Vertices|Edges]. } template TRecord &RecordAccessor::update() { - db_accessor().Update(*this); + auto &t = db_accessor_->transaction(); + // can't update a deleted record if: + // - we only have old_ and it hasn't been deleted + // - we have new_ and it hasn't been deleted + if (!new_) { + DCHECK(!old_->is_expired_by(t)) + << "Can't update a record deleted in the current transaction+commad"; + } else { + DCHECK(!new_->is_expired_by(t)) + << "Can't update a record deleted in the current transaction+command"; + } + + if (!new_) new_ = vlist_->update(t); DCHECK(new_ != nullptr) << "RecordAccessor.new_ is null after update"; return *new_; } diff --git a/src/storage/record_accessor.hpp b/src/storage/record_accessor.hpp index 51f43b8cd..e156d097b 100644 --- a/src/storage/record_accessor.hpp +++ b/src/storage/record_accessor.hpp @@ -130,34 +130,31 @@ class RecordAccessor : public TotalOrdering> { RecordAccessor &SwitchNew(); /** - * Attempts to switch this accessor to use the - * latest version not updated by the current transaction+command. - * If that is not possible (vertex/edge was created - * by the current transaction/command), it does nothing - * (current remains pointing to the new version). - * + * Attempts to switch this accessor to use the latest version not updated by + * the current transaction+command. If that is not possible (vertex/edge was + * created by the current transaction/command), it does nothing (current + * remains pointing to the new version). * @return A reference to this. */ RecordAccessor &SwitchOld(); /** - * Reconstructs the internal state of the record - * accessor so it uses the versions appropriate - * to this transaction+command. + Reconstructs the internal state of the record accessor so it uses the + versions appropriate to this transaction+command. * - * @return True if this accessor is valid after reconstruction. - * This means that at least one record pointer was found - * (either new_ or old_), possibly both. + @return True if this accessor is valid after reconstruction. This means that + at least one record pointer was found (either new_ or old_), possibly both. */ bool Reconstruct(); protected: /** - * Ensures there is an updateable version of the record - * in the version_list, and that the `new_` pointer - * points to it. Returns a reference to that version. + * Ensures there is an updateable version of the record in the version_list, + * and that the `new_` pointer points to it. Returns a reference to that + * version. * - * @return See above. + * It is not legal to call this function on a Vertex/Edge that has been + * deleted in the current transaction+command. */ TRecord &update(); diff --git a/src/transactions/transaction.cpp b/src/transactions/transaction.cpp index 3442ce7be..88d542e15 100644 --- a/src/transactions/transaction.cpp +++ b/src/transactions/transaction.cpp @@ -8,7 +8,7 @@ Transaction::Transaction(transaction_id_t id, const Snapshot &snapshot, Engine &engine) : id_(id), engine_(engine), snapshot_(snapshot) {} -void Transaction::TakeLock(RecordLock &lock) { +void Transaction::TakeLock(RecordLock &lock) const { locks_.Take(&lock, *this, engine_); } diff --git a/src/transactions/transaction.hpp b/src/transactions/transaction.hpp index f8a1e385f..9b21d5920 100644 --- a/src/transactions/transaction.hpp +++ b/src/transactions/transaction.hpp @@ -41,7 +41,7 @@ class Transaction { public: /** Acquires the lock over the given RecordLock, preventing other transactions * from doing the same */ - void TakeLock(RecordLock &lock); + void TakeLock(RecordLock &lock) const; /** Commits this transaction. After this call this transaction object is no * longer valid for use (it gets deleted by the engine that owns it). */ @@ -82,7 +82,7 @@ class Transaction { const Snapshot snapshot_; // Record locks held by this transaction. - LockStore locks_; + mutable LockStore locks_; // True if transaction should abort. Used to signal query executor that it // should stop execution, it is only a hint, transaction can disobey.