GraphDb - index garbage collection fix

Summary:
A single line (graph_db.cpp:109 in the new code) was missing. This should have been done in D355 (made by DGleich, approved by Flor AND Buda AND Mislav :D).

Converted a lambda to a method for convenience.

Reviewers: buda, dgleich, mislav.bradac

Reviewed By: buda

Subscribers: pullbot

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

src/database/graph_db.cpp

@@ -33,33 +33,8 @@ GraphDb::GraphDb(const std::string &name, const fs::path &snapshot_db_dir)
       gc_edges_(edges_, edge_record_deleter_, edge_version_list_deleter_) {
   // Pause of -1 means we shouldn't run the GC.
   if (FLAGS_gc_cycle_sec != -1) {
-    gc_scheduler_.Run(std::chrono::seconds(FLAGS_gc_cycle_sec), [this]() {
-      // main garbage collection logic
-      // see wiki documentation for logic explanation
-      const auto snapshot = this->tx_engine_.GcSnapshot();
-      {
-        // This can be run concurrently
-        this->gc_vertices_.Run(snapshot, this->tx_engine_);
-        this->gc_edges_.Run(snapshot, this->tx_engine_);
-      }
-      // This has to be run sequentially after gc because gc modifies
-      // version_lists and changes the oldest visible record, on which Refresh
-      // depends.
-      {
-        // This can be run concurrently
-        this->labels_index_.Refresh(snapshot, this->tx_engine_);
-        this->edge_types_index_.Refresh(snapshot, this->tx_engine_);
-      }
-      // we free expired objects with snapshot.back(), which is
-      // the ID of the oldest active transaction (or next active, if there
-      // are no currently active). that's legal because that was the
-      // last possible transaction that could have obtained pointers
-      // to those records
-      this->edge_record_deleter_.FreeExpiredObjects(snapshot.back());
-      this->vertex_record_deleter_.FreeExpiredObjects(snapshot.back());
-      this->edge_version_list_deleter_.FreeExpiredObjects(snapshot.back());
-      this->vertex_version_list_deleter_.FreeExpiredObjects(snapshot.back());
-    });
+    gc_scheduler_.Run(std::chrono::seconds(FLAGS_gc_cycle_sec),
+                      [this]() { CollectGarbage(); });
   }
 
   RecoverDatabase(snapshot_db_dir);
@@ -115,6 +90,35 @@ void GraphDb::RecoverDatabase(const fs::path &snapshot_db_dir) {
   }
 }
 
+void GraphDb::CollectGarbage() {
+  // main garbage collection logic
+  // see wiki documentation for logic explanation
+  const auto snapshot = this->tx_engine_.GcSnapshot();
+  {
+    // This can be run concurrently
+    this->gc_vertices_.Run(snapshot, this->tx_engine_);
+    this->gc_edges_.Run(snapshot, this->tx_engine_);
+  }
+  // This has to be run sequentially after gc because gc modifies
+  // version_lists and changes the oldest visible record, on which Refresh
+  // depends.
+  {
+    // This can be run concurrently
+    this->labels_index_.Refresh(snapshot, this->tx_engine_);
+    this->edge_types_index_.Refresh(snapshot, this->tx_engine_);
+    this->label_property_index_.Refresh(snapshot, this->tx_engine_);
+  }
+  // we free expired objects with snapshot.back(), which is
+  // the ID of the oldest active transaction (or next active, if there
+  // are no currently active). that's legal because that was the
+  // last possible transaction that could have obtained pointers
+  // to those records
+  this->edge_record_deleter_.FreeExpiredObjects(snapshot.back());
+  this->vertex_record_deleter_.FreeExpiredObjects(snapshot.back());
+  this->edge_version_list_deleter_.FreeExpiredObjects(snapshot.back());
+  this->vertex_version_list_deleter_.FreeExpiredObjects(snapshot.back());
+}
+
 GraphDb::~GraphDb() {
   // Stop the gc scheduler to not run into race conditions for deletions.
   gc_scheduler_.Stop();

src/database/graph_db.hpp

@@ -67,6 +67,11 @@ class GraphDb {
    */
   void RecoverDatabase(const fs::path &snapshot_db_path);
 
+  /**
+   * Collects garbage.
+   */
+  void CollectGarbage();
+
   /** transaction engine related to this database */
   tx::Engine tx_engine_;
 

tests/unit/graph_db.cpp

@@ -0,0 +1,43 @@
+#include <memory>
+
+#include "gtest/gtest.h"
+
+#include "database/graph_db.hpp"
+#include "database/graph_db_accessor.hpp"
+#include "database/graph_db_datatypes.hpp"
+#include "database/indexes/label_property_index.hpp"
+
+class GraphDbTest : public testing::Test {
+ protected:
+  GraphDb graph_db{"default", fs::path()};
+  std::unique_ptr<GraphDbAccessor> dba =
+      std::make_unique<GraphDbAccessor>(graph_db);
+
+  void Commit() {
+    dba->Commit();
+    auto dba2 = std::make_unique<GraphDbAccessor>(graph_db);
+    dba.swap(dba2);
+  }
+};
+
+TEST_F(GraphDbTest, GarbageCollectIndices) {
+  auto label = dba->Label("label");
+  auto property = dba->Property("property");
+  dba->BuildIndex(label, property);
+  Commit();
+
+  auto vertex = dba->InsertVertex();
+  vertex.add_label(label);
+  vertex.PropsSet(property, 42);
+  Commit();
+
+  EXPECT_EQ(dba->VerticesCount(label, property), 1);
+  auto vertex_transferred = dba->Transfer(vertex);
+  dba->RemoveVertex(vertex_transferred.value());
+  EXPECT_EQ(dba->VerticesCount(label, property), 1);
+  Commit();
+  EXPECT_EQ(dba->VerticesCount(label, property), 1);
+  graph_db.CollectGarbage();
+  EXPECT_EQ(dba->VerticesCount(label, property), 0);
+
+}