GraphDbAccessor::BuildIndex - deadlock bugfix

Reviewers: buda, mislav.bradac, dgleich

Reviewed By: buda

Subscribers: pullbot

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

CHANGELOG.md

@@ -4,12 +4,27 @@
 
 ### Major Features and Improvements
 
-* Support for variable length path `MATCH`.
-* Support for map literal.
-* Support for `all` function in openCypher.
+### Bug Fixes and Other Changes
+
+## v0.7.0
+
+### Major Features and Improvements
+
+* Variable length path `MATCH`.
+* Explicitly started transactions (multi-query transactions).
+* Map literal.
+* Query parameters (except for parameters in place of property maps).
+* `all` function in openCypher.
+* `degree` function in openCypher.
 * User specified transaction execution timeout.
-* Support for query parameters (except for parameters in place of property maps).
-* `degree` function added.
+
+### Bug Fixes and Other Changes
+
+* Concurrent `BUILD INDEX` deadlock now returns an error to the client.
+* A `MATCH` preceeded by `OPTIONAL MATCH` expansion inconsistencies.
+* High concurrency Antlr parsing bug.
+* Indexing improvements.
+* Query stripping and caching speedups.
 
 ## v0.6.0
 

docs/user_technical/open-cypher.md

@@ -378,6 +378,11 @@ Currently, once an index is created it cannot be deleted. This feature will be
 implemented very soon. The expected syntax for removing an index will be `DROP
 INDEX ON :Label(property)`.
 
+Note that Memgraph does not support concurrent index building. Attempting to
+build an index from two concurrent transactions is likely to result in an
+error (reported to the client) for one of those transactions, regardless of
+index `label` and `property`. Do not create indices concurrently.
+
 ### Other Features
 
 The following sections describe some of the other supported features.

src/database/graph_db.hpp

@@ -71,7 +71,6 @@ class GraphDb {
   tx::Engine tx_engine_;
 
   // database name
-  // TODO consider if this is even necessary
   const std::string name_;
 
   // main storage for the graph
@@ -101,6 +100,12 @@ class GraphDb {
   KeyIndex<GraphDbTypes::EdgeType, Edge> edge_types_index_;
   LabelPropertyIndex label_property_index_;
 
+  // Flag indicating if index building is in progress. Memgraph does not support
+  // concurrent index builds on the same database (transaction engine), so we
+  // reject index builds if there is one in progress. See
+  // GraphDbAccessor::BuildIndex.
+  std::atomic<bool> index_build_in_progress_{false};
+
   // snapshooter
   Snapshooter snapshooter_;
 

src/database/graph_db_accessor.cpp

@@ -1,11 +1,12 @@
-#include "database/graph_db_accessor.hpp"
 #include "database/creation_exception.hpp"
+#include "database/graph_db_accessor.hpp"
 
 #include "storage/edge.hpp"
 #include "storage/edge_accessor.hpp"
 #include "storage/vertex.hpp"
 #include "storage/vertex_accessor.hpp"
 #include "utils/assert.hpp"
+#include "utils/on_scope_exit.hpp"
 
 GraphDbAccessor::GraphDbAccessor(GraphDb &db)
     : db_(db), transaction_(db.tx_engine_.Begin()) {}
@@ -63,6 +64,21 @@ void GraphDbAccessor::BuildIndex(const GraphDbTypes::Label &label,
         "Index is either being created by another transaction or already "
         "exists.");
   }
+
+  {
+    // switch the build_in_progress to true
+    bool expected = false;
+    if (!db_.index_build_in_progress_.compare_exchange_strong(expected, true))
+      throw IndexBuildInProgressException();
+  }
+  // on function exit switch the build_in_progress to false
+  utils::OnScopeExit on_exit([this] {
+    bool expected = true;
+    bool success =
+        db_.index_build_in_progress_.compare_exchange_strong(expected, false);
+    debug_assert(success, "BuildIndexInProgress flag was not set during build");
+  });
+
   // Everything that happens after the line above ended will be added to the
   // index automatically, but we still have to add to index everything that
   // happened earlier. We have to first wait for every transaction that

src/database/graph_db_accessor.hpp

@@ -21,6 +21,13 @@ class IndexExistsException : public utils::BasicException {
   using utils::BasicException::BasicException;
 };
 
+/** Thrown when attempting to build indexes concurrently */
+class IndexBuildInProgressException : public utils::BasicException {
+ public:
+  IndexBuildInProgressException()
+      : utils::BasicException("Concurrent index build on the same database") {}
+};
+
 /**
  * An accessor for the database object: exposes functions
  * for operating on the database. All the functions in
@@ -338,6 +345,9 @@ class GraphDbAccessor {
    * exists (even if it's being built by a concurrent transaction and is not yet
    * ready for use).
    *
+   * It also throws if there is another index being built concurrently on the
+   * same database this accessor is for.
+   *
    * @param label - label to build for
    * @param property - property to build for
    */

src/query/plan/operator.cpp

@@ -2264,6 +2264,12 @@ class CreateIndexCursor : public Cursor {
       db_.BuildIndex(self_.label(), self_.property());
     } catch (const IndexExistsException &) {
       // Ignore creating an existing index.
+    } catch (const IndexBuildInProgressException &) {
+      // Report to the end user.
+      did_create_ = false;
+      throw QueryRuntimeException(
+          "Index building already in progress on this database. Memgraph does "
+          "not support concurrent index building.");
     }
     did_create_ = true;
     return true;

tests/unit/graph_db_accessor_index_api.cpp

@@ -1,5 +1,6 @@
 #include <experimental/optional>
 #include <memory>
+#include <thread>
 
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
@@ -136,6 +137,44 @@ TEST_F(GraphDbAccessorIndex, LabelPropertyIndexCount) {
   EXPECT_EQ(Count(dba->Vertices(label, property)), 14);
 }
 
+TEST(GraphDbAccessorIndexApi, LabelPropertyBuildIndexConcurrent) {
+  Dbms dbms;
+  auto dba = dbms.active();
+
+  // We need to build indices in other threads.
+  auto build_index_async = [&dbms](int &success, int index) {
+    std::thread([&dbms, &success, index]() {
+      auto dba = dbms.active();
+      try {
+        dba->BuildIndex(dba->Label("l" + std::to_string(index)),
+                        dba->Property("p" + std::to_string(index)));
+        dba->Commit();
+        success = 1;
+      } catch (IndexBuildInProgressException &) {
+        dba->Abort();
+        success = 0;
+      }
+    }).detach();
+  };
+
+  int build_1_success = -1;
+  build_index_async(build_1_success, 1);
+  std::this_thread::sleep_for(std::chrono::milliseconds(100));
+
+  // First index build should now be inside the BuildIndex function waiting for
+  // dba to commit. A second built attempt should fail.
+  int build_2_success = -1;
+  build_index_async(build_2_success, 2);
+  std::this_thread::sleep_for(std::chrono::milliseconds(30));
+  EXPECT_EQ(build_1_success, -1);
+  EXPECT_EQ(build_2_success, 0);
+
+  // End dba and expect that first build index finished successfully.
+  dba->Commit();
+  std::this_thread::sleep_for(std::chrono::milliseconds(30));
+  EXPECT_EQ(build_1_success, 1);
+}
+
 #define EXPECT_WITH_MARGIN(x, center) \
   EXPECT_THAT(                        \
       x, testing::AllOf(testing::Ge(center - 2), testing::Le(center + 2)));