From 0eed7a8344af16486b08fe3ead7b003e7cfe086e Mon Sep 17 00:00:00 2001 From: Max Neunhoeffer Date: Mon, 8 Sep 2014 14:42:21 +0200 Subject: [PATCH 1/4] Handle exception correctly in AqlValue. --- arangod/Aql/AqlValue.cpp | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/arangod/Aql/AqlValue.cpp b/arangod/Aql/AqlValue.cpp index efa9cb2576..4d7bdece5b 100644 --- a/arangod/Aql/AqlValue.cpp +++ b/arangod/Aql/AqlValue.cpp @@ -116,8 +116,17 @@ AqlValue AqlValue::clone () const { case DOCVEC: { auto c = new std::vector; c->reserve(_vector->size()); - for (auto it = _vector->begin(); it != _vector->end(); ++it) { - c->push_back((*it)->slice(0, (*it)->size())); + try { + for (auto it = _vector->begin(); it != _vector->end(); ++it) { + c->push_back((*it)->slice(0, (*it)->size())); + } + } + catch (...) { + for (auto x : *c) { + delete x; + } + delete c; + throw; } return AqlValue(c); } From 68d30cef8b46d6bc8ad44e77e0b24ad46cfdb6c2 Mon Sep 17 00:00:00 2001 From: Willi Goesgens Date: Mon, 8 Sep 2014 15:06:30 +0200 Subject: [PATCH 2/4] Transaction: abort initialisation process on error so our internal error state stays sane. --- arangod/Utils/AqlTransaction.h | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/arangod/Utils/AqlTransaction.h b/arangod/Utils/AqlTransaction.h index 99b666f92c..a112499b33 100644 --- a/arangod/Utils/AqlTransaction.h +++ b/arangod/Utils/AqlTransaction.h @@ -70,7 +70,8 @@ namespace triagens { this->addHint(TRI_TRANSACTION_HINT_LOCK_ENTIRELY, false); for (auto it = collections->begin(); it != collections->end(); ++it) { - processCollection((*it).second); + if (processCollection((*it).second) != TRI_ERROR_NO_ERROR) + break; } } @@ -91,12 +92,12 @@ namespace triagens { /// @brief add a collection to the transaction //////////////////////////////////////////////////////////////////////////////// - void processCollection (triagens::aql::Collection* collection) { + int processCollection (triagens::aql::Collection* collection) { if (ServerState::instance()->isCoordinator()) { - processCollectionCoordinator(collection); + return processCollectionCoordinator(collection); } else { - processCollectionNormal(collection); + return processCollectionNormal(collection); } } @@ -104,17 +105,17 @@ namespace triagens { /// @brief add a coordinator collection to the transaction //////////////////////////////////////////////////////////////////////////////// - void processCollectionCoordinator (triagens::aql::Collection* collection) { + int processCollectionCoordinator (triagens::aql::Collection* collection) { TRI_voc_cid_t cid = this->resolver()->getCollectionIdCluster(collection->name); - this->addCollection(cid, collection->name.c_str(), collection->accessType); + return this->addCollection(cid, collection->name.c_str(), collection->accessType); } //////////////////////////////////////////////////////////////////////////////// /// @brief add a regular collection to the transaction //////////////////////////////////////////////////////////////////////////////// - void processCollectionNormal (triagens::aql::Collection* collection) { + int processCollectionNormal (triagens::aql::Collection* collection) { TRI_vocbase_col_t const* col = this->resolver()->getCollectionStruct(collection->name); TRI_voc_cid_t cid = 0; @@ -128,6 +129,8 @@ namespace triagens { col != nullptr) { collection->collection = const_cast(col); } + + return res; } //////////////////////////////////////////////////////////////////////////////// From 6e1fbbddfcb97235107326c5b33dc2da387e71a3 Mon Sep 17 00:00:00 2001 From: Willi Goesgens Date: Mon, 8 Sep 2014 15:07:05 +0200 Subject: [PATCH 3/4] More userfriendly errormessages if creating the transaction fails. --- arangod/Aql/Query.cpp | 26 +++++++++++++++++++++++--- arangod/Aql/Query.h | 8 ++++++++ 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/arangod/Aql/Query.cpp b/arangod/Aql/Query.cpp index 45e4e4e01e..fe110b0574 100644 --- a/arangod/Aql/Query.cpp +++ b/arangod/Aql/Query.cpp @@ -219,7 +219,7 @@ QueryResult Query::execute () { int res = trx.begin(); if (res != TRI_ERROR_NO_ERROR) { - return QueryResult(res, TRI_errno_string(res)); + return transactionError(res, trx); } plan = ExecutionPlan::instanciateFromAst(parser.ast()); @@ -243,7 +243,7 @@ QueryResult Query::execute () { int res = trx.begin(); if (res != TRI_ERROR_NO_ERROR) { - return QueryResult(res, TRI_errno_string(res)); + return transactionError(res, trx); } } @@ -368,7 +368,7 @@ QueryResult Query::explain () { int res = trx.begin(); if (res != TRI_ERROR_NO_ERROR) { - return QueryResult(res, TRI_errno_string(res)); + return transactionError(res, trx); } plan = ExecutionPlan::instanciateFromAst(parser.ast()); @@ -482,6 +482,26 @@ char* Query::registerString (std::string const& p, // --SECTION-- private methods // ----------------------------------------------------------------------------- +//////////////////////////////////////////////////////////////////////////////// +/// @brief neatly format transaction errors to the user. +//////////////////////////////////////////////////////////////////////////////// + +QueryResult Query::transactionError (int errorCode, AQL_TRANSACTION_V8 const& trx) +{ + std::string err; + err += std::string(TRI_errno_string(errorCode)); + + auto detail = trx.getErrorData(); + if (detail.size() > 0) { + err += std::string(" (") + detail + std::string(")"); + } + + err += std::string("\nwhile executing:\n") + _queryString + std::string("\n"); + + return QueryResult(errorCode, err); +} + + //////////////////////////////////////////////////////////////////////////////// /// @brief read the "optimizer.rules" section from the options //////////////////////////////////////////////////////////////////////////////// diff --git a/arangod/Aql/Query.h b/arangod/Aql/Query.h index c583b71fe7..b367bac119 100644 --- a/arangod/Aql/Query.h +++ b/arangod/Aql/Query.h @@ -35,6 +35,8 @@ #include "Aql/BindParameters.h" #include "Aql/Collections.h" #include "Aql/QueryResult.h" +#include "Utils/AqlTransaction.h" +#include "Utils/V8TransactionContext.h" struct TRI_json_s; struct TRI_vocbase_s; @@ -218,6 +220,12 @@ namespace triagens { std::vector getRulesFromOptions () const; +//////////////////////////////////////////////////////////////////////////////// +/// @brief neatly format transaction errors to the user. +//////////////////////////////////////////////////////////////////////////////// + + QueryResult transactionError (int errorCode, AQL_TRANSACTION_V8 const& trx); + // ----------------------------------------------------------------------------- // --SECTION-- private variables // ----------------------------------------------------------------------------- From e30227195a6932392daca683cecedb60bf62d53b Mon Sep 17 00:00:00 2001 From: Willi Goesgens Date: Mon, 8 Sep 2014 15:38:38 +0200 Subject: [PATCH 4/4] Start implementation for remove unecessary filters. --- ...timizer-rule-remove-unnecessary-filters.js | 159 ++++++++++++++++++ 1 file changed, 159 insertions(+) create mode 100644 js/server/tests/aql-optimizer-rule-remove-unnecessary-filters.js diff --git a/js/server/tests/aql-optimizer-rule-remove-unnecessary-filters.js b/js/server/tests/aql-optimizer-rule-remove-unnecessary-filters.js new file mode 100644 index 0000000000..9cc8111b32 --- /dev/null +++ b/js/server/tests/aql-optimizer-rule-remove-unnecessary-filters.js @@ -0,0 +1,159 @@ +/*jslint indent: 2, nomen: true, maxlen: 200, sloppy: true, vars: true, white: true, plusplus: true */ +/*global require, exports, assertTrue, assertEqual, AQL_EXECUTE, AQL_EXPLAIN, fail, loopmax */ +//////////////////////////////////////////////////////////////////////////////// +/// @brief tests for optimizer rules +/// +/// @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 2012, triAGENS GmbH, Cologne, Germany +//////////////////////////////////////////////////////////////////////////////// + +var jsunity = require("jsunity"); +var errors = require("internal").errors; +var helper = require("org/arangodb/aql-helper"); +var getQueryResults = helper.getQueryResults2; +var assertQueryError = helper.assertQueryError2; +var isEqual = helper.isEqual; + +//////////////////////////////////////////////////////////////////////////////// +/// @brief test suite +//////////////////////////////////////////////////////////////////////////////// + +function optimizerRuleTestSuite () { + var ruleName = "remove-unnecessary-calculations"; + // various choices to control the optimizer: + var paramNone = { optimizer: { rules: [ "-all" ] } }; + var paramEnabled = { optimizer: { rules: [ "-all", "+" + ruleName ] } }; + var paramDisabled = { optimizer: { rules: [ "+all", "-" + ruleName ] } }; + return { + +//////////////////////////////////////////////////////////////////////////////// +/// @brief set up +//////////////////////////////////////////////////////////////////////////////// + + setUp : function () { + }, + +//////////////////////////////////////////////////////////////////////////////// +/// @brief tear down +//////////////////////////////////////////////////////////////////////////////// + + tearDown : function () { + }, + +//////////////////////////////////////////////////////////////////////////////// +/// @brief test that rule has no effect when explicitly disabled +//////////////////////////////////////////////////////////////////////////////// + + testRuleDisabled : function () { + var queries = [ + "LET a = SLEEP(2) RETURN 1" + ]; + + queries.forEach(function(query) { + var result = AQL_EXPLAIN(query, { }, paramNone); + assertEqual([ ], result.plan.rules); + }); + }, + +//////////////////////////////////////////////////////////////////////////////// +/// @brief test that rule has no effect +//////////////////////////////////////////////////////////////////////////////// + + testRuleNoEffect : function () { + var queries = [ + "FOR a IN 1 RETURN a + 1" + ]; + + queries.forEach(function(query) { + var result = AQL_EXPLAIN(query, { }, paramEnabled); + assertEqual([ ], result.plan.rules, query); + }); + }, + +//////////////////////////////////////////////////////////////////////////////// +/// @brief test that rule has an effect +//////////////////////////////////////////////////////////////////////////////// + + testRuleHasEffect : function () { + var queries = [ + "FOR i IN 1..10 LET a = 1 FILTER i == a RETURN i" +// "FOR i IN 1..10 LET a = i + 1 FILTER i != a RETURN i" + ]; + + queries.forEach(function(query) { + var result = AQL_EXPLAIN(query, { }, paramEnabled); + //require("internal").print(result); + assertEqual([ ruleName ], result.plan.rules); + }); + }, + +//////////////////////////////////////////////////////////////////////////////// +/// @brief test generated plans +//////////////////////////////////////////////////////////////////////////////// + + + testPlans : function () { + var plans = [ + ["FOR i IN 1..10 LET a = 1 FILTER i == a RETURN i", ["SingletonNode", "CalculationNode", "EnumerateListNode", "CalculationNode", "FilterNode", "ReturnNode" ]] + ]; + + plans.forEach(function(plan) { + var result = AQL_EXPLAIN(plan[0], { }, paramEnabled); + assertEqual([ ruleName ], result.plan.rules, plan[0]); + //require("internal").print(helper.getCompactPlan(result).map(function(node) { return node.type; })); + assertEqual(plan[1], helper.getCompactPlan(result).map(function(node) { return node.type; }), plan[0]); + }); + }, + +//////////////////////////////////////////////////////////////////////////////// +/// @brief test results +//////////////////////////////////////////////////////////////////////////////// + + testResults : function () { + var queries = [ + ["FOR i IN 1..10 LET a = 1 FILTER i == a RETURN i", ["SingletonNode", "CalculationNode", "EnumerateListNode", "CalculationNode", "FilterNode", "ReturnNode" ]] + ]; + + queries.forEach(function(query) { + var resultDisabled = AQL_EXECUTE(query[0], { }, paramDisabled).json; + var resultEnabled = AQL_EXECUTE(query[0], { }, paramEnabled).json; + + assertTrue(isEqual(resultDisabled, resultEnabled), query[0]); + }); + } + + }; +} + +//////////////////////////////////////////////////////////////////////////////// +/// @brief executes the test suite +//////////////////////////////////////////////////////////////////////////////// + +jsunity.run(optimizerRuleTestSuite); + +return jsunity.done(); + +// Local Variables: +// mode: outline-minor +// outline-regexp: "^\\(/// @brief\\|/// @addtogroup\\|// --SECTION--\\|/// @page\\|/// @}\\)" +// End: