From 07037242956d8370ac72f5304b421f6d3437f6fe Mon Sep 17 00:00:00 2001 From: florijan Date: Fri, 11 Aug 2017 09:48:13 +0200 Subject: [PATCH] GraphDbAccessor::BuildIndex - deadlock bugfix Reviewers: buda, mislav.bradac, dgleich Reviewed By: buda Subscribers: pullbot Differential Revision: https://phabricator.memgraph.io/D658 --- CHANGELOG.md | 25 +++++++++++--- docs/user_technical/open-cypher.md | 5 +++ src/database/graph_db.hpp | 7 +++- src/database/graph_db_accessor.cpp | 18 +++++++++- src/database/graph_db_accessor.hpp | 10 ++++++ src/query/plan/operator.cpp | 6 ++++ tests/unit/graph_db_accessor_index_api.cpp | 39 ++++++++++++++++++++++ 7 files changed, 103 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index efed40cc1..c8f6eb02f 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/docs/user_technical/open-cypher.md b/docs/user_technical/open-cypher.md index 13c7db593..adc33bfcf 100644 --- a/docs/user_technical/open-cypher.md +++ b/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. diff --git a/src/database/graph_db.hpp b/src/database/graph_db.hpp index c4b460414..a66850b34 100644 --- a/src/database/graph_db.hpp +++ b/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 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 index_build_in_progress_{false}; + // snapshooter Snapshooter snapshooter_; diff --git a/src/database/graph_db_accessor.cpp b/src/database/graph_db_accessor.cpp index 322734428..d8465c57a 100644 --- a/src/database/graph_db_accessor.cpp +++ b/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 diff --git a/src/database/graph_db_accessor.hpp b/src/database/graph_db_accessor.hpp index 21f0ddccb..edf156910 100644 --- a/src/database/graph_db_accessor.hpp +++ b/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 */ diff --git a/src/query/plan/operator.cpp b/src/query/plan/operator.cpp index 256758bf8..4fe956556 100644 --- a/src/query/plan/operator.cpp +++ b/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; diff --git a/tests/unit/graph_db_accessor_index_api.cpp b/tests/unit/graph_db_accessor_index_api.cpp index 81e542138..514669a5c 100644 --- a/tests/unit/graph_db_accessor_index_api.cpp +++ b/tests/unit/graph_db_accessor_index_api.cpp @@ -1,5 +1,6 @@ #include #include +#include #include #include @@ -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)));