fixed issue #6601 (#6608)

2018-09-26 13:42:50 +02:00 · 2018-09-26 13:42:50 +02:00 · fbf2904ab0
parent c8a0c074fe
commit fbf2904ab0
5 changed files with 244 additions and 24 deletions
--- a/6
+++ b/6
@ -1,10 +1,12 @@
 devel
 -----

+* fixed issue #6601: Context cancelled (never ending query)
+
 * added more AQL query results cache inspection and control functionality

-* the query editor within the web ui is now catching http 501 responses
-  propertly.
+* the query editor within the web ui is now catching HTTP 501 responses
+  properly

 * upgraded JEMalloc version to 5.1.0

--- a/arangod/Aql/ClusterBlocks.cpp
+++ b/arangod/Aql/ClusterBlocks.cpp
@ -70,7 +70,7 @@ class OurLessThan {
 public:
  OurLessThan(
      arangodb::transaction::Methods* trx,
-      std::vector<std::deque<AqlItemBlock*>>& gatherBlockBuffer,
+      std::vector<std::deque<AqlItemBlock*>> const& gatherBlockBuffer,
      std::vector<SortRegister>& sortRegisters) noexcept
    : _trx(trx),
      _gatherBlockBuffer(gatherBlockBuffer),
@ -84,7 +84,7 @@ class OurLessThan {

 private:
  arangodb::transaction::Methods* _trx;
-  std::vector<std::deque<AqlItemBlock*>>& _gatherBlockBuffer;
+  std::vector<std::deque<AqlItemBlock*>> const& _gatherBlockBuffer;
  std::vector<SortRegister>& _sortRegisters;
 }; // OurLessThan

@ -148,7 +148,7 @@ class HeapSorting final : public SortingStrategy, private OurLessThan  {
 public:
  HeapSorting(
      arangodb::transaction::Methods* trx,
-      std::vector<std::deque<AqlItemBlock*>>& gatherBlockBuffer,
+      std::vector<std::deque<AqlItemBlock*>> const& gatherBlockBuffer,
      std::vector<SortRegister>& sortRegisters) noexcept
    : OurLessThan(trx, gatherBlockBuffer, sortRegisters) {
  }
@ -196,7 +196,7 @@ class MinElementSorting final : public SortingStrategy, public OurLessThan {
 public:
  MinElementSorting(
      arangodb::transaction::Methods* trx,
-      std::vector<std::deque<AqlItemBlock*>>& gatherBlockBuffer,
+      std::vector<std::deque<AqlItemBlock*>> const& gatherBlockBuffer,
      std::vector<SortRegister>& sortRegisters) noexcept
    : OurLessThan(trx, gatherBlockBuffer, sortRegisters),
      _blockPos(nullptr) {
@ -1248,14 +1248,12 @@ SortingGatherBlock::~SortingGatherBlock() {
 }

 void SortingGatherBlock::clearBuffers() noexcept {
-  for (std::deque<AqlItemBlock*>& x : _gatherBlockBuffer) {
-    for (AqlItemBlock* y : x) {
-      delete y;
+  for (std::deque<AqlItemBlock*>& it : _gatherBlockBuffer) {
+    for (AqlItemBlock* b : it) {
+      delete b;
    }
-    x.clear();
+    it.clear();
  }
-  _gatherBlockBuffer.clear();
-  _gatherBlockPos.clear();
 }

 /// @brief initializeCursor
@ -1268,12 +1266,31 @@ SortingGatherBlock::initializeCursor(AqlItemBlock* items, size_t pos) {
  }

  clearBuffers();
-  _gatherBlockBuffer.reserve(_dependencies.size());
-  _gatherBlockPos.reserve(_dependencies.size());
-  for (size_t i = 0; i < _dependencies.size(); i++) {
-    _gatherBlockBuffer.emplace_back();
-    _gatherBlockPos.emplace_back(i, 0);
+
+  TRI_ASSERT(!_dependencies.empty());
+
+  if (_gatherBlockBuffer.empty()) {
+    // only do this initialization once
+    _gatherBlockBuffer.reserve(_dependencies.size());
+    _gatherBlockPos.reserve(_dependencies.size());
+    _gatherBlockPosDone.reserve(_dependencies.size());
+
+    for (size_t i = 0; i < _dependencies.size(); ++i) {
+      _gatherBlockBuffer.emplace_back();
+      _gatherBlockPos.emplace_back(i, 0);
+      _gatherBlockPosDone.push_back(false);
+    }
+  } else {
+    for (size_t i = 0; i < _dependencies.size(); i++) {
+      TRI_ASSERT(_gatherBlockBuffer[i].empty());
+      _gatherBlockPos[i].second = 0;
+      _gatherBlockPosDone[i] = false;
+    }
  }
+  
+  TRI_ASSERT(_gatherBlockBuffer.size() == _dependencies.size());
+  TRI_ASSERT(_gatherBlockPos.size() == _dependencies.size());
+  TRI_ASSERT(_gatherBlockPosDone.size() == _dependencies.size());

  _strategy->reset();

@ -1297,6 +1314,9 @@ SortingGatherBlock::initializeCursor(AqlItemBlock* items, size_t pos) {
 std::pair<ExecutionState, size_t> SortingGatherBlock::fillBuffers(
    size_t atMost) {
  size_t available = 0;
+  
+  TRI_ASSERT(_gatherBlockBuffer.size() == _dependencies.size());
+  TRI_ASSERT(_gatherBlockPos.size() == _dependencies.size());

  // In the future, we should request all blocks in parallel. But not everything
  // is yet thread safe for that to work, so we have to return immediately on
@ -1305,7 +1325,7 @@ std::pair<ExecutionState, size_t> SortingGatherBlock::fillBuffers(
    // reset position to 0 if we're going to fetch a new block.
    // this doesn't hurt, even if we don't get one.
    if (_gatherBlockBuffer[i].empty()) {
-      _gatherBlockPos[i] = std::make_pair(i, 0);
+      _gatherBlockPos[i].second = 0;
    }
    ExecutionState state;
    bool blockAppended;
@ -1324,8 +1344,11 @@ std::pair<ExecutionState, size_t> SortingGatherBlock::fillBuffers(
 size_t SortingGatherBlock::availableRows(size_t i) const {
  size_t available = 0;

+  TRI_ASSERT(_gatherBlockBuffer.size() == _dependencies.size());
+  TRI_ASSERT(i < _dependencies.size());
+
  auto const& blocks = _gatherBlockBuffer[i];
-  auto const& curRowIdx = _gatherBlockPos[i].second;
+  size_t curRowIdx = _gatherBlockPos[i].second;

  if (!blocks.empty()) {
    TRI_ASSERT(blocks[0]->size() >= curRowIdx);
@ -1383,7 +1406,7 @@ SortingGatherBlock::getSome(size_t atMost) {

  // the following is similar to AqlItemBlock's slice method . . .
  std::vector<std::unordered_map<AqlValue, AqlValue>> cache;
-  cache.resize(_gatherBlockBuffer.size());
+  cache.resize(_dependencies.size());

  size_t nrRegs = getNrInputRegisters();

@ -1396,7 +1419,7 @@ SortingGatherBlock::getSome(size_t atMost) {
  for (size_t i = 0; i < toSend; i++) {
    // get the next smallest row from the buffer . . .
    auto const val = _strategy->nextValue();
-    auto& blocks = _gatherBlockBuffer[val.first];
+    auto const& blocks = _gatherBlockBuffer[val.first];

    // copy the row in to the outgoing block . . .
    for (RegisterId col = 0; col < nrRegs; col++) {
@ -1481,6 +1504,10 @@ std::pair<ExecutionState, size_t> SortingGatherBlock::skipSome(size_t atMost) {
 /// updates _gatherBlockBuffer and _gatherBlockPos. If necessary, steps to the
 /// next block and removes the previous one. Will not fetch more blocks.
 void SortingGatherBlock::nextRow(size_t i) {
+  TRI_ASSERT(i < _dependencies.size());
+  TRI_ASSERT(_gatherBlockBuffer.size() == _dependencies.size());
+  TRI_ASSERT(_gatherBlockPos.size() == _dependencies.size());
+
  auto& blocks = _gatherBlockBuffer[i];
  auto& blocksPos = _gatherBlockPos[i];
  if (++blocksPos.second == blocks.front()->size()) {
@ -1499,6 +1526,13 @@ void SortingGatherBlock::nextRow(size_t i) {
 std::pair<ExecutionState, bool> SortingGatherBlock::getBlocks(size_t i,
                                                              size_t atMost) {
  TRI_ASSERT(i < _dependencies.size());
+  TRI_ASSERT(_gatherBlockBuffer.size() == _dependencies.size());
+  TRI_ASSERT(_gatherBlockPos.size() == _dependencies.size());
+  TRI_ASSERT(_gatherBlockPosDone.size() == _dependencies.size());
+
+  if (_gatherBlockPosDone[i]) {
+    return {ExecutionState::DONE, false};
+  }

  bool blockAppended = false;
  size_t rowsAvailable = availableRows(i);
@ -1515,6 +1549,10 @@ std::pair<ExecutionState, bool> SortingGatherBlock::getBlocks(size_t i,
    // Assert that state == WAITING => itemBlock == nullptr
    TRI_ASSERT(state != ExecutionState::WAITING || itemBlock == nullptr);

+    if (state == ExecutionState::DONE) {
+      _gatherBlockPosDone[i] = true;
+    }
+
    if (itemBlock && itemBlock->size() > 0) {
      rowsAvailable += itemBlock->size();
      _gatherBlockBuffer[i].emplace_back(itemBlock.get());
--- a/arangod/Aql/ClusterBlocks.h
+++ b/arangod/Aql/ClusterBlocks.h
@ -387,6 +387,9 @@ class SortingGatherBlock final : public ExecutionBlock {
  /// @brief _gatherBlockPos: pairs (i, _pos in _buffer.at(i)), i.e. the same as
  /// the usual _pos but one pair per dependency
  std::vector<std::pair<size_t, size_t>> _gatherBlockPos;
+  
+  /// @brief whether or not the dependency at index is done
+  std::vector<bool> _gatherBlockPosDone;

  /// @brief sort elements for this block
  std::vector<SortRegister> _sortRegisters;
@ -395,8 +398,6 @@ class SortingGatherBlock final : public ExecutionBlock {
  std::unique_ptr<SortingStrategy> _strategy;
 }; // SortingGatherBlock

-
-
 class SingleRemoteOperationBlock final : public ExecutionBlock {
  /// @brief constructors/destructors
 private:
--- a/arangod/Aql/ClusterNodes.cpp
+++ b/arangod/Aql/ClusterNodes.cpp
@ -369,7 +369,7 @@ std::unique_ptr<ExecutionBlock> GatherNode::createBlock(
    ExecutionEngine& engine,
    std::unordered_map<ExecutionNode*, ExecutionBlock*> const&
 ) const {
-  if (elements().empty()) {
+  if (_elements.empty()) {
    return std::make_unique<UnsortingGatherBlock>(engine, *this);
  }

--- a/tests/js/server/aql/aql-gather-block-cluster.js
+++ b/tests/js/server/aql/aql-gather-block-cluster.js
@ -84,6 +84,185 @@ function gatherBlockTestSuite () {
      c2 = null;
      c3 = null;
    },
+    
+    testMoreShardsThanDocumentsHeapSort : function () {
+      db._drop(cn3);
+      c3 = db._create(cn3, {numberOfShards:40});
+      for (let i = 0; i < 39; ++i) {
+        c3.insert({ value : i });
+      }
+      
+      let query = "FOR doc IN " + cn3 + " SORT doc.value RETURN doc.value";
+      // check the return value
+      let result = AQL_EXECUTE(query).json;
+      assertEqual(39, result.length);
+      let last = -1;
+      result.forEach(function(value) {
+        assertTrue(value >= last);
+        last = value;
+      });
+      
+      let nodes = AQL_EXPLAIN(query).plan.nodes;
+      let nodeTypes = nodes.map(function(node) { return node.type; });
+      // must have a sort and gather node
+      assertNotEqual(-1, nodeTypes.indexOf("SortNode"));
+      let sortNode = nodes[nodeTypes.indexOf("SortNode")];
+      assertEqual(1, sortNode.elements.length);
+      assertTrue(sortNode.elements[0].ascending);
+      assertNotEqual(-1, nodeTypes.indexOf("GatherNode"));
+      let gatherNode = nodes[nodeTypes.indexOf("GatherNode")];
+      assertEqual(1, gatherNode.elements.length); // must be a sorting GatherNode
+      assertEqual("heap", gatherNode.sortmode);
+    },
+    
+    testMoreShardsThanDocumentsMinElementSort : function () {
+      db._drop(cn3);
+      c3 = db._create(cn3, {numberOfShards:4});
+      for (let i = 0; i < 3; ++i) {
+        c3.insert({ value : i });
+      }
+      
+      let query = "FOR doc IN " + cn3 + " SORT doc.value RETURN doc.value";
+      // check the return value
+      let result = AQL_EXECUTE(query).json;
+      assertEqual(3, result.length);
+      let last = -1;
+      result.forEach(function(value) {
+        assertTrue(value >= last);
+        last = value;
+      });
+      
+      let nodes = AQL_EXPLAIN(query).plan.nodes;
+      let nodeTypes = nodes.map(function(node) { return node.type; });
+      // must have a sort and gather node
+      assertNotEqual(-1, nodeTypes.indexOf("SortNode"));
+      let sortNode = nodes[nodeTypes.indexOf("SortNode")];
+      assertEqual(1, sortNode.elements.length);
+      assertTrue(sortNode.elements[0].ascending);
+      assertNotEqual(-1, nodeTypes.indexOf("GatherNode"));
+      let gatherNode = nodes[nodeTypes.indexOf("GatherNode")];
+      assertEqual(1, gatherNode.elements.length); // must be a sorting GatherNode
+      assertEqual("minelement", gatherNode.sortmode);
+    },
+    
+    testMoreShardsThanDocumentsNoSort : function () {
+      db._drop(cn3);
+      c3 = db._create(cn3, {numberOfShards:40});
+      for (let i = 0; i < 39; ++i) {
+        c3.insert({ value : i });
+      }
+      
+      let query = "FOR doc IN " + cn3 + " RETURN doc.value";
+      // check the return value
+      let result = AQL_EXECUTE(query).json;
+      assertEqual(39, result.length);
+      
+      let nodes = AQL_EXPLAIN(query).plan.nodes;
+      let nodeTypes = nodes.map(function(node) { return node.type; });
+      // must not have a sort, but a gather node
+      assertEqual(-1, nodeTypes.indexOf("SortNode"));
+      assertNotEqual(-1, nodeTypes.indexOf("GatherNode"));
+      let gatherNode = nodes[nodeTypes.indexOf("GatherNode")];
+      assertEqual(0, gatherNode.elements.length); // must be a non-sorting GatherNode
+      assertEqual("unset", gatherNode.sortmode);
+    },
+    
+    testMoreDocumentsThanShardsHeapSort : function () {
+      db._drop(cn3);
+      c3 = db._create(cn3, {numberOfShards:10});
+      let docs = [];
+      for (let i = 0; i < 20000; ++i) {
+        docs.push({ value: i });
+        if (docs.length === 1000) {
+          c3.insert(docs);
+          docs = [];
+        }
+      }
+      
+      let query = "FOR doc IN " + cn3 + " SORT doc.value RETURN doc.value";
+      // check the return value
+      let result = AQL_EXECUTE(query).json;
+      assertEqual(20000, result.length);
+      let last = -1;
+      result.forEach(function(value) {
+        assertTrue(value >= last);
+        last = value;
+      });
+      
+      let nodes = AQL_EXPLAIN(query).plan.nodes;
+      let nodeTypes = nodes.map(function(node) { return node.type; });
+      // must have a sort and gather node
+      assertNotEqual(-1, nodeTypes.indexOf("SortNode"));
+      let sortNode = nodes[nodeTypes.indexOf("SortNode")];
+      assertEqual(1, sortNode.elements.length);
+      assertTrue(sortNode.elements[0].ascending);
+      assertNotEqual(-1, nodeTypes.indexOf("GatherNode"));
+      let gatherNode = nodes[nodeTypes.indexOf("GatherNode")];
+      assertEqual(1, gatherNode.elements.length); // must be a sorting GatherNode
+      assertEqual("heap", gatherNode.sortmode);
+    },
+    
+    testMoreDocumentsThanShardsMinElementSort : function () {
+      db._drop(cn3);
+      c3 = db._create(cn3, {numberOfShards:4});
+      let docs = [];
+      for (let i = 0; i < 20000; ++i) {
+        docs.push({ value: i });
+        if (docs.length === 1000) {
+          c3.insert(docs);
+          docs = [];
+        }
+      }
+      
+      let query = "FOR doc IN " + cn3 + " SORT doc.value RETURN doc.value";
+      // check the return value
+      let result = AQL_EXECUTE(query).json;
+      assertEqual(20000, result.length);
+      let last = -1;
+      result.forEach(function(value) {
+        assertTrue(value >= last);
+        last = value;
+      });
+      
+      let nodes = AQL_EXPLAIN(query).plan.nodes;
+      let nodeTypes = nodes.map(function(node) { return node.type; });
+      // must have a sort and gather node
+      assertNotEqual(-1, nodeTypes.indexOf("SortNode"));
+      let sortNode = nodes[nodeTypes.indexOf("SortNode")];
+      assertEqual(1, sortNode.elements.length);
+      assertTrue(sortNode.elements[0].ascending);
+      assertNotEqual(-1, nodeTypes.indexOf("GatherNode"));
+      let gatherNode = nodes[nodeTypes.indexOf("GatherNode")];
+      assertEqual(1, gatherNode.elements.length); // must be a sorting GatherNode
+      assertEqual("minelement", gatherNode.sortmode);
+    },
+    
+    testMoreDocumentsThanShardsNoSort : function () {
+      db._drop(cn3);
+      c3 = db._create(cn3, {numberOfShards:20});
+      let docs = [];
+      for (let i = 0; i < 20000; ++i) {
+        docs.push({ value: i });
+        if (docs.length === 1000) {
+          c3.insert(docs);
+          docs = [];
+        }
+      }
+      
+      let query = "FOR doc IN " + cn3 + " RETURN doc.value";
+      // check the return value
+      let result = AQL_EXECUTE(query).json;
+      assertEqual(20000, result.length);
+      
+      let nodes = AQL_EXPLAIN(query).plan.nodes;
+      let nodeTypes = nodes.map(function(node) { return node.type; });
+      // must not have a sort, but a gather node
+      assertEqual(-1, nodeTypes.indexOf("SortNode"));
+      assertNotEqual(-1, nodeTypes.indexOf("GatherNode"));
+      let gatherNode = nodes[nodeTypes.indexOf("GatherNode")];
+      assertEqual(0, gatherNode.elements.length); // must be a non-sorting GatherNode
+      assertEqual("unset", gatherNode.sortmode);
+    },

    testSingleShard : function () {
      let query = "FOR doc IN " + cn2 + " SORT doc.Hallo RETURN doc.Hallo";