From 5bee042b65ce2b6c6ad74f0bf74f06043581173f Mon Sep 17 00:00:00 2001 From: Dronplane Date: Thu, 22 Aug 2019 14:25:14 +0300 Subject: [PATCH] Bug fix 3.5/internal issue #622 (#9787) * Bug fix/internal issue #622 (#9781) * Added analyzer cache invalidation for dropped database * Fixed jslint reported errors * Update CHANGELOG --- CHANGELOG | 2 + .../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 ++++++- 6 files changed, 100 insertions(+), 30 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index f807b38326..0116b8cc4e 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,8 @@ v3.5.1 (XXXX-XX-XX) ------------------- +* Fixed internal issue #622: Analyzer cache is now invalidated for dropped database. + * Show query string length and cacheability information in query explain output. * The AQL functions `FULLTEXT`, `NEAR`, `WITHIN` and `WITHIN_RECTANGLE` are now diff --git a/arangod/IResearch/IResearchAnalyzerFeature.cpp b/arangod/IResearch/IResearchAnalyzerFeature.cpp index 6051dc70ed..a02799fc70 100644 --- a/arangod/IResearch/IResearchAnalyzerFeature.cpp +++ b/arangod/IResearch/IResearchAnalyzerFeature.cpp @@ -655,6 +655,7 @@ bool dropLegacyAnalyzersCollection( return lookupRes.is(TRI_ERROR_ARANGO_DATA_SOURCE_NOT_FOUND); } + void registerUpgradeTasks() { using namespace arangodb; using namespace arangodb::application_features; @@ -1708,38 +1709,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 @@ -1748,7 +1726,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 @@ -2472,6 +2452,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 68fa56bbc4..13efcf73b0 100644 --- a/arangod/IResearch/IResearchAnalyzerFeature.h +++ b/arangod/IResearch/IResearchAnalyzerFeature.h @@ -251,6 +251,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 @@ -314,6 +320,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 7db05e816c..8d6fc3da5e 100644 --- a/arangod/RestServer/DatabaseFeature.cpp +++ b/arangod/RestServer/DatabaseFeature.cpp @@ -61,6 +61,7 @@ #include "VocBase/LogicalCollection.h" #include "VocBase/ticks.h" #include "VocBase/vocbase.h" +#include "IResearch/IResearchAnalyzerFeature.h" #include @@ -816,6 +817,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); }