10000 fixed issue #16279 · arangodb/arangodb@ac51069 · GitHub
[go: up one dir, main page]
Skip to content

Commit ac51069

Browse files
jsteemannjsteemann
committed
fixed issue #16279 
1 parent 1599363 commit ac51069

4 files changed

+117
-101
lines changed

‎CHANGELOG

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
devel
22
-----
33

4+
* Fixed Github issue #16279: assertion failure/crash in AQL query optimizer
5+
when permuting adjacent FOR loops that depended on each other.
6+
47
* Put hotbackup requests on the HIGH priority queue to make hotbackups work
58
under high load (BTS-865).
69

‎arangod/Aql/OptimizerRules.cpp

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3564,6 +3564,8 @@ void arangodb::aql::interchangeAdjacentEnumerationsRule(
35643564
std::vector<size_t> starts;
35653565
std::vector<ExecutionNode*> nn;
35663566

3567+
VarSet inputVars;
3568+
35673569
// We use that the order of the nodes is such that a node B that is among the
35683570
// recursive dependencies of a node A is later in the vector.
35693571
for (auto const& n : nodes) {
@@ -3572,6 +3574,8 @@ void arangodb::aql::interchangeAdjacentEnumerationsRule(
35723574
nn.emplace_back(n);
35733575
nodesSet.erase(n);
35743576

3577+
inputVars.clear();
3578+
35753579
// Now follow the dependencies as long as we see further such nodes:
35763580
auto nwalker = n;
35773581

@@ -3582,6 +3586,9 @@ void arangodb::aql::interchangeAdjacentEnumerationsRule(
35823586

35833587
auto dep = nwalker->getFirstDependency();
35843588

3589+
// track variables that we rely on
3590+
nwalker->getVariablesUsedHere(inputVars);
3591+
35853592
if (dep->getType() != EN::ENUMERATE_COLLECTION &&
35863593
dep->getType() != EN::ENUMERATE_LIST) {
35873594
break;
@@ -3592,6 +3599,26 @@ void arangodb::aql::interchangeAdjacentEnumerationsRule(
35923599
break;
35933600
}
35943601

3602+
// check if nodes depend on each other (i.e. node C consumes a variable
3603+
// introduced by node B or A):
3604+
// - FOR a IN A
3605+
// - FOR b IN a.values
3606+
// - FOR c IN b.values
3607+
// or
3608+
// - FOR a IN A
3609+
// - FOR b IN ...
3610+
// - FOR c IN a.values
3611+
bool foundDependency = false;
3612+
for (auto const& outVar : dep->getVariablesSetHere()) {
3613+
if (inputVars.contains(outVar)) {
3614+
foundDependency = true;
3615+
break;
3616+
}
3617+
}
3618+
if (foundDependency) {
3619+
break;
3620+
}
3621+
35953622
nwalker = dep;
35963623
nn.emplace_back(nwalker);
35973624
nodesSet.erase(nwalker);

‎tests/js/server/aql/aql-optimizer-rule-interchange-adjacent-enumerations-cluster.js

Lines changed: 41 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -28,46 +28,39 @@
2828
/// @author Copyright 2012, triAGENS GmbH, Cologne, Germany
2929
////////////////////////////////////////////////////////////////////////////////
3030

31-
var jsunity = require("jsunity");
32-
var helper = require("@arangodb/aql-helper");
33-
var isEqual = helper.isEqual;
34-
var db = require("@arangodb").db;
35-
var _ = require("lodash");
31+
const jsunity = require("jsunity");
32+
const helper = require("@arangodb/aql-helper");
33+
const isEqual = helper.isEqual;
34+
const db = require("@arangodb").db;
35+
const _ = require("lodash");
3636

3737
////////////////////////////////////////////////////////////////////////////////
3838
/// @brief test suite
3939
////////////////////////////////////////////////////////////////////////////////
4040

4141
function optimizerRuleTestSuite () {
42-
var ruleName = "interchange-adjacent-enumerations";
42+
const ruleName = "interchange-adjacent-enumerations";
4343

4444
// various choices to control the optimizer:
45-
var paramNone = { optimizer: { rules: [ "-all" ] } };
46-
var paramEnabled = { optimizer: { rules: [ "-all", "+" + ruleName ] } };
47-
var paramDisabled = { optimizer: { rules: [ "+all", "-" + ruleName ] } };
45+
const paramNone = { optimizer: { rules: [ "-all" ] } };
46+
const paramEnabled = { optimizer: { rules: [ "-all", "+" + ruleName ] } };
47+
const paramDisabled = { optimizer: { rules: [ "+all", "-" + ruleName ] } };
4848

49-
var collection = null;
50-
var collectionName = "UnitTestsAhuacatlOptimizer";
49+
const collectionName = "UnitTestsAhuacatlOptimizer";
5150

5251
return {
5352

54-
////////////////////////////////////////////////////////////////////////////////
55-
/// @brief set up
56-
////////////////////////////////////////////////////////////////////////////////
57-
5853
setUpAll : function () {
5954
db._drop(collectionName);
60-
collection = db._create(collectionName);
55+
let collection = db._create(collectionName);
6156

62-
for (var i = 0; i < 10; ++i) {
63-
collection.save({ value: i });
57+
let docs = [];
58+
for (let i = 0; i < 10; ++i) {
59+
docs.push({ value: i });
6460
}
61+
collection.insert(docs);
6562
},
6663

67-
////////////////////////////////////////////////////////////////////////////////
68-
/// @brief tear down
69-
////////////////////////////////////////////////////////////////////////////////
70-
7164
tearDownAll : function () {
7265
db._drop(collectionName);
7366
},
@@ -77,17 +70,17 @@ function optimizerRuleTestSuite () {
7770
////////////////////////////////////////////////////////////////////////////////
7871

7972
testRuleDisabled : function () {
80-
var queries = [
73+
let queries = [
8174
"FOR i IN " + collectionName + " FOR j IN " + collectionName + " RETURN 1",
8275
"FOR j IN " + collectionName + " FILTER j.i == 1 FOR i IN " + collectionName + " RETURN j"
8376
];
8477

85-
var opts = _.clone(paramNone);
78+
let opts = _.clone(paramNone);
8679
opts.allPlans = true;
8780
opts.verbosePlans = true;
8881

8982
queries.forEach(function(query) {
90-
var result = AQL_EXPLAIN(query, { }, opts);
83+
let result = AQL_EXPLAIN(query, { }, opts);
9184
result.plans.forEach(function(plan) {
9285
assertEqual([ "scatter-in-cluster" ], plan.rules);
9386
});
@@ -99,22 +92,27 @@ function optimizerRuleTestSuite () {
9992
////////////////////////////////////////////////////////////////////////////////
10093

10194
testRuleNoEffect : function () {
102-
var queries = [
95+
let queries = [
10396
"FOR i IN 1..10 RETURN i",
10497
"FOR i IN " + collectionName + " RETURN i",
10598
"FOR i IN " + collectionName + " FILTER i == 1 FOR j IN " + collectionName + " RETURN i",
10699
"FOR i IN " + collectionName + " LIMIT 1 FOR j IN " + collectionName + " RETURN i",
107-
"FOR i IN " + collectionName + " RETURN (FOR j IN " + collectionName + " RETURN j)"
100+
"FOR i IN " + collectionName + " RETURN (FOR j IN " + collectionName + " RETURN j)",
101+
// the following query must not be optimized because "sub" depends on "i"
102+
"FOR i IN " + collectionName + " FOR sub IN i FILTER sub.value1 == 'test' && sub.value2 != '' RETURN i",
103+
"FOR i IN " + collectionName + " FOR sub1 IN i FOR sub2 IN sub1 FILTER sub2.value1 == 'test' && sub2.value2 != '' RETURN i",
104+
"FOR i IN " + collectionName + " FOR sub1 IN i FOR sub2 IN i FILTER sub2.value1 == 'test' && sub2.value2 != '' RETURN i",
105+
"FOR i IN " + collectionName + " FOR sub1 IN i FOR sub2 IN i FILTER sub2.value1 == 'test' && sub2.value2 != '' && sub2.value != sub1 RETURN i",
108106
];
109107

110-
var opts = _.clone(paramEnabled);
108+
let opts = _.clone(paramEnabled);
111109
opts.allPlans = true;
112110
opts.verbosePlans = true;
113111

114112
queries.forEach(function(query) {
115-
var result = AQL_EXPLAIN(query, { }, opts);
113+
let result = AQL_EXPLAIN(query, { }, opts);
116114
result.plans.forEach(function(plan) {
117-
assertTrue(plan.rules.indexOf(ruleName) === -1, query);
115+
assertEqual(-1, plan.rules.indexOf(ruleName), query);
118116
});
119117
});
120118
},
@@ -124,7 +122,7 @@ function optimizerRuleTestSuite () {
124122
////////////////////////////////////////////////////////////////////////////////
125123

126124
testRuleHasEffect : function () {
127-
var queries = [
125+
let queries = [
128126
[ "FOR i IN " + collectionName + " FOR j IN " + collectionName + " RETURN i", 1 ],
129127
[ "FOR i IN 1..10 FOR j IN " + collectionName + " FOR k IN " + collectionName + " RETURN i", 5 ],
130128
[ "FOR i IN " + collectionName + " FOR j IN " + collectionName + " FOR k IN " + collectionName + " RETURN i", 5 ],
@@ -134,15 +132,15 @@ function optimizerRuleTestSuite () {
134132
[ "FOR x IN (FOR i IN " + collectionName + " FOR j IN " + collectionName + " FOR k IN " + collectionName + " RETURN i) FOR y IN (FOR i IN " + collectionName + " FOR j IN " + collectionName + " RETURN i) RETURN x", 11 ]
135133
];
136134

137-
var opts = _.clone(paramEnabled);
135+
let opts = _.clone(paramEnabled);
138136
opts.allPlans = true;
139137
opts.verbosePlans = true;
140138

141139
queries.forEach(function(query) {
142-
var withRule = 0;
143-
var withoutRule = 0;
140+
let withRule = 0;
141+
let withoutRule = 0;
144142

145-
var result = AQL_EXPLAIN(query[0], { }, opts);
143+
let result = AQL_EXPLAIN(query[0], { }, opts);
146144
result.plans.forEach(function(plan) {
147145
if (plan.rules.indexOf(ruleName) === -1) {
148146
withoutRule++;
@@ -165,30 +163,30 @@ function optimizerRuleTestSuite () {
165163
////////////////////////////////////////////////////////////////////////////////
166164

167165
testResults : function () {
168-
var queries = [
166+
let queries = [
169167
[ "FOR i IN " + collectionName + " FOR j IN " + collectionName + " SORT i.value, j.value FILTER i.value == j.value RETURN i.value", [ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 ] ],
170168
[ "FOR j IN " + collectionName + " FOR i IN " + collectionName + " SORT i.value, j.value FILTER i.value == j.value RETURN i.value", [ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 ] ],
171169
[ "FOR x IN (FOR i IN " + collectionName + " FOR j IN " + collectionName + " RETURN { i: i.value, j: j.value }) FILTER x.i == x.j SORT x.i RETURN x.i", [ 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 ] ]
172170
];
173171

174-
var opts = _.clone(paramEnabled);
172+
let opts = _.clone(paramEnabled);
175173
opts.allPlans = true;
176174
opts.verbosePlans = true;
177175

178176
queries.forEach(function(query) {
179-
var planDisabled = AQL_EXPLAIN(query[0], { }, paramDisabled);
180-
var plansEnabled = AQL_EXPLAIN(query[0], { }, opts);
181-
var resultDisabled = AQL_EXECUTE(query[0], { }, paramDisabled).json;
177+
let planDisabled = AQL_EXPLAIN(query[0], { }, paramDisabled);
178+
let plansEnabled = AQL_EXPLAIN(query[0], { }, opts);
179+
let resultDisabled = AQL_EXECUTE(query[0], { }, paramDisabled).json;
182180

183-
assertTrue(planDisabled.plan.rules.indexOf(ruleName) === -1, query[0]);
181+
assertEqual(-1, planDisabled.plan.rules.indexOf(ruleName), query[0]);
184182
assertEqual(resultDisabled, query[1]);
185183

186184
assertTrue(plansEnabled.plans.length > 1);
187185

188186
// iterate over all plans
189-
var withRule = 0;
187+
let withRule = 0;
190188
plansEnabled.plans.forEach(function(plan) {
191-
var resultEnabled = AQL_EXECUTEJSON(plan).json;
189+
let resultEnabled = AQL_EXECUTEJSON(plan).json;
192190
assertTrue(isEqual(resultDisabled, resultEnabled), query[0]);
193191
if (plan.rules.indexOf(ruleName) !== -1) {
194192
withRule++;
@@ -204,11 +202,6 @@ function optimizerRuleTestSuite () {
204202
};
205203
}
206204

207-
////////////////////////////////////////////////////////////////////////////////
208-
/// @brief executes the test suite
209-
////////////////////////////////////////////////////////////////////////////////
210-
211205
jsunity.run(optimizerRuleTestSuite);
212206

213207
return jsunity.done();
214-

0 commit comments

Comments
 (0)
0