From e49017ff8ff9baa883585323f52451baad77e7df Mon Sep 17 00:00:00 2001 From: jsteemann Date: Wed, 13 Nov 2019 19:09:22 +0100 Subject: [PATCH 1/4] don't return any in-progress indexes --- arangod/RestHandler/RestIndexHandler.cpp | 3 +++ arangod/Transaction/Methods.cpp | 29 ++++++++++++++++-------- arangod/Transaction/Methods.h | 2 +- 3 files changed, 24 insertions(+), 10 deletions(-) diff --git a/arangod/RestHandler/RestIndexHandler.cpp b/arangod/RestHandler/RestIndexHandler.cpp index c583532aa41a..3395e97a45a7 100644 --- a/arangod/RestHandler/RestIndexHandler.cpp +++ b/arangod/RestHandler/RestIndexHandler.cpp @@ -215,6 +215,9 @@ RestStatus RestIndexHandler::getSelectivityEstimates() { builder.add(StaticStrings::Code, VPackValue(static_cast(rest::ResponseCode::OK))); builder.add("indexes", VPackValue(VPackValueType::Object)); for (std::shared_ptr idx : idxs) { + if (idx->inProgress()) { + continue; + } std::string name = coll->name(); name.push_back(TRI_INDEX_HANDLE_SEPARATOR_CHR); name.append(std::to_string(idx->id())); diff --git a/arangod/Transaction/Methods.cpp b/arangod/Transaction/Methods.cpp index 8db156aa239d..5eec2a2ead43 100644 --- a/arangod/Transaction/Methods.cpp +++ b/arangod/Transaction/Methods.cpp @@ -565,6 +565,8 @@ std::pair transaction::Methods::findIndexHandleForAndNode( auto considerIndex = [&bestIndex, &bestCost, &bestSupportsFilter, &bestSupportsSort, &indexes, node, reference, itemsInCollection, &sortCondition](std::shared_ptr const& idx) -> void { + TRI_ASSERT(!idx->inProgress()); + double filterCost = 0.0; double sortCost = 0.0; size_t itemsInIndex = itemsInCollection; @@ -2941,6 +2943,8 @@ bool transaction::Methods::getIndexForSortCondition( auto considerIndex = [reference, sortCondition, itemsInIndex, &bestCost, &bestIndex, &coveredAttributes](std::shared_ptr const& idx) -> void { + TRI_ASSERT(!idx->inProgress()); + Index::SortCosts costs = idx->supportsSortCondition(sortCondition, reference, itemsInIndex); if (costs.supportsCondition && @@ -3016,6 +3020,7 @@ std::unique_ptr transaction::Methods::indexScanForCondition( } // Now create the Iterator + TRI_ASSERT(!idx->inProgress()); return idx->iteratorForCondition(this, condition, var, opts); } @@ -3218,7 +3223,7 @@ Result transaction::Methods::unlockRecursive(TRI_voc_cid_t cid, AccessMode::Type /// @brief get list of indexes for a collection std::vector> transaction::Methods::indexesForCollection( - std::string const& collectionName, bool withHidden) { + std::string const& collectionName) { if (_state->isCoordinator()) { return indexesForCollectionCoordinator(collectionName); } @@ -3227,13 +3232,12 @@ std::vector> transaction::Methods::indexesForCollection( TRI_voc_cid_t cid = addCollectionAtRuntime(collectionName, AccessMode::Type::READ); LogicalCollection* document = documentCollection(trxCollection(cid)); std::vector> indexes = document->getIndexes(); - if (!withHidden) { - indexes.erase(std::remove_if(indexes.begin(), indexes.end(), - [](std::shared_ptr x) { - return x->isHidden(); - }), - indexes.end()); - } + + indexes.erase(std::remove_if(indexes.begin(), indexes.end(), + [](std::shared_ptr const& x) { + return x->isHidden(); + }), + indexes.end()); return indexes; } @@ -3264,7 +3268,14 @@ std::vector> transaction::Methods::indexesForCollectionCo collection->clusterIndexEstimates(true); } - return collection->getIndexes(); + std::vector> indexes = collection->getIndexes(); + + indexes.erase(std::remove_if(indexes.begin(), indexes.end(), + [](std::shared_ptr const& x) { + return x->isHidden(); + }), + indexes.end()); + return indexes; } /// @brief get the index by it's identifier. Will either throw or diff --git a/arangod/Transaction/Methods.h b/arangod/Transaction/Methods.h index d188642ba76f..e5932123032f 100644 --- a/arangod/Transaction/Methods.h +++ b/arangod/Transaction/Methods.h @@ -392,7 +392,7 @@ class Methods { /// @brief get all indexes for a collection name ENTERPRISE_VIRT std::vector> indexesForCollection( - std::string const&, bool withHidden = false); + std::string const& collectionName); /// @brief Lock all collections. Only works for selected sub-classes virtual int lockCollections(); From 266ea96d873c803e4a842310b67d8c67ab149d9d Mon Sep 17 00:00:00 2001 From: jsteemann Date: Wed, 13 Nov 2019 21:58:09 +0100 Subject: [PATCH 2/4] fix handling of in-progress indexes --- arangod/IResearch/IResearchView.cpp | 7 ++++--- arangod/IResearch/IResearchViewCoordinator.cpp | 4 +++- arangod/MMFiles/MMFilesCollection.cpp | 6 +++--- arangod/RocksDBEngine/RocksDBCollection.cpp | 4 ++-- arangod/RocksDBEngine/RocksDBEngine.cpp | 12 ++++++------ arangod/VocBase/LogicalCollection.cpp | 7 ++++--- arangod/VocBase/LogicalDataSource.cpp | 2 +- arangod/VocBase/LogicalDataSource.h | 2 ++ 8 files changed, 25 insertions(+), 19 deletions(-) diff --git a/arangod/IResearch/IResearchView.cpp b/arangod/IResearch/IResearchView.cpp index f4f290f7ec5c..a2c47ca18970 100644 --- a/arangod/IResearch/IResearchView.cpp +++ b/arangod/IResearch/IResearchView.cpp @@ -363,11 +363,12 @@ arangodb::Result IResearchView::appendVelocyPackImpl( // append JSON static const std::function persistenceAcceptor = [](irs::string_ref const&) -> bool { return true; }; - auto& acceptor = context == Serialization::Persistence || context == Serialization::Inventory + auto& acceptor = + (context == Serialization::Persistence || context == Serialization::PersistenceWithInProgress || context == Serialization::Inventory) ? persistenceAcceptor : propertiesAcceptor; - if (context == Serialization::Persistence) { + if (context == Serialization::Persistence || context == Serialization::PersistenceWithInProgress) { if (arangodb::ServerState::instance()->isSingleServer()) { auto res = arangodb::LogicalViewHelperStorageEngine::properties(builder, *this); @@ -404,7 +405,7 @@ arangodb::Result IResearchView::appendVelocyPackImpl( // append JSON return {}; } - if (context == Serialization::Persistence) { + if (context == Serialization::Persistence || context == Serialization::PersistenceWithInProgress) { IResearchViewMetaState metaState; for (auto& entry : _links) { diff --git a/arangod/IResearch/IResearchViewCoordinator.cpp b/arangod/IResearch/IResearchViewCoordinator.cpp index da0980e75887..d025bb533a2e 100644 --- a/arangod/IResearch/IResearchViewCoordinator.cpp +++ b/arangod/IResearch/IResearchViewCoordinator.cpp @@ -190,7 +190,9 @@ arangodb::Result IResearchViewCoordinator::appendVelocyPackImpl( auto* acceptor = &propertiesAcceptor; - if (context == Serialization::Persistence || context == Serialization::Inventory) { + if (context == Serialization::Persistence || + context == Serialization::PersistenceWithInProgress || + context == Serialization::Inventory) { auto res = arangodb::LogicalViewHelperClusterInfo::properties(builder, *this); if (!res.ok()) { diff --git a/arangod/MMFiles/MMFilesCollection.cpp b/arangod/MMFiles/MMFilesCollection.cpp index 8413f959591a..fcf752a9f3f3 100644 --- a/arangod/MMFiles/MMFilesCollection.cpp +++ b/arangod/MMFiles/MMFilesCollection.cpp @@ -296,7 +296,7 @@ arangodb::Result MMFilesCollection::persistProperties() { try { auto infoBuilder = _logicalCollection.toVelocyPackIgnore( {"path", "statusString"}, - LogicalDataSource::Serialization::Persistence); + LogicalDataSource::Serialization::PersistenceWithInProgress); MMFilesCollectionMarker marker(TRI_DF_MARKER_VPACK_CHANGE_COLLECTION, _logicalCollection.vocbase().id(), _logicalCollection.id(), infoBuilder.slice()); @@ -2284,7 +2284,7 @@ std::shared_ptr MMFilesCollection::createIndex(transaction::Methods& trx, if (!engine->inRecovery()) { auto builder = _logicalCollection.toVelocyPackIgnore( {"path", "statusString"}, - LogicalDataSource::Serialization::Persistence); + LogicalDataSource::Serialization::PersistenceWithInProgress); _logicalCollection.properties(builder.slice(), false); // always a full-update } @@ -2422,7 +2422,7 @@ bool MMFilesCollection::dropIndex(TRI_idx_iid_t iid) { { auto builder = _logicalCollection.toVelocyPackIgnore( {"path", "statusString"}, - LogicalDataSource::Serialization::Persistence); + LogicalDataSource::Serialization::PersistenceWithInProgress); _logicalCollection.properties(builder.slice(), false); // always a full-update diff --git a/arangod/RocksDBEngine/RocksDBCollection.cpp b/arangod/RocksDBEngine/RocksDBCollection.cpp index 2c640be0240e..f5589b9a014e 100644 --- a/arangod/RocksDBEngine/RocksDBCollection.cpp +++ b/arangod/RocksDBEngine/RocksDBCollection.cpp @@ -466,7 +466,7 @@ std::shared_ptr RocksDBCollection::createIndex(VPackSlice const& info, if (!engine->inRecovery()) { // write new collection marker auto builder = _logicalCollection.toVelocyPackIgnore( {"path", "statusString"}, - LogicalDataSource::Serialization::Persistence); + LogicalDataSource::Serialization::PersistenceWithInProgress); VPackBuilder indexInfo; idx->toVelocyPack(indexInfo, Index::makeFlags(Index::Serialize::Internals)); res = engine->writeCreateCollectionMarker(_logicalCollection.vocbase().id(), @@ -548,7 +548,7 @@ bool RocksDBCollection::dropIndex(TRI_idx_iid_t iid) { auto builder = // RocksDB path _logicalCollection.toVelocyPackIgnore( {"path", "statusString"}, - LogicalDataSource::Serialization::Persistence); + LogicalDataSource::Serialization::PersistenceWithInProgress); // log this event in the WAL and in the collection meta-data res = engine->writeCreateCollectionMarker( // write marker diff --git a/arangod/RocksDBEngine/RocksDBEngine.cpp b/arangod/RocksDBEngine/RocksDBEngine.cpp index 82bd0091912d..c36b72a70fd9 100644 --- a/arangod/RocksDBEngine/RocksDBEngine.cpp +++ b/arangod/RocksDBEngine/RocksDBEngine.cpp @@ -1241,7 +1241,7 @@ std::string RocksDBEngine::createCollection(TRI_vocbase_t& vocbase, auto builder = collection.toVelocyPackIgnore( {"path", "statusString"}, - LogicalDataSource::Serialization::Persistence); + LogicalDataSource::Serialization::PersistenceWithInProgress); TRI_UpdateTickServer(static_cast(cid)); int res = @@ -1399,7 +1399,7 @@ void RocksDBEngine::changeCollection(TRI_vocbase_t& vocbase, LogicalCollection const& collection, bool doSync) { auto builder = collection.toVelocyPackIgnore( {"path", "statusString"}, - LogicalDataSource::Serialization::Persistence); + LogicalDataSource::Serialization::PersistenceWithInProgress); int res = writeCreateCollectionMarker(vocbase.id(), collection.id(), builder.slice(), RocksDBLogValue::CollectionChange(vocbase.id(), @@ -1415,7 +1415,7 @@ arangodb::Result RocksDBEngine::renameCollection(TRI_vocbase_t& vocbase, std::string const& oldName) { auto builder = collection.toVelocyPackIgnore( {"path", "statusString"}, - LogicalDataSource::Serialization::Persistence); + LogicalDataSource::Serialization::PersistenceWithInProgress); int res = writeCreateCollectionMarker( vocbase.id(), collection.id(), builder.slice(), RocksDBLogValue::CollectionRename(vocbase.id(), collection.id(), arangodb::velocypack::StringRef(oldName))); @@ -1442,7 +1442,7 @@ Result RocksDBEngine::createView(TRI_vocbase_t& vocbase, TRI_voc_cid_t id, VPackBuilder props; props.openObject(); - view.properties(props, LogicalDataSource::Serialization::Persistence); + view.properties(props, LogicalDataSource::Serialization::PersistenceWithInProgress); props.close(); RocksDBValue const value = RocksDBValue::View(props.slice()); @@ -1467,7 +1467,7 @@ arangodb::Result RocksDBEngine::dropView(TRI_vocbase_t const& vocbase, VPackBuilder builder; builder.openObject(); - view.properties(builder, LogicalDataSource::Serialization::Persistence); + view.properties(builder, LogicalDataSource::Serialization::PersistenceWithInProgress); builder.close(); auto logValue = @@ -1512,7 +1512,7 @@ Result RocksDBEngine::changeView(TRI_vocbase_t& vocbase, VPackBuilder infoBuilder; infoBuilder.openObject(); - view.properties(infoBuilder, LogicalDataSource::Serialization::Persistence); + view.properties(infoBuilder, LogicalDataSource::Serialization::PersistenceWithInProgress); infoBuilder.close(); RocksDBLogValue log = RocksDBLogValue::ViewChange(vocbase.id(), view.id()); diff --git a/arangod/VocBase/LogicalCollection.cpp b/arangod/VocBase/LogicalCollection.cpp index 5679936c271e..d885ed6022ef 100644 --- a/arangod/VocBase/LogicalCollection.cpp +++ b/arangod/VocBase/LogicalCollection.cpp @@ -630,7 +630,8 @@ void LogicalCollection::toVelocyPackForClusterInventory(VPackBuilder& result, arangodb::Result LogicalCollection::appendVelocyPack(arangodb::velocypack::Builder& result, Serialization context) const { - bool const forPersistence = (context == Serialization::Persistence); + bool const forPersistence = (context == Serialization::Persistence || context == Serialization::PersistenceWithInProgress); + bool const showInProgress = (context == Serialization::PersistenceWithInProgress); // We write into an open object TRI_ASSERT(result.isOpenObject()); @@ -673,8 +674,8 @@ arangodb::Result LogicalCollection::appendVelocyPack(arangodb::velocypack::Build if (forPersistence) { indexFlags = Index::makeFlags(Index::Serialize::Internals); } - auto filter = [indexFlags, forPersistence](arangodb::Index const* idx, decltype(Index::makeFlags())& flags) { - if (forPersistence || (!idx->inProgress() && !idx->isHidden())) { + auto filter = [indexFlags, forPersistence, showInProgress](arangodb::Index const* idx, decltype(Index::makeFlags())& flags) { + if ((forPersistence || !idx->isHidden()) && (showInProgress || !idx->inProgress())) { flags = indexFlags; return true; } diff --git a/arangod/VocBase/LogicalDataSource.cpp b/arangod/VocBase/LogicalDataSource.cpp index 45580f946ee3..9ae7eb458203 100644 --- a/arangod/VocBase/LogicalDataSource.cpp +++ b/arangod/VocBase/LogicalDataSource.cpp @@ -210,7 +210,7 @@ Result LogicalDataSource::properties(velocypack::Builder& builder, // note: includeSystem and forPersistence are not 100% synonymous, // however, for our purposes this is an okay mapping; we only set // includeSystem if we are persisting the properties - if (context == Serialization::Persistence) { + if (context == Serialization::Persistence || context == Serialization::PersistenceWithInProgress) { builder.add(StaticStrings::DataSourceDeleted, velocypack::Value(deleted())); builder.add(StaticStrings::DataSourceSystem, velocypack::Value(system())); diff --git a/arangod/VocBase/LogicalDataSource.h b/arangod/VocBase/LogicalDataSource.h index d432a55de747..dab1bc4c62f0 100644 --- a/arangod/VocBase/LogicalDataSource.h +++ b/arangod/VocBase/LogicalDataSource.h @@ -137,6 +137,8 @@ class LogicalDataSource { Properties, // object will be saved in storage engine Persistence, + // object will be saved in storage engine + PersistenceWithInProgress, // object will be replicated or dumped/restored Inventory }; From 090933f916511814beeea1975fc6b4a46d9a980c Mon Sep 17 00:00:00 2001 From: jsteemann Date: Wed, 13 Nov 2019 21:58:31 +0100 Subject: [PATCH 3/4] add test --- .../common/shell/aql-index-usage-rocksdb.js | 130 ++++++++++++++++++ 1 file changed, 130 insertions(+) create mode 100644 tests/js/common/shell/aql-index-usage-rocksdb.js diff --git a/tests/js/common/shell/aql-index-usage-rocksdb.js b/tests/js/common/shell/aql-index-usage-rocksdb.js new file mode 100644 index 000000000000..9fd169e14555 --- /dev/null +++ b/tests/js/common/shell/aql-index-usage-rocksdb.js @@ -0,0 +1,130 @@ +/*jshint globalstrict:false, strict:false */ +/*global assertEqual, assertNotEqual, assertTrue */ + +//////////////////////////////////////////////////////////////////////////////// +/// @brief test index usage +/// +/// @file +/// +/// DISCLAIMER +/// +/// Copyright 2010-2012 triagens GmbH, Cologne, Germany +/// +/// Licensed under the Apache License, Version 2.0 (the "License"); +/// you may not use this file except in compliance with the License. +/// You may obtain a copy of the License at +/// +/// http://www.apache.org/licenses/LICENSE-2.0 +/// +/// Unless required by applicable law or agreed to in writing, software +/// distributed under the License is distributed on an "AS IS" BASIS, +/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +/// See the License for the specific language governing permissions and +/// limitations under the License. +/// +/// Copyright holder is triAGENS GmbH, Cologne, Germany +/// +/// @author Jan Steemann +/// @author Copyright 2018, triAGENS GmbH, Cologne, Germany +//////////////////////////////////////////////////////////////////////////////// + +let jsunity = require("jsunity"); + +let arangodb = require("@arangodb"); +let db = arangodb.db; +let tasks = require("@arangodb/tasks"); + +function IndexUsageSuite () { + const cnData = "UnitTestsCollection"; // used for test data + const cnComm = "UnitTestsCommunication"; // used for communication + + return { + + setUp : function () { + db._drop(cnData); + db._drop(cnComm); + db._create(cnData); + db._create(cnComm); + + let docs = []; + for (let i = 0; i < 5000; ++i) { + docs.push({ value: "test" + i }); + } + db[cnData].insert(docs); + }, + + tearDown : function () { + db._drop(cnData); + db._drop(cnComm); + }, + + testIndexUsage : function () { + let task = tasks.register({ + command: function(params) { + require('jsunity').jsUnity.attachAssertions(); + let db = require("internal").db; + let comm = db[params.cnComm]; + let errors = require("@arangodb").errors; + comm.insert({ _key: "runner1", value: 0 }); + + while (!comm.exists("runner2")) { + require("internal").sleep(0.02); + } + + let success = 0; + let time = require("internal").time; + let start = time(); + do { + try { + db._query("FOR doc IN " + params.cnData + " FILTER doc.value > 10 LIMIT 10 RETURN doc"); + comm.update("runner1", { value: ++success }); + } catch (err) { + // if the index that was picked for the query is dropped in the meantime, + // we will get the following error back + assertEqual(err.errorNum, errors.ERROR_QUERY_BAD_JSON_PLAN.code); + } + } while (time() - start < 10.0); + }, + params: { cnComm, cnData } + }); + + let comm = db[cnComm]; + comm.insert({ _key: "runner2" }); + while (!comm.exists("runner1")) { + require("internal").sleep(0.02); + } + + let time = require("internal").time; + let start = time(); + let success = 0; + do { + let indexes = db[cnData].indexes(); + if (indexes.length > 1) { + db[cnData].dropIndex(indexes[1]); + } + db[cnData].ensureIndex({ type: "hash", fields: ["value"], inBackground: true }); + ++success; + } while (time() - start < 10.0); + + while (true) { + try { + tasks.get(task); + require("internal").wait(0.25, false); + } catch (err) { + // "task not found" means the task is finished + break; + } + } + + assertEqual(2, comm.count()); + let doc = comm.document("runner1"); + assertTrue(doc.value > 0, doc); + assertTrue(success > 0, success); + }, + + }; +} + +jsunity.run(IndexUsageSuite); + +return jsunity.done(); From 5b08a516759cc27e51b620179c2679ae6b8c814f Mon Sep 17 00:00:00 2001 From: jsteemann Date: Thu, 14 Nov 2019 11:39:19 +0100 Subject: [PATCH 4/4] address review comment --- arangod/RestHandler/RestIndexHandler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arangod/RestHandler/RestIndexHandler.cpp b/arangod/RestHandler/RestIndexHandler.cpp index 3395e97a45a7..ccf8b512ef5f 100644 --- a/arangod/RestHandler/RestIndexHandler.cpp +++ b/arangod/RestHandler/RestIndexHandler.cpp @@ -215,7 +215,7 @@ RestStatus RestIndexHandler::getSelectivityEstimates() { builder.add(StaticStrings::Code, VPackValue(static_cast(rest::ResponseCode::OK))); builder.add("indexes", VPackValue(VPackValueType::Object)); for (std::shared_ptr idx : idxs) { - if (idx->inProgress()) { + if (idx->inProgress() || idx->isHidden()) { continue; } std::string name = coll->name();