From 9559c8a80ed0060bcc0148c129c1848bc438f5e6 Mon Sep 17 00:00:00 2001 From: Dronplane Date: Wed, 21 Aug 2019 23:34:24 +0300 Subject: [PATCH] Bug fix/internal issue #622 (#9781) * Added analyzer cache invalidation for dropped database * Fixed jslint reported errors --- .../IResearch/IResearchAnalyzerFeature.cpp | 63 ++++++++++--------- arangod/IResearch/IResearchAnalyzerFeature.h | 12 ++++ arangod/RestServer/DatabaseFeature.cpp | 7 +++ .../aql/aql-view-arangosearch-ddl-cluster.js | 23 ++++++- .../aql-view-arangosearch-ddl-noncluster.js | 23 ++++++- 5 files changed, 98 insertions(+), 30 deletions(-) diff --git a/arangod/IResearch/IResearchAnalyzerFeature.cpp b/arangod/IResearch/IResearchAnalyzerFeature.cpp index bd60bd0d7a..d4357235be 100644 --- a/arangod/IResearch/IResearchAnalyzerFeature.cpp +++ b/arangod/IResearch/IResearchAnalyzerFeature.cpp @@ -764,6 +764,7 @@ bool dropLegacyAnalyzersCollection( return lookupRes.is(TRI_ERROR_ARANGO_DATA_SOURCE_NOT_FOUND); } + void registerUpgradeTasks() { using namespace arangodb; using namespace arangodb::application_features; @@ -1820,38 +1821,15 @@ arangodb::Result IResearchAnalyzerFeature::loadAnalyzers( } auto* vocbase = dbFeature->lookupDatabase(database); - static auto cleanupAnalyzers = []( // remove invalid analyzers - IResearchAnalyzerFeature& feature, // analyzer feature - decltype(_lastLoad)::iterator& lastLoadItr, // iterator - irs::string_ref const& database // database - )->void { - if (lastLoadItr == feature._lastLoad.end()) { - return; // nothing to do (if not in '_lastLoad' then not in '_analyzers') - } - - // remove invalid database and analyzers - feature._lastLoad.erase(lastLoadItr); - - // remove no longer valid analyzers (force remove) - for (auto itr = feature._analyzers.begin(), - end = feature._analyzers.end(); - itr != end;) { - auto split = splitAnalyzerName(itr->first); - - if (split.first == database) { - itr = feature._analyzers.erase(itr); - } else { - ++itr; - } - } - }; if (!vocbase) { if (engine && engine->inRecovery()) { return arangodb::Result(); // database might not have come up yet } - - cleanupAnalyzers(*this, itr, database); // cleanup any analyzers for 'database' + if (itr != _lastLoad.end()) { + cleanupAnalyzers(database); // cleanup any analyzers for 'database' + _lastLoad.erase(itr); + } return arangodb::Result( // result TRI_ERROR_ARANGO_DATABASE_NOT_FOUND, // code @@ -1860,7 +1838,9 @@ arangodb::Result IResearchAnalyzerFeature::loadAnalyzers( } if (!getAnalyzerCollection(*vocbase)) { - cleanupAnalyzers(*this, itr, database); // cleanup any analyzers for 'database' + if (itr != _lastLoad.end()) { + cleanupAnalyzers(database); // cleanup any analyzers for 'database' + } _lastLoad[databaseKey] = currentTimestamp; // update timestamp return arangodb::Result(); // no collection means nothing to load @@ -2584,6 +2564,33 @@ bool IResearchAnalyzerFeature::visit( // visit analyzers return true; } +void IResearchAnalyzerFeature::cleanupAnalyzers(irs::string_ref const& database) { + if (ADB_UNLIKELY(database.empty())) { + TRI_ASSERT(FALSE); + return; + } + for (auto itr = _analyzers.begin(), end = _analyzers.end(); itr != end;) { + auto split = splitAnalyzerName(itr->first); + if (split.first == database) { + itr = _analyzers.erase(itr); + } else { + ++itr; + } + } +} + +void IResearchAnalyzerFeature::invalidate(const TRI_vocbase_t& vocbase) { + WriteMutex mutex(_mutex); + SCOPED_LOCK(mutex); + auto database = irs::string_ref(vocbase.name()); + auto itr = _lastLoad.find( + irs::make_hashed_ref(database, std::hash())); + if (itr != _lastLoad.end()) { + cleanupAnalyzers(database); + _lastLoad.erase(itr); + } +} + } // namespace iresearch } // namespace arangodb diff --git a/arangod/IResearch/IResearchAnalyzerFeature.h b/arangod/IResearch/IResearchAnalyzerFeature.h index e43a2274b5..926b7cbef0 100644 --- a/arangod/IResearch/IResearchAnalyzerFeature.h +++ b/arangod/IResearch/IResearchAnalyzerFeature.h @@ -265,6 +265,12 @@ class IResearchAnalyzerFeature final : public arangodb::application_features::Ap bool visit(std::function const& visitor, TRI_vocbase_t const* vocbase) const; + /////////////////////////////////////////////////////////////////////////////// + // @brief removes analyzers for specified database from cache + // @param vocbase database to invalidate analyzers + /////////////////////////////////////////////////////////////////////////////// + void invalidate(const TRI_vocbase_t& vocbase); + private: // map of caches of irs::analysis::analyzer pools indexed by analyzer name and // their associated metas @@ -328,6 +334,12 @@ class IResearchAnalyzerFeature final : public arangodb::application_features::Ap irs::string_ref const& database = irs::string_ref::NIL // database to load ); + //////////////////////////////////////////////////////////////////////////////// + /// removes analyzers for database from feature cache + /// Write lock must be acquired by caller + /// @param database the database to cleanup analyzers for + void cleanupAnalyzers(irs::string_ref const& database); + ////////////////////////////////////////////////////////////////////////////// /// @brief store the definition for the speicifed pool in the corresponding /// vocbase diff --git a/arangod/RestServer/DatabaseFeature.cpp b/arangod/RestServer/DatabaseFeature.cpp index 91e48de47c..0bc27f82d6 100644 --- a/arangod/RestServer/DatabaseFeature.cpp +++ b/arangod/RestServer/DatabaseFeature.cpp @@ -65,6 +65,7 @@ #include "VocBase/LogicalCollection.h" #include "VocBase/ticks.h" #include "VocBase/vocbase.h" +#include "IResearch/IResearchAnalyzerFeature.h" #include @@ -823,6 +824,12 @@ int DatabaseFeature::dropDatabase(std::string const& name, bool waitForDeletion, #endif arangodb::aql::QueryCache::instance()->invalidate(vocbase); + auto* analyzers = + arangodb::application_features::ApplicationServer::lookupFeature(); + if (analyzers != nullptr) { + analyzers->invalidate(*vocbase); + } + engine->prepareDropDatabase(*vocbase, !engine->inRecovery(), res); } // must not use the database after here, as it may now be diff --git a/tests/js/common/aql/aql-view-arangosearch-ddl-cluster.js b/tests/js/common/aql/aql-view-arangosearch-ddl-cluster.js index 3da72d3e1c..5a69bbcb25 100644 --- a/tests/js/common/aql/aql-view-arangosearch-ddl-cluster.js +++ b/tests/js/common/aql/aql-view-arangosearch-ddl-cluster.js @@ -1,5 +1,5 @@ /*jshint globalstrict:false, strict:false, maxlen: 500 */ -/*global fail, assertUndefined, assertEqual, assertNotEqual, assertTrue, assertFalse*/ +/*global fail, assertUndefined, assertEqual, assertNotEqual, assertTrue, assertFalse, assertNull*/ //////////////////////////////////////////////////////////////////////////////// /// DISCLAIMER @@ -1094,6 +1094,27 @@ function IResearchFeatureDDLTestSuite () { assertTrue(String === properties.links.col2.analyzers[0].constructor); assertEqual("identity", properties.links.col2.analyzers[0]); + db._useDatabase("_system"); + db._dropDatabase(dbName); + }, + testLeftAnalyzerInDroppedDatabase: function () { + const dbName = "TestNameDroppedDB"; + const analyzerName = "TestAnalyzer"; + db._useDatabase("_system"); + try { db._dropDatabase(dbName); } catch (e) {} + db._createDatabase(dbName); + db._useDatabase(dbName); + analyzers.save(analyzerName, "identity"); + // recreating database + db._useDatabase("_system"); + db._dropDatabase(dbName); + db._createDatabase(dbName); + db._useDatabase(dbName); + + assertNull(analyzers.analyzer(analyzerName)); + // this should be no name conflict + analyzers.save(analyzerName, "text", {"stopwords" : [], "locale":"en"}); + db._useDatabase("_system"); db._dropDatabase(dbName); } diff --git a/tests/js/common/aql/aql-view-arangosearch-ddl-noncluster.js b/tests/js/common/aql/aql-view-arangosearch-ddl-noncluster.js index 3d7ef7565c..4b4f3207ea 100644 --- a/tests/js/common/aql/aql-view-arangosearch-ddl-noncluster.js +++ b/tests/js/common/aql/aql-view-arangosearch-ddl-noncluster.js @@ -1,5 +1,5 @@ /*jshint globalstrict:false, strict:false, maxlen: 500 */ -/*global fail, assertUndefined, assertEqual, assertNotEqual, assertTrue, assertFalse*/ +/*global fail, assertUndefined, assertEqual, assertNotEqual, assertTrue, assertFalse, assertNull*/ //////////////////////////////////////////////////////////////////////////////// /// DISCLAIMER @@ -1094,6 +1094,27 @@ function IResearchFeatureDDLTestSuite () { assertTrue(String === properties.links.col2.analyzers[0].constructor); assertEqual("identity", properties.links.col2.analyzers[0]); + db._useDatabase("_system"); + db._dropDatabase(dbName); + }, + testLeftAnalyzerInDroppedDatabase: function () { + const dbName = "TestNameDroppedDB"; + const analyzerName = "TestAnalyzer"; + db._useDatabase("_system"); + try { db._dropDatabase(dbName); } catch (e) {} + db._createDatabase(dbName); + db._useDatabase(dbName); + analyzers.save(analyzerName, "identity"); + // recreating database + db._useDatabase("_system"); + db._dropDatabase(dbName); + db._createDatabase(dbName); + db._useDatabase(dbName); + + assertNull(analyzers.analyzer(analyzerName)); + // this should be no name conflict + analyzers.save(analyzerName, "text", {"stopwords" : [], "locale":"en"}); + db._useDatabase("_system"); db._dropDatabase(dbName); }