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

@@ -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 <typename TRecord>
-  bool Reconstruct(RecordAccessor<TRecord> &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 <typename TRecord>
-  void Update(RecordAccessor<TRecord> &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,

src/mvcc/version_list.hpp

@@ -28,7 +28,7 @@ class VersionList {
    * creating the first Record (Version) in this VersionList.
    */
   template <typename... Args>
-  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>(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
-

src/storage/record_accessor.cpp

@@ -108,12 +108,29 @@ RecordAccessor<TRecord> &RecordAccessor<TRecord>::SwitchOld() {
 
 template <typename TRecord>
 bool RecordAccessor<TRecord>::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 <typename TRecord>
 TRecord &RecordAccessor<TRecord>::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_;
 }

src/storage/record_accessor.hpp

@@ -130,34 +130,31 @@ class RecordAccessor : public TotalOrdering<RecordAccessor<TRecord>> {
   RecordAccessor<TRecord> &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<TRecord> &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();
 

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_);
 }
 

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.