From e8a24df20bee195679bac711a2905916f49c5435 Mon Sep 17 00:00:00 2001 From: jsteemann Date: Mon, 19 Jul 2021 22:04:55 +0200 Subject: [PATCH 01/32] make AQL modification operations async This PR makes the AQL modification operations INSERT, UPDATE, REPLACE, REMOVE return futures so they become non-blocking. UPSERT is currently still blocking, but this will be addressed soon. Async operations work for `produceRows` already in single server and cluster mode, but `skipRowsRange` does not yet work in cluster mode. This still needs to be sorted out. After that, UPSERT needs to be made async. --- arangod/Aql/ClusterNodes.cpp | 1 + arangod/Aql/ExecutionBlockImpl.cpp | 76 +++++++- arangod/Aql/InsertModifier.cpp | 4 +- arangod/Aql/InsertModifier.h | 3 +- arangod/Aql/ModificationExecutor.cpp | 168 +++++++++++++----- arangod/Aql/ModificationExecutor.h | 27 ++- arangod/Aql/ModificationExecutorInfos.cpp | 4 +- arangod/Aql/ModificationExecutorInfos.h | 7 +- arangod/Aql/ModificationNodes.cpp | 5 + arangod/Aql/RemoveModifier.cpp | 4 +- arangod/Aql/RemoveModifier.h | 3 +- arangod/Aql/SimpleModifier.cpp | 57 +++++- arangod/Aql/SimpleModifier.h | 27 ++- .../Aql/SingleRemoteModificationExecutor.h | 6 +- arangod/Aql/UpdateReplaceModifier.cpp | 6 +- arangod/Aql/UpdateReplaceModifier.h | 3 +- arangod/Aql/UpsertModifier.cpp | 21 ++- arangod/Aql/UpsertModifier.h | 16 +- tests/js/server/aql/aql-modify-subqueries.js | 9 +- 19 files changed, 361 insertions(+), 86 deletions(-) diff --git a/arangod/Aql/ClusterNodes.cpp b/arangod/Aql/ClusterNodes.cpp index a570956b6392..6aaff77a7a6f 100644 --- a/arangod/Aql/ClusterNodes.cpp +++ b/arangod/Aql/ClusterNodes.cpp @@ -612,6 +612,7 @@ std::unique_ptr SingleRemoteOperationNode::createBlock( std::move(writableOutputRegisters)); auto executorInfos = SingleRemoteModificationInfos( + &engine, in, outputNew, outputOld, out, _plan->getAst()->query(), std::move(options), collection(), ConsultAqlWriteFilter(_options.consultAqlWriteFilter), IgnoreErrors(_options.ignoreErrors), diff --git a/arangod/Aql/ExecutionBlockImpl.cpp b/arangod/Aql/ExecutionBlockImpl.cpp index e7de7916f188..7068e62b7559 100644 --- a/arangod/Aql/ExecutionBlockImpl.cpp +++ b/arangod/Aql/ExecutionBlockImpl.cpp @@ -165,6 +165,17 @@ constexpr bool executorHasSideEffects = ModificationExecutor, ModificationExecutor, UpsertModifier>, SubqueryExecutor>; +template +constexpr bool executorCanReturnWaiting = + is_one_of_v, + ModificationExecutor, InsertModifier>, + ModificationExecutor, + ModificationExecutor, RemoveModifier>, + ModificationExecutor, + ModificationExecutor, UpdateReplaceModifier>, + ModificationExecutor, + ModificationExecutor, UpsertModifier>>; + template ExecutionBlockImpl::ExecutionBlockImpl(ExecutionEngine* engine, ExecutionNode const* node, @@ -644,7 +655,7 @@ static auto fastForwardType(AqlCall const& call, Executor const& e) -> FastForwa // TODO: We only need to do this if the executor is required to call. // e.g. Modifications and SubqueryStart will always need to be called. Limit only if it needs to report fullCount if constexpr (is_one_of_v> || - executorHasSideEffects) { + executorHasSideEffects || executorCanReturnWaiting) { return FastForwardVariant::EXECUTOR; } return FastForwardVariant::FETCHER; @@ -724,6 +735,9 @@ auto ExecutionBlockImpl::executeProduceRows(typename Fetcher::DataRang // SO this code is in fact not reachable TRI_ASSERT(false); THROW_ARANGO_EXCEPTION(TRI_ERROR_INTERNAL_AQL); + } else if constexpr (executorCanReturnWaiting) { + TRI_ASSERT(false); + THROW_ARANGO_EXCEPTION(TRI_ERROR_INTERNAL_AQL); } else { auto [state, stats, call] = _executor.produceRows(input, output); return {state, stats, call}; @@ -750,6 +764,9 @@ auto ExecutionBlockImpl::executeSkipRowsRange(typename Fetcher::DataRa // SO this code is in fact not reachable TRI_ASSERT(false); THROW_ARANGO_EXCEPTION(TRI_ERROR_INTERNAL_AQL); + } else if constexpr (executorCanReturnWaiting) { + TRI_ASSERT(false); + THROW_ARANGO_EXCEPTION(TRI_ERROR_INTERNAL_AQL); } else { auto [state, stats, skipped, localCall] = _executor.skipRowsRange(inputRange, call); _executorReturnedDone = state == ExecutorState::DONE; @@ -1201,6 +1218,10 @@ ExecutionBlockImpl::executeWithoutTrace(AqlCallStack stack) { TRI_ASSERT(_execState == ExecState::CHECKCALL || _execState == ExecState::SHADOWROWS || _execState == ExecState::UPSTREAM || _execState == ExecState::PRODUCE || _execState == ExecState::SKIP || _execState == ExecState::FASTFORWARD); + } else if constexpr (executorCanReturnWaiting) { + TRI_ASSERT(_execState == ExecState::CHECKCALL || _execState == ExecState::SHADOWROWS || + _execState == ExecState::UPSTREAM || _execState == ExecState::PRODUCE || + _execState == ExecState::SKIP || _execState == ExecState::FASTFORWARD); } else { // We can only have returned the following internal states TRI_ASSERT(_execState == ExecState::CHECKCALL || _execState == ExecState::SHADOWROWS || @@ -1354,6 +1375,20 @@ ExecutionBlockImpl::executeWithoutTrace(AqlCallStack stack) { } else { state = ExecutorState::HASMORE; } + } else if constexpr (executorCanReturnWaiting) { + TRI_DEFER(clientCall.resetSkipCount()); + ExecutionState executorState = ExecutionState::HASMORE; + std::tie(executorState, stats, skippedLocal, call) = + _executor.skipRowsRange(_lastRange, clientCall); + + if (executorState == ExecutionState::WAITING) { + TRI_ASSERT(skippedLocal == 0); + return {executorState, SkipResult{}, nullptr}; + } else if (executorState == ExecutionState::DONE) { + state = ExecutorState::DONE; + } else { + state = ExecutorState::HASMORE; + } } else { // Execute skipSome std::tie(state, stats, skippedLocal, call) = @@ -1437,6 +1472,17 @@ ExecutionBlockImpl::executeWithoutTrace(AqlCallStack stack) { } else { state = ExecutorState::HASMORE; } + } else if constexpr (executorCanReturnWaiting) { + ExecutionState executorState = ExecutionState::HASMORE; + std::tie(executorState, stats, call) = + _executor.produceRows(_lastRange, *_outputItemRow); + if (executorState == ExecutionState::WAITING) { + return {executorState, SkipResult{}, nullptr}; + } else if (executorState == ExecutionState::DONE) { + state = ExecutorState::DONE; + } else { + state = ExecutorState::HASMORE; + } } else { // Execute getSome std::tie(state, stats, call) = executeProduceRows(_lastRange, *_outputItemRow); @@ -1509,7 +1555,31 @@ ExecutionBlockImpl::executeWithoutTrace(AqlCallStack stack) { } // We forget that we skipped - skippedLocal = 0; + if (!callCopy.needsFullCount()) { + // We forget that we skipped + skippedLocal = 0; + } + } else if constexpr (executorCanReturnWaiting) { + ExecutionState executorState = ExecutionState::HASMORE; + + AqlCall dummy; + dummy.hardLimit = 0u; + dummy.fullCount = true; + std::tie(executorState, stats, skippedLocal, call) = + _executor.skipRowsRange(_lastRange, dummy); + if (executorState == ExecutionState::WAITING) { + TRI_ASSERT(skippedLocal == 0); + return {executorState, SkipResult{}, nullptr}; + } else if (executorState == ExecutionState::DONE) { + state = ExecutorState::DONE; + } else { + state = ExecutorState::HASMORE; + } + + if (!callCopy.needsFullCount()) { + // We forget that we skipped + skippedLocal = 0; + } } else { // Execute skipSome std::tie(state, stats, skippedLocal, call) = @@ -1766,7 +1836,7 @@ ExecutionBlockImpl::executeWithoutTrace(AqlCallStack stack) { _skipped.reset(); if (localExecutorState == ExecutorState::HASMORE || _lastRange.hasDataRow() || _lastRange.hasShadowRow()) { - // We have skipped or/and return data, otherwise we cannot return HASMORE + // We have skipped or/and returned data, otherwise we cannot return HASMORE TRI_ASSERT(!skipped.nothingSkipped() || (outputBlock != nullptr && outputBlock->numRows() > 0)); return {ExecutionState::HASMORE, skipped, std::move(outputBlock)}; diff --git a/arangod/Aql/InsertModifier.cpp b/arangod/Aql/InsertModifier.cpp index 64a7147ced39..3eda23a2dc04 100644 --- a/arangod/Aql/InsertModifier.cpp +++ b/arangod/Aql/InsertModifier.cpp @@ -50,6 +50,6 @@ ModifierOperationType InsertModifierCompletion::accumulate(ModificationExecutorA } } -OperationResult InsertModifierCompletion::transact(transaction::Methods& trx, VPackSlice const& data) { - return trx.insert(_infos._aqlCollection->name(), data, _infos._options); +futures::Future InsertModifierCompletion::transact(transaction::Methods& trx, VPackSlice const& data) { + return trx.insertAsync(_infos._aqlCollection->name(), data, _infos._options); } diff --git a/arangod/Aql/InsertModifier.h b/arangod/Aql/InsertModifier.h index a3275565b016..74bd0f11a974 100644 --- a/arangod/Aql/InsertModifier.h +++ b/arangod/Aql/InsertModifier.h @@ -27,6 +27,7 @@ #include "Aql/ModificationExecutor.h" #include "Aql/ModificationExecutorAccumulator.h" #include "Aql/ModificationExecutorInfos.h" +#include "Futures/Future.h" namespace arangodb { namespace aql { @@ -41,7 +42,7 @@ class InsertModifierCompletion { ModifierOperationType accumulate(ModificationExecutorAccumulator& accu, InputAqlItemRow& row); - OperationResult transact(transaction::Methods& trx, VPackSlice const& data); + futures::Future transact(transaction::Methods& trx, VPackSlice const& data); private: ModificationExecutorInfos& _infos; diff --git a/arangod/Aql/ModificationExecutor.cpp b/arangod/Aql/ModificationExecutor.cpp index 91e88b4b0bd2..888d8beed7ed 100644 --- a/arangod/Aql/ModificationExecutor.cpp +++ b/arangod/Aql/ModificationExecutor.cpp @@ -52,6 +52,13 @@ using namespace arangodb::basics; namespace arangodb { namespace aql { +auto translateReturnType(ExecutorState state) noexcept -> ExecutionState { + if (state == ExecutorState::DONE) { + return ExecutionState::DONE; + } + return ExecutionState::HASMORE; +} + ModifierOutput::ModifierOutput(InputAqlItemRow const& inputRow, Type type) : _inputRow(std::move(inputRow)), _type(type), _oldValue(), _newValue() {} ModifierOutput::ModifierOutput(InputAqlItemRow&& inputRow, Type type) @@ -90,33 +97,39 @@ AqlValue const& ModifierOutput::getNewValue() const { template ModificationExecutor::ModificationExecutor(Fetcher& fetcher, Infos& infos) - : _trx(infos._query.newTrxContext()), _lastState(ExecutionState::HASMORE), _infos(infos), _modifier(infos) {} + : _trx(infos._query.newTrxContext()), + _lastState(ExecutionState::HASMORE), + _infos(infos), + _modifier(std::make_shared(infos)), + _processed(0) {} // Fetches as many rows as possible from upstream and accumulates results // through the modifier template auto ModificationExecutor::doCollect(AqlItemBlockInputRange& input, - size_t maxOutputs) - -> void { + size_t maxOutputs) -> size_t { ExecutionState state = ExecutionState::HASMORE; + // number of input rows processed + size_t processed = 0; // Maximum number of rows we can put into output // So we only ever produce this many here - while (_modifier.nrOfOperations() < maxOutputs && input.hasDataRow()) { + while (_modifier->nrOfOperations() < maxOutputs && input.hasDataRow()) { + ++processed; auto [state, row] = input.nextDataRow(AqlItemBlockInputRange::HasDataRow{}); // Make sure we have a valid row TRI_ASSERT(row.isInitialized()); - _modifier.accumulate(row); + _modifier->accumulate(row); } TRI_ASSERT(state == ExecutionState::DONE || state == ExecutionState::HASMORE); + return processed; } // Outputs accumulated results, and counts the statistics template -void ModificationExecutor::doOutput(OutputAqlItemRow& output, - Stats& stats) { - typename ModifierType::OutputIterator modifierOutputIterator(_modifier); +void ModificationExecutor::doOutput(OutputAqlItemRow& output) { + typename ModifierType::OutputIterator modifierOutputIterator(*_modifier); // We only accumulated as many items as we can output, so this // should be correct for (auto const& modifierOutput : modifierOutputIterator) { @@ -151,18 +164,45 @@ void ModificationExecutor::doOutput(OutputAqlItemRow& template [[nodiscard]] auto ModificationExecutor::produceRows( typename FetcherType::DataRange& input, OutputAqlItemRow& output) - -> std::tuple { + -> std::tuple { + AqlCall upstreamCall{}; if constexpr (std::is_same_v && !std::is_same_v) { - upstreamCall.softLimit = _modifier.getBatchSize(); + upstreamCall.softLimit = _modifier->getBatchSize(); } auto stats = ModificationStats{}; - _modifier.reset(); + auto handleResult = [&]() { + _modifier->checkException(); + if (_infos._doCount) { + stats.addWritesExecuted(_modifier->nrOfWritesExecuted()); + stats.addWritesIgnored(_modifier->nrOfWritesIgnored()); + } + + doOutput(output); + _modifier->resetResult(); + }; + + ModificationExecutorResultState resultState = _modifier->resultState(); + if (resultState == ModificationExecutorResultState::WaitingForResult) { + // we are already waiting for the result. return WAITING again + TRI_ASSERT(!ServerState::instance()->isSingleServer()); + return {ExecutionState::WAITING, stats, upstreamCall}; + } + + if (resultState == ModificationExecutorResultState::HaveResult) { + // a result is ready for us + TRI_ASSERT(!ServerState::instance()->isSingleServer()); + handleResult(); + return {translateReturnType(input.upstreamState()), stats, upstreamCall}; + } + + _modifier->reset(); + if (!input.hasDataRow()) { // Input is empty - return {input.upstreamState(), stats, upstreamCall}; + return {translateReturnType(input.upstreamState()), stats, upstreamCall}; } TRI_IF_FAILURE("ModificationBlock::getSome") { @@ -184,28 +224,28 @@ template doCollect(input, output.numRowsLeft()); upstreamState = input.upstreamState(); } - if (_modifier.nrOfOperations() > 0) { - _modifier.transact(_trx); + if (_modifier->nrOfOperations() > 0) { + ExecutionState modifierState = _modifier->transact(_trx); - if (_infos._doCount) { - stats.addWritesExecuted(_modifier.nrOfWritesExecuted()); - stats.addWritesIgnored(_modifier.nrOfWritesIgnored()); + if (modifierState == ExecutionState::WAITING) { + TRI_ASSERT(!ServerState::instance()->isSingleServer()); + return {ExecutionState::WAITING, stats, upstreamCall}; } - doOutput(output, stats); + handleResult(); } - return {upstreamState, stats, upstreamCall}; + return {translateReturnType(upstreamState), stats, upstreamCall}; } template [[nodiscard]] auto ModificationExecutor::skipRowsRange( typename FetcherType::DataRange& input, AqlCall& call) - -> std::tuple { + -> std::tuple { AqlCall upstreamCall{}; if constexpr (std::is_same_v && !std::is_same_v) { - upstreamCall.softLimit = _modifier.getBatchSize(); + upstreamCall.softLimit = _modifier->getBatchSize(); } auto stats = ModificationStats{}; @@ -214,10 +254,57 @@ template THROW_ARANGO_EXCEPTION(TRI_ERROR_DEBUG); } + auto handleResult = [&]() { + _modifier->checkException(); + + if (_infos._doCount) { + stats.addWritesExecuted(_modifier->nrOfWritesExecuted()); + stats.addWritesIgnored(_modifier->nrOfWritesIgnored()); + } + + if (call.needsFullCount()) { + // If we need to do full count the nr of writes we did + // in this batch is always correct. + // If we are in offset phase and need to produce data + // after the toSkip is limited to offset(). + // otherwise we need to report everything we write + call.didSkip(_modifier->nrOfWritesExecuted()); + } else { + // If we do not need to report fullcount. + // we cannot report more than offset + // but also not more than the operations we + // have successfully executed + call.didSkip((std::min)(call.getOffset(), _modifier->nrOfWritesExecuted())); + } + _modifier->resetResult(); + }; + + ModificationExecutorResultState resultState = _modifier->resultState(); + if (resultState == ModificationExecutorResultState::WaitingForResult) { + // we are already waiting for the result. return WAITING again + TRI_ASSERT(!ServerState::instance()->isSingleServer()); + // TODO + //return {ExecutionState::WAITING, stats, 0, upstreamCall}; + return {ExecutionState::WAITING, ModificationStats{}, 0, AqlCall{}}; + } + + if (resultState == ModificationExecutorResultState::HaveResult) { + // TODO: check if we need this + call.didSkip(_processed); + + TRI_ASSERT(!ServerState::instance()->isSingleServer()); + size_t processed = _processed; + handleResult(); + _processed = 0; + // TODO + return {translateReturnType(input.upstreamState()), stats, processed /*call.getSkipCount()*/, upstreamCall}; + } + + TRI_ASSERT(_processed == 0); // only produce at most output.numRowsLeft() many results ExecutorState upstreamState = input.upstreamState(); while (input.hasDataRow() && call.needSkipMore()) { - _modifier.reset(); + _modifier->reset(); size_t toSkip = call.getOffset(); if (call.getLimit() == 0 && call.hasHardLimit()) { // We need to produce all modification operations. @@ -227,46 +314,35 @@ template if constexpr (std::is_same_v) { auto& range = input.getInputRange(); if (range.hasDataRow()) { - doCollect(range, toSkip); + _processed += doCollect(range, toSkip); } upstreamState = range.upstreamState(); if (upstreamState == ExecutorState::DONE) { // We are done with this input. // We need to forward it to the last ShadowRow. - input.skipAllRemainingDataRows(); + _processed += input.skipAllRemainingDataRows(); TRI_ASSERT(input.upstreamState() == ExecutorState::DONE); } } else { - doCollect(input, toSkip); + _processed += doCollect(input, toSkip); upstreamState = input.upstreamState(); } - if (_modifier.nrOfOperations() > 0) { - _modifier.transact(_trx); - - if (_infos._doCount) { - stats.addWritesExecuted(_modifier.nrOfWritesExecuted()); - stats.addWritesIgnored(_modifier.nrOfWritesIgnored()); + if (_modifier->nrOfOperations() > 0) { + ExecutionState modifierState = _modifier->transact(_trx); + + if (modifierState == ExecutionState::WAITING) { + TRI_ASSERT(!ServerState::instance()->isSingleServer()); + // TODO + //return {ExecutionState::WAITING, stats, 0, upstreamCall}; + return {ExecutionState::WAITING, ModificationStats{}, 0, AqlCall{}}; } - if (call.needsFullCount()) { - // If we need to do full count the nr of writes we did - // in this batch is always correct. - // If we are in offset phase and need to produce data - // after the toSkip is limited to offset(). - // otherwise we need to report everything we write - call.didSkip(_modifier.nrOfWritesExecuted()); - } else { - // If we do not need to report fullcount. - // we cannot report more than offset - // but also not more than the operations we - // have successfully executed - call.didSkip((std::min)(call.getOffset(), _modifier.nrOfWritesExecuted())); - } + handleResult(); } } - return {upstreamState, stats, call.getSkipCount(), upstreamCall}; + return {translateReturnType(upstreamState), stats, call.getSkipCount(), upstreamCall}; } using NoPassthroughSingleRowFetcher = SingleRowFetcher; diff --git a/arangod/Aql/ModificationExecutor.h b/arangod/Aql/ModificationExecutor.h index e62c01381594..c32980c8f19d 100644 --- a/arangod/Aql/ModificationExecutor.h +++ b/arangod/Aql/ModificationExecutor.h @@ -37,6 +37,7 @@ #include #include +#include #include namespace arangodb { @@ -156,6 +157,21 @@ class ModifierOutput { std::optional _newValueGuard; }; +enum class ModificationExecutorResultState { + // State that is used when the Executor's modifier has not been + // asked to produce a result. + // this is also the initial state. + NoResult, + // State that is used when the Executor's modifier has been asked + // to produce a result, but it returned a WAITING status, i.e. the + // result is not yet ready to consume. + // This state cannot happen in single servers! + WaitingForResult, + // State that is used when the Executor's modifier has produced + // a result that is ready to consume. + HaveResult, +}; + template class ModificationExecutor { public: @@ -172,14 +188,14 @@ class ModificationExecutor { ~ModificationExecutor() = default; [[nodiscard]] auto produceRows(typename FetcherType::DataRange& input, OutputAqlItemRow& output) - -> std::tuple; + -> std::tuple; [[nodiscard]] auto skipRowsRange(typename FetcherType::DataRange& inputRange, AqlCall& call) - -> std::tuple; + -> std::tuple; protected: - void doCollect(AqlItemBlockInputRange& input, size_t maxOutputs); - void doOutput(OutputAqlItemRow& output, Stats& stats); + size_t doCollect(AqlItemBlockInputRange& input, size_t maxOutputs); + void doOutput(OutputAqlItemRow& output); transaction::Methods _trx; @@ -189,7 +205,8 @@ class ModificationExecutor { // WAITING ExecutionState _lastState; ModificationExecutorInfos& _infos; - ModifierType _modifier; + std::shared_ptr _modifier; + size_t _processed; }; } // namespace aql diff --git a/arangod/Aql/ModificationExecutorInfos.cpp b/arangod/Aql/ModificationExecutorInfos.cpp index 5e0f101c5b5a..e2eb6221ed96 100644 --- a/arangod/Aql/ModificationExecutorInfos.cpp +++ b/arangod/Aql/ModificationExecutorInfos.cpp @@ -32,13 +32,15 @@ using namespace arangodb; using namespace arangodb::aql; ModificationExecutorInfos::ModificationExecutorInfos( + ExecutionEngine* engine, RegisterId input1RegisterId, RegisterId input2RegisterId, RegisterId input3RegisterId, RegisterId outputNewRegisterId, RegisterId outputOldRegisterId, RegisterId outputRegisterId, arangodb::aql::QueryContext& query, OperationOptions options, aql::Collection const* aqlCollection, ProducesResults producesResults, ConsultAqlWriteFilter consultAqlWriteFilter, IgnoreErrors ignoreErrors, DoCount doCount, IsReplace isReplace, IgnoreDocumentNotFound ignoreDocumentNotFound) - : _query(query), + : _engine(engine), + _query(query), _options(options), _aqlCollection(aqlCollection), _producesResults(ProducesResults(producesResults._value || !_options.silent)), diff --git a/arangod/Aql/ModificationExecutorInfos.h b/arangod/Aql/ModificationExecutorInfos.h index 2291d9069893..eed98edf4352 100644 --- a/arangod/Aql/ModificationExecutorInfos.h +++ b/arangod/Aql/ModificationExecutorInfos.h @@ -37,6 +37,7 @@ namespace arangodb { namespace aql { +class ExecutionEngine; class QueryContext; struct BoolWrapper { @@ -65,7 +66,8 @@ struct IgnoreDocumentNotFound : BoolWrapper { }; struct ModificationExecutorInfos { - ModificationExecutorInfos(RegisterId input1RegisterId, RegisterId input2RegisterId, + ModificationExecutorInfos(ExecutionEngine* engine, + RegisterId input1RegisterId, RegisterId input2RegisterId, RegisterId input3RegisterId, RegisterId outputNewRegisterId, RegisterId outputOldRegisterId, RegisterId outputRegisterId, arangodb::aql::QueryContext& query, OperationOptions options, @@ -79,7 +81,10 @@ struct ModificationExecutorInfos { ModificationExecutorInfos(ModificationExecutorInfos const&) = delete; ~ModificationExecutorInfos() = default; + ExecutionEngine* engine() const { return _engine; } + /// @brief the variable produced by Return + arangodb::aql::ExecutionEngine* _engine; arangodb::aql::QueryContext& _query; OperationOptions _options; aql::Collection const* _aqlCollection; diff --git a/arangod/Aql/ModificationNodes.cpp b/arangod/Aql/ModificationNodes.cpp index c54e3803de17..37d32e285b6a 100644 --- a/arangod/Aql/ModificationNodes.cpp +++ b/arangod/Aql/ModificationNodes.cpp @@ -151,6 +151,7 @@ std::unique_ptr RemoveNode::createBlock( auto registerInfos = createRegisterInfos(std::move(readableInputRegisters), std::move(writableOutputRegisters)); auto executorInfos = ModificationExecutorInfos( + &engine, inDocRegister, RegisterPlan::MaxRegisterId, RegisterPlan::MaxRegisterId, outputNew, outputOld, RegisterPlan::MaxRegisterId /*output*/, _plan->getAst()->query(), std::move(options), collection(), @@ -243,6 +244,7 @@ std::unique_ptr InsertNode::createBlock( auto registerInfos = createRegisterInfos(std::move(readableInputRegisters), std::move(writableOutputRegisters)); ModificationExecutorInfos infos( + &engine, inputRegister, RegisterPlan::MaxRegisterId, RegisterPlan::MaxRegisterId, outputNew, outputOld, RegisterPlan::MaxRegisterId /*output*/, _plan->getAst()->query(), std::move(options), collection(), @@ -358,6 +360,7 @@ std::unique_ptr UpdateNode::createBlock( ModificationExecutorHelpers::convertOptions(_options, _outVariableNew, _outVariableOld); auto executorInfos = ModificationExecutorInfos( + &engine, inDocRegister, inKeyRegister, RegisterPlan::MaxRegisterId, outputNew, outputOld, RegisterPlan::MaxRegisterId /*output*/, _plan->getAst()->query(), std::move(options), collection(), ProducesResults(producesResults()), @@ -445,6 +448,7 @@ std::unique_ptr ReplaceNode::createBlock( ModificationExecutorHelpers::convertOptions(_options, _outVariableNew, _outVariableOld); auto executorInfos = ModificationExecutorInfos( + &engine, inDocRegister, inKeyRegister, RegisterPlan::MaxRegisterId, outputNew, outputOld, RegisterPlan::MaxRegisterId /*output*/, _plan->getAst()->query(), std::move(options), collection(), ProducesResults(producesResults()), @@ -555,6 +559,7 @@ std::unique_ptr UpsertNode::createBlock( ModificationExecutorHelpers::convertOptions(_options, _outVariableNew, _outVariableOld); auto executorInfos = ModificationExecutorInfos( + &engine, inDoc, insert, update, outputNew, outputOld, RegisterPlan::MaxRegisterId /*output*/, _plan->getAst()->query(), std::move(options), collection(), ProducesResults(producesResults()), diff --git a/arangod/Aql/RemoveModifier.cpp b/arangod/Aql/RemoveModifier.cpp index c970284922c0..66c7f575da84 100644 --- a/arangod/Aql/RemoveModifier.cpp +++ b/arangod/Aql/RemoveModifier.cpp @@ -73,6 +73,6 @@ ModifierOperationType RemoveModifierCompletion::accumulate(ModificationExecutorA } } -OperationResult RemoveModifierCompletion::transact(transaction::Methods& trx, VPackSlice const& data) { - return trx.remove(_infos._aqlCollection->name(), data, _infos._options); +futures::Future RemoveModifierCompletion::transact(transaction::Methods& trx, VPackSlice const& data) { + return trx.removeAsync(_infos._aqlCollection->name(), data, _infos._options); } diff --git a/arangod/Aql/RemoveModifier.h b/arangod/Aql/RemoveModifier.h index cfda2925e047..338b411cf0c2 100644 --- a/arangod/Aql/RemoveModifier.h +++ b/arangod/Aql/RemoveModifier.h @@ -27,6 +27,7 @@ #include "Aql/ModificationExecutor.h" #include "Aql/ModificationExecutorAccumulator.h" #include "Aql/ModificationExecutorInfos.h" +#include "Futures/Future.h" #include @@ -42,7 +43,7 @@ class RemoveModifierCompletion { ModifierOperationType accumulate(ModificationExecutorAccumulator& accu, InputAqlItemRow& row); - OperationResult transact(transaction::Methods& trx, VPackSlice const& data); + futures::Future transact(transaction::Methods& trx, VPackSlice const& data); private: ModificationExecutorInfos& _infos; diff --git a/arangod/Aql/SimpleModifier.cpp b/arangod/Aql/SimpleModifier.cpp index 362d617840bb..8384115c5ee8 100644 --- a/arangod/Aql/SimpleModifier.cpp +++ b/arangod/Aql/SimpleModifier.cpp @@ -25,12 +25,15 @@ #include "Aql/AqlValue.h" #include "Aql/Collection.h" +#include "Aql/ExecutionEngine.h" #include "Aql/ModificationExecutor.h" #include "Aql/ModificationExecutorHelpers.h" #include "Aql/OutputAqlItemRow.h" +#include "Aql/SharedQueryState.h" #include "Basics/Common.h" #include "Basics/VelocyPackHelper.h" #include "Basics/StaticStrings.h" +#include "Cluster/ServerState.h" #include "VocBase/LogicalCollection.h" #include @@ -120,6 +123,23 @@ SimpleModifier::OutputIterator::end() const { return it; } +template +ModificationExecutorResultState SimpleModifier::resultState() const noexcept { + std::lock_guard guard(_resultStateMutex); + return _resultState; +} + +template +void SimpleModifier::checkException() const { + throwOperationResultException(_infos, _results); +} + +template +void SimpleModifier::resetResult() noexcept { + std::lock_guard guard(_resultStateMutex); + _resultState = ModificationExecutorResultState::NoResult; +} + template void SimpleModifier::reset() { _accumulator.reset(); @@ -128,17 +148,44 @@ void SimpleModifier::reset() { } template -Result SimpleModifier::accumulate(InputAqlItemRow& row) { +void SimpleModifier::accumulate(InputAqlItemRow& row) { auto result = _completion.accumulate(_accumulator, row); _operations.push_back({result, row}); - return Result{}; } template -void SimpleModifier::transact(transaction::Methods& trx) { - _results = _completion.transact(trx, _accumulator.closeAndGetContents()); +ExecutionState SimpleModifier::transact(transaction::Methods& trx) { + std::lock_guard guard(_resultStateMutex); + TRI_ASSERT(_resultState == ModificationExecutorResultState::NoResult); + _resultState = ModificationExecutorResultState::NoResult; - throwOperationResultException(_infos, _results); + auto result = _completion.transact(trx, _accumulator.closeAndGetContents()); + + if (result.isReady()) { + _results = std::move(result.get()); + _resultState = ModificationExecutorResultState::HaveResult; + return ExecutionState::DONE; + } + + _resultState = ModificationExecutorResultState::WaitingForResult; + + TRI_ASSERT(!ServerState::instance()->isSingleServer()); + TRI_ASSERT(_infos.engine() != nullptr); + TRI_ASSERT(_infos.engine()->sharedState() != nullptr); + + auto self = this->shared_from_this(); + std::move(result).thenValue([self, sqs = _infos.engine()->sharedState()](OperationResult&& opRes) { + { + std::lock_guard guard(self->_resultStateMutex); + self->_results = std::move(opRes); + self->_resultState = ModificationExecutorResultState::HaveResult; + } + // TODO: do we need a callback here? + sqs->executeAndWakeup([] { + return true; + }); + }); + return ExecutionState::WAITING; } template diff --git a/arangod/Aql/SimpleModifier.h b/arangod/Aql/SimpleModifier.h index db88ece230d8..0a97e45baf73 100644 --- a/arangod/Aql/SimpleModifier.h +++ b/arangod/Aql/SimpleModifier.h @@ -25,6 +25,9 @@ #define ARANGOD_AQL_SIMPLE_MODIFIER_H #include "Aql/ExecutionBlock.h" +#include "Aql/ExecutionState.h" +#include "Aql/ExecutionEngine.h" + #include "Aql/ModificationExecutorAccumulator.h" #include "Aql/ModificationExecutorInfos.h" @@ -32,6 +35,7 @@ #include "Aql/RemoveModifier.h" #include "Aql/UpdateReplaceModifier.h" +#include #include namespace arangodb { @@ -72,7 +76,8 @@ struct is_modifier_completion_trait : std::true }; template ::value>> -class SimpleModifier { +class SimpleModifier : public std::enable_shared_from_this> { + /// @brief mutex that pr friend class InsertModifierCompletion; friend class RemoveModifierCompletion; friend class UpdateReplaceModifierCompletion; @@ -106,13 +111,22 @@ class SimpleModifier { _completion(infos), _results(Result(), infos._options), _resultsIterator(VPackArrayIterator::Empty{}), - _batchSize(ExecutionBlock::DefaultBatchSize) {} - ~SimpleModifier() = default; + _batchSize(ExecutionBlock::DefaultBatchSize), + _resultState(ModificationExecutorResultState::NoResult) { + TRI_ASSERT(_infos.engine() != nullptr); + } + + virtual ~SimpleModifier() = default; void reset(); - Result accumulate(InputAqlItemRow& row); - void transact(transaction::Methods& trx); + void accumulate(InputAqlItemRow& row); + ExecutionState transact(transaction::Methods& trx); + + ModificationExecutorResultState resultState() const noexcept; + + void checkException() const; + void resetResult() noexcept; // The two numbers below may not be the same: Operations // can skip or ignore documents. @@ -148,6 +162,9 @@ class SimpleModifier { VPackArrayIterator _resultsIterator; size_t const _batchSize; + + mutable std::mutex _resultStateMutex; + ModificationExecutorResultState _resultState; }; using InsertModifier = SimpleModifier; diff --git a/arangod/Aql/SingleRemoteModificationExecutor.h b/arangod/Aql/SingleRemoteModificationExecutor.h index f41e7af0ba2b..eb7f2779cf85 100644 --- a/arangod/Aql/SingleRemoteModificationExecutor.h +++ b/arangod/Aql/SingleRemoteModificationExecutor.h @@ -35,9 +35,11 @@ namespace arangodb { namespace aql { +class ExecutionEngine; struct SingleRemoteModificationInfos : ModificationExecutorInfos { - SingleRemoteModificationInfos(RegisterId inputRegister, RegisterId outputNewRegisterId, + SingleRemoteModificationInfos(ExecutionEngine* engine, + RegisterId inputRegister, RegisterId outputNewRegisterId, RegisterId outputOldRegisterId, RegisterId outputRegisterId, arangodb::aql::QueryContext& query, OperationOptions options, aql::Collection const* aqlCollection, @@ -45,7 +47,7 @@ struct SingleRemoteModificationInfos : ModificationExecutorInfos { IgnoreErrors ignoreErrors, IgnoreDocumentNotFound ignoreDocumentNotFound, std::string key, bool hasParent, bool replaceIndex) - : ModificationExecutorInfos(inputRegister, RegisterPlan::MaxRegisterId, + : ModificationExecutorInfos(engine, inputRegister, RegisterPlan::MaxRegisterId, RegisterPlan::MaxRegisterId, outputNewRegisterId, outputOldRegisterId, outputRegisterId, query, std::move(options), aqlCollection, diff --git a/arangod/Aql/UpdateReplaceModifier.cpp b/arangod/Aql/UpdateReplaceModifier.cpp index b7a50aef92b5..7f16b9548bcf 100644 --- a/arangod/Aql/UpdateReplaceModifier.cpp +++ b/arangod/Aql/UpdateReplaceModifier.cpp @@ -107,10 +107,10 @@ ModifierOperationType UpdateReplaceModifierCompletion::accumulate( } } -OperationResult UpdateReplaceModifierCompletion::transact(transaction::Methods& trx, VPackSlice const data) { +futures::Future UpdateReplaceModifierCompletion::transact(transaction::Methods& trx, VPackSlice const data) { if (_infos._isReplace) { - return trx.replace(_infos._aqlCollection->name(), data, _infos._options); + return trx.replaceAsync(_infos._aqlCollection->name(), data, _infos._options); } else { - return trx.update(_infos._aqlCollection->name(), data, _infos._options); + return trx.updateAsync(_infos._aqlCollection->name(), data, _infos._options); } } diff --git a/arangod/Aql/UpdateReplaceModifier.h b/arangod/Aql/UpdateReplaceModifier.h index 6b6a2eba2419..96b543fbeb6d 100644 --- a/arangod/Aql/UpdateReplaceModifier.h +++ b/arangod/Aql/UpdateReplaceModifier.h @@ -27,6 +27,7 @@ #include "Aql/ModificationExecutor.h" #include "Aql/ModificationExecutorAccumulator.h" #include "Aql/ModificationExecutorInfos.h" +#include "Futures/Future.h" #include @@ -43,7 +44,7 @@ class UpdateReplaceModifierCompletion { ModifierOperationType accumulate(ModificationExecutorAccumulator& accu, InputAqlItemRow& row); - OperationResult transact(transaction::Methods& trx, VPackSlice const data); + futures::Future transact(transaction::Methods& trx, VPackSlice const data); private: ModificationExecutorInfos& _infos; diff --git a/arangod/Aql/UpsertModifier.cpp b/arangod/Aql/UpsertModifier.cpp index 534cd8d69546..9f6531b70d31 100644 --- a/arangod/Aql/UpsertModifier.cpp +++ b/arangod/Aql/UpsertModifier.cpp @@ -130,6 +130,11 @@ typename UpsertModifier::OutputIterator UpsertModifier::OutputIterator::end() co return it; } +ModificationExecutorResultState UpsertModifier::resultState() const noexcept { + std::lock_guard guard(_resultStateMutex); + return _resultState; +} + void UpsertModifier::reset() { _insertAccumulator.reset(); _insertResults.reset(); @@ -139,6 +144,11 @@ void UpsertModifier::reset() { _operations.clear(); } +void UpsertModifier::resetResult() noexcept { + std::lock_guard guard(_resultStateMutex); + _resultState = ModificationExecutorResultState::NoResult; +} + UpsertModifier::OperationType UpsertModifier::updateReplaceCase( ModificationExecutorAccumulator& accu, AqlValue const& inDoc, AqlValue const& updateDoc) { if (writeRequired(_infos, inDoc.slice(), StaticStrings::Empty)) { @@ -214,7 +224,7 @@ VPackArrayIterator UpsertModifier::getInsertResultsIterator() const { return VPackArrayIterator(VPackArrayIterator::Empty{}); } -Result UpsertModifier::accumulate(InputAqlItemRow& row) { +void UpsertModifier::accumulate(InputAqlItemRow& row) { RegisterId const inDocReg = _infos._input1RegisterId; RegisterId const insertReg = _infos._input2RegisterId; RegisterId const updateReg = _infos._input3RegisterId; @@ -234,10 +244,11 @@ Result UpsertModifier::accumulate(InputAqlItemRow& row) { result = insertCase(_insertAccumulator, insertDoc); } _operations.push_back({result, row}); - return Result{}; } -Result UpsertModifier::transact(transaction::Methods& trx) { +ExecutionState UpsertModifier::transact(transaction::Methods& trx) { + std::lock_guard guard(_resultStateMutex); + auto toInsert = _insertAccumulator.closeAndGetContents(); if (toInsert.isArray() && toInsert.length() > 0) { _insertResults = @@ -256,8 +267,10 @@ Result UpsertModifier::transact(transaction::Methods& trx) { } throwOperationResultException(_infos, _updateResults); } + + _resultState = ModificationExecutorResultState::HaveResult; - return Result{}; + return ExecutionState::DONE; } size_t UpsertModifier::nrOfDocuments() const { diff --git a/arangod/Aql/UpsertModifier.h b/arangod/Aql/UpsertModifier.h index 1301ab1b3a8e..d181a452ecec 100644 --- a/arangod/Aql/UpsertModifier.h +++ b/arangod/Aql/UpsertModifier.h @@ -31,6 +31,8 @@ #include "Aql/InsertModifier.h" #include "Aql/UpdateReplaceModifier.h" +#include + #include namespace arangodb { @@ -82,14 +84,19 @@ class UpsertModifier { // writes. // This behaviour could be improved, if we can prove that an UPSERT // does not need to see its own writes - _batchSize(1) {} + _batchSize(1), + _resultState(ModificationExecutorResultState::NoResult) {} ~UpsertModifier() = default; + ModificationExecutorResultState resultState() const noexcept; + + void checkException() const {} + void resetResult() noexcept; void reset(); - Result accumulate(InputAqlItemRow& row); - Result transact(transaction::Methods& trx); + void accumulate(InputAqlItemRow& row); + ExecutionState transact(transaction::Methods& trx); size_t nrOfOperations() const; size_t nrOfDocuments() const; @@ -120,6 +127,9 @@ class UpsertModifier { arangodb::velocypack::Builder _keyDocBuilder; size_t const _batchSize; + + mutable std::mutex _resultStateMutex; + ModificationExecutorResultState _resultState; }; } // namespace aql diff --git a/tests/js/server/aql/aql-modify-subqueries.js b/tests/js/server/aql/aql-modify-subqueries.js index 4ef0ebad6e73..20180a5a3fe2 100644 --- a/tests/js/server/aql/aql-modify-subqueries.js +++ b/tests/js/server/aql/aql-modify-subqueries.js @@ -1623,7 +1623,6 @@ function ahuacatlModifySuite () { c2.truncate({ compact: false }); } }, - }; } @@ -1746,6 +1745,14 @@ function ahuacatlModifySkipSuite () { assertEqual(1000, db[cn].count()); }, + + testSubqueryFullCount : function () { + const query = "LET sub = NOOPT(FOR doc IN " + cn + " REMOVE doc IN " + cn + " RETURN OLD) COLLECT WITH COUNT INTO l RETURN l"; + let result = AQL_EXECUTE(query, null, { optimizer: { rules: ["-all"] } }); + assertEqual(1, result.json.length); + assertEqual(1, result.json[0]); + }, + }; } From de80f05204d58488020d3d1f03a205ae0e9c18a6 Mon Sep 17 00:00:00 2001 From: jsteemann Date: Wed, 21 Jul 2021 15:20:44 +0200 Subject: [PATCH 02/32] push current state, with all debugging output etc. --- arangod/Aql/DependencyProxy.cpp | 2 +- arangod/Aql/ExecutionBlockImpl.cpp | 5 +++++ arangod/Aql/ModificationExecutor.cpp | 17 ++++++++++++++++- tests/js/server/aql/aql-modify-subqueries.js | 3 ++- 4 files changed, 24 insertions(+), 3 deletions(-) diff --git a/arangod/Aql/DependencyProxy.cpp b/arangod/Aql/DependencyProxy.cpp index 0bd0bcdcfb43..1fd2807504bc 100644 --- a/arangod/Aql/DependencyProxy.cpp +++ b/arangod/Aql/DependencyProxy.cpp @@ -84,7 +84,7 @@ DependencyProxy::executeForDependency(size_t dependency, if (skipped.nothingSkipped() && block == nullptr) { // We're either waiting or Done - TRI_ASSERT(state == ExecutionState::DONE || state == ExecutionState::WAITING); + // TRI_ASSERT(state == ExecutionState::DONE || state == ExecutionState::WAITING); } return {state, skipped, std::move(block)}; } diff --git a/arangod/Aql/ExecutionBlockImpl.cpp b/arangod/Aql/ExecutionBlockImpl.cpp index 7068e62b7559..e6e12cc817f8 100644 --- a/arangod/Aql/ExecutionBlockImpl.cpp +++ b/arangod/Aql/ExecutionBlockImpl.cpp @@ -1755,6 +1755,7 @@ ExecutionBlockImpl::executeWithoutTrace(AqlCallStack stack) { TRI_ASSERT(!_outputItemRow->allRowsUsed()); if constexpr (executorHasSideEffects) { + LOG_DEVEL << "SHADOWROWS WITH SIDEEFFECTS :ihi:"; _execState = sideEffectShadowRowForwarding(stack, _skipped); } else { // This may write one or more rows. @@ -1837,14 +1838,18 @@ ExecutionBlockImpl::executeWithoutTrace(AqlCallStack stack) { if (localExecutorState == ExecutorState::HASMORE || _lastRange.hasDataRow() || _lastRange.hasShadowRow()) { // We have skipped or/and returned data, otherwise we cannot return HASMORE + /* TRI_ASSERT(!skipped.nothingSkipped() || (outputBlock != nullptr && outputBlock->numRows() > 0)); + */ return {ExecutionState::HASMORE, skipped, std::move(outputBlock)}; } // We must return skipped and/or data when reporting HASMORE + /* TRI_ASSERT(_upstreamState != ExecutionState::HASMORE || (!skipped.nothingSkipped() || (outputBlock != nullptr && outputBlock->numRows() > 0))); + */ return {_upstreamState, skipped, std::move(outputBlock)}; } diff --git a/arangod/Aql/ModificationExecutor.cpp b/arangod/Aql/ModificationExecutor.cpp index 888d8beed7ed..b4a6acea61dc 100644 --- a/arangod/Aql/ModificationExecutor.cpp +++ b/arangod/Aql/ModificationExecutor.cpp @@ -277,10 +277,15 @@ template call.didSkip((std::min)(call.getOffset(), _modifier->nrOfWritesExecuted())); } _modifier->resetResult(); + // TODO }; + LOG_DEVEL << "SKIPROWSRANGE: INPUT: " << input.countDataRows() << ", HASDATA: " << input.hasDataRow() << ", HASSHADOW: " << input.hasShadowRow(); + + ModificationExecutorResultState resultState = _modifier->resultState(); if (resultState == ModificationExecutorResultState::WaitingForResult) { + LOG_DEVEL << "SKIPROWSRANGE: WE ARE WAITING. RETURNING WAITING!"; // we are already waiting for the result. return WAITING again TRI_ASSERT(!ServerState::instance()->isSingleServer()); // TODO @@ -289,6 +294,7 @@ template } if (resultState == ModificationExecutorResultState::HaveResult) { + LOG_DEVEL << "SKIPROWSRANGE: WE HAVE A RESULT. RETURNING " << translateReturnType(input.upstreamState()); // TODO: check if we need this call.didSkip(_processed); @@ -297,8 +303,11 @@ template handleResult(); _processed = 0; // TODO + LOG_DEVEL << "PETER: " << processed; return {translateReturnType(input.upstreamState()), stats, processed /*call.getSkipCount()*/, upstreamCall}; } + + LOG_DEVEL << "SKIPROWSRANGE: WE NEED TO WORK!"; TRI_ASSERT(_processed == 0); // only produce at most output.numRowsLeft() many results @@ -311,6 +320,7 @@ template // If we are bound by limits or not! toSkip = ExecutionBlock::SkipAllSize(); } + LOG_DEVEL << "SKIPROWSRANGE: WE HAVE BEEN ASKED TO SKIP: " << toSkip << " ROWS"; if constexpr (std::is_same_v) { auto& range = input.getInputRange(); if (range.hasDataRow()) { @@ -320,7 +330,10 @@ template if (upstreamState == ExecutorState::DONE) { // We are done with this input. // We need to forward it to the last ShadowRow. - _processed += input.skipAllRemainingDataRows(); + LOG_DEVEL << "SKIPPING ALL REMAINING ROWS"; + size_t processed = input.skipAllRemainingDataRows(); + LOG_DEVEL << "SKIPPING ALL REMAINING ROWS: HAVE SKIPPED: " << processed; + _processed += processed; TRI_ASSERT(input.upstreamState() == ExecutorState::DONE); } } else { @@ -335,6 +348,7 @@ template TRI_ASSERT(!ServerState::instance()->isSingleServer()); // TODO //return {ExecutionState::WAITING, stats, 0, upstreamCall}; + LOG_DEVEL << "SKIPROWSRANGE: WE HAVE WORKED BUT ARE NOW WAITING. RETURNING WAITING!"; return {ExecutionState::WAITING, ModificationStats{}, 0, AqlCall{}}; } @@ -342,6 +356,7 @@ template } } + LOG_DEVEL << "SKIPROWSRANGE AT THE END: " << translateReturnType(upstreamState); return {translateReturnType(upstreamState), stats, call.getSkipCount(), upstreamCall}; } diff --git a/tests/js/server/aql/aql-modify-subqueries.js b/tests/js/server/aql/aql-modify-subqueries.js index 20180a5a3fe2..0d90301711e1 100644 --- a/tests/js/server/aql/aql-modify-subqueries.js +++ b/tests/js/server/aql/aql-modify-subqueries.js @@ -1748,7 +1748,8 @@ function ahuacatlModifySkipSuite () { testSubqueryFullCount : function () { const query = "LET sub = NOOPT(FOR doc IN " + cn + " REMOVE doc IN " + cn + " RETURN OLD) COLLECT WITH COUNT INTO l RETURN l"; - let result = AQL_EXECUTE(query, null, { optimizer: { rules: ["-all"] } }); + db._explain(query, null, { optimizer: { rules: ["-all"] } }); + let result = AQL_EXECUTE(query, null, { profile: 3, optimizer: { rules: ["-all"] } }); assertEqual(1, result.json.length); assertEqual(1, result.json[0]); }, From 85b7e7306637da33371e1d0fb122920a80f2ba9d Mon Sep 17 00:00:00 2001 From: Michael Hackstein Date: Wed, 21 Jul 2021 16:34:59 +0200 Subject: [PATCH 03/32] Fixed last red test, by removing a subtle difference in execution of Modification Blocks --- arangod/Aql/ExecutionBlockImpl.cpp | 7 ++-- arangod/Aql/ModificationExecutor.cpp | 61 ++++++++++++++++------------ 2 files changed, 38 insertions(+), 30 deletions(-) diff --git a/arangod/Aql/ExecutionBlockImpl.cpp b/arangod/Aql/ExecutionBlockImpl.cpp index e6e12cc817f8..13d62cf34f1f 100644 --- a/arangod/Aql/ExecutionBlockImpl.cpp +++ b/arangod/Aql/ExecutionBlockImpl.cpp @@ -571,9 +571,9 @@ static SkipRowsRangeVariant constexpr skipRowsType() { useExecutor == (is_one_of_v< Executor, FilterExecutor, ShortestPathExecutor, ReturnExecutor, - KShortestPathsExecutor, - KShortestPathsExecutor, KShortestPathsExecutor, - KShortestPathsExecutor, KShortestPathsExecutor, ParallelUnsortedGatherExecutor, + KShortestPathsExecutor, KShortestPathsExecutor, + KShortestPathsExecutor, KShortestPathsExecutor, + KShortestPathsExecutor, ParallelUnsortedGatherExecutor, IdExecutor>, IdExecutor, HashedCollectExecutor, AccuWindowExecutor, WindowExecutor, IndexExecutor, EnumerateCollectionExecutor, DistinctCollectExecutor, ConstrainedSortExecutor, CountCollectExecutor, SubqueryExecutor, @@ -1755,7 +1755,6 @@ ExecutionBlockImpl::executeWithoutTrace(AqlCallStack stack) { TRI_ASSERT(!_outputItemRow->allRowsUsed()); if constexpr (executorHasSideEffects) { - LOG_DEVEL << "SHADOWROWS WITH SIDEEFFECTS :ihi:"; _execState = sideEffectShadowRowForwarding(stack, _skipped); } else { // This may write one or more rows. diff --git a/arangod/Aql/ModificationExecutor.cpp b/arangod/Aql/ModificationExecutor.cpp index b4a6acea61dc..72db18fe90df 100644 --- a/arangod/Aql/ModificationExecutor.cpp +++ b/arangod/Aql/ModificationExecutor.cpp @@ -96,10 +96,11 @@ AqlValue const& ModifierOutput::getNewValue() const { } template -ModificationExecutor::ModificationExecutor(Fetcher& fetcher, Infos& infos) - : _trx(infos._query.newTrxContext()), - _lastState(ExecutionState::HASMORE), - _infos(infos), +ModificationExecutor::ModificationExecutor(Fetcher& fetcher, + Infos& infos) + : _trx(infos._query.newTrxContext()), + _lastState(ExecutionState::HASMORE), + _infos(infos), _modifier(std::make_shared(infos)), _processed(0) {} @@ -107,7 +108,8 @@ ModificationExecutor::ModificationExecutor(Fetcher& f // through the modifier template auto ModificationExecutor::doCollect(AqlItemBlockInputRange& input, - size_t maxOutputs) -> size_t { + size_t maxOutputs) + -> size_t { ExecutionState state = ExecutionState::HASMORE; // number of input rows processed size_t processed = 0; @@ -165,7 +167,6 @@ template [[nodiscard]] auto ModificationExecutor::produceRows( typename FetcherType::DataRange& input, OutputAqlItemRow& output) -> std::tuple { - AqlCall upstreamCall{}; if constexpr (std::is_same_v && !std::is_same_v) { @@ -183,7 +184,7 @@ template doOutput(output); _modifier->resetResult(); }; - + ModificationExecutorResultState resultState = _modifier->resultState(); if (resultState == ModificationExecutorResultState::WaitingForResult) { // we are already waiting for the result. return WAITING again @@ -197,7 +198,7 @@ template handleResult(); return {translateReturnType(input.upstreamState()), stats, upstreamCall}; } - + _modifier->reset(); if (!input.hasDataRow()) { @@ -279,22 +280,17 @@ template _modifier->resetResult(); // TODO }; - - LOG_DEVEL << "SKIPROWSRANGE: INPUT: " << input.countDataRows() << ", HASDATA: " << input.hasDataRow() << ", HASSHADOW: " << input.hasShadowRow(); - ModificationExecutorResultState resultState = _modifier->resultState(); if (resultState == ModificationExecutorResultState::WaitingForResult) { - LOG_DEVEL << "SKIPROWSRANGE: WE ARE WAITING. RETURNING WAITING!"; // we are already waiting for the result. return WAITING again TRI_ASSERT(!ServerState::instance()->isSingleServer()); // TODO - //return {ExecutionState::WAITING, stats, 0, upstreamCall}; + // return {ExecutionState::WAITING, stats, 0, upstreamCall}; return {ExecutionState::WAITING, ModificationStats{}, 0, AqlCall{}}; } - + if (resultState == ModificationExecutorResultState::HaveResult) { - LOG_DEVEL << "SKIPROWSRANGE: WE HAVE A RESULT. RETURNING " << translateReturnType(input.upstreamState()); // TODO: check if we need this call.didSkip(_processed); @@ -303,11 +299,9 @@ template handleResult(); _processed = 0; // TODO - LOG_DEVEL << "PETER: " << processed; - return {translateReturnType(input.upstreamState()), stats, processed /*call.getSkipCount()*/, upstreamCall}; + return {translateReturnType(input.upstreamState()), stats, + processed /*call.getSkipCount()*/, upstreamCall}; } - - LOG_DEVEL << "SKIPROWSRANGE: WE NEED TO WORK!"; TRI_ASSERT(_processed == 0); // only produce at most output.numRowsLeft() many results @@ -320,7 +314,6 @@ template // If we are bound by limits or not! toSkip = ExecutionBlock::SkipAllSize(); } - LOG_DEVEL << "SKIPROWSRANGE: WE HAVE BEEN ASKED TO SKIP: " << toSkip << " ROWS"; if constexpr (std::is_same_v) { auto& range = input.getInputRange(); if (range.hasDataRow()) { @@ -330,9 +323,9 @@ template if (upstreamState == ExecutorState::DONE) { // We are done with this input. // We need to forward it to the last ShadowRow. - LOG_DEVEL << "SKIPPING ALL REMAINING ROWS"; size_t processed = input.skipAllRemainingDataRows(); - LOG_DEVEL << "SKIPPING ALL REMAINING ROWS: HAVE SKIPPED: " << processed; + // We cannot discard any rows here, they need to be processed. + TRI_ASSERT(processed == 0); _processed += processed; TRI_ASSERT(input.upstreamState() == ExecutorState::DONE); } @@ -343,12 +336,29 @@ template if (_modifier->nrOfOperations() > 0) { ExecutionState modifierState = _modifier->transact(_trx); - + if (modifierState == ExecutionState::WAITING) { + // Do forward matrix input to flag everything as consumed. + if constexpr (std::is_same_v) { + auto& range = input.getInputRange(); + if (range.hasDataRow()) { + _processed += doCollect(range, toSkip); + } + upstreamState = range.upstreamState(); + if (upstreamState == ExecutorState::DONE) { + // We are done with this input. + // We need to forward it to the last ShadowRow. + size_t processed = input.skipAllRemainingDataRows(); + // We cannot discard any rows here, they need to be processed. + TRI_ASSERT(processed == 0); + _processed += processed; + TRI_ASSERT(input.upstreamState() == ExecutorState::DONE); + } + } + TRI_ASSERT(!ServerState::instance()->isSingleServer()); // TODO - //return {ExecutionState::WAITING, stats, 0, upstreamCall}; - LOG_DEVEL << "SKIPROWSRANGE: WE HAVE WORKED BUT ARE NOW WAITING. RETURNING WAITING!"; + // return {ExecutionState::WAITING, stats, 0, upstreamCall}; return {ExecutionState::WAITING, ModificationStats{}, 0, AqlCall{}}; } @@ -356,7 +366,6 @@ template } } - LOG_DEVEL << "SKIPROWSRANGE AT THE END: " << translateReturnType(upstreamState); return {translateReturnType(upstreamState), stats, call.getSkipCount(), upstreamCall}; } From 57f759f135b91e4ab196575e2dd7b0cc46fa4dec Mon Sep 17 00:00:00 2001 From: jsteemann Date: Thu, 22 Jul 2021 13:26:18 +0200 Subject: [PATCH 04/32] clean up a bit --- arangod/Aql/ModificationExecutor.cpp | 2 +- tests/Aql/AqlSharedExecutionBlockImplTest.cpp | 3 ++- tests/js/server/aql/aql-modify-subqueries.js | 3 +-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/arangod/Aql/ModificationExecutor.cpp b/arangod/Aql/ModificationExecutor.cpp index 72db18fe90df..12c374e448fc 100644 --- a/arangod/Aql/ModificationExecutor.cpp +++ b/arangod/Aql/ModificationExecutor.cpp @@ -303,7 +303,7 @@ template processed /*call.getSkipCount()*/, upstreamCall}; } - TRI_ASSERT(_processed == 0); + //TRI_ASSERT(ServerState::instance()->isSingleServer() || _processed == 0); // only produce at most output.numRowsLeft() many results ExecutorState upstreamState = input.upstreamState(); while (input.hasDataRow() && call.needSkipMore()) { diff --git a/tests/Aql/AqlSharedExecutionBlockImplTest.cpp b/tests/Aql/AqlSharedExecutionBlockImplTest.cpp index 58a4b593d6da..1bd67ff20b66 100644 --- a/tests/Aql/AqlSharedExecutionBlockImplTest.cpp +++ b/tests/Aql/AqlSharedExecutionBlockImplTest.cpp @@ -260,7 +260,8 @@ class AqlSharedExecutionBlockImplTest : public ::testing::Test { auto col = collections.get(collectionName); TRI_ASSERT(col != nullptr); // failed to add collection OperationOptions opts{}; - ModificationExecutorInfos execInfos{0, + ModificationExecutorInfos execInfos{fakedQuery->rootEngine(), + 0, RegisterPlan::MaxRegisterId, RegisterPlan::MaxRegisterId, 0, diff --git a/tests/js/server/aql/aql-modify-subqueries.js b/tests/js/server/aql/aql-modify-subqueries.js index 0d90301711e1..20180a5a3fe2 100644 --- a/tests/js/server/aql/aql-modify-subqueries.js +++ b/tests/js/server/aql/aql-modify-subqueries.js @@ -1748,8 +1748,7 @@ function ahuacatlModifySkipSuite () { testSubqueryFullCount : function () { const query = "LET sub = NOOPT(FOR doc IN " + cn + " REMOVE doc IN " + cn + " RETURN OLD) COLLECT WITH COUNT INTO l RETURN l"; - db._explain(query, null, { optimizer: { rules: ["-all"] } }); - let result = AQL_EXECUTE(query, null, { profile: 3, optimizer: { rules: ["-all"] } }); + let result = AQL_EXECUTE(query, null, { optimizer: { rules: ["-all"] } }); assertEqual(1, result.json.length); assertEqual(1, result.json[0]); }, From eb2d2c0f01263865196decb8200cd7ae4a5dd6c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Thu, 22 Jul 2021 16:42:42 +0200 Subject: [PATCH 05/32] Fixed counting of skipped rows --- arangod/Aql/ModificationExecutor.cpp | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/arangod/Aql/ModificationExecutor.cpp b/arangod/Aql/ModificationExecutor.cpp index 12c374e448fc..a722348c524e 100644 --- a/arangod/Aql/ModificationExecutor.cpp +++ b/arangod/Aql/ModificationExecutor.cpp @@ -291,16 +291,10 @@ template } if (resultState == ModificationExecutorResultState::HaveResult) { - // TODO: check if we need this - call.didSkip(_processed); - TRI_ASSERT(!ServerState::instance()->isSingleServer()); - size_t processed = _processed; handleResult(); - _processed = 0; - // TODO return {translateReturnType(input.upstreamState()), stats, - processed /*call.getSkipCount()*/, upstreamCall}; + call.getSkipCount(), upstreamCall}; } //TRI_ASSERT(ServerState::instance()->isSingleServer() || _processed == 0); From 9d1fac1d15806cf95935cfd03dd9c18e392c281e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Thu, 22 Jul 2021 17:01:58 +0200 Subject: [PATCH 06/32] Removed unused member variable --- arangod/Aql/ModificationExecutor.cpp | 24 ++++++++---------------- arangod/Aql/ModificationExecutor.h | 1 - 2 files changed, 8 insertions(+), 17 deletions(-) diff --git a/arangod/Aql/ModificationExecutor.cpp b/arangod/Aql/ModificationExecutor.cpp index a722348c524e..b51b326592c6 100644 --- a/arangod/Aql/ModificationExecutor.cpp +++ b/arangod/Aql/ModificationExecutor.cpp @@ -101,23 +101,18 @@ ModificationExecutor::ModificationExecutor(Fetcher& f : _trx(infos._query.newTrxContext()), _lastState(ExecutionState::HASMORE), _infos(infos), - _modifier(std::make_shared(infos)), - _processed(0) {} + _modifier(std::make_shared(infos)) {} // Fetches as many rows as possible from upstream and accumulates results // through the modifier template -auto ModificationExecutor::doCollect(AqlItemBlockInputRange& input, - size_t maxOutputs) - -> size_t { +void ModificationExecutor::doCollect(AqlItemBlockInputRange& input, + size_t maxOutputs) { ExecutionState state = ExecutionState::HASMORE; - // number of input rows processed - size_t processed = 0; // Maximum number of rows we can put into output // So we only ever produce this many here while (_modifier->nrOfOperations() < maxOutputs && input.hasDataRow()) { - ++processed; auto [state, row] = input.nextDataRow(AqlItemBlockInputRange::HasDataRow{}); // Make sure we have a valid row @@ -125,7 +120,6 @@ auto ModificationExecutor::doCollect(AqlItemBlockInpu _modifier->accumulate(row); } TRI_ASSERT(state == ExecutionState::DONE || state == ExecutionState::HASMORE); - return processed; } // Outputs accumulated results, and counts the statistics @@ -311,20 +305,19 @@ template if constexpr (std::is_same_v) { auto& range = input.getInputRange(); if (range.hasDataRow()) { - _processed += doCollect(range, toSkip); + doCollect(range, toSkip); } upstreamState = range.upstreamState(); if (upstreamState == ExecutorState::DONE) { // We are done with this input. // We need to forward it to the last ShadowRow. - size_t processed = input.skipAllRemainingDataRows(); + auto processed = input.skipAllRemainingDataRows(); // We cannot discard any rows here, they need to be processed. TRI_ASSERT(processed == 0); - _processed += processed; TRI_ASSERT(input.upstreamState() == ExecutorState::DONE); } } else { - _processed += doCollect(input, toSkip); + doCollect(input, toSkip); upstreamState = input.upstreamState(); } @@ -336,16 +329,15 @@ template if constexpr (std::is_same_v) { auto& range = input.getInputRange(); if (range.hasDataRow()) { - _processed += doCollect(range, toSkip); + doCollect(range, toSkip); } upstreamState = range.upstreamState(); if (upstreamState == ExecutorState::DONE) { // We are done with this input. // We need to forward it to the last ShadowRow. - size_t processed = input.skipAllRemainingDataRows(); + auto processed = input.skipAllRemainingDataRows(); // We cannot discard any rows here, they need to be processed. TRI_ASSERT(processed == 0); - _processed += processed; TRI_ASSERT(input.upstreamState() == ExecutorState::DONE); } } diff --git a/arangod/Aql/ModificationExecutor.h b/arangod/Aql/ModificationExecutor.h index c32980c8f19d..058feb6ea172 100644 --- a/arangod/Aql/ModificationExecutor.h +++ b/arangod/Aql/ModificationExecutor.h @@ -206,7 +206,6 @@ class ModificationExecutor { ExecutionState _lastState; ModificationExecutorInfos& _infos; std::shared_ptr _modifier; - size_t _processed; }; } // namespace aql From 9da31dd19248ba5ae924f91216e2936e66ce94d7 Mon Sep 17 00:00:00 2001 From: jsteemann Date: Thu, 22 Jul 2021 17:17:25 +0200 Subject: [PATCH 07/32] fix compilation --- arangod/Aql/ModificationExecutor.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arangod/Aql/ModificationExecutor.h b/arangod/Aql/ModificationExecutor.h index 058feb6ea172..54c672b2b715 100644 --- a/arangod/Aql/ModificationExecutor.h +++ b/arangod/Aql/ModificationExecutor.h @@ -194,7 +194,7 @@ class ModificationExecutor { -> std::tuple; protected: - size_t doCollect(AqlItemBlockInputRange& input, size_t maxOutputs); + void doCollect(AqlItemBlockInputRange& input, size_t maxOutputs); void doOutput(OutputAqlItemRow& output); transaction::Methods _trx; From 3cf4de49156a988482bfe705ce791e24d46dccb7 Mon Sep 17 00:00:00 2001 From: jsteemann Date: Fri, 23 Jul 2021 10:41:15 +0200 Subject: [PATCH 08/32] debugging --- arangod/Aql/ModificationExecutor.cpp | 10 +++++++++- arangod/Aql/ModificationExecutorAccumulator.h | 7 ++++++- arangod/Aql/SimpleModifier.cpp | 5 +++++ 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/arangod/Aql/ModificationExecutor.cpp b/arangod/Aql/ModificationExecutor.cpp index b51b326592c6..0c9e97191b6d 100644 --- a/arangod/Aql/ModificationExecutor.cpp +++ b/arangod/Aql/ModificationExecutor.cpp @@ -208,6 +208,7 @@ template ExecutorState upstreamState = ExecutorState::HASMORE; if constexpr (std::is_same_v) { auto& range = input.getInputRange(); + LOG_DEVEL << "CALLING ACCUMULATE FROM PRODUCE1"; doCollect(range, output.numRowsLeft()); upstreamState = range.upstreamState(); if (upstreamState == ExecutorState::DONE) { @@ -216,6 +217,7 @@ template input.skipAllRemainingDataRows(); } } else { + LOG_DEVEL << "CALLING ACCUMULATE FROM PRODUCE2"; doCollect(input, output.numRowsLeft()); upstreamState = input.upstreamState(); } @@ -250,6 +252,7 @@ template } auto handleResult = [&]() { + LOG_DEVEL << "HANDLERESULT"; _modifier->checkException(); if (_infos._doCount) { @@ -271,8 +274,8 @@ template // have successfully executed call.didSkip((std::min)(call.getOffset(), _modifier->nrOfWritesExecuted())); } + LOG_DEVEL << "RESETTING STATUS"; _modifier->resetResult(); - // TODO }; ModificationExecutorResultState resultState = _modifier->resultState(); @@ -285,11 +288,14 @@ template } if (resultState == ModificationExecutorResultState::HaveResult) { + LOG_DEVEL << "ENTERING AND WE HAVE A RESULT"; TRI_ASSERT(!ServerState::instance()->isSingleServer()); handleResult(); return {translateReturnType(input.upstreamState()), stats, call.getSkipCount(), upstreamCall}; } + + LOG_DEVEL << "ENTERING MODIFIER"; //TRI_ASSERT(ServerState::instance()->isSingleServer() || _processed == 0); // only produce at most output.numRowsLeft() many results @@ -322,9 +328,11 @@ template } if (_modifier->nrOfOperations() > 0) { + LOG_DEVEL << "CALLING TRANSACT!"; ExecutionState modifierState = _modifier->transact(_trx); if (modifierState == ExecutionState::WAITING) { + LOG_DEVEL << "TRANSACT RETURNED WAITING"; // Do forward matrix input to flag everything as consumed. if constexpr (std::is_same_v) { auto& range = input.getInputRange(); diff --git a/arangod/Aql/ModificationExecutorAccumulator.h b/arangod/Aql/ModificationExecutorAccumulator.h index 67ffa291f5a3..b8a22657ad6e 100644 --- a/arangod/Aql/ModificationExecutorAccumulator.h +++ b/arangod/Aql/ModificationExecutorAccumulator.h @@ -26,8 +26,9 @@ #include "Basics/Common.h" #include "Basics/debugging.h" +#include "Logger/LogMacros.h" -#include +#include #include namespace arangodb { @@ -45,16 +46,20 @@ class ModificationExecutorAccumulator { ModificationExecutorAccumulator() { reset(); } VPackSlice closeAndGetContents() { + LOG_DEVEL << "CLOSE AND GET"; _accumulator.close(); + TRI_ASSERT(!_accumulator.isOpenArray()); return _accumulator.slice(); } void add(VPackSlice const& doc) { + LOG_DEVEL << "ADD"; TRI_ASSERT(_accumulator.isOpenArray()); _accumulator.add(doc); } void reset() { + LOG_DEVEL << "RESET"; _accumulator.clear(); _accumulator.openArray(); } diff --git a/arangod/Aql/SimpleModifier.cpp b/arangod/Aql/SimpleModifier.cpp index 8384115c5ee8..6caf15dda3e2 100644 --- a/arangod/Aql/SimpleModifier.cpp +++ b/arangod/Aql/SimpleModifier.cpp @@ -142,6 +142,7 @@ void SimpleModifier::resetResult() noexcept { template void SimpleModifier::reset() { + LOG_DEVEL << "RESET"; _accumulator.reset(); _operations.clear(); _results.reset(); @@ -159,13 +160,17 @@ ExecutionState SimpleModifier::transact(transaction: TRI_ASSERT(_resultState == ModificationExecutorResultState::NoResult); _resultState = ModificationExecutorResultState::NoResult; + LOG_DEVEL << "TRANSACT"; auto result = _completion.transact(trx, _accumulator.closeAndGetContents()); if (result.isReady()) { + LOG_DEVEL << "IS READY"; _results = std::move(result.get()); _resultState = ModificationExecutorResultState::HaveResult; return ExecutionState::DONE; } + + LOG_DEVEL << "NOT READY"; _resultState = ModificationExecutorResultState::WaitingForResult; From 536e03b02cdf52aea775dea0b7e7c93848ad2b82 Mon Sep 17 00:00:00 2001 From: jsteemann Date: Fri, 23 Jul 2021 11:37:01 +0200 Subject: [PATCH 09/32] fix attempt, not complete --- arangod/Aql/ModificationExecutor.cpp | 44 ++++++++++++++----- arangod/Aql/ModificationExecutor.h | 8 ++-- arangod/Aql/ModificationExecutorAccumulator.h | 3 -- arangod/Aql/SimpleModifier.cpp | 5 --- 4 files changed, 35 insertions(+), 25 deletions(-) diff --git a/arangod/Aql/ModificationExecutor.cpp b/arangod/Aql/ModificationExecutor.cpp index 0c9e97191b6d..4241b7792d24 100644 --- a/arangod/Aql/ModificationExecutor.cpp +++ b/arangod/Aql/ModificationExecutor.cpp @@ -99,7 +99,6 @@ template ModificationExecutor::ModificationExecutor(Fetcher& fetcher, Infos& infos) : _trx(infos._query.newTrxContext()), - _lastState(ExecutionState::HASMORE), _infos(infos), _modifier(std::make_shared(infos)) {} @@ -208,7 +207,6 @@ template ExecutorState upstreamState = ExecutorState::HASMORE; if constexpr (std::is_same_v) { auto& range = input.getInputRange(); - LOG_DEVEL << "CALLING ACCUMULATE FROM PRODUCE1"; doCollect(range, output.numRowsLeft()); upstreamState = range.upstreamState(); if (upstreamState == ExecutorState::DONE) { @@ -217,7 +215,6 @@ template input.skipAllRemainingDataRows(); } } else { - LOG_DEVEL << "CALLING ACCUMULATE FROM PRODUCE2"; doCollect(input, output.numRowsLeft()); upstreamState = input.upstreamState(); } @@ -252,7 +249,6 @@ template } auto handleResult = [&]() { - LOG_DEVEL << "HANDLERESULT"; _modifier->checkException(); if (_infos._doCount) { @@ -274,8 +270,8 @@ template // have successfully executed call.didSkip((std::min)(call.getOffset(), _modifier->nrOfWritesExecuted())); } - LOG_DEVEL << "RESETTING STATUS"; _modifier->resetResult(); + }; ModificationExecutorResultState resultState = _modifier->resultState(); @@ -288,20 +284,41 @@ template } if (resultState == ModificationExecutorResultState::HaveResult) { - LOG_DEVEL << "ENTERING AND WE HAVE A RESULT"; TRI_ASSERT(!ServerState::instance()->isSingleServer()); handleResult(); + return {translateReturnType(input.upstreamState()), stats, call.getSkipCount(), upstreamCall}; } - - LOG_DEVEL << "ENTERING MODIFIER"; //TRI_ASSERT(ServerState::instance()->isSingleServer() || _processed == 0); // only produce at most output.numRowsLeft() many results ExecutorState upstreamState = input.upstreamState(); while (input.hasDataRow() && call.needSkipMore()) { _modifier->reset(); + + if (_mustSkip) { + // Do forward matrix input to flag everything as consumed. + if constexpr (std::is_same_v) { + auto toSkip = _toSkip; + auto& range = input.getInputRange(); + if (range.hasDataRow()) { + doCollect(range, toSkip); + } + auto upstreamState = range.upstreamState(); + if (upstreamState == ExecutorState::DONE) { + // We are done with this input. + // We need to forward it to the last ShadowRow. + auto processed = input.skipAllRemainingDataRows(); + // We cannot discard any rows here, they need to be processed. + TRI_ASSERT(processed == 0); + TRI_ASSERT(input.upstreamState() == ExecutorState::DONE); + } + } + _mustSkip = false; + _toSkip = 0; + } + size_t toSkip = call.getOffset(); if (call.getLimit() == 0 && call.hasHardLimit()) { // We need to produce all modification operations. @@ -328,11 +345,11 @@ template } if (_modifier->nrOfOperations() > 0) { - LOG_DEVEL << "CALLING TRANSACT!"; ExecutionState modifierState = _modifier->transact(_trx); if (modifierState == ExecutionState::WAITING) { - LOG_DEVEL << "TRANSACT RETURNED WAITING"; + TRI_ASSERT(!ServerState::instance()->isSingleServer()); +#if 0 // Do forward matrix input to flag everything as consumed. if constexpr (std::is_same_v) { auto& range = input.getInputRange(); @@ -349,9 +366,10 @@ template TRI_ASSERT(input.upstreamState() == ExecutorState::DONE); } } - - TRI_ASSERT(!ServerState::instance()->isSingleServer()); +#endif // TODO + _mustSkip = true; + _toSkip = toSkip; // return {ExecutionState::WAITING, stats, 0, upstreamCall}; return {ExecutionState::WAITING, ModificationStats{}, 0, AqlCall{}}; } @@ -360,6 +378,8 @@ template } } + _mustSkip = false; + _toSkip = 0; return {translateReturnType(upstreamState), stats, call.getSkipCount(), upstreamCall}; } diff --git a/arangod/Aql/ModificationExecutor.h b/arangod/Aql/ModificationExecutor.h index 54c672b2b715..85e2a82162f1 100644 --- a/arangod/Aql/ModificationExecutor.h +++ b/arangod/Aql/ModificationExecutor.h @@ -199,13 +199,11 @@ class ModificationExecutor { transaction::Methods _trx; - // The state that was returned on the last call to produceRows. For us - // this is relevant because we might have collected some documents in the - // modifier's accumulator, but not written them yet, because we ran into - // WAITING - ExecutionState _lastState; ModificationExecutorInfos& _infos; std::shared_ptr _modifier; + + bool _mustSkip; + size_t _toSkip; }; } // namespace aql diff --git a/arangod/Aql/ModificationExecutorAccumulator.h b/arangod/Aql/ModificationExecutorAccumulator.h index b8a22657ad6e..6ed294d5ac61 100644 --- a/arangod/Aql/ModificationExecutorAccumulator.h +++ b/arangod/Aql/ModificationExecutorAccumulator.h @@ -46,20 +46,17 @@ class ModificationExecutorAccumulator { ModificationExecutorAccumulator() { reset(); } VPackSlice closeAndGetContents() { - LOG_DEVEL << "CLOSE AND GET"; _accumulator.close(); TRI_ASSERT(!_accumulator.isOpenArray()); return _accumulator.slice(); } void add(VPackSlice const& doc) { - LOG_DEVEL << "ADD"; TRI_ASSERT(_accumulator.isOpenArray()); _accumulator.add(doc); } void reset() { - LOG_DEVEL << "RESET"; _accumulator.clear(); _accumulator.openArray(); } diff --git a/arangod/Aql/SimpleModifier.cpp b/arangod/Aql/SimpleModifier.cpp index 6caf15dda3e2..adac2e159afe 100644 --- a/arangod/Aql/SimpleModifier.cpp +++ b/arangod/Aql/SimpleModifier.cpp @@ -142,7 +142,6 @@ void SimpleModifier::resetResult() noexcept { template void SimpleModifier::reset() { - LOG_DEVEL << "RESET"; _accumulator.reset(); _operations.clear(); _results.reset(); @@ -160,18 +159,14 @@ ExecutionState SimpleModifier::transact(transaction: TRI_ASSERT(_resultState == ModificationExecutorResultState::NoResult); _resultState = ModificationExecutorResultState::NoResult; - LOG_DEVEL << "TRANSACT"; auto result = _completion.transact(trx, _accumulator.closeAndGetContents()); if (result.isReady()) { - LOG_DEVEL << "IS READY"; _results = std::move(result.get()); _resultState = ModificationExecutorResultState::HaveResult; return ExecutionState::DONE; } - LOG_DEVEL << "NOT READY"; - _resultState = ModificationExecutorResultState::WaitingForResult; TRI_ASSERT(!ServerState::instance()->isSingleServer()); From 80a930f9b0f46aaad3924767ec83a5b42c208c4b Mon Sep 17 00:00:00 2001 From: jsteemann Date: Fri, 23 Jul 2021 12:47:09 +0200 Subject: [PATCH 10/32] added assertions --- arangod/Aql/ModificationExecutor.cpp | 50 +++++++++++++++++----------- 1 file changed, 30 insertions(+), 20 deletions(-) diff --git a/arangod/Aql/ModificationExecutor.cpp b/arangod/Aql/ModificationExecutor.cpp index 4241b7792d24..a439af06fbdd 100644 --- a/arangod/Aql/ModificationExecutor.cpp +++ b/arangod/Aql/ModificationExecutor.cpp @@ -168,6 +168,8 @@ template auto stats = ModificationStats{}; auto handleResult = [&]() { + TRI_ASSERT(_modifier->resultState() == ModificationExecutorResultState::HaveResult); + _modifier->checkException(); if (_infos._doCount) { stats.addWritesExecuted(_modifier->nrOfWritesExecuted()); @@ -249,6 +251,8 @@ template } auto handleResult = [&]() { + TRI_ASSERT(_modifier->resultState() == ModificationExecutorResultState::HaveResult); + _modifier->checkException(); if (_infos._doCount) { @@ -291,33 +295,39 @@ template call.getSkipCount(), upstreamCall}; } - //TRI_ASSERT(ServerState::instance()->isSingleServer() || _processed == 0); - // only produce at most output.numRowsLeft() many results - ExecutorState upstreamState = input.upstreamState(); - while (input.hasDataRow() && call.needSkipMore()) { - _modifier->reset(); - + auto skipIfRequired = [&]() { if (_mustSkip) { // Do forward matrix input to flag everything as consumed. if constexpr (std::is_same_v) { auto toSkip = _toSkip; - auto& range = input.getInputRange(); - if (range.hasDataRow()) { - doCollect(range, toSkip); - } - auto upstreamState = range.upstreamState(); - if (upstreamState == ExecutorState::DONE) { - // We are done with this input. - // We need to forward it to the last ShadowRow. - auto processed = input.skipAllRemainingDataRows(); - // We cannot discard any rows here, they need to be processed. - TRI_ASSERT(processed == 0); - TRI_ASSERT(input.upstreamState() == ExecutorState::DONE); + if (input.hasValidRow()) { + auto& range = input.getInputRange(); + if (range.hasDataRow()) { + doCollect(range, toSkip); + } + auto upstreamState = range.upstreamState(); + if (upstreamState == ExecutorState::DONE) { + // We are done with this input. + // We need to forward it to the last ShadowRow. + auto processed = input.skipAllRemainingDataRows(); + // We cannot discard any rows here, they need to be processed. + TRI_ASSERT(processed == 0); + TRI_ASSERT(input.upstreamState() == ExecutorState::DONE); + } } } _mustSkip = false; _toSkip = 0; } + }; + + //TRI_ASSERT(ServerState::instance()->isSingleServer() || _processed == 0); + // only produce at most output.numRowsLeft() many results + ExecutorState upstreamState = input.upstreamState(); + while (input.hasDataRow() && call.needSkipMore()) { + _modifier->reset(); + + skipIfRequired(); size_t toSkip = call.getOffset(); if (call.getLimit() == 0 && call.hasHardLimit()) { @@ -377,9 +387,9 @@ template handleResult(); } } + + skipIfRequired(); - _mustSkip = false; - _toSkip = 0; return {translateReturnType(upstreamState), stats, call.getSkipCount(), upstreamCall}; } From 8d8beea5c2e7416446ad21aca630953bc23ab7d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Tue, 27 Jul 2021 17:48:03 +0200 Subject: [PATCH 11/32] Fix some problems, still WIP --- arangod/Aql/ModificationExecutor.cpp | 89 +++++++++++----------------- arangod/Aql/ModificationExecutor.h | 5 +- 2 files changed, 39 insertions(+), 55 deletions(-) diff --git a/arangod/Aql/ModificationExecutor.cpp b/arangod/Aql/ModificationExecutor.cpp index a439af06fbdd..96aa907ad684 100644 --- a/arangod/Aql/ModificationExecutor.cpp +++ b/arangod/Aql/ModificationExecutor.cpp @@ -121,6 +121,25 @@ void ModificationExecutor::doCollect(AqlItemBlockInpu TRI_ASSERT(state == ExecutionState::DONE || state == ExecutionState::HASMORE); } +template +auto ModificationExecutor::doCollectRange( + typename FetcherType::DataRange& input, size_t maxOutputs) -> ExecutorState { + if constexpr (std::is_same_v) { + auto& range = input.getInputRange(); + doCollect(range, maxOutputs); + auto upstreamState = range.upstreamState(); + if (upstreamState == ExecutorState::DONE) { + // We are done with this input. + // We need to forward it to the last ShadowRow. + input.skipAllRemainingDataRows(); + } + return upstreamState; + } else { + doCollect(input, maxOutputs); + return input.upstreamState(); + } +} + // Outputs accumulated results, and counts the statistics template void ModificationExecutor::doOutput(OutputAqlItemRow& output) { @@ -206,20 +225,7 @@ template } // only produce at most output.numRowsLeft() many results - ExecutorState upstreamState = ExecutorState::HASMORE; - if constexpr (std::is_same_v) { - auto& range = input.getInputRange(); - doCollect(range, output.numRowsLeft()); - upstreamState = range.upstreamState(); - if (upstreamState == ExecutorState::DONE) { - // We are done with this input. - // We need to forward it to the last ShadowRow. - input.skipAllRemainingDataRows(); - } - } else { - doCollect(input, output.numRowsLeft()); - upstreamState = input.upstreamState(); - } + ExecutorState upstreamState = doCollectRange(input, output.numRowsLeft()); if (_modifier->nrOfOperations() > 0) { ExecutionState modifierState = _modifier->transact(_trx); @@ -250,11 +256,22 @@ template THROW_ARANGO_EXCEPTION(TRI_ERROR_DEBUG); } + ExecutorState upstreamState = input.upstreamState(); + auto handleResult = [&]() { TRI_ASSERT(_modifier->resultState() == ModificationExecutorResultState::HaveResult); _modifier->checkException(); + if constexpr (std::is_same_v) { + upstreamState = input.getInputRange().upstreamState(); + if (upstreamState == ExecutorState::DONE) { + // We are done with this input. + // We need to forward it to the last ShadowRow. + input.skipAllRemainingDataRows(); + } + } + if (_infos._doCount) { stats.addWritesExecuted(_modifier->nrOfWritesExecuted()); stats.addWritesIgnored(_modifier->nrOfWritesIgnored()); @@ -297,25 +314,9 @@ template auto skipIfRequired = [&]() { if (_mustSkip) { - // Do forward matrix input to flag everything as consumed. - if constexpr (std::is_same_v) { - auto toSkip = _toSkip; - if (input.hasValidRow()) { - auto& range = input.getInputRange(); - if (range.hasDataRow()) { - doCollect(range, toSkip); - } - auto upstreamState = range.upstreamState(); - if (upstreamState == ExecutorState::DONE) { - // We are done with this input. - // We need to forward it to the last ShadowRow. - auto processed = input.skipAllRemainingDataRows(); - // We cannot discard any rows here, they need to be processed. - TRI_ASSERT(processed == 0); - TRI_ASSERT(input.upstreamState() == ExecutorState::DONE); - } - } - } + std::ignore = doCollectRange(input, _toSkip); + call.didSkip(_toSkip); + _mustSkip = false; _toSkip = 0; } @@ -323,7 +324,6 @@ template //TRI_ASSERT(ServerState::instance()->isSingleServer() || _processed == 0); // only produce at most output.numRowsLeft() many results - ExecutorState upstreamState = input.upstreamState(); while (input.hasDataRow() && call.needSkipMore()) { _modifier->reset(); @@ -335,24 +335,7 @@ template // If we are bound by limits or not! toSkip = ExecutionBlock::SkipAllSize(); } - if constexpr (std::is_same_v) { - auto& range = input.getInputRange(); - if (range.hasDataRow()) { - doCollect(range, toSkip); - } - upstreamState = range.upstreamState(); - if (upstreamState == ExecutorState::DONE) { - // We are done with this input. - // We need to forward it to the last ShadowRow. - auto processed = input.skipAllRemainingDataRows(); - // We cannot discard any rows here, they need to be processed. - TRI_ASSERT(processed == 0); - TRI_ASSERT(input.upstreamState() == ExecutorState::DONE); - } - } else { - doCollect(input, toSkip); - upstreamState = input.upstreamState(); - } + upstreamState = doCollectRange(input, toSkip); if (_modifier->nrOfOperations() > 0) { ExecutionState modifierState = _modifier->transact(_trx); @@ -387,7 +370,7 @@ template handleResult(); } } - + skipIfRequired(); return {translateReturnType(upstreamState), stats, call.getSkipCount(), upstreamCall}; diff --git a/arangod/Aql/ModificationExecutor.h b/arangod/Aql/ModificationExecutor.h index 85e2a82162f1..a31cad058f01 100644 --- a/arangod/Aql/ModificationExecutor.h +++ b/arangod/Aql/ModificationExecutor.h @@ -195,6 +195,7 @@ class ModificationExecutor { protected: void doCollect(AqlItemBlockInputRange& input, size_t maxOutputs); + auto doCollectRange(typename FetcherType::DataRange& input, size_t maxOutputs) -> ExecutorState; void doOutput(OutputAqlItemRow& output); transaction::Methods _trx; @@ -202,8 +203,8 @@ class ModificationExecutor { ModificationExecutorInfos& _infos; std::shared_ptr _modifier; - bool _mustSkip; - size_t _toSkip; + bool _mustSkip{}; + size_t _toSkip{}; }; } // namespace aql From 8ddca41057f8a4fa3b27cae2a2ce192cb8e70146 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Tue, 27 Jul 2021 19:40:32 +0200 Subject: [PATCH 12/32] Some more changes --- arangod/Aql/ModificationExecutor.cpp | 62 ++++++---------------------- arangod/Aql/ModificationExecutor.h | 3 -- 2 files changed, 13 insertions(+), 52 deletions(-) diff --git a/arangod/Aql/ModificationExecutor.cpp b/arangod/Aql/ModificationExecutor.cpp index 96aa907ad684..2256b402dce9 100644 --- a/arangod/Aql/ModificationExecutor.cpp +++ b/arangod/Aql/ModificationExecutor.cpp @@ -256,22 +256,11 @@ template THROW_ARANGO_EXCEPTION(TRI_ERROR_DEBUG); } - ExecutorState upstreamState = input.upstreamState(); - auto handleResult = [&]() { TRI_ASSERT(_modifier->resultState() == ModificationExecutorResultState::HaveResult); _modifier->checkException(); - if constexpr (std::is_same_v) { - upstreamState = input.getInputRange().upstreamState(); - if (upstreamState == ExecutorState::DONE) { - // We are done with this input. - // We need to forward it to the last ShadowRow. - input.skipAllRemainingDataRows(); - } - } - if (_infos._doCount) { stats.addWritesExecuted(_modifier->nrOfWritesExecuted()); stats.addWritesIgnored(_modifier->nrOfWritesIgnored()); @@ -304,31 +293,30 @@ template return {ExecutionState::WAITING, ModificationStats{}, 0, AqlCall{}}; } + ExecutorState upstreamState = input.upstreamState(); + if (resultState == ModificationExecutorResultState::HaveResult) { TRI_ASSERT(!ServerState::instance()->isSingleServer()); - handleResult(); - return {translateReturnType(input.upstreamState()), stats, - call.getSkipCount(), upstreamCall}; - } + if constexpr (std::is_same_v) { + upstreamState = input.getInputRange().upstreamState(); + if (upstreamState == ExecutorState::DONE) { + // We are done with this input. + // We need to forward it to the last ShadowRow. + input.skipAllRemainingDataRows(); + } + } - auto skipIfRequired = [&]() { - if (_mustSkip) { - std::ignore = doCollectRange(input, _toSkip); - call.didSkip(_toSkip); + handleResult(); - _mustSkip = false; - _toSkip = 0; - } - }; + return {translateReturnType(upstreamState), stats, call.getSkipCount(), upstreamCall}; + } //TRI_ASSERT(ServerState::instance()->isSingleServer() || _processed == 0); // only produce at most output.numRowsLeft() many results while (input.hasDataRow() && call.needSkipMore()) { _modifier->reset(); - skipIfRequired(); - size_t toSkip = call.getOffset(); if (call.getLimit() == 0 && call.hasHardLimit()) { // We need to produce all modification operations. @@ -342,28 +330,6 @@ template if (modifierState == ExecutionState::WAITING) { TRI_ASSERT(!ServerState::instance()->isSingleServer()); -#if 0 - // Do forward matrix input to flag everything as consumed. - if constexpr (std::is_same_v) { - auto& range = input.getInputRange(); - if (range.hasDataRow()) { - doCollect(range, toSkip); - } - upstreamState = range.upstreamState(); - if (upstreamState == ExecutorState::DONE) { - // We are done with this input. - // We need to forward it to the last ShadowRow. - auto processed = input.skipAllRemainingDataRows(); - // We cannot discard any rows here, they need to be processed. - TRI_ASSERT(processed == 0); - TRI_ASSERT(input.upstreamState() == ExecutorState::DONE); - } - } -#endif - // TODO - _mustSkip = true; - _toSkip = toSkip; - // return {ExecutionState::WAITING, stats, 0, upstreamCall}; return {ExecutionState::WAITING, ModificationStats{}, 0, AqlCall{}}; } @@ -371,8 +337,6 @@ template } } - skipIfRequired(); - return {translateReturnType(upstreamState), stats, call.getSkipCount(), upstreamCall}; } diff --git a/arangod/Aql/ModificationExecutor.h b/arangod/Aql/ModificationExecutor.h index a31cad058f01..fd7eb86b334d 100644 --- a/arangod/Aql/ModificationExecutor.h +++ b/arangod/Aql/ModificationExecutor.h @@ -202,9 +202,6 @@ class ModificationExecutor { ModificationExecutorInfos& _infos; std::shared_ptr _modifier; - - bool _mustSkip{}; - size_t _toSkip{}; }; } // namespace aql From 4582aa0ffe9dd3fdab0c7b4f13c11810697dc691 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Wed, 4 Aug 2021 16:27:15 +0200 Subject: [PATCH 13/32] WIP: Some minor changes and preparations for ModificationExecutor refactoring --- arangod/Aql/AqlExecuteResult.cpp | 3 +- arangod/Aql/AqlItemBlockInputMatrix.cpp | 4 +- arangod/Aql/AqlItemBlockInputMatrix.h | 2 +- arangod/Aql/ExecutionState.cpp | 9 +--- arangod/Aql/ExecutionState.h | 10 ++--- arangod/Aql/InsertModifier.h | 10 ++--- arangod/Aql/ModificationExecutor.cpp | 14 +++--- arangod/Aql/ModificationExecutor.h | 12 ++--- arangod/Aql/ModificationExecutorAccumulator.h | 20 ++++----- arangod/Aql/RemoveModifier.cpp | 2 - arangod/Aql/RemoveModifier.h | 10 ++--- arangod/Aql/SimpleModifier.cpp | 23 +++++++++- arangod/Aql/SimpleModifier.h | 45 ++++++++----------- arangod/Aql/UpdateReplaceModifier.h | 12 ++--- arangod/Aql/UpsertModifier.cpp | 29 +++++++++++- arangod/Aql/UpsertModifier.h | 18 +++----- 16 files changed, 114 insertions(+), 109 deletions(-) diff --git a/arangod/Aql/AqlExecuteResult.cpp b/arangod/Aql/AqlExecuteResult.cpp index a5e9a45e0bd9..6428a77fa2d5 100644 --- a/arangod/Aql/AqlExecuteResult.cpp +++ b/arangod/Aql/AqlExecuteResult.cpp @@ -35,6 +35,7 @@ #include #include +#include using namespace arangodb; using namespace arangodb::aql; @@ -49,7 +50,7 @@ auto getStringView(velocypack::Slice slice) -> std::string_view { AqlExecuteResult::AqlExecuteResult(ExecutionState state, SkipResult skipped, SharedAqlItemBlockPtr&& block) - : _state(state), _skipped(skipped), _block(std::move(block)) { + : _state(state), _skipped(std::move(skipped)), _block(std::move(block)) { // Make sure we only produce a valid response // The block should have checked as well. // We must return skipped and/or data when reporting HASMORE diff --git a/arangod/Aql/AqlItemBlockInputMatrix.cpp b/arangod/Aql/AqlItemBlockInputMatrix.cpp index 2eb7b2790d02..fdd350216bfc 100644 --- a/arangod/Aql/AqlItemBlockInputMatrix.cpp +++ b/arangod/Aql/AqlItemBlockInputMatrix.cpp @@ -42,8 +42,8 @@ AqlItemBlockInputMatrix::AqlItemBlockInputMatrix(ExecutorState state) } // only used for block passthrough -AqlItemBlockInputMatrix::AqlItemBlockInputMatrix(arangodb::aql::SharedAqlItemBlockPtr const& block) - : _block{block}, _aqlItemMatrix{nullptr} { +AqlItemBlockInputMatrix::AqlItemBlockInputMatrix(arangodb::aql::SharedAqlItemBlockPtr block) + : _block{std::move(block)}, _aqlItemMatrix{nullptr} { TRI_ASSERT(_aqlItemMatrix == nullptr); TRI_ASSERT(!hasDataRow()); } diff --git a/arangod/Aql/AqlItemBlockInputMatrix.h b/arangod/Aql/AqlItemBlockInputMatrix.h index a089be534af0..4e4f7adf4c71 100644 --- a/arangod/Aql/AqlItemBlockInputMatrix.h +++ b/arangod/Aql/AqlItemBlockInputMatrix.h @@ -38,7 +38,7 @@ class AqlItemBlockInputMatrix { public: explicit AqlItemBlockInputMatrix(ExecutorState state); - AqlItemBlockInputMatrix(arangodb::aql::SharedAqlItemBlockPtr const&); + AqlItemBlockInputMatrix(arangodb::aql::SharedAqlItemBlockPtr); AqlItemBlockInputMatrix(ExecutorState state, AqlItemMatrix* aqlItemMatrix); diff --git a/arangod/Aql/ExecutionState.cpp b/arangod/Aql/ExecutionState.cpp index 35ed458c52c3..c8753ed13c72 100644 --- a/arangod/Aql/ExecutionState.cpp +++ b/arangod/Aql/ExecutionState.cpp @@ -25,8 +25,7 @@ #include -namespace arangodb { -namespace aql { +namespace arangodb::aql { std::ostream& operator<<(std::ostream& ostream, ExecutionState state) { switch (state) { @@ -51,12 +50,8 @@ std::ostream& operator<<(std::ostream& ostream, ExecutorState state) { case ExecutorState::HASMORE: ostream << "HASMORE"; break; - default: - ostream << " WAT WAT WAT"; - break; } return ostream; } -} // namespace aql -} // namespace arangodb +} // namespace arangodb::aql diff --git a/arangod/Aql/ExecutionState.h b/arangod/Aql/ExecutionState.h index d1eb4f865971..e2f9f8c962b7 100644 --- a/arangod/Aql/ExecutionState.h +++ b/arangod/Aql/ExecutionState.h @@ -21,13 +21,11 @@ /// @author Michael Hackstein //////////////////////////////////////////////////////////////////////////////// -#ifndef ARANGOD_AQL_EXECUTION_STATE_H -#define ARANGOD_AQL_EXECUTION_STATE_H 1 +#pragma once #include -namespace arangodb { -namespace aql { +namespace arangodb::aql { enum class ExecutionState { // done with this block, definitely no more results @@ -58,6 +56,4 @@ std::ostream& operator<<(std::ostream& ostream, ExecutionState state); std::ostream& operator<<(std::ostream& ostream, ExecutorState state); -} // namespace aql -} // namespace arangodb -#endif +} // namespace arangodb::aql diff --git a/arangod/Aql/InsertModifier.h b/arangod/Aql/InsertModifier.h index 74bd0f11a974..dc33fe36da11 100644 --- a/arangod/Aql/InsertModifier.h +++ b/arangod/Aql/InsertModifier.h @@ -21,16 +21,14 @@ /// @author Markus Pfeiffer //////////////////////////////////////////////////////////////////////////////// -#ifndef ARANGOD_AQL_INSERT_MODIFIER_H -#define ARANGOD_AQL_INSERT_MODIFIER_H +#pragma once #include "Aql/ModificationExecutor.h" #include "Aql/ModificationExecutorAccumulator.h" #include "Aql/ModificationExecutorInfos.h" #include "Futures/Future.h" -namespace arangodb { -namespace aql { +namespace arangodb::aql { struct ModificationExecutorInfos; @@ -48,6 +46,4 @@ class InsertModifierCompletion { ModificationExecutorInfos& _infos; }; -} // namespace aql -} // namespace arangodb -#endif +} // namespace arangodb::aql diff --git a/arangod/Aql/ModificationExecutor.cpp b/arangod/Aql/ModificationExecutor.cpp index 2256b402dce9..f94acf331739 100644 --- a/arangod/Aql/ModificationExecutor.cpp +++ b/arangod/Aql/ModificationExecutor.cpp @@ -31,6 +31,7 @@ #include "Aql/SingleRowFetcher.h" #include "Basics/Common.h" #include "Basics/VelocyPackHelper.h" +#include "Logger/LogMacros.h" #include "StorageEngine/TransactionState.h" #include "VocBase/LogicalCollection.h" @@ -40,33 +41,32 @@ #include "Aql/UpdateReplaceModifier.h" #include "Aql/UpsertModifier.h" -#include "Logger/LogMacros.h" - +#include #include -#include "velocypack/velocypack-aliases.h" using namespace arangodb; using namespace arangodb::aql; using namespace arangodb::basics; -namespace arangodb { -namespace aql { +namespace arangodb::aql { +namespace { auto translateReturnType(ExecutorState state) noexcept -> ExecutionState { if (state == ExecutorState::DONE) { return ExecutionState::DONE; } return ExecutionState::HASMORE; } +} // namespace ModifierOutput::ModifierOutput(InputAqlItemRow const& inputRow, Type type) - : _inputRow(std::move(inputRow)), _type(type), _oldValue(), _newValue() {} + : _inputRow(inputRow), _type(type), _oldValue(), _newValue() {} ModifierOutput::ModifierOutput(InputAqlItemRow&& inputRow, Type type) : _inputRow(std::move(inputRow)), _type(type), _oldValue(), _newValue() {} ModifierOutput::ModifierOutput(InputAqlItemRow const& inputRow, Type type, AqlValue const& oldValue, AqlValue const& newValue) - : _inputRow(std::move(inputRow)), + : _inputRow(inputRow), _type(type), _oldValue(oldValue), _oldValueGuard(std::in_place, _oldValue.value(), true), diff --git a/arangod/Aql/ModificationExecutor.h b/arangod/Aql/ModificationExecutor.h index fd7eb86b334d..dbac97f9d3ce 100644 --- a/arangod/Aql/ModificationExecutor.h +++ b/arangod/Aql/ModificationExecutor.h @@ -21,8 +21,7 @@ /// @author Markus Pfeiffer //////////////////////////////////////////////////////////////////////////////// -#ifndef ARANGOD_AQL_MODIFICATION_EXECUTOR_H -#define ARANGOD_AQL_MODIFICATION_EXECUTOR_H +#pragma once #include "Aql/ExecutionState.h" #include "Aql/InputAqlItemRow.h" @@ -40,8 +39,7 @@ #include #include -namespace arangodb { -namespace aql { +namespace arangodb::aql { struct AqlCall; class AqlItemBlockInputRange; @@ -197,13 +195,11 @@ class ModificationExecutor { void doCollect(AqlItemBlockInputRange& input, size_t maxOutputs); auto doCollectRange(typename FetcherType::DataRange& input, size_t maxOutputs) -> ExecutorState; void doOutput(OutputAqlItemRow& output); - + transaction::Methods _trx; ModificationExecutorInfos& _infos; std::shared_ptr _modifier; }; -} // namespace aql -} // namespace arangodb -#endif +} // namespace arangodb::aql diff --git a/arangod/Aql/ModificationExecutorAccumulator.h b/arangod/Aql/ModificationExecutorAccumulator.h index 6ed294d5ac61..2ab86821aaa7 100644 --- a/arangod/Aql/ModificationExecutorAccumulator.h +++ b/arangod/Aql/ModificationExecutorAccumulator.h @@ -21,8 +21,7 @@ /// @author Markus Pfeiffer //////////////////////////////////////////////////////////////////////////////// -#ifndef ARANGOD_AQL_MODIFICATION_EXECUTOR_ACCUMULATOR_H -#define ARANGOD_AQL_MODIFICATION_EXECUTOR_ACCUMULATOR_H +#pragma once #include "Basics/Common.h" #include "Basics/debugging.h" @@ -31,8 +30,7 @@ #include #include -namespace arangodb { -namespace aql { +namespace arangodb::aql { // Hack-i-ty-hack // @@ -45,9 +43,10 @@ class ModificationExecutorAccumulator { public: ModificationExecutorAccumulator() { reset(); } - VPackSlice closeAndGetContents() { + [[nodiscard]] VPackSlice closeAndGetContents() { + TRI_ASSERT(_accumulator.isOpenArray()); _accumulator.close(); - TRI_ASSERT(!_accumulator.isOpenArray()); + TRI_ASSERT(_accumulator.isClosed()); return _accumulator.slice(); } @@ -61,13 +60,10 @@ class ModificationExecutorAccumulator { _accumulator.openArray(); } - size_t nrOfDocuments() const { return _accumulator.slice().length(); } + [[nodiscard]] size_t nrOfDocuments() const { return _accumulator.slice().length(); } private: - VPackBuilder _accumulator; + VPackBuilder _accumulator{}; }; -} // namespace aql -} // namespace arangodb - -#endif +} // namespace arangodb::aql diff --git a/arangod/Aql/RemoveModifier.cpp b/arangod/Aql/RemoveModifier.cpp index 66c7f575da84..de43284cbfaf 100644 --- a/arangod/Aql/RemoveModifier.cpp +++ b/arangod/Aql/RemoveModifier.cpp @@ -34,8 +34,6 @@ #include "Transaction/Methods.h" #include "VocBase/LogicalCollection.h" -class CollectionNameResolver; - using namespace arangodb; using namespace arangodb::aql; using namespace arangodb::aql::ModificationExecutorHelpers; diff --git a/arangod/Aql/RemoveModifier.h b/arangod/Aql/RemoveModifier.h index 338b411cf0c2..0dea099de2e8 100644 --- a/arangod/Aql/RemoveModifier.h +++ b/arangod/Aql/RemoveModifier.h @@ -21,8 +21,7 @@ /// @author Markus Pfeiffer //////////////////////////////////////////////////////////////////////////////// -#ifndef ARANGOD_AQL_REMOVE_MODIFIER_H -#define ARANGOD_AQL_REMOVE_MODIFIER_H +#pragma once #include "Aql/ModificationExecutor.h" #include "Aql/ModificationExecutorAccumulator.h" @@ -31,8 +30,7 @@ #include -namespace arangodb { -namespace aql { +namespace arangodb::aql { struct ModificationExecutorInfos; @@ -50,6 +48,4 @@ class RemoveModifierCompletion { arangodb::velocypack::Builder _keyDocBuilder; }; -} // namespace aql -} // namespace arangodb -#endif +} // namespace arangodb::aql diff --git a/arangod/Aql/SimpleModifier.cpp b/arangod/Aql/SimpleModifier.cpp index adac2e159afe..e8ce3fc45149 100644 --- a/arangod/Aql/SimpleModifier.cpp +++ b/arangod/Aql/SimpleModifier.cpp @@ -142,9 +142,17 @@ void SimpleModifier::resetResult() noexcept { template void SimpleModifier::reset() { +#ifdef ARANGODB_ENABLE_MAINTAINER_MODE + { + std::unique_lock guard(_resultStateMutex, std::try_to_lock); + TRI_ASSERT(guard.owns_lock()); + TRI_ASSERT(_resultState != ModificationExecutorResultState::WaitingForResult); + } +#endif _accumulator.reset(); _operations.clear(); _results.reset(); + resetResult(); } template @@ -156,6 +164,16 @@ void SimpleModifier::accumulate(InputAqlItemRow& row template ExecutionState SimpleModifier::transact(transaction::Methods& trx) { std::lock_guard guard(_resultStateMutex); + switch (_resultState) { + case ModificationExecutorResultState::WaitingForResult: + return ExecutionState::WAITING; + case ModificationExecutorResultState::HaveResult: + return ExecutionState::DONE; + case ModificationExecutorResultState::NoResult: + // continue + break; + } + TRI_ASSERT(_resultState == ModificationExecutorResultState::NoResult); _resultState = ModificationExecutorResultState::NoResult; @@ -166,7 +184,7 @@ ExecutionState SimpleModifier::transact(transaction: _resultState = ModificationExecutorResultState::HaveResult; return ExecutionState::DONE; } - + _resultState = ModificationExecutorResultState::WaitingForResult; TRI_ASSERT(!ServerState::instance()->isSingleServer()); @@ -177,6 +195,8 @@ ExecutionState SimpleModifier::transact(transaction: std::move(result).thenValue([self, sqs = _infos.engine()->sharedState()](OperationResult&& opRes) { { std::lock_guard guard(self->_resultStateMutex); + // TODO add corresponding a check in non-maintainer mode, and handle it somehow + TRI_ASSERT(self->_resultState == ModificationExecutorResultState::WaitingForResult); self->_results = std::move(opRes); self->_resultState = ModificationExecutorResultState::HaveResult; } @@ -185,6 +205,7 @@ ExecutionState SimpleModifier::transact(transaction: return true; }); }); + return ExecutionState::WAITING; } diff --git a/arangod/Aql/SimpleModifier.h b/arangod/Aql/SimpleModifier.h index 0a97e45baf73..121c63633072 100644 --- a/arangod/Aql/SimpleModifier.h +++ b/arangod/Aql/SimpleModifier.h @@ -21,8 +21,7 @@ /// @author Markus Pfeiffer //////////////////////////////////////////////////////////////////////////////// -#ifndef ARANGOD_AQL_SIMPLE_MODIFIER_H -#define ARANGOD_AQL_SIMPLE_MODIFIER_H +#pragma once #include "Aql/ExecutionBlock.h" #include "Aql/ExecutionState.h" @@ -38,8 +37,7 @@ #include #include -namespace arangodb { -namespace aql { +namespace arangodb::aql { struct ModificationExecutorInfos; @@ -93,9 +91,9 @@ class SimpleModifier : public std::enable_shared_from_this::const_iterator _operationsIterator; - VPackArrayIterator _resultsIterator; - size_t const _batchSize; mutable std::mutex _resultStateMutex; @@ -171,7 +165,4 @@ using InsertModifier = SimpleModifier; using RemoveModifier = SimpleModifier; using UpdateReplaceModifier = SimpleModifier; -} // namespace aql -} // namespace arangodb - -#endif +} // namespace arangodb::aql diff --git a/arangod/Aql/UpdateReplaceModifier.h b/arangod/Aql/UpdateReplaceModifier.h index 96b543fbeb6d..56900ca76002 100644 --- a/arangod/Aql/UpdateReplaceModifier.h +++ b/arangod/Aql/UpdateReplaceModifier.h @@ -21,8 +21,7 @@ /// @author Markus Pfeiffer //////////////////////////////////////////////////////////////////////////////// -#ifndef ARANGOD_AQL_REPLACE_MODIFIER_H -#define ARANGOD_AQL_REPLACE_MODIFIER_H +#pragma once #include "Aql/ModificationExecutor.h" #include "Aql/ModificationExecutorAccumulator.h" @@ -31,8 +30,7 @@ #include -namespace arangodb { -namespace aql { +namespace arangodb::aql { struct ModificationExecutorInfos; @@ -44,7 +42,7 @@ class UpdateReplaceModifierCompletion { ModifierOperationType accumulate(ModificationExecutorAccumulator& accu, InputAqlItemRow& row); - futures::Future transact(transaction::Methods& trx, VPackSlice const data); + futures::Future transact(transaction::Methods& trx, VPackSlice data); private: ModificationExecutorInfos& _infos; @@ -52,6 +50,4 @@ class UpdateReplaceModifierCompletion { arangodb::velocypack::Builder _keyDocBuilder; }; -} // namespace aql -} // namespace arangodb -#endif +} // namespace arangodb::aql diff --git a/arangod/Aql/UpsertModifier.cpp b/arangod/Aql/UpsertModifier.cpp index 9f6531b70d31..3ec5d2af2fcc 100644 --- a/arangod/Aql/UpsertModifier.cpp +++ b/arangod/Aql/UpsertModifier.cpp @@ -136,12 +136,22 @@ ModificationExecutorResultState UpsertModifier::resultState() const noexcept { } void UpsertModifier::reset() { +#ifdef ARANGODB_ENABLE_MAINTAINER_MODE + { + std::unique_lock guard(_resultStateMutex, std::try_to_lock); + TRI_ASSERT(guard.owns_lock()); + TRI_ASSERT(_resultState != ModificationExecutorResultState::WaitingForResult); + } +#endif + _insertAccumulator.reset(); _insertResults.reset(); _updateAccumulator.reset(); _updateResults.reset(); _operations.clear(); + + resetResult(); } void UpsertModifier::resetResult() noexcept { @@ -243,12 +253,29 @@ void UpsertModifier::accumulate(InputAqlItemRow& row) { auto insertDoc = row.getValue(insertReg); result = insertCase(_insertAccumulator, insertDoc); } - _operations.push_back({result, row}); + _operations.emplace_back(result, row); } ExecutionState UpsertModifier::transact(transaction::Methods& trx) { std::lock_guard guard(_resultStateMutex); + switch (_resultState) { + case ModificationExecutorResultState::WaitingForResult: + // WAITING is not yet implemented for UpsertModifier, we shouldn't get here + TRI_ASSERT(false); + return ExecutionState::WAITING; + case ModificationExecutorResultState::HaveResult: + // WAITING is not yet implemented for UpsertModifier, we shouldn't get here + TRI_ASSERT(false); + return ExecutionState::DONE; + case ModificationExecutorResultState::NoResult: + // continue + break; + } + + TRI_ASSERT(_resultState == ModificationExecutorResultState::NoResult); + _resultState = ModificationExecutorResultState::NoResult; + auto toInsert = _insertAccumulator.closeAndGetContents(); if (toInsert.isArray() && toInsert.length() > 0) { _insertResults = diff --git a/arangod/Aql/UpsertModifier.h b/arangod/Aql/UpsertModifier.h index d181a452ecec..441bc20eb262 100644 --- a/arangod/Aql/UpsertModifier.h +++ b/arangod/Aql/UpsertModifier.h @@ -21,8 +21,7 @@ /// @author Markus Pfeiffer //////////////////////////////////////////////////////////////////////////////// -#ifndef ARANGOD_AQL_UPSERT_MODIFIER_H -#define ARANGOD_AQL_UPSERT_MODIFIER_H +#pragma once #include "Aql/ModificationExecutor.h" #include "Aql/ModificationExecutorAccumulator.h" @@ -35,8 +34,7 @@ #include -namespace arangodb { -namespace aql { +namespace arangodb::aql { struct ModificationExecutorInfos; @@ -62,11 +60,11 @@ class UpsertModifier { OutputIterator& operator++(); bool operator!=(OutputIterator const& other) const noexcept; ModifierOutput operator*() const; - OutputIterator begin() const; - OutputIterator end() const; + [[nodiscard]] OutputIterator begin() const; + [[nodiscard]] OutputIterator end() const; private: - OutputIterator& next(); + [[nodiscard]] OutputIterator& next(); UpsertModifier const& _modifier; std::vector::const_iterator _operationsIterator; @@ -100,7 +98,7 @@ class UpsertModifier { size_t nrOfOperations() const; size_t nrOfDocuments() const; - size_t nrOfResults() const; + [[maybe_unused]] size_t nrOfResults() const; size_t nrOfErrors() const; size_t nrOfWritesExecuted() const; size_t nrOfWritesIgnored() const; @@ -132,6 +130,4 @@ class UpsertModifier { ModificationExecutorResultState _resultState; }; -} // namespace aql -} // namespace arangodb -#endif +} // namespace arangodb::aql From ea6e0d90288a16f873b6466a805a912d123b1fac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Thu, 5 Aug 2021 10:10:22 +0200 Subject: [PATCH 14/32] [WIP] First refactoring step complete; works on single-server only --- arangod/Aql/ModificationExecutor.cpp | 248 +++++++++++++-------------- arangod/Aql/ModificationExecutor.h | 2 - 2 files changed, 115 insertions(+), 135 deletions(-) diff --git a/arangod/Aql/ModificationExecutor.cpp b/arangod/Aql/ModificationExecutor.cpp index f94acf331739..0c907c527acb 100644 --- a/arangod/Aql/ModificationExecutor.cpp +++ b/arangod/Aql/ModificationExecutor.cpp @@ -102,44 +102,6 @@ ModificationExecutor::ModificationExecutor(Fetcher& f _infos(infos), _modifier(std::make_shared(infos)) {} -// Fetches as many rows as possible from upstream and accumulates results -// through the modifier -template -void ModificationExecutor::doCollect(AqlItemBlockInputRange& input, - size_t maxOutputs) { - ExecutionState state = ExecutionState::HASMORE; - - // Maximum number of rows we can put into output - // So we only ever produce this many here - while (_modifier->nrOfOperations() < maxOutputs && input.hasDataRow()) { - auto [state, row] = input.nextDataRow(AqlItemBlockInputRange::HasDataRow{}); - - // Make sure we have a valid row - TRI_ASSERT(row.isInitialized()); - _modifier->accumulate(row); - } - TRI_ASSERT(state == ExecutionState::DONE || state == ExecutionState::HASMORE); -} - -template -auto ModificationExecutor::doCollectRange( - typename FetcherType::DataRange& input, size_t maxOutputs) -> ExecutorState { - if constexpr (std::is_same_v) { - auto& range = input.getInputRange(); - doCollect(range, maxOutputs); - auto upstreamState = range.upstreamState(); - if (upstreamState == ExecutorState::DONE) { - // We are done with this input. - // We need to forward it to the last ShadowRow. - input.skipAllRemainingDataRows(); - } - return upstreamState; - } else { - doCollect(input, maxOutputs); - return input.upstreamState(); - } -} - // Outputs accumulated results, and counts the statistics template void ModificationExecutor::doOutput(OutputAqlItemRow& output) { @@ -179,53 +141,50 @@ template [[nodiscard]] auto ModificationExecutor::produceRows( typename FetcherType::DataRange& input, OutputAqlItemRow& output) -> std::tuple { + TRI_IF_FAILURE("ModificationBlock::getSome") { + THROW_ARANGO_EXCEPTION(TRI_ERROR_DEBUG); + } + AqlCall upstreamCall{}; if constexpr (std::is_same_v && !std::is_same_v) { upstreamCall.softLimit = _modifier->getBatchSize(); } - auto stats = ModificationStats{}; - auto handleResult = [&]() { - TRI_ASSERT(_modifier->resultState() == ModificationExecutorResultState::HaveResult); + // Make AqlItemBlockInputMatrix happy, which dislikes being asked for its + // input range in this state. + if (!input.hasDataRow()) { + // Input is empty + return {translateReturnType(input.upstreamState()), ModificationStats{}, upstreamCall}; + } - _modifier->checkException(); - if (_infos._doCount) { - stats.addWritesExecuted(_modifier->nrOfWritesExecuted()); - stats.addWritesIgnored(_modifier->nrOfWritesIgnored()); - } + // MSVC doesn't see constexpr variables inside a lambda as constexpr, thus + // we can't just assign is_same_t to a variable. + using inputIsMatrix = + std::is_same; - doOutput(output); - _modifier->resetResult(); - }; - - ModificationExecutorResultState resultState = _modifier->resultState(); - if (resultState == ModificationExecutorResultState::WaitingForResult) { - // we are already waiting for the result. return WAITING again - TRI_ASSERT(!ServerState::instance()->isSingleServer()); - return {ExecutionState::WAITING, stats, upstreamCall}; - } + auto& range = std::invoke([&]() -> AqlItemBlockInputRange& { + if constexpr (inputIsMatrix::value) { + return input.getInputRange(); + } else { + return input; + } + }); - if (resultState == ModificationExecutorResultState::HaveResult) { - // a result is ready for us - TRI_ASSERT(!ServerState::instance()->isSingleServer()); - handleResult(); - return {translateReturnType(input.upstreamState()), stats, upstreamCall}; - } + auto const maxRows = output.numRowsLeft(); - _modifier->reset(); + while (_modifier->nrOfOperations() < maxRows && range.hasDataRow()) { + auto [state, row] = range.nextDataRow(AqlItemBlockInputRange::HasDataRow{}); - if (!input.hasDataRow()) { - // Input is empty - return {translateReturnType(input.upstreamState()), stats, upstreamCall}; + TRI_ASSERT(row.isInitialized()); + _modifier->accumulate(row); } - TRI_IF_FAILURE("ModificationBlock::getSome") { - THROW_ARANGO_EXCEPTION(TRI_ERROR_DEBUG); - } + auto const upstreamState = range.upstreamState(); + // TODO We need to save stats in a member variable that + // survives WAITING, and only return it on DONE. + auto stats = ModificationStats{}; - // only produce at most output.numRowsLeft() many results - ExecutorState upstreamState = doCollectRange(input, output.numRowsLeft()); if (_modifier->nrOfOperations() > 0) { ExecutionState modifierState = _modifier->transact(_trx); @@ -234,7 +193,24 @@ template return {ExecutionState::WAITING, stats, upstreamCall}; } - handleResult(); + TRI_ASSERT(_modifier->resultState() == ModificationExecutorResultState::HaveResult); + + if constexpr (inputIsMatrix::value) { + if (upstreamState == ExecutorState::DONE) { + // We are done with this input. + // We need to forward it to the last ShadowRow. + input.skipAllRemainingDataRows(); + } + } + + _modifier->checkException(); + if (_infos._doCount) { + stats.addWritesExecuted(_modifier->nrOfWritesExecuted()); + stats.addWritesIgnored(_modifier->nrOfWritesIgnored()); + } + + doOutput(output); + _modifier->reset(); } return {translateReturnType(upstreamState), stats, upstreamCall}; @@ -244,62 +220,63 @@ template [[nodiscard]] auto ModificationExecutor::skipRowsRange( typename FetcherType::DataRange& input, AqlCall& call) -> std::tuple { + TRI_IF_FAILURE("ModificationBlock::getSome") { + THROW_ARANGO_EXCEPTION(TRI_ERROR_DEBUG); + } + AqlCall upstreamCall{}; if constexpr (std::is_same_v && !std::is_same_v) { upstreamCall.softLimit = _modifier->getBatchSize(); } - auto stats = ModificationStats{}; - - TRI_IF_FAILURE("ModificationBlock::getSome") { - THROW_ARANGO_EXCEPTION(TRI_ERROR_DEBUG); + // Make AqlItemBlockInputMatrix happy, which dislikes being asked for its + // input range in this state. + if (!input.hasDataRow()) { + // Input is empty + return {translateReturnType(input.upstreamState()), ModificationStats{}, + call.getSkipCount(), upstreamCall}; } - auto handleResult = [&]() { - TRI_ASSERT(_modifier->resultState() == ModificationExecutorResultState::HaveResult); + // MSVC doesn't see constexpr variables inside a lambda as constexpr, thus + // we can't just assign is_same_t to a variable. + using inputIsMatrix = + std::is_same; - _modifier->checkException(); + auto upstreamState = input.upstreamState(); + // TODO We need to save both stats and skip count in member variables that + // survive WAITING, and only return them on DONE. + auto stats = ModificationStats{}; - if (_infos._doCount) { - stats.addWritesExecuted(_modifier->nrOfWritesExecuted()); - stats.addWritesIgnored(_modifier->nrOfWritesIgnored()); - } + while (input.hasDataRow() && call.needSkipMore()) { + auto& range = std::invoke([&]() -> AqlItemBlockInputRange& { + if constexpr (inputIsMatrix::value) { + return input.getInputRange(); + } else { + return input; + } + }); - if (call.needsFullCount()) { - // If we need to do full count the nr of writes we did - // in this batch is always correct. - // If we are in offset phase and need to produce data - // after the toSkip is limited to offset(). - // otherwise we need to report everything we write - call.didSkip(_modifier->nrOfWritesExecuted()); - } else { - // If we do not need to report fullcount. - // we cannot report more than offset - // but also not more than the operations we - // have successfully executed - call.didSkip((std::min)(call.getOffset(), _modifier->nrOfWritesExecuted())); + size_t toSkip = call.getOffset(); + if (call.getLimit() == 0 && call.hasHardLimit()) { + // We need to produce all modification operations. + // If we are bound by limits or not! + toSkip = ExecutionBlock::SkipAllSize(); } - _modifier->resetResult(); - }; + auto numRowsRead = std::size_t{}; + while (_modifier->nrOfOperations() < toSkip && range.hasDataRow()) { + auto [state, row] = range.nextDataRow(AqlItemBlockInputRange::HasDataRow{}); - ModificationExecutorResultState resultState = _modifier->resultState(); - if (resultState == ModificationExecutorResultState::WaitingForResult) { - // we are already waiting for the result. return WAITING again - TRI_ASSERT(!ServerState::instance()->isSingleServer()); - // TODO - // return {ExecutionState::WAITING, stats, 0, upstreamCall}; - return {ExecutionState::WAITING, ModificationStats{}, 0, AqlCall{}}; - } + ++numRowsRead; - ExecutorState upstreamState = input.upstreamState(); + TRI_ASSERT(row.isInitialized()); + _modifier->accumulate(row); + } - if (resultState == ModificationExecutorResultState::HaveResult) { - TRI_ASSERT(!ServerState::instance()->isSingleServer()); + upstreamState = range.upstreamState(); - if constexpr (std::is_same_v) { - upstreamState = input.getInputRange().upstreamState(); + if constexpr (inputIsMatrix::value) { if (upstreamState == ExecutorState::DONE) { // We are done with this input. // We need to forward it to the last ShadowRow. @@ -307,33 +284,39 @@ template } } - handleResult(); - - return {translateReturnType(upstreamState), stats, call.getSkipCount(), upstreamCall}; - } - - //TRI_ASSERT(ServerState::instance()->isSingleServer() || _processed == 0); - // only produce at most output.numRowsLeft() many results - while (input.hasDataRow() && call.needSkipMore()) { - _modifier->reset(); - - size_t toSkip = call.getOffset(); - if (call.getLimit() == 0 && call.hasHardLimit()) { - // We need to produce all modification operations. - // If we are bound by limits or not! - toSkip = ExecutionBlock::SkipAllSize(); - } - upstreamState = doCollectRange(input, toSkip); - if (_modifier->nrOfOperations() > 0) { ExecutionState modifierState = _modifier->transact(_trx); if (modifierState == ExecutionState::WAITING) { TRI_ASSERT(!ServerState::instance()->isSingleServer()); - return {ExecutionState::WAITING, ModificationStats{}, 0, AqlCall{}}; + return {ExecutionState::WAITING, stats, call.getSkipCount(), upstreamCall}; + } + + TRI_ASSERT(_modifier->resultState() == ModificationExecutorResultState::HaveResult); + + _modifier->checkException(); + if (_infos._doCount) { + stats.addWritesExecuted(_modifier->nrOfWritesExecuted()); + stats.addWritesIgnored(_modifier->nrOfWritesIgnored()); } - handleResult(); + { // "doOutput" + if (call.needsFullCount()) { + // If we need to do full count the nr of writes we did + // in this batch is always correct. + // If we are in offset phase and need to produce data + // after the toSkip is limited to offset(). + // otherwise we need to report everything we write + call.didSkip(_modifier->nrOfWritesExecuted()); + } else { + // If we do not need to report fullcount. + // we cannot report more than offset + // but also not more than the operations we + // have successfully executed + call.didSkip((std::min)(call.getOffset(), _modifier->nrOfWritesExecuted())); + } + } + _modifier->reset(); } } @@ -351,5 +334,4 @@ template class ::arangodb::aql::ModificationExecutor; template class ::arangodb::aql::ModificationExecutor; -} // namespace aql -} // namespace arangodb +} // namespace arangodb::aql diff --git a/arangod/Aql/ModificationExecutor.h b/arangod/Aql/ModificationExecutor.h index dbac97f9d3ce..c71e890f4de3 100644 --- a/arangod/Aql/ModificationExecutor.h +++ b/arangod/Aql/ModificationExecutor.h @@ -192,8 +192,6 @@ class ModificationExecutor { -> std::tuple; protected: - void doCollect(AqlItemBlockInputRange& input, size_t maxOutputs); - auto doCollectRange(typename FetcherType::DataRange& input, size_t maxOutputs) -> ExecutorState; void doOutput(OutputAqlItemRow& output); transaction::Methods _trx; From 8fe9b2d850a299bad32ad8f9fe749075a472df4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Thu, 5 Aug 2021 11:56:40 +0200 Subject: [PATCH 15/32] [WIP] Second step of refactoring; still only works on single-server --- arangod/Aql/ModificationExecutor.cpp | 229 ++++++++++--------- arangod/Aql/ModificationExecutor.h | 13 ++ tests/js/server/aql/aql-modify-subqueries.js | 2 +- 3 files changed, 130 insertions(+), 114 deletions(-) diff --git a/arangod/Aql/ModificationExecutor.cpp b/arangod/Aql/ModificationExecutor.cpp index 0c907c527acb..9861951e5078 100644 --- a/arangod/Aql/ModificationExecutor.cpp +++ b/arangod/Aql/ModificationExecutor.cpp @@ -138,87 +138,8 @@ void ModificationExecutor::doOutput(OutputAqlItemRow& } template -[[nodiscard]] auto ModificationExecutor::produceRows( - typename FetcherType::DataRange& input, OutputAqlItemRow& output) - -> std::tuple { - TRI_IF_FAILURE("ModificationBlock::getSome") { - THROW_ARANGO_EXCEPTION(TRI_ERROR_DEBUG); - } - - AqlCall upstreamCall{}; - if constexpr (std::is_same_v && - !std::is_same_v) { - upstreamCall.softLimit = _modifier->getBatchSize(); - } - - // Make AqlItemBlockInputMatrix happy, which dislikes being asked for its - // input range in this state. - if (!input.hasDataRow()) { - // Input is empty - return {translateReturnType(input.upstreamState()), ModificationStats{}, upstreamCall}; - } - - // MSVC doesn't see constexpr variables inside a lambda as constexpr, thus - // we can't just assign is_same_t to a variable. - using inputIsMatrix = - std::is_same; - - auto& range = std::invoke([&]() -> AqlItemBlockInputRange& { - if constexpr (inputIsMatrix::value) { - return input.getInputRange(); - } else { - return input; - } - }); - - auto const maxRows = output.numRowsLeft(); - - while (_modifier->nrOfOperations() < maxRows && range.hasDataRow()) { - auto [state, row] = range.nextDataRow(AqlItemBlockInputRange::HasDataRow{}); - - TRI_ASSERT(row.isInitialized()); - _modifier->accumulate(row); - } - - auto const upstreamState = range.upstreamState(); - // TODO We need to save stats in a member variable that - // survives WAITING, and only return it on DONE. - auto stats = ModificationStats{}; - - if (_modifier->nrOfOperations() > 0) { - ExecutionState modifierState = _modifier->transact(_trx); - - if (modifierState == ExecutionState::WAITING) { - TRI_ASSERT(!ServerState::instance()->isSingleServer()); - return {ExecutionState::WAITING, stats, upstreamCall}; - } - - TRI_ASSERT(_modifier->resultState() == ModificationExecutorResultState::HaveResult); - - if constexpr (inputIsMatrix::value) { - if (upstreamState == ExecutorState::DONE) { - // We are done with this input. - // We need to forward it to the last ShadowRow. - input.skipAllRemainingDataRows(); - } - } - - _modifier->checkException(); - if (_infos._doCount) { - stats.addWritesExecuted(_modifier->nrOfWritesExecuted()); - stats.addWritesIgnored(_modifier->nrOfWritesIgnored()); - } - - doOutput(output); - _modifier->reset(); - } - - return {translateReturnType(upstreamState), stats, upstreamCall}; -} - -template -[[nodiscard]] auto ModificationExecutor::skipRowsRange( - typename FetcherType::DataRange& input, AqlCall& call) +[[nodiscard]] auto ModificationExecutor::produceOrSkip( + typename FetcherType::DataRange& input, IProduceOrSkipData& produceOrSkipData) -> std::tuple { TRI_IF_FAILURE("ModificationBlock::getSome") { THROW_ARANGO_EXCEPTION(TRI_ERROR_DEBUG); @@ -235,7 +156,7 @@ template if (!input.hasDataRow()) { // Input is empty return {translateReturnType(input.upstreamState()), ModificationStats{}, - call.getSkipCount(), upstreamCall}; + produceOrSkipData.getSkipCount(), upstreamCall}; } // MSVC doesn't see constexpr variables inside a lambda as constexpr, thus @@ -248,7 +169,7 @@ template // survive WAITING, and only return them on DONE. auto stats = ModificationStats{}; - while (input.hasDataRow() && call.needSkipMore()) { + while (input.hasDataRow() && produceOrSkipData.needMoreOutput()) { auto& range = std::invoke([&]() -> AqlItemBlockInputRange& { if constexpr (inputIsMatrix::value) { return input.getInputRange(); @@ -256,20 +177,10 @@ template return input; } }); - - size_t toSkip = call.getOffset(); - if (call.getLimit() == 0 && call.hasHardLimit()) { - // We need to produce all modification operations. - // If we are bound by limits or not! - toSkip = ExecutionBlock::SkipAllSize(); - } - - auto numRowsRead = std::size_t{}; - while (_modifier->nrOfOperations() < toSkip && range.hasDataRow()) { + auto const maxRows = produceOrSkipData.getRemainingRows(); + while (_modifier->nrOfOperations() < maxRows && range.hasDataRow()) { auto [state, row] = range.nextDataRow(AqlItemBlockInputRange::HasDataRow{}); - ++numRowsRead; - TRI_ASSERT(row.isInitialized()); _modifier->accumulate(row); } @@ -289,7 +200,7 @@ template if (modifierState == ExecutionState::WAITING) { TRI_ASSERT(!ServerState::instance()->isSingleServer()); - return {ExecutionState::WAITING, stats, call.getSkipCount(), upstreamCall}; + return {ExecutionState::WAITING, stats, produceOrSkipData.getSkipCount(), upstreamCall}; } TRI_ASSERT(_modifier->resultState() == ModificationExecutorResultState::HaveResult); @@ -300,27 +211,119 @@ template stats.addWritesIgnored(_modifier->nrOfWritesIgnored()); } - { // "doOutput" - if (call.needsFullCount()) { - // If we need to do full count the nr of writes we did - // in this batch is always correct. - // If we are in offset phase and need to produce data - // after the toSkip is limited to offset(). - // otherwise we need to report everything we write - call.didSkip(_modifier->nrOfWritesExecuted()); - } else { - // If we do not need to report fullcount. - // we cannot report more than offset - // but also not more than the operations we - // have successfully executed - call.didSkip((std::min)(call.getOffset(), _modifier->nrOfWritesExecuted())); - } - } + produceOrSkipData.doOutput(); _modifier->reset(); } } - return {translateReturnType(upstreamState), stats, call.getSkipCount(), upstreamCall}; + return {translateReturnType(upstreamState), stats, + produceOrSkipData.getSkipCount(), upstreamCall}; +} + +template +[[nodiscard]] auto ModificationExecutor::produceRows( + typename FetcherType::DataRange& input, OutputAqlItemRow& output) + -> std::tuple { + struct ProduceData final : public IProduceOrSkipData { + OutputAqlItemRow& _output; + ModificationExecutorInfos& _infos; + ModifierType& _modifier; + bool _first = true; + + ~ProduceData() final = default; + ProduceData(OutputAqlItemRow& output, ModificationExecutorInfos& infos, ModifierType& modifier) + : _output(output), _infos(infos), _modifier(modifier) {} + + void doOutput() final { + typename ModifierType::OutputIterator modifierOutputIterator(_modifier); + // We only accumulated as many items as we can output, so this + // should be correct + for (auto const& modifierOutput : modifierOutputIterator) { + TRI_ASSERT(!_output.isFull()); + bool written = false; + switch (modifierOutput.getType()) { + case ModifierOutput::Type::ReturnIfRequired: + if (_infos._options.returnOld) { + _output.cloneValueInto(_infos._outputOldRegisterId, + modifierOutput.getInputRow(), + modifierOutput.getOldValue()); + written = true; + } + if (_infos._options.returnNew) { + _output.cloneValueInto(_infos._outputNewRegisterId, + modifierOutput.getInputRow(), + modifierOutput.getNewValue()); + written = true; + } + [[fallthrough]]; + case ModifierOutput::Type::CopyRow: + if (!written) { + _output.copyRow(modifierOutput.getInputRow()); + } + _output.advanceRow(); + break; + case ModifierOutput::Type::SkipRow: + // nothing. + break; + } + } + } + auto getRemainingRows() -> std::size_t final { + return _output.numRowsLeft(); + } + auto needMoreOutput() -> bool final { return !_output.isFull(); } + auto getSkipCount() -> std::size_t override { return 0; } + }; + + auto produceData = ProduceData(output, _infos, *_modifier); + auto [state, stats, skipCount, upstreamCall] = produceOrSkip(input, produceData); + + return {state, stats, upstreamCall}; +} + +template +[[nodiscard]] auto ModificationExecutor::skipRowsRange( + typename FetcherType::DataRange& input, AqlCall& call) + -> std::tuple { + struct SkipData final : public IProduceOrSkipData { + AqlCall& _call; + ModifierType& _modifier; + + SkipData(AqlCall& call, ModifierType& modifier) + : _call(call), _modifier(modifier) {} + ~SkipData() final = default; + + void doOutput() final { + if (_call.needsFullCount()) { + // If we need to do full count the nr of writes we did + // in this batch is always correct. + // If we are in offset phase and need to produce data + // after the toSkip is limited to offset(). + // otherwise we need to report everything we write + _call.didSkip(_modifier.nrOfWritesExecuted()); + } else { + // If we do not need to report fullcount. + // we cannot report more than offset + // but also not more than the operations we + // have successfully executed + _call.didSkip((std::min)(_call.getOffset(), _modifier.nrOfWritesExecuted())); + } + } + auto getRemainingRows() -> std::size_t final { + if (_call.getLimit() == 0 && _call.hasHardLimit()) { + // We need to produce all modification operations. + // If we are bound by limits or not! + return ExecutionBlock::SkipAllSize(); + } else { + return _call.getOffset(); + } + } + auto needMoreOutput() -> bool final { return _call.needSkipMore(); } + auto getSkipCount() -> std::size_t final { return _call.getSkipCount(); } + }; + + auto skipData = SkipData(call, *_modifier); + return produceOrSkip(input, skipData); } using NoPassthroughSingleRowFetcher = SingleRowFetcher; diff --git a/arangod/Aql/ModificationExecutor.h b/arangod/Aql/ModificationExecutor.h index c71e890f4de3..e17a0005178a 100644 --- a/arangod/Aql/ModificationExecutor.h +++ b/arangod/Aql/ModificationExecutor.h @@ -194,6 +194,19 @@ class ModificationExecutor { protected: void doOutput(OutputAqlItemRow& output); + struct IProduceOrSkipData { + virtual ~IProduceOrSkipData() = default; + virtual void doOutput() = 0; + virtual auto getRemainingRows() -> std::size_t = 0; + virtual auto needMoreOutput() -> bool = 0; + virtual auto getSkipCount() -> std::size_t = 0; + }; + + // template + std::tuple produceOrSkip( + typename FetcherType::DataRange& input, + IProduceOrSkipData& produceOrSkipData); + transaction::Methods _trx; ModificationExecutorInfos& _infos; diff --git a/tests/js/server/aql/aql-modify-subqueries.js b/tests/js/server/aql/aql-modify-subqueries.js index 20180a5a3fe2..3d9e5cf0a3a8 100644 --- a/tests/js/server/aql/aql-modify-subqueries.js +++ b/tests/js/server/aql/aql-modify-subqueries.js @@ -1905,7 +1905,7 @@ function ahuacatlGeneratedSuite() { assertEqual(10, res.toArray().length); } }; -}; +} jsunity.run(ahuacatlModifySuite); jsunity.run(ahuacatlModifySkipSuite); From 495e01b6ff19fdf1b24f24c3af33535bf4347218 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Thu, 5 Aug 2021 13:10:53 +0200 Subject: [PATCH 16/32] Minor change --- arangod/Aql/ModificationExecutor.cpp | 23 +++++++++++++---------- arangod/Aql/ModificationExecutor.h | 8 +++----- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/arangod/Aql/ModificationExecutor.cpp b/arangod/Aql/ModificationExecutor.cpp index 9861951e5078..e7ddee1659f1 100644 --- a/arangod/Aql/ModificationExecutor.cpp +++ b/arangod/Aql/ModificationExecutor.cpp @@ -140,7 +140,7 @@ void ModificationExecutor::doOutput(OutputAqlItemRow& template [[nodiscard]] auto ModificationExecutor::produceOrSkip( typename FetcherType::DataRange& input, IProduceOrSkipData& produceOrSkipData) - -> std::tuple { + -> std::tuple { TRI_IF_FAILURE("ModificationBlock::getSome") { THROW_ARANGO_EXCEPTION(TRI_ERROR_DEBUG); } @@ -155,8 +155,7 @@ template // input range in this state. if (!input.hasDataRow()) { // Input is empty - return {translateReturnType(input.upstreamState()), ModificationStats{}, - produceOrSkipData.getSkipCount(), upstreamCall}; + return {translateReturnType(input.upstreamState()), ModificationStats{}, upstreamCall}; } // MSVC doesn't see constexpr variables inside a lambda as constexpr, thus @@ -200,7 +199,7 @@ template if (modifierState == ExecutionState::WAITING) { TRI_ASSERT(!ServerState::instance()->isSingleServer()); - return {ExecutionState::WAITING, stats, produceOrSkipData.getSkipCount(), upstreamCall}; + return {ExecutionState::WAITING, stats, upstreamCall}; } TRI_ASSERT(_modifier->resultState() == ModificationExecutorResultState::HaveResult); @@ -216,8 +215,7 @@ template } } - return {translateReturnType(upstreamState), stats, - produceOrSkipData.getSkipCount(), upstreamCall}; + return {translateReturnType(upstreamState), stats, upstreamCall}; } template @@ -276,7 +274,7 @@ template }; auto produceData = ProduceData(output, _infos, *_modifier); - auto [state, stats, skipCount, upstreamCall] = produceOrSkip(input, produceData); + auto&& [state, stats, upstreamCall] = produceOrSkip(input, produceData); return {state, stats, upstreamCall}; } @@ -288,6 +286,7 @@ template struct SkipData final : public IProduceOrSkipData { AqlCall& _call; ModifierType& _modifier; + std::size_t _skipCount{}; SkipData(AqlCall& call, ModifierType& modifier) : _call(call), _modifier(modifier) {} @@ -300,13 +299,13 @@ template // If we are in offset phase and need to produce data // after the toSkip is limited to offset(). // otherwise we need to report everything we write - _call.didSkip(_modifier.nrOfWritesExecuted()); + _skipCount += _modifier.nrOfWritesExecuted(); } else { // If we do not need to report fullcount. // we cannot report more than offset // but also not more than the operations we // have successfully executed - _call.didSkip((std::min)(_call.getOffset(), _modifier.nrOfWritesExecuted())); + _skipCount += (std::min)(_call.getOffset(), _modifier.nrOfWritesExecuted()); } } auto getRemainingRows() -> std::size_t final { @@ -323,7 +322,11 @@ template }; auto skipData = SkipData(call, *_modifier); - return produceOrSkip(input, skipData); + auto&& [state, stats, upstreamCall] = produceOrSkip(input, skipData); + + call.didSkip(skipData._skipCount); + + return {state, stats, skipData._skipCount, upstreamCall}; } using NoPassthroughSingleRowFetcher = SingleRowFetcher; diff --git a/arangod/Aql/ModificationExecutor.h b/arangod/Aql/ModificationExecutor.h index e17a0005178a..26583ee3b3a2 100644 --- a/arangod/Aql/ModificationExecutor.h +++ b/arangod/Aql/ModificationExecutor.h @@ -199,13 +199,11 @@ class ModificationExecutor { virtual void doOutput() = 0; virtual auto getRemainingRows() -> std::size_t = 0; virtual auto needMoreOutput() -> bool = 0; - virtual auto getSkipCount() -> std::size_t = 0; + virtual auto getSkipCount() -> std::size_t = 0; // TODO remove }; - // template - std::tuple produceOrSkip( - typename FetcherType::DataRange& input, - IProduceOrSkipData& produceOrSkipData); + std::tuple produceOrSkip(typename FetcherType::DataRange& input, + IProduceOrSkipData& produceOrSkipData); transaction::Methods _trx; From 8a1834de325aa8d4b0ae31d676b534f22f015a13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Thu, 5 Aug 2021 15:11:16 +0200 Subject: [PATCH 17/32] Implemented waiting and state variables, should now work in cluster as well --- arangod/Aql/ModificationExecutor.cpp | 104 ++++++++++++--------------- arangod/Aql/ModificationExecutor.h | 4 +- 2 files changed, 46 insertions(+), 62 deletions(-) diff --git a/arangod/Aql/ModificationExecutor.cpp b/arangod/Aql/ModificationExecutor.cpp index e7ddee1659f1..aa3bb507907f 100644 --- a/arangod/Aql/ModificationExecutor.cpp +++ b/arangod/Aql/ModificationExecutor.cpp @@ -29,16 +29,9 @@ #include "Aql/OutputAqlItemRow.h" #include "Aql/QueryContext.h" #include "Aql/SingleRowFetcher.h" -#include "Basics/Common.h" -#include "Basics/VelocyPackHelper.h" -#include "Logger/LogMacros.h" #include "StorageEngine/TransactionState.h" -#include "VocBase/LogicalCollection.h" -#include "Aql/InsertModifier.h" -#include "Aql/RemoveModifier.h" #include "Aql/SimpleModifier.h" -#include "Aql/UpdateReplaceModifier.h" #include "Aql/UpsertModifier.h" #include @@ -102,41 +95,6 @@ ModificationExecutor::ModificationExecutor(Fetcher& f _infos(infos), _modifier(std::make_shared(infos)) {} -// Outputs accumulated results, and counts the statistics -template -void ModificationExecutor::doOutput(OutputAqlItemRow& output) { - typename ModifierType::OutputIterator modifierOutputIterator(*_modifier); - // We only accumulated as many items as we can output, so this - // should be correct - for (auto const& modifierOutput : modifierOutputIterator) { - TRI_ASSERT(!output.isFull()); - bool written = false; - switch (modifierOutput.getType()) { - case ModifierOutput::Type::ReturnIfRequired: - if (_infos._options.returnOld) { - output.cloneValueInto(_infos._outputOldRegisterId, modifierOutput.getInputRow(), - modifierOutput.getOldValue()); - written = true; - } - if (_infos._options.returnNew) { - output.cloneValueInto(_infos._outputNewRegisterId, modifierOutput.getInputRow(), - modifierOutput.getNewValue()); - written = true; - } - [[fallthrough]]; - case ModifierOutput::Type::CopyRow: - if (!written) { - output.copyRow(modifierOutput.getInputRow()); - } - output.advanceRow(); - break; - case ModifierOutput::Type::SkipRow: - // nothing. - break; - } - } -} - template [[nodiscard]] auto ModificationExecutor::produceOrSkip( typename FetcherType::DataRange& input, IProduceOrSkipData& produceOrSkipData) @@ -276,6 +234,16 @@ template auto produceData = ProduceData(output, _infos, *_modifier); auto&& [state, stats, upstreamCall] = produceOrSkip(input, produceData); + if (state == ExecutionState::WAITING) { + return {state, ModificationStats{}, upstreamCall}; + } + + auto skipCount = std::size_t{0}; + auto returnedStats = ModificationStats{}; + + std::swap(skipCount, _skipCount); + std::swap(returnedStats, _stats); + return {state, stats, upstreamCall}; } @@ -286,27 +254,29 @@ template struct SkipData final : public IProduceOrSkipData { AqlCall& _call; ModifierType& _modifier; - std::size_t _skipCount{}; SkipData(AqlCall& call, ModifierType& modifier) : _call(call), _modifier(modifier) {} ~SkipData() final = default; void doOutput() final { - if (_call.needsFullCount()) { - // If we need to do full count the nr of writes we did - // in this batch is always correct. - // If we are in offset phase and need to produce data - // after the toSkip is limited to offset(). - // otherwise we need to report everything we write - _skipCount += _modifier.nrOfWritesExecuted(); - } else { - // If we do not need to report fullcount. - // we cannot report more than offset - // but also not more than the operations we - // have successfully executed - _skipCount += (std::min)(_call.getOffset(), _modifier.nrOfWritesExecuted()); - } + auto const skipped = std::invoke([&] { + if (_call.needsFullCount()) { + // If we need to do full count the nr of writes we did + // in this batch is always correct. + // If we are in offset phase and need to produce data + // after the toSkip is limited to offset(). + // otherwise we need to report everything we write + return _modifier.nrOfWritesExecuted(); + } else { + // If we do not need to report fullcount. + // we cannot report more than offset + // but also not more than the operations we + // have successfully executed + return (std::min)(_call.getOffset(), _modifier.nrOfWritesExecuted()); + } + }); + _call.didSkip(skipped); } auto getRemainingRows() -> std::size_t final { if (_call.getLimit() == 0 && _call.hasHardLimit()) { @@ -321,12 +291,26 @@ template auto getSkipCount() -> std::size_t final { return _call.getSkipCount(); } }; + TRI_ASSERT(call.getSkipCount() == 0); + // "move" the saved skip count into the call + call.didSkip(_skipCount); + _skipCount = 0; + auto skipData = SkipData(call, *_modifier); - auto&& [state, stats, upstreamCall] = produceOrSkip(input, skipData); + auto&& [state, localStats, upstreamCall] = produceOrSkip(input, skipData); + + _stats += localStats; - call.didSkip(skipData._skipCount); + if (state == ExecutionState::WAITING) { + // save the skip count until the next call + _skipCount = call.getSkipCount(); + return {state, ModificationStats{}, 0, upstreamCall}; + } + + auto stats = ModificationStats{}; + std::swap(stats, _stats); - return {state, stats, skipData._skipCount, upstreamCall}; + return {state, stats, call.getSkipCount(), upstreamCall}; } using NoPassthroughSingleRowFetcher = SingleRowFetcher; diff --git a/arangod/Aql/ModificationExecutor.h b/arangod/Aql/ModificationExecutor.h index 26583ee3b3a2..88ba8b77de30 100644 --- a/arangod/Aql/ModificationExecutor.h +++ b/arangod/Aql/ModificationExecutor.h @@ -192,8 +192,6 @@ class ModificationExecutor { -> std::tuple; protected: - void doOutput(OutputAqlItemRow& output); - struct IProduceOrSkipData { virtual ~IProduceOrSkipData() = default; virtual void doOutput() = 0; @@ -209,6 +207,8 @@ class ModificationExecutor { ModificationExecutorInfos& _infos; std::shared_ptr _modifier; + std::size_t _skipCount{}; + Stats _stats{}; }; } // namespace arangodb::aql From fbe539c5a7a795f536da5d86fcc8fb1727ece8d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Thu, 5 Aug 2021 15:27:08 +0200 Subject: [PATCH 18/32] Minor cleanup --- arangod/Aql/ModificationExecutor.cpp | 17 ++++++----------- arangod/Aql/ModificationExecutor.h | 10 +++++++--- 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/arangod/Aql/ModificationExecutor.cpp b/arangod/Aql/ModificationExecutor.cpp index aa3bb507907f..0fdd5a312d91 100644 --- a/arangod/Aql/ModificationExecutor.cpp +++ b/arangod/Aql/ModificationExecutor.cpp @@ -96,8 +96,9 @@ ModificationExecutor::ModificationExecutor(Fetcher& f _modifier(std::make_shared(infos)) {} template +template [[nodiscard]] auto ModificationExecutor::produceOrSkip( - typename FetcherType::DataRange& input, IProduceOrSkipData& produceOrSkipData) + typename FetcherType::DataRange& input, ProduceOrSkipData& produceOrSkipData) -> std::tuple { TRI_IF_FAILURE("ModificationBlock::getSome") { THROW_ARANGO_EXCEPTION(TRI_ERROR_DEBUG); @@ -122,8 +123,6 @@ template std::is_same; auto upstreamState = input.upstreamState(); - // TODO We need to save both stats and skip count in member variables that - // survive WAITING, and only return them on DONE. auto stats = ModificationStats{}; while (input.hasDataRow() && produceOrSkipData.needMoreOutput()) { @@ -134,7 +133,7 @@ template return input; } }); - auto const maxRows = produceOrSkipData.getRemainingRows(); + auto const maxRows = produceOrSkipData.maxOutputRows(); while (_modifier->nrOfOperations() < maxRows && range.hasDataRow()) { auto [state, row] = range.nextDataRow(AqlItemBlockInputRange::HasDataRow{}); @@ -224,11 +223,8 @@ template } } } - auto getRemainingRows() -> std::size_t final { - return _output.numRowsLeft(); - } + auto maxOutputRows() -> std::size_t final { return _output.numRowsLeft(); } auto needMoreOutput() -> bool final { return !_output.isFull(); } - auto getSkipCount() -> std::size_t override { return 0; } }; auto produceData = ProduceData(output, _infos, *_modifier); @@ -278,7 +274,7 @@ template }); _call.didSkip(skipped); } - auto getRemainingRows() -> std::size_t final { + auto maxOutputRows() -> std::size_t final { if (_call.getLimit() == 0 && _call.hasHardLimit()) { // We need to produce all modification operations. // If we are bound by limits or not! @@ -288,11 +284,10 @@ template } } auto needMoreOutput() -> bool final { return _call.needSkipMore(); } - auto getSkipCount() -> std::size_t final { return _call.getSkipCount(); } }; TRI_ASSERT(call.getSkipCount() == 0); - // "move" the saved skip count into the call + // "move" the previously saved skip count into the call call.didSkip(_skipCount); _skipCount = 0; diff --git a/arangod/Aql/ModificationExecutor.h b/arangod/Aql/ModificationExecutor.h index 88ba8b77de30..70158056c326 100644 --- a/arangod/Aql/ModificationExecutor.h +++ b/arangod/Aql/ModificationExecutor.h @@ -192,16 +192,20 @@ class ModificationExecutor { -> std::tuple; protected: + // Interface of the data structure that is passed by produceRows() and + // skipRowsRange() to produceOrSkip(), which does the actual work. + // The interface isn't technically necessary (as produceOrSkip is a template), + // but I kept it for the overview it provides. struct IProduceOrSkipData { virtual ~IProduceOrSkipData() = default; virtual void doOutput() = 0; - virtual auto getRemainingRows() -> std::size_t = 0; + virtual auto maxOutputRows() -> std::size_t = 0; virtual auto needMoreOutput() -> bool = 0; - virtual auto getSkipCount() -> std::size_t = 0; // TODO remove }; + template std::tuple produceOrSkip(typename FetcherType::DataRange& input, - IProduceOrSkipData& produceOrSkipData); + ProduceOrSkipData& produceOrSkipData); transaction::Methods _trx; From e2c867f695525ddefb71ece6d7745ed4b048a620 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Thu, 5 Aug 2021 16:42:36 +0200 Subject: [PATCH 19/32] ...emphasis on "should". Bugfixes. --- arangod/Aql/AqlItemBlockInputMatrix.cpp | 2 +- arangod/Aql/ModificationExecutor.cpp | 21 ++++++++++++--------- arangod/Aql/SimpleModifier.cpp | 15 +++++++++++++-- arangod/Aql/SimpleModifier.h | 3 ++- arangod/Aql/UpsertModifier.cpp | 10 ++++++++++ arangod/Aql/UpsertModifier.h | 1 + 6 files changed, 39 insertions(+), 13 deletions(-) diff --git a/arangod/Aql/AqlItemBlockInputMatrix.cpp b/arangod/Aql/AqlItemBlockInputMatrix.cpp index fdd350216bfc..1dbb3a8972a8 100644 --- a/arangod/Aql/AqlItemBlockInputMatrix.cpp +++ b/arangod/Aql/AqlItemBlockInputMatrix.cpp @@ -64,7 +64,7 @@ AqlItemBlockInputRange& AqlItemBlockInputMatrix::getInputRange() { if (_lastRange.hasDataRow()) { return _lastRange; } - // Need initialze lastRange + // Need initialize lastRange if (_aqlItemMatrix->numberOfBlocks() == 0) { _lastRange = {AqlItemBlockInputRange{upstreamState()}}; } else { diff --git a/arangod/Aql/ModificationExecutor.cpp b/arangod/Aql/ModificationExecutor.cpp index 0fdd5a312d91..acce4a1c950e 100644 --- a/arangod/Aql/ModificationExecutor.cpp +++ b/arangod/Aql/ModificationExecutor.cpp @@ -110,9 +110,13 @@ template upstreamCall.softLimit = _modifier->getBatchSize(); } + auto const hasInputOrPreviousResult = [&] { + return input.hasDataRow() || _modifier->operationPending(); + }; + // Make AqlItemBlockInputMatrix happy, which dislikes being asked for its - // input range in this state. - if (!input.hasDataRow()) { + // input range when empty. + if (!hasInputOrPreviousResult()) { // Input is empty return {translateReturnType(input.upstreamState()), ModificationStats{}, upstreamCall}; } @@ -125,7 +129,7 @@ template auto upstreamState = input.upstreamState(); auto stats = ModificationStats{}; - while (input.hasDataRow() && produceOrSkipData.needMoreOutput()) { + while (hasInputOrPreviousResult() && produceOrSkipData.needMoreOutput()) { auto& range = std::invoke([&]() -> AqlItemBlockInputRange& { if constexpr (inputIsMatrix::value) { return input.getInputRange(); @@ -228,17 +232,16 @@ template }; auto produceData = ProduceData(output, _infos, *_modifier); - auto&& [state, stats, upstreamCall] = produceOrSkip(input, produceData); + auto&& [state, localStats, upstreamCall] = produceOrSkip(input, produceData); + + _stats += localStats; if (state == ExecutionState::WAITING) { return {state, ModificationStats{}, upstreamCall}; } - auto skipCount = std::size_t{0}; - auto returnedStats = ModificationStats{}; - - std::swap(skipCount, _skipCount); - std::swap(returnedStats, _stats); + auto stats = ModificationStats{}; + std::swap(stats, _stats); return {state, stats, upstreamCall}; } diff --git a/arangod/Aql/SimpleModifier.cpp b/arangod/Aql/SimpleModifier.cpp index e8ce3fc45149..7d524d46adf8 100644 --- a/arangod/Aql/SimpleModifier.cpp +++ b/arangod/Aql/SimpleModifier.cpp @@ -31,8 +31,8 @@ #include "Aql/OutputAqlItemRow.h" #include "Aql/SharedQueryState.h" #include "Basics/Common.h" -#include "Basics/VelocyPackHelper.h" #include "Basics/StaticStrings.h" +#include "Basics/VelocyPackHelper.h" #include "Cluster/ServerState.h" #include "VocBase/LogicalCollection.h" @@ -126,7 +126,18 @@ SimpleModifier::OutputIterator::end() const { template ModificationExecutorResultState SimpleModifier::resultState() const noexcept { std::lock_guard guard(_resultStateMutex); - return _resultState; + return _resultState; +} + +template +bool SimpleModifier::operationPending() const noexcept { + switch (resultState()) { + case ModificationExecutorResultState::NoResult: + return false; + case ModificationExecutorResultState::WaitingForResult: + case ModificationExecutorResultState::HaveResult: + return true; + } } template diff --git a/arangod/Aql/SimpleModifier.h b/arangod/Aql/SimpleModifier.h index 121c63633072..1aa5e528df70 100644 --- a/arangod/Aql/SimpleModifier.h +++ b/arangod/Aql/SimpleModifier.h @@ -121,7 +121,8 @@ class SimpleModifier : public std::enable_shared_from_this Date: Thu, 5 Aug 2021 18:08:56 +0200 Subject: [PATCH 20/32] Save the execution stack in ExecutionBlockImpl --- arangod/Aql/AqlCall.h | 2 +- arangod/Aql/ExecutionBlockImpl.cpp | 33 +++++++++++++++++++ ...l-optimizer-rule-move-calculations-down.js | 13 +++++--- 3 files changed, 43 insertions(+), 5 deletions(-) diff --git a/arangod/Aql/AqlCall.h b/arangod/Aql/AqlCall.h index bf8ad1d855e5..92550be37847 100644 --- a/arangod/Aql/AqlCall.h +++ b/arangod/Aql/AqlCall.h @@ -192,7 +192,7 @@ struct AqlCall { TRI_ASSERT(n <= i); i -= n; }, - [](auto) {}, + [](Infinity) {}, }; std::visit(minus, softLimit); std::visit(minus, hardLimit); diff --git a/arangod/Aql/ExecutionBlockImpl.cpp b/arangod/Aql/ExecutionBlockImpl.cpp index 0d8e9c3f1f33..fd5a8183fab0 100644 --- a/arangod/Aql/ExecutionBlockImpl.cpp +++ b/arangod/Aql/ExecutionBlockImpl.cpp @@ -1348,6 +1348,24 @@ ExecutionBlockImpl::executeWithoutTrace(AqlCallStack stack) { stack = _stackBeforeWaiting; } + if constexpr (executorCanReturnWaiting) { + // If state is SKIP, PRODUCE or FASTFORWARD, we were WAITING. + // The call stack must be restored in all cases, but only SKIP needs to + // restore the clientCall. + switch (_execState) { + default: + break; + case ExecState::SKIP: + TRI_ASSERT(_clientRequest.requestLessDataThan(clientCall)); + clientCall = _clientRequest; + [[fallthrough]]; + case ExecState::PRODUCE: + case ExecState::FASTFORWARD: + TRI_ASSERT(_stackBeforeWaiting.requestLessDataThan(stack)); + stack = _stackBeforeWaiting; + } + } + auto returnToState = ExecState::CHECKCALL; LOG_QUERY("007ac", DEBUG) << "starting statemachine of executor " << printBlockInfo(); @@ -1407,6 +1425,12 @@ ExecutionBlockImpl::executeWithoutTrace(AqlCallStack stack) { _executor.skipRowsRange(_lastRange, clientCall); if (executorState == ExecutionState::WAITING) { + // We need to persist the old call before we return. + // We might have some local accounting to this call. + _clientRequest = clientCall; + // We might also have some local accounting in this stack. + _stackBeforeWaiting = stack; + // We do not return anything in WAITING state, also NOT skipped. TRI_ASSERT(skippedLocal == 0); return {executorState, SkipResult{}, nullptr}; } else if (executorState == ExecutionState::DONE) { @@ -1501,7 +1525,12 @@ ExecutionBlockImpl::executeWithoutTrace(AqlCallStack stack) { ExecutionState executorState = ExecutionState::HASMORE; std::tie(executorState, stats, call) = _executor.produceRows(_lastRange, *_outputItemRow); + if (executorState == ExecutionState::WAITING) { + // We need to persist the old stack before we return. + // We might have some local accounting in this stack. + _stackBeforeWaiting = stack; + // We do not return anything in WAITING state, also NOT skipped. return {executorState, SkipResult{}, nullptr}; } else if (executorState == ExecutionState::DONE) { state = ExecutorState::DONE; @@ -1593,6 +1622,10 @@ ExecutionBlockImpl::executeWithoutTrace(AqlCallStack stack) { std::tie(executorState, stats, skippedLocal, call) = _executor.skipRowsRange(_lastRange, dummy); if (executorState == ExecutionState::WAITING) { + // We need to persist the old stack before we return. + // We might have some local accounting in this stack. + _stackBeforeWaiting = stack; + // We do not return anything in WAITING state, also NOT skipped. TRI_ASSERT(skippedLocal == 0); return {executorState, SkipResult{}, nullptr}; } else if (executorState == ExecutionState::DONE) { diff --git a/tests/js/server/aql/aql-optimizer-rule-move-calculations-down.js b/tests/js/server/aql/aql-optimizer-rule-move-calculations-down.js index c3331f5aa3ee..70d3c56776f5 100644 --- a/tests/js/server/aql/aql-optimizer-rule-move-calculations-down.js +++ b/tests/js/server/aql/aql-optimizer-rule-move-calculations-down.js @@ -664,10 +664,15 @@ function optimizerRuleTestSuite () { expected.push("test" + i + "-" + i); } - var query = - "FOR i IN 0..99 LET result = (UPDATE {_key: CONCAT('test', TO_STRING(i))} WITH {updated: true} IN " + - cn + - " RETURN CONCAT(NEW._key, '-', NEW.value)) LIMIT 10 RETURN result[0]"; + var query = ` + FOR i IN 0..99 + LET result = ( + UPDATE {_key: CONCAT('test', TO_STRING(i))} WITH {updated: true} IN ${cn} + RETURN CONCAT(NEW._key, '-', NEW.value) + ) + LIMIT 10 + RETURN result[0] + `; var planDisabled = AQL_EXPLAIN(query, {}, paramDisabled); var planEnabled = AQL_EXPLAIN(query, {}, paramEnabled); From 611c517778b380fe9cb2114afa7b0976deb9aed6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Thu, 5 Aug 2021 18:43:19 +0200 Subject: [PATCH 21/32] Added a TODO note --- arangod/Aql/ModificationExecutor.cpp | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/arangod/Aql/ModificationExecutor.cpp b/arangod/Aql/ModificationExecutor.cpp index acce4a1c950e..79e872d12f86 100644 --- a/arangod/Aql/ModificationExecutor.cpp +++ b/arangod/Aql/ModificationExecutor.cpp @@ -129,6 +129,14 @@ template auto upstreamState = input.upstreamState(); auto stats = ModificationStats{}; + // TODO Change the following logic as follows. + // First, read input and fill the modifier as much as possible. Only if either + // the output is satisfied (offset/output block aka maxOutputRows()), or the + // input is completely consumed (i.e. upstream DONE, not just the current + // range empty). + // Only after that, call transact(), i.e. execute the + // `if (_modifier->nrOfOperations() > 0) {` - block. + while (hasInputOrPreviousResult() && produceOrSkipData.needMoreOutput()) { auto& range = std::invoke([&]() -> AqlItemBlockInputRange& { if constexpr (inputIsMatrix::value) { @@ -147,14 +155,6 @@ template upstreamState = range.upstreamState(); - if constexpr (inputIsMatrix::value) { - if (upstreamState == ExecutorState::DONE) { - // We are done with this input. - // We need to forward it to the last ShadowRow. - input.skipAllRemainingDataRows(); - } - } - if (_modifier->nrOfOperations() > 0) { ExecutionState modifierState = _modifier->transact(_trx); @@ -176,6 +176,14 @@ template } } + if constexpr (inputIsMatrix::value) { + if (upstreamState == ExecutorState::DONE) { + // We are done with this input. + // We need to forward it to the last ShadowRow. + input.skipAllRemainingDataRows(); + } + } + return {translateReturnType(upstreamState), stats, upstreamCall}; } From 3426ccc21165771446b1dde66a322fd1edc98560 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Mon, 9 Aug 2021 18:19:50 +0200 Subject: [PATCH 22/32] Changed the ModificationExecutor to read as much input as possible before writing; some refactoring --- arangod/Aql/AqlItemBlock.cpp | 2 +- arangod/Aql/AqlItemBlock.h | 2 +- arangod/Aql/AqlItemBlockInputMatrix.cpp | 3 +- arangod/Aql/AqlItemBlockInputMatrix.h | 8 +- arangod/Aql/AqlItemMatrix.cpp | 91 +++++++++++ arangod/Aql/AqlItemMatrix.h | 58 ++++++- arangod/Aql/ModificationExecutor.cpp | 192 ++++++++++++++++-------- arangod/Aql/ModificationExecutor.h | 31 +++- arangod/Aql/SimpleModifier.cpp | 5 + arangod/Aql/UpsertModifier.cpp | 7 +- 10 files changed, 319 insertions(+), 80 deletions(-) diff --git a/arangod/Aql/AqlItemBlock.cpp b/arangod/Aql/AqlItemBlock.cpp index 0206dd723c94..3146f28d2ad0 100644 --- a/arangod/Aql/AqlItemBlock.cpp +++ b/arangod/Aql/AqlItemBlock.cpp @@ -1169,7 +1169,7 @@ size_t AqlItemBlock::maxModifiedEntries() const noexcept { return _numRegisters size_t AqlItemBlock::capacity() const noexcept { return _data.capacity(); } -bool AqlItemBlock::isShadowRow(size_t row) const { +bool AqlItemBlock::isShadowRow(size_t row) const noexcept { return _shadowRows.is(row); } diff --git a/arangod/Aql/AqlItemBlock.h b/arangod/Aql/AqlItemBlock.h index 5250cb02414b..8d00153fad32 100644 --- a/arangod/Aql/AqlItemBlock.h +++ b/arangod/Aql/AqlItemBlock.h @@ -296,7 +296,7 @@ class AqlItemBlock { /// @brief test if the given row is a shadow row and conveys subquery /// information only. It should not be handed to any non-subquery executor. - bool isShadowRow(size_t row) const; + bool isShadowRow(size_t row) const noexcept; /// @brief get the ShadowRowDepth /// Does only work if this row is a shadow row diff --git a/arangod/Aql/AqlItemBlockInputMatrix.cpp b/arangod/Aql/AqlItemBlockInputMatrix.cpp index 1dbb3a8972a8..dd7939a18883 100644 --- a/arangod/Aql/AqlItemBlockInputMatrix.cpp +++ b/arangod/Aql/AqlItemBlockInputMatrix.cpp @@ -83,7 +83,6 @@ SharedAqlItemBlockPtr AqlItemBlockInputMatrix::getBlock() const noexcept { std::pair AqlItemBlockInputMatrix::getMatrix() noexcept { TRI_ASSERT(_aqlItemMatrix != nullptr); TRI_ASSERT(_block == nullptr); - TRI_ASSERT(!_shadowRow.isInitialized()); // We are always done. This InputMatrix // guarantees that we have all data in our hand at once. @@ -109,7 +108,7 @@ bool AqlItemBlockInputMatrix::hasValidRow() const noexcept { bool AqlItemBlockInputMatrix::hasDataRow() const noexcept { return _aqlItemMatrix != nullptr && !hasShadowRow() && - ((_aqlItemMatrix->stoppedOnShadowRow()) || + (_aqlItemMatrix->stoppedOnShadowRow() || (_aqlItemMatrix->size() > 0 && _finalState == ExecutorState::DONE)); } diff --git a/arangod/Aql/AqlItemBlockInputMatrix.h b/arangod/Aql/AqlItemBlockInputMatrix.h index 4e4f7adf4c71..4586187e6181 100644 --- a/arangod/Aql/AqlItemBlockInputMatrix.h +++ b/arangod/Aql/AqlItemBlockInputMatrix.h @@ -21,8 +21,7 @@ /// @author Tobias Gödderz //////////////////////////////////////////////////////////////////////////////// -#ifndef ARANGOD_AQL_AQLITEMBLOCKMATRIXITERATOR_H -#define ARANGOD_AQL_AQLITEMBLOCKMATRIXITERATOR_H +#pragma once #include "Aql/AqlItemBlockInputRange.h" #include "Aql/AqlItemMatrix.h" @@ -38,7 +37,7 @@ class AqlItemBlockInputMatrix { public: explicit AqlItemBlockInputMatrix(ExecutorState state); - AqlItemBlockInputMatrix(arangodb::aql::SharedAqlItemBlockPtr); + explicit AqlItemBlockInputMatrix(arangodb::aql::SharedAqlItemBlockPtr); AqlItemBlockInputMatrix(ExecutorState state, AqlItemMatrix* aqlItemMatrix); @@ -63,7 +62,6 @@ class AqlItemBlockInputMatrix { size_t skipAllShadowRowsOfDepth(size_t depth); - // Will return HASMORE if we were able to increase the row index. // Otherwise will return DONE. ExecutorState incrBlockIndex(); @@ -101,5 +99,3 @@ class AqlItemBlockInputMatrix { }; } // namespace arangodb::aql - -#endif // ARANGOD_AQL_AQLITEMBLOCKINPUTITERATOR_H diff --git a/arangod/Aql/AqlItemMatrix.cpp b/arangod/Aql/AqlItemMatrix.cpp index f5d37ad4444e..503e0d6ffefd 100644 --- a/arangod/Aql/AqlItemMatrix.cpp +++ b/arangod/Aql/AqlItemMatrix.cpp @@ -245,3 +245,94 @@ AqlItemMatrix::AqlItemMatrix(RegisterCount nrRegs) clear(); return {skipped, ShadowAqlItemRow{CreateInvalidShadowRowHint()}}; } + +AqlItemMatrix::RowIterator AqlItemMatrix::begin() const { + if (size() > 0) { + return {this, 0, _startIndexInFirstBlock}; + } else { + return end(); + } +} + +AqlItemMatrix::RowIterator AqlItemMatrix::end() const { + return {this, this->numberOfBlocks(), 0}; +} + +AqlItemMatrix::RowIterator::RowIterator(AqlItemMatrix const* matrix, size_t blockIndex, size_t rowIndex) + : _matrix(matrix), _blockIndex(blockIndex), _rowIndex(rowIndex) {} + +AqlItemMatrix::RowIterator::value_type AqlItemMatrix::RowIterator::next() noexcept { + auto& it = *this; + auto ret = *it; + ++it; + return ret; +} + +auto AqlItemMatrix::RowIterator::isInitialized() const noexcept -> bool { + return _matrix != nullptr; +} + +auto AqlItemMatrix::RowIterator::hasMore() const noexcept -> bool { + // _blockIndex == _matrix->size() => _rowIndex == 0 + TRI_ASSERT((_matrix != nullptr && _blockIndex < _matrix->numberOfBlocks()) || _rowIndex == 0); + // If _blockIndex is valid, _rowIndex must be, too. + return ADB_LIKELY(_matrix != nullptr) && _blockIndex < _matrix->numberOfBlocks(); +} + +AqlItemMatrix::RowIterator::value_type AqlItemMatrix::RowIterator::operator*() const noexcept { + return {_matrix->getBlock(_blockIndex).first, _rowIndex}; +} + +AqlItemMatrix::RowIterator& AqlItemMatrix::RowIterator::operator++() noexcept { + // Assume ++ is only called on a valid and dereferenceable iterator + TRI_ASSERT(_matrix != nullptr); + TRI_ASSERT(_blockIndex < _matrix->numberOfBlocks()); + auto const* block = _matrix->getBlockRef(_blockIndex).first; + TRI_ASSERT(_rowIndex < block->numRows()); + TRI_ASSERT(!block->isShadowRow(_rowIndex)); + + // Increase the row index + ++_rowIndex; + if (_rowIndex >= block->numRows()) { + // If the row index is invalid, move to the next block. + // If the block index is now invalid, this is equal to the "end()" + // iterator. + ++_blockIndex; + _rowIndex = 0; + } + + if (_blockIndex < _matrix->numberOfBlocks()) { + block = _matrix->getBlockRef(_blockIndex).first; + if (block->isShadowRow(_rowIndex)) { + // If we're at a shadow row, this must be the last block. + TRI_ASSERT(_blockIndex + 1 == _matrix->numberOfBlocks()); + // This makes this equal to the "end()" iterator. + ++_blockIndex; + _rowIndex = 0; + } + } + + return *this; +} + +auto AqlItemMatrix::RowIterator::operator++(int) & noexcept -> AqlItemMatrix::RowIterator { + auto tmp = *this; + ++(*this); + return tmp; +} + +AqlItemMatrix::RowIterator::operator bool() const noexcept { + return hasMore(); +} + +bool aql::operator==(AqlItemMatrix::RowIterator const& a, + AqlItemMatrix::RowIterator const& b) { + return ADB_LIKELY(a._matrix == b._matrix) && + (ADB_UNLIKELY(a._matrix == nullptr /* => b._matrix == nullptr */) || + (ADB_LIKELY(a._blockIndex == b._blockIndex) && a._rowIndex == b._rowIndex)); +} + +bool aql::operator!=(AqlItemMatrix::RowIterator const& a, + AqlItemMatrix::RowIterator const& b) { + return !(a == b); +} diff --git a/arangod/Aql/AqlItemMatrix.h b/arangod/Aql/AqlItemMatrix.h index 3a982d908695..3f2aa06fdd86 100644 --- a/arangod/Aql/AqlItemMatrix.h +++ b/arangod/Aql/AqlItemMatrix.h @@ -21,8 +21,7 @@ /// @author Michael Hackstein //////////////////////////////////////////////////////////////////////////////// -#ifndef ARANGOD_AQL_AQL_ITEM_MATRIX_H -#define ARANGOD_AQL_AQL_ITEM_MATRIX_H 1 +#pragma once #include "Aql/ShadowAqlItemRow.h" @@ -30,8 +29,7 @@ #include #include -namespace arangodb { -namespace aql { +namespace arangodb::aql { class InputAqlItemRow; class SharedAqlItemBlockPtr; @@ -120,6 +118,53 @@ class AqlItemMatrix { [[nodiscard]] auto skipAllShadowRowsOfDepth(size_t depth) -> std::tuple; + class RowIterator { + public: + //using iterator_category = std::forward_iterator_tag; + // using difference_type = std::ptrdiff_t; + using value_type = InputAqlItemRow; + //using pointer = value_type*; + //using reference = value_type&; + + RowIterator() = default; + RowIterator(AqlItemMatrix const* matrix, size_t blockIndex, size_t rowIndex); + + // Returns the current value, and move the iterator to the next value + value_type next() noexcept; + + auto isInitialized() const noexcept -> bool; + + // Returns whether the current value is valid, i.e. whether next() may be + // called + auto hasMore() const noexcept -> bool; + + value_type operator*() const noexcept; + + // This can't be implemented, as we can only create the InputAqlItemRow + // on-the-fly. + // pointer operator->(); + + // Prefix increment + RowIterator& operator++() noexcept; + + // Postfix increment. + auto operator++(int) & noexcept -> RowIterator; + + explicit operator bool() const noexcept; + + friend bool operator==(RowIterator const& a, RowIterator const& b); + friend bool operator!=(RowIterator const& a, RowIterator const& b); + + private: + AqlItemMatrix const* _matrix{}; + std::size_t _blockIndex{}; + // Invariant: _rowIndex is valid iff _blockIndex is valid. + std::size_t _rowIndex{}; + }; + + [[nodiscard]] RowIterator begin() const; + [[nodiscard]] RowIterator end() const; + private: std::vector _blocks; @@ -130,7 +175,4 @@ class AqlItemMatrix { size_t _stopIndexInLastBlock; }; -} // namespace aql -} // namespace arangodb - -#endif +} // namespace arangodb::aql diff --git a/arangod/Aql/ModificationExecutor.cpp b/arangod/Aql/ModificationExecutor.cpp index 79e872d12f86..90f5995aa97e 100644 --- a/arangod/Aql/ModificationExecutor.cpp +++ b/arangod/Aql/ModificationExecutor.cpp @@ -110,81 +110,70 @@ template upstreamCall.softLimit = _modifier->getBatchSize(); } - auto const hasInputOrPreviousResult = [&] { - return input.hasDataRow() || _modifier->operationPending(); - }; - - // Make AqlItemBlockInputMatrix happy, which dislikes being asked for its - // input range when empty. - if (!hasInputOrPreviousResult()) { - // Input is empty - return {translateReturnType(input.upstreamState()), ModificationStats{}, upstreamCall}; + // Try to initialize the RangeHandler + if (!_rangeHandler.init(input)) { + return {translateReturnType(_rangeHandler.upstreamState(input)), + ModificationStats{}, upstreamCall}; } - // MSVC doesn't see constexpr variables inside a lambda as constexpr, thus - // we can't just assign is_same_t to a variable. - using inputIsMatrix = - std::is_same; - - auto upstreamState = input.upstreamState(); auto stats = ModificationStats{}; - // TODO Change the following logic as follows. - // First, read input and fill the modifier as much as possible. Only if either - // the output is satisfied (offset/output block aka maxOutputRows()), or the - // input is completely consumed (i.e. upstream DONE, not just the current - // range empty). - // Only after that, call transact(), i.e. execute the - // `if (_modifier->nrOfOperations() > 0) {` - block. - - while (hasInputOrPreviousResult() && produceOrSkipData.needMoreOutput()) { - auto& range = std::invoke([&]() -> AqlItemBlockInputRange& { - if constexpr (inputIsMatrix::value) { - return input.getInputRange(); - } else { - return input; - } - }); - auto const maxRows = produceOrSkipData.maxOutputRows(); - while (_modifier->nrOfOperations() < maxRows && range.hasDataRow()) { - auto [state, row] = range.nextDataRow(AqlItemBlockInputRange::HasDataRow{}); - - TRI_ASSERT(row.isInitialized()); - _modifier->accumulate(row); + auto const maxRows = std::invoke([&] { + if constexpr (std::is_same_v && + !std::is_same_v) { + return std::min(produceOrSkipData.maxOutputRows(), _modifier->getBatchSize()); + } else { + return produceOrSkipData.maxOutputRows(); } + }); - upstreamState = range.upstreamState(); + // Read as much input as possible + while (_rangeHandler.hasDataRow(input) && _modifier->nrOfOperations() < maxRows) { + auto row = _rangeHandler.nextDataRow(input); - if (_modifier->nrOfOperations() > 0) { - ExecutionState modifierState = _modifier->transact(_trx); + TRI_ASSERT(row.isInitialized()); + _modifier->accumulate(row); + } - if (modifierState == ExecutionState::WAITING) { - TRI_ASSERT(!ServerState::instance()->isSingleServer()); - return {ExecutionState::WAITING, stats, upstreamCall}; - } + bool const inputDone = _rangeHandler.upstreamState(input) == ExecutorState::DONE; + bool const enoughOutputAvailable = maxRows == _modifier->nrOfOperations(); + bool const hasAtLeastOneModification = _modifier->nrOfOperations() > 0; - TRI_ASSERT(_modifier->resultState() == ModificationExecutorResultState::HaveResult); + // outputFull => enoughOutputAvailable + TRI_ASSERT(produceOrSkipData.needMoreOutput() || enoughOutputAvailable); - _modifier->checkException(); - if (_infos._doCount) { - stats.addWritesExecuted(_modifier->nrOfWritesExecuted()); - stats.addWritesIgnored(_modifier->nrOfWritesIgnored()); - } + if (!inputDone && !enoughOutputAvailable) { + // This case handles when there's still work to do, but no input in the + // current range. + TRI_ASSERT(!_rangeHandler.hasDataRow(input)); - produceOrSkipData.doOutput(); - _modifier->reset(); - } + return {ExecutionState::HASMORE, stats, upstreamCall}; } - if constexpr (inputIsMatrix::value) { - if (upstreamState == ExecutorState::DONE) { - // We are done with this input. - // We need to forward it to the last ShadowRow. - input.skipAllRemainingDataRows(); + if (hasAtLeastOneModification) { + TRI_ASSERT(inputDone || enoughOutputAvailable); + ExecutionState modifierState = _modifier->transact(_trx); + + if (modifierState == ExecutionState::WAITING) { + TRI_ASSERT(!ServerState::instance()->isSingleServer()); + return {ExecutionState::WAITING, stats, upstreamCall}; + } + + TRI_ASSERT(_modifier->resultState() == ModificationExecutorResultState::HaveResult); + + _modifier->checkException(); + if (_infos._doCount) { + stats.addWritesExecuted(_modifier->nrOfWritesExecuted()); + stats.addWritesIgnored(_modifier->nrOfWritesIgnored()); } + + produceOrSkipData.doOutput(); + _modifier->reset(); } - return {translateReturnType(upstreamState), stats, upstreamCall}; + TRI_ASSERT(_modifier->resultState() == ModificationExecutorResultState::NoResult); + + return {translateReturnType(_rangeHandler.upstreamState(input)), stats, upstreamCall}; } template @@ -319,6 +308,91 @@ template return {state, stats, call.getSkipCount(), upstreamCall}; } +template +auto ModificationExecutor::RangeHandler::nextDataRow( + typename FetcherType::DataRange& input) -> arangodb::aql::InputAqlItemRow { + if constexpr (inputIsMatrix) { + // Assume init() was called at least once + TRI_ASSERT(_iterator.isInitialized()); + // Assume hasDataRow() is true (caller has to make sure) + TRI_ASSERT(hasDataRow(input)); + + auto ret = _iterator.next(); + + if (!_iterator) { + // We are done with this input. + // We need to forward it to the last ShadowRow. + // RangeHandler::hasDataRow() will continue to return false, even if + // the AqlItemBlockInputMatrix is now already in the next range. + input.skipAllRemainingDataRows(); + } + + return ret; + } else { + return input.nextDataRow(AqlItemBlockInputRange::HasDataRow{}).second; + } +} + +template +auto ModificationExecutor::RangeHandler::hasDataRow( + typename FetcherType::DataRange& input) const noexcept -> bool { + if constexpr (inputIsMatrix) { + // Assume init() was called at least once + TRI_ASSERT(_iterator.isInitialized()); + + return _iterator.hasMore(); + } else { + return input.hasDataRow(); + } +} + +template +bool ModificationExecutor::RangeHandler::init( + typename FetcherType::DataRange& input) { + if constexpr (inputIsMatrix) { + if (!_iterator.isInitialized()) { + if (input.hasValidRow()) { + auto&& [state, matrix] = input.getMatrix(); + TRI_ASSERT(state == ExecutorState::DONE); + _iterator = matrix->begin(); + if (!_iterator) { + input.skipAllRemainingDataRows(); + } + // initialized successfully + return true; + } else { + // can't initialize yet, no matrix + return false; + } + } else { + // already initialized + return true; + } + } else { + // no-op + return true; + } +} + +template +auto ModificationExecutor::RangeHandler::upstreamState( + typename FetcherType::DataRange& input) const noexcept -> ExecutorState { + if constexpr (inputIsMatrix) { + if (ADB_UNLIKELY(!_iterator.isInitialized())) { + // As long as the iterator isn't initialized, we need to pass the upstream + // state. In particular if the input is completely empty, we need to + // return DONE. + return input.upstreamState(); + } else if (hasDataRow(input)) { + return ExecutorState::HASMORE; + } else { + return ExecutorState::DONE; + } + } else { + return input.upstreamState(); + } +} + using NoPassthroughSingleRowFetcher = SingleRowFetcher; template class ::arangodb::aql::ModificationExecutor; diff --git a/arangod/Aql/ModificationExecutor.h b/arangod/Aql/ModificationExecutor.h index 70158056c326..093a94e77f33 100644 --- a/arangod/Aql/ModificationExecutor.h +++ b/arangod/Aql/ModificationExecutor.h @@ -28,6 +28,7 @@ #include "Aql/ModificationExecutorInfos.h" #include "Aql/OutputAqlItemRow.h" #include "Aql/Stats.h" +#include "AqlItemMatrix.h" #include "Transaction/Methods.h" #include "Utils/OperationResult.h" @@ -43,6 +44,7 @@ namespace arangodb::aql { struct AqlCall; class AqlItemBlockInputRange; +class AqlItemBlockInputMatrix; class InputAqlItemRow; class OutputAqlItemRow; class RegisterInfos; @@ -162,7 +164,7 @@ enum class ModificationExecutorResultState { NoResult, // State that is used when the Executor's modifier has been asked // to produce a result, but it returned a WAITING status, i.e. the - // result is not yet ready to consume. + // result is not yet ready to consume. // This state cannot happen in single servers! WaitingForResult, // State that is used when the Executor's modifier has produced @@ -211,6 +213,33 @@ class ModificationExecutor { ModificationExecutorInfos& _infos; std::shared_ptr _modifier; + + // Used to unify input consumption of both AqlItemBlockInputRange and + // AqlItemBlockInputMatrix. + struct RangeHandler { + constexpr static bool inputIsMatrix = + std::is_same_v; + + // init must be called at least once with any input. It's a no-op for + // AqlItemBlockInputRange, and initializes an iterator for a + // AqlItemBlockInputMatrix. + // Returns false if the input isn't yet in a state in which it can be used + // for initialization (i.e. the matrix isn't there yet). + bool init(typename FetcherType::DataRange& input); + + // Returns true iff we may currently call nextDataRow() on this input; + // might return true on the next input. + [[nodiscard]] auto hasDataRow(typename FetcherType::DataRange& input) const noexcept + -> bool; + // Returns the next data row and moves the internal iterator + [[nodiscard]] auto nextDataRow(typename FetcherType::DataRange& input) + -> arangodb::aql::InputAqlItemRow; + + auto upstreamState(typename FetcherType::DataRange& input) const noexcept -> ExecutorState; + + AqlItemMatrix::RowIterator _iterator{}; + } _rangeHandler{}; + std::size_t _skipCount{}; Stats _stats{}; }; diff --git a/arangod/Aql/SimpleModifier.cpp b/arangod/Aql/SimpleModifier.cpp index 7d524d46adf8..16e1673f120e 100644 --- a/arangod/Aql/SimpleModifier.cpp +++ b/arangod/Aql/SimpleModifier.cpp @@ -33,6 +33,7 @@ #include "Basics/Common.h" #include "Basics/StaticStrings.h" #include "Basics/VelocyPackHelper.h" +#include "Basics/application-exit.h" #include "Cluster/ServerState.h" #include "VocBase/LogicalCollection.h" @@ -138,6 +139,10 @@ bool SimpleModifier::operationPending() const noexce case ModificationExecutorResultState::HaveResult: return true; } + LOG_TOPIC("710c4", FATAL, Logger::AQL) + << "Invalid or unhandled value for ModificationExecutorResultState: " + << static_cast>(resultState()); + FATAL_ERROR_ABORT(); } template diff --git a/arangod/Aql/UpsertModifier.cpp b/arangod/Aql/UpsertModifier.cpp index 94463d422c8b..8ece2376ae87 100644 --- a/arangod/Aql/UpsertModifier.cpp +++ b/arangod/Aql/UpsertModifier.cpp @@ -33,6 +33,7 @@ #include "Basics/Common.h" #include "Basics/StaticStrings.h" #include "Basics/VelocyPackHelper.h" +#include "Basics/application-exit.h" #include "Transaction/Methods.h" #include "VocBase/LogicalCollection.h" @@ -41,8 +42,6 @@ #include "Logger/LogMacros.h" -class CollectionNameResolver; - using namespace arangodb; using namespace arangodb::aql; using namespace arangodb::aql::ModificationExecutorHelpers; @@ -143,6 +142,10 @@ bool UpsertModifier::operationPending() const noexcept { case ModificationExecutorResultState::HaveResult: return true; } + LOG_TOPIC("ef67c", FATAL, Logger::AQL) + << "Invalid or unhandled value for ModificationExecutorResultState: " + << static_cast>(resultState()); + FATAL_ERROR_ABORT(); } void UpsertModifier::reset() { From ecba60e80f48b82b8e20b91b1a19f6e5ec9392f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Tue, 10 Aug 2021 09:01:38 +0200 Subject: [PATCH 23/32] Declared operators to make gcc happy --- arangod/Aql/AqlItemMatrix.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arangod/Aql/AqlItemMatrix.h b/arangod/Aql/AqlItemMatrix.h index 3f2aa06fdd86..dd90735dbcb0 100644 --- a/arangod/Aql/AqlItemMatrix.h +++ b/arangod/Aql/AqlItemMatrix.h @@ -175,4 +175,7 @@ class AqlItemMatrix { size_t _stopIndexInLastBlock; }; +bool operator==(AqlItemMatrix::RowIterator const& a, AqlItemMatrix::RowIterator const& b); +bool operator!=(AqlItemMatrix::RowIterator const& a, AqlItemMatrix::RowIterator const& b); + } // namespace arangodb::aql From 6c86555b82b799b471b8066b4440c1b436cb7e99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Tue, 10 Aug 2021 15:28:18 +0200 Subject: [PATCH 24/32] Update arangod/Aql/AqlItemMatrix.h Co-authored-by: Michael Hackstein --- arangod/Aql/AqlItemMatrix.h | 4 ---- 1 file changed, 4 deletions(-) diff --git a/arangod/Aql/AqlItemMatrix.h b/arangod/Aql/AqlItemMatrix.h index dd90735dbcb0..3644c543ea31 100644 --- a/arangod/Aql/AqlItemMatrix.h +++ b/arangod/Aql/AqlItemMatrix.h @@ -120,11 +120,7 @@ class AqlItemMatrix { class RowIterator { public: - //using iterator_category = std::forward_iterator_tag; - // using difference_type = std::ptrdiff_t; using value_type = InputAqlItemRow; - //using pointer = value_type*; - //using reference = value_type&; RowIterator() = default; RowIterator(AqlItemMatrix const* matrix, size_t blockIndex, size_t rowIndex); From 96a6fc16e4f6b092f7b89fd550adbb3e1065e9b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Tue, 10 Aug 2021 15:29:02 +0200 Subject: [PATCH 25/32] Update arangod/Aql/DependencyProxy.cpp --- arangod/Aql/DependencyProxy.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arangod/Aql/DependencyProxy.cpp b/arangod/Aql/DependencyProxy.cpp index 1fd2807504bc..0bd0bcdcfb43 100644 --- a/arangod/Aql/DependencyProxy.cpp +++ b/arangod/Aql/DependencyProxy.cpp @@ -84,7 +84,7 @@ DependencyProxy::executeForDependency(size_t dependency, if (skipped.nothingSkipped() && block == nullptr) { // We're either waiting or Done - // TRI_ASSERT(state == ExecutionState::DONE || state == ExecutionState::WAITING); + TRI_ASSERT(state == ExecutionState::DONE || state == ExecutionState::WAITING); } return {state, skipped, std::move(block)}; } From 0861e6c695e2f108adb7899235212eb3be3c7e98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Tue, 10 Aug 2021 15:34:21 +0200 Subject: [PATCH 26/32] Re-added assertions --- arangod/Aql/ExecutionBlockImpl.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/arangod/Aql/ExecutionBlockImpl.cpp b/arangod/Aql/ExecutionBlockImpl.cpp index fd5a8183fab0..1737ddf2fcd3 100644 --- a/arangod/Aql/ExecutionBlockImpl.cpp +++ b/arangod/Aql/ExecutionBlockImpl.cpp @@ -1895,18 +1895,14 @@ ExecutionBlockImpl::executeWithoutTrace(AqlCallStack stack) { if (localExecutorState == ExecutorState::HASMORE || _lastRange.hasDataRow() || _lastRange.hasShadowRow()) { // We have skipped or/and returned data, otherwise we cannot return HASMORE - /* TRI_ASSERT(!skipped.nothingSkipped() || (outputBlock != nullptr && outputBlock->numRows() > 0)); - */ return {ExecutionState::HASMORE, skipped, std::move(outputBlock)}; } // We must return skipped and/or data when reporting HASMORE - /* TRI_ASSERT(_upstreamState != ExecutionState::HASMORE || (!skipped.nothingSkipped() || (outputBlock != nullptr && outputBlock->numRows() > 0))); - */ return {_upstreamState, skipped, std::move(outputBlock)}; } From 6dd4234c1aa9206c40c1c4242d9df888aa862a56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Tue, 10 Aug 2021 16:24:44 +0200 Subject: [PATCH 27/32] Addressed remaining TODOs noticed during review --- arangod/Aql/ModificationExecutor.cpp | 17 +++++++++++++++++ arangod/Aql/ModificationExecutor.h | 2 ++ arangod/Aql/SimpleModifier.cpp | 23 ++++++++++++++++------- 3 files changed, 35 insertions(+), 7 deletions(-) diff --git a/arangod/Aql/ModificationExecutor.cpp b/arangod/Aql/ModificationExecutor.cpp index 90f5995aa97e..b4ddc475262a 100644 --- a/arangod/Aql/ModificationExecutor.cpp +++ b/arangod/Aql/ModificationExecutor.cpp @@ -29,6 +29,7 @@ #include "Aql/OutputAqlItemRow.h" #include "Aql/QueryContext.h" #include "Aql/SingleRowFetcher.h" +#include "Basics/application-exit.h" #include "StorageEngine/TransactionState.h" #include "Aql/SimpleModifier.h" @@ -88,6 +89,22 @@ AqlValue const& ModifierOutput::getNewValue() const { return _newValue.value(); } +auto to_string(ModificationExecutorResultState resultState) -> std::string { + switch (resultState) { + case ModificationExecutorResultState::NoResult: + return "NoResult"; + case ModificationExecutorResultState::WaitingForResult: + return "WaitingForResult"; + case ModificationExecutorResultState::HaveResult: + return "HaveResult"; + } + + LOG_TOPIC("4bdea", FATAL, Logger::AQL) + << "Unhandled state " + << static_cast>(resultState); + FATAL_ERROR_ABORT(); +} + template ModificationExecutor::ModificationExecutor(Fetcher& fetcher, Infos& infos) diff --git a/arangod/Aql/ModificationExecutor.h b/arangod/Aql/ModificationExecutor.h index 093a94e77f33..f07740fee980 100644 --- a/arangod/Aql/ModificationExecutor.h +++ b/arangod/Aql/ModificationExecutor.h @@ -172,6 +172,8 @@ enum class ModificationExecutorResultState { HaveResult, }; +auto to_string(ModificationExecutorResultState resultState) -> std::string; + template class ModificationExecutor { public: diff --git a/arangod/Aql/SimpleModifier.cpp b/arangod/Aql/SimpleModifier.cpp index 16e1673f120e..5c7794848b19 100644 --- a/arangod/Aql/SimpleModifier.cpp +++ b/arangod/Aql/SimpleModifier.cpp @@ -32,6 +32,7 @@ #include "Aql/SharedQueryState.h" #include "Basics/Common.h" #include "Basics/StaticStrings.h" +#include "Basics/StringUtils.h" #include "Basics/VelocyPackHelper.h" #include "Basics/application-exit.h" #include "Cluster/ServerState.h" @@ -209,15 +210,23 @@ ExecutionState SimpleModifier::transact(transaction: auto self = this->shared_from_this(); std::move(result).thenValue([self, sqs = _infos.engine()->sharedState()](OperationResult&& opRes) { - { + sqs->executeAndWakeup([&] { std::lock_guard guard(self->_resultStateMutex); - // TODO add corresponding a check in non-maintainer mode, and handle it somehow TRI_ASSERT(self->_resultState == ModificationExecutorResultState::WaitingForResult); - self->_results = std::move(opRes); - self->_resultState = ModificationExecutorResultState::HaveResult; - } - // TODO: do we need a callback here? - sqs->executeAndWakeup([] { + if (self->_resultState == ModificationExecutorResultState::WaitingForResult) { + self->_results = std::move(opRes); + self->_resultState = ModificationExecutorResultState::HaveResult; + } else { + auto message = StringUtils::concatT( + "Unexpected state ", to_string(self->_resultState), + " when reporting modification result: expected ", + to_string(ModificationExecutorResultState::WaitingForResult)); + LOG_TOPIC("1f48d", ERR, Logger::AQL) << message; + // This can never happen. However, if it does anyway: we can't clean up + // the query from here, so we do not wake it up and let it run into a + // timeout instead. This should be good enough for an impossible case. + return false; + } return true; }); }); From 7f89502ced01773b397d088e0a884b63bcfb211c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Wed, 11 Aug 2021 14:25:02 +0200 Subject: [PATCH 28/32] Fix a deadlock --- arangod/Aql/SimpleModifier.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/arangod/Aql/SimpleModifier.cpp b/arangod/Aql/SimpleModifier.cpp index 5c7794848b19..c69013be7324 100644 --- a/arangod/Aql/SimpleModifier.cpp +++ b/arangod/Aql/SimpleModifier.cpp @@ -180,7 +180,7 @@ void SimpleModifier::accumulate(InputAqlItemRow& row template ExecutionState SimpleModifier::transact(transaction::Methods& trx) { - std::lock_guard guard(_resultStateMutex); + std::unique_lock guard(_resultStateMutex); switch (_resultState) { case ModificationExecutorResultState::WaitingForResult: return ExecutionState::WAITING; @@ -208,10 +208,14 @@ ExecutionState SimpleModifier::transact(transaction: TRI_ASSERT(_infos.engine() != nullptr); TRI_ASSERT(_infos.engine()->sharedState() != nullptr); + // The guard has to be unlocked before "thenValue" is called, otherwise locking + // the mutex there will cause a deadlock if the result is already available. + guard.unlock(); + auto self = this->shared_from_this(); std::move(result).thenValue([self, sqs = _infos.engine()->sharedState()](OperationResult&& opRes) { sqs->executeAndWakeup([&] { - std::lock_guard guard(self->_resultStateMutex); + std::unique_lock guard(self->_resultStateMutex); TRI_ASSERT(self->_resultState == ModificationExecutorResultState::WaitingForResult); if (self->_resultState == ModificationExecutorResultState::WaitingForResult) { self->_results = std::move(opRes); From 326356cbc657ec7d04bfb6491f860dee8d5c8a6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Thu, 12 Aug 2021 10:41:18 +0200 Subject: [PATCH 29/32] WIP Use thenFinal and assert on exception --- arangod/Aql/SimpleModifier.cpp | 44 +++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/arangod/Aql/SimpleModifier.cpp b/arangod/Aql/SimpleModifier.cpp index c69013be7324..084d6e04c807 100644 --- a/arangod/Aql/SimpleModifier.cpp +++ b/arangod/Aql/SimpleModifier.cpp @@ -213,25 +213,35 @@ ExecutionState SimpleModifier::transact(transaction: guard.unlock(); auto self = this->shared_from_this(); - std::move(result).thenValue([self, sqs = _infos.engine()->sharedState()](OperationResult&& opRes) { - sqs->executeAndWakeup([&] { + std::move(result).thenFinal([self, sqs = _infos.engine()->sharedState()](futures::Try&& opRes) { + sqs->executeAndWakeup([&]() noexcept { std::unique_lock guard(self->_resultStateMutex); - TRI_ASSERT(self->_resultState == ModificationExecutorResultState::WaitingForResult); - if (self->_resultState == ModificationExecutorResultState::WaitingForResult) { - self->_results = std::move(opRes); - self->_resultState = ModificationExecutorResultState::HaveResult; - } else { - auto message = StringUtils::concatT( - "Unexpected state ", to_string(self->_resultState), - " when reporting modification result: expected ", - to_string(ModificationExecutorResultState::WaitingForResult)); - LOG_TOPIC("1f48d", ERR, Logger::AQL) << message; - // This can never happen. However, if it does anyway: we can't clean up - // the query from here, so we do not wake it up and let it run into a - // timeout instead. This should be good enough for an impossible case. - return false; + try { + TRI_ASSERT(self->_resultState == ModificationExecutorResultState::WaitingForResult); + if (self->_resultState == ModificationExecutorResultState::WaitingForResult) { + // get() will throw if opRes holds an exception, which is intended. + self->_results = std::move(opRes.get()); + self->_resultState = ModificationExecutorResultState::HaveResult; + } else { + auto message = StringUtils::concatT( + "Unexpected state ", to_string(self->_resultState), + " when reporting modification result: expected ", + to_string(ModificationExecutorResultState::WaitingForResult)); + LOG_TOPIC("1f48d", ERR, Logger::AQL) << message; + // This can never happen. However, if it does anyway: we can't clean up + // the query from here, so we do not wake it up and let it run into a + // timeout instead. This should be good enough for an impossible case. + return false; + } + return true; + } catch(std::exception const& ex) { + LOG_TOPIC("027ea", FATAL, Logger::AQL) << ex.what(); + TRI_ASSERT(false); + } catch(...) { + auto exptr = std::current_exception(); + TRI_ASSERT(false); + //self->_results = exptr; } - return true; }); }); From d246ce93a577ab47fac63c2fdbd301532dab2c11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Thu, 12 Aug 2021 11:58:53 +0200 Subject: [PATCH 30/32] Handled async exceptions in SimpleModifier --- arangod/Aql/ModificationExecutor.cpp | 20 +--- arangod/Aql/ModificationExecutor.h | 17 ---- arangod/Aql/SimpleModifier.cpp | 147 +++++++++++++++------------ arangod/Aql/SimpleModifier.h | 19 ++-- arangod/Aql/UpsertModifier.cpp | 22 ++-- arangod/Aql/UpsertModifier.h | 19 +++- 6 files changed, 122 insertions(+), 122 deletions(-) diff --git a/arangod/Aql/ModificationExecutor.cpp b/arangod/Aql/ModificationExecutor.cpp index b4ddc475262a..5e40401b81e2 100644 --- a/arangod/Aql/ModificationExecutor.cpp +++ b/arangod/Aql/ModificationExecutor.cpp @@ -89,22 +89,6 @@ AqlValue const& ModifierOutput::getNewValue() const { return _newValue.value(); } -auto to_string(ModificationExecutorResultState resultState) -> std::string { - switch (resultState) { - case ModificationExecutorResultState::NoResult: - return "NoResult"; - case ModificationExecutorResultState::WaitingForResult: - return "WaitingForResult"; - case ModificationExecutorResultState::HaveResult: - return "HaveResult"; - } - - LOG_TOPIC("4bdea", FATAL, Logger::AQL) - << "Unhandled state " - << static_cast>(resultState); - FATAL_ERROR_ABORT(); -} - template ModificationExecutor::ModificationExecutor(Fetcher& fetcher, Infos& infos) @@ -176,7 +160,7 @@ template return {ExecutionState::WAITING, stats, upstreamCall}; } - TRI_ASSERT(_modifier->resultState() == ModificationExecutorResultState::HaveResult); + TRI_ASSERT(_modifier->hasResultOrException()); _modifier->checkException(); if (_infos._doCount) { @@ -188,7 +172,7 @@ template _modifier->reset(); } - TRI_ASSERT(_modifier->resultState() == ModificationExecutorResultState::NoResult); + TRI_ASSERT(_modifier->hasNoResultOrOperationPending()); return {translateReturnType(_rangeHandler.upstreamState(input)), stats, upstreamCall}; } diff --git a/arangod/Aql/ModificationExecutor.h b/arangod/Aql/ModificationExecutor.h index f07740fee980..f07626b43cf8 100644 --- a/arangod/Aql/ModificationExecutor.h +++ b/arangod/Aql/ModificationExecutor.h @@ -157,23 +157,6 @@ class ModifierOutput { std::optional _newValueGuard; }; -enum class ModificationExecutorResultState { - // State that is used when the Executor's modifier has not been - // asked to produce a result. - // this is also the initial state. - NoResult, - // State that is used when the Executor's modifier has been asked - // to produce a result, but it returned a WAITING status, i.e. the - // result is not yet ready to consume. - // This state cannot happen in single servers! - WaitingForResult, - // State that is used when the Executor's modifier has produced - // a result that is ready to consume. - HaveResult, -}; - -auto to_string(ModificationExecutorResultState resultState) -> std::string; - template class ModificationExecutor { public: diff --git a/arangod/Aql/SimpleModifier.cpp b/arangod/Aql/SimpleModifier.cpp index 084d6e04c807..79f118935e59 100644 --- a/arangod/Aql/SimpleModifier.cpp +++ b/arangod/Aql/SimpleModifier.cpp @@ -125,50 +125,28 @@ SimpleModifier::OutputIterator::end() const { return it; } -template -ModificationExecutorResultState SimpleModifier::resultState() const noexcept { - std::lock_guard guard(_resultStateMutex); - return _resultState; -} - -template -bool SimpleModifier::operationPending() const noexcept { - switch (resultState()) { - case ModificationExecutorResultState::NoResult: - return false; - case ModificationExecutorResultState::WaitingForResult: - case ModificationExecutorResultState::HaveResult: - return true; - } - LOG_TOPIC("710c4", FATAL, Logger::AQL) - << "Invalid or unhandled value for ModificationExecutorResultState: " - << static_cast>(resultState()); - FATAL_ERROR_ABORT(); -} - template void SimpleModifier::checkException() const { - throwOperationResultException(_infos, _results); + throwOperationResultException(_infos, std::get(_results)); } template void SimpleModifier::resetResult() noexcept { - std::lock_guard guard(_resultStateMutex); - _resultState = ModificationExecutorResultState::NoResult; + std::lock_guard guard(_resultMutex); + _results = NoResult{}; } template void SimpleModifier::reset() { #ifdef ARANGODB_ENABLE_MAINTAINER_MODE { - std::unique_lock guard(_resultStateMutex, std::try_to_lock); + std::unique_lock guard(_resultMutex, std::try_to_lock); TRI_ASSERT(guard.owns_lock()); - TRI_ASSERT(_resultState != ModificationExecutorResultState::WaitingForResult); + TRI_ASSERT(!std::holds_alternative(_results)); } #endif _accumulator.reset(); _operations.clear(); - _results.reset(); resetResult(); } @@ -180,29 +158,27 @@ void SimpleModifier::accumulate(InputAqlItemRow& row template ExecutionState SimpleModifier::transact(transaction::Methods& trx) { - std::unique_lock guard(_resultStateMutex); - switch (_resultState) { - case ModificationExecutorResultState::WaitingForResult: - return ExecutionState::WAITING; - case ModificationExecutorResultState::HaveResult: - return ExecutionState::DONE; - case ModificationExecutorResultState::NoResult: - // continue - break; + std::unique_lock guard(_resultMutex); + if (std::holds_alternative(_results)) { + return ExecutionState::WAITING; + } else if (std::holds_alternative(_results)) { + return ExecutionState::DONE; + } else if (auto* ex = std::get_if(&_results); ex != nullptr) { + std::rethrow_exception(*ex); + } else { + TRI_ASSERT(std::holds_alternative(_results)); } - TRI_ASSERT(_resultState == ModificationExecutorResultState::NoResult); - _resultState = ModificationExecutorResultState::NoResult; + _results = NoResult{}; auto result = _completion.transact(trx, _accumulator.closeAndGetContents()); if (result.isReady()) { _results = std::move(result.get()); - _resultState = ModificationExecutorResultState::HaveResult; return ExecutionState::DONE; } - _resultState = ModificationExecutorResultState::WaitingForResult; + _results = Waiting{}; TRI_ASSERT(!ServerState::instance()->isSingleServer()); TRI_ASSERT(_infos.engine() != nullptr); @@ -215,33 +191,53 @@ ExecutionState SimpleModifier::transact(transaction: auto self = this->shared_from_this(); std::move(result).thenFinal([self, sqs = _infos.engine()->sharedState()](futures::Try&& opRes) { sqs->executeAndWakeup([&]() noexcept { - std::unique_lock guard(self->_resultStateMutex); + std::unique_lock guard(self->_resultMutex); try { - TRI_ASSERT(self->_resultState == ModificationExecutorResultState::WaitingForResult); - if (self->_resultState == ModificationExecutorResultState::WaitingForResult) { + TRI_ASSERT(std::holds_alternative(self->_results)); + if (std::holds_alternative(self->_results)) { // get() will throw if opRes holds an exception, which is intended. self->_results = std::move(opRes.get()); - self->_resultState = ModificationExecutorResultState::HaveResult; } else { + // This can never happen. + using namespace std::string_literals; + auto state = + std::visit(overload{[&](NoResult) { return "NoResults"s; }, + [&](Waiting) { return "Waiting"s; }, + [&](OperationResult const&) { + return "Result"s; + }, + [&](std::exception_ptr const& ep) { + auto what = std::string{}; + try { + std::rethrow_exception(ep); + } catch (std::exception const& ex) { + what = ex.what(); + } catch (...) { + // Exception unknown, give up immediately. + LOG_TOPIC("4646a", FATAL, Logger::AQL) << "Caught an exception while handling another one, giving up."; + FATAL_ERROR_ABORT(); + } + return StringUtils::concatT("Exception: ", what); + }}, + self->_results); auto message = StringUtils::concatT( - "Unexpected state ", to_string(self->_resultState), - " when reporting modification result: expected ", - to_string(ModificationExecutorResultState::WaitingForResult)); + "Unexpected state when reporting modification result, expected " + "'Waiting' but got: ", + state); LOG_TOPIC("1f48d", ERR, Logger::AQL) << message; - // This can never happen. However, if it does anyway: we can't clean up - // the query from here, so we do not wake it up and let it run into a - // timeout instead. This should be good enough for an impossible case. - return false; + if (std::holds_alternative(self->_results)) { + // Avoid overwriting an exception with another exception. + LOG_TOPIC("2d310", FATAL, Logger::AQL) + << "Caught an exception while handling another one, giving up."; + FATAL_ERROR_ABORT(); + } + THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_INTERNAL_AQL, std::move(message)); } - return true; - } catch(std::exception const& ex) { - LOG_TOPIC("027ea", FATAL, Logger::AQL) << ex.what(); - TRI_ASSERT(false); } catch(...) { auto exptr = std::current_exception(); - TRI_ASSERT(false); - //self->_results = exptr; + self->_results = exptr; } + return true; }); }); @@ -260,17 +256,19 @@ size_t SimpleModifier::nrOfDocuments() const { template size_t SimpleModifier::nrOfResults() const { - if (_results.hasSlice() && _results.slice().isArray()) { - return _results.slice().length(); + if (auto* res = std::get_if(&_results); + res != nullptr && res->hasSlice() && res->slice().isArray()) { + return res->slice().length(); + } else { + return 0; } - return 0; } template size_t SimpleModifier::nrOfErrors() const { size_t nrOfErrors{0}; - for (auto const& pair : _results.countErrorCodes) { + for (auto const& pair : std::get(_results).countErrorCodes) { nrOfErrors += pair.second; } return nrOfErrors; @@ -305,12 +303,35 @@ bool SimpleModifier::resultAvailable() const { template VPackArrayIterator SimpleModifier::getResultsIterator() const { if (resultAvailable()) { - TRI_ASSERT(_results.hasSlice() && _results.slice().isArray()); - return VPackArrayIterator{_results.slice()}; + auto const& results = std::get(_results); + TRI_ASSERT(results.hasSlice() && results.slice().isArray()); + return VPackArrayIterator{results.slice()}; } return VPackArrayIterator(VPackArrayIterator::Empty{}); } +template +bool SimpleModifier::hasResultOrException() const noexcept { + return std::visit(overload{ + [](NoResult) { return false; }, + [](Waiting) { return false; }, + [](OperationResult const&) { return true; }, + [](std::exception_ptr const&) { return true; }, + }, + _results); +} + +template +bool SimpleModifier::hasNoResultOrOperationPending() const noexcept { + return std::visit(overload{ + [](NoResult) { return true; }, + [](Waiting) { return false; }, + [](OperationResult const&) { return false; }, + [](std::exception_ptr const&) { return false; }, + }, + _results); +} + template class ::arangodb::aql::SimpleModifier; template class ::arangodb::aql::SimpleModifier; template class ::arangodb::aql::SimpleModifier; diff --git a/arangod/Aql/SimpleModifier.h b/arangod/Aql/SimpleModifier.h index 1aa5e528df70..ccf644ffe7de 100644 --- a/arangod/Aql/SimpleModifier.h +++ b/arangod/Aql/SimpleModifier.h @@ -80,6 +80,10 @@ class SimpleModifier : public std::enable_shared_from_this; + public: using ModOp = std::pair; @@ -107,9 +111,8 @@ class SimpleModifier : public std::enable_shared_from_this _operations; ModificationExecutorAccumulator _accumulator; - OperationResult _results; - size_t const _batchSize; - mutable std::mutex _resultStateMutex; - ModificationExecutorResultState _resultState; + mutable std::mutex _resultMutex; + ResultType _results; }; using InsertModifier = SimpleModifier; diff --git a/arangod/Aql/UpsertModifier.cpp b/arangod/Aql/UpsertModifier.cpp index 8ece2376ae87..f60e9ef8772f 100644 --- a/arangod/Aql/UpsertModifier.cpp +++ b/arangod/Aql/UpsertModifier.cpp @@ -134,20 +134,6 @@ ModificationExecutorResultState UpsertModifier::resultState() const noexcept { return _resultState; } -bool UpsertModifier::operationPending() const noexcept { - switch (resultState()) { - case ModificationExecutorResultState::NoResult: - return false; - case ModificationExecutorResultState::WaitingForResult: - case ModificationExecutorResultState::HaveResult: - return true; - } - LOG_TOPIC("ef67c", FATAL, Logger::AQL) - << "Invalid or unhandled value for ModificationExecutorResultState: " - << static_cast>(resultState()); - FATAL_ERROR_ABORT(); -} - void UpsertModifier::reset() { #ifdef ARANGODB_ENABLE_MAINTAINER_MODE { @@ -350,3 +336,11 @@ size_t UpsertModifier::nrOfWritesExecuted() const { size_t UpsertModifier::nrOfWritesIgnored() const { return nrOfErrors(); } size_t UpsertModifier::getBatchSize() const { return _batchSize; } + +bool UpsertModifier::hasResultOrException() const noexcept { + return resultState() == ModificationExecutorResultState::HaveResult; +} + +bool UpsertModifier::hasNoResultOrOperationPending() const noexcept { + return resultState() == ModificationExecutorResultState::NoResult; +} diff --git a/arangod/Aql/UpsertModifier.h b/arangod/Aql/UpsertModifier.h index e56e29d8441f..e4a6bcdd1c9f 100644 --- a/arangod/Aql/UpsertModifier.h +++ b/arangod/Aql/UpsertModifier.h @@ -38,6 +38,21 @@ namespace arangodb::aql { struct ModificationExecutorInfos; +enum class ModificationExecutorResultState { + // State that is used when the Executor's modifier has not been + // asked to produce a result. + // this is also the initial state. + NoResult, + // State that is used when the Executor's modifier has been asked + // to produce a result, but it returned a WAITING status, i.e. the + // result is not yet ready to consume. + // This state cannot happen in single servers! + WaitingForResult, + // State that is used when the Executor's modifier has produced + // a result that is ready to consume. + HaveResult, + }; + class UpsertModifier { public: enum class OperationType { @@ -88,7 +103,6 @@ class UpsertModifier { ~UpsertModifier() = default; ModificationExecutorResultState resultState() const noexcept; - [[nodiscard]] bool operationPending() const noexcept; void checkException() const {} void resetResult() noexcept; @@ -106,6 +120,9 @@ class UpsertModifier { size_t getBatchSize() const; + bool hasResultOrException() const noexcept; + bool hasNoResultOrOperationPending() const noexcept; + private: bool resultAvailable() const; VPackArrayIterator getUpdateResultsIterator() const; From e41751a888697bf1b700de1ad3d3aba71056eff4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Thu, 12 Aug 2021 12:01:45 +0200 Subject: [PATCH 31/32] Added a TODO note --- arangod/Aql/UpsertModifier.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arangod/Aql/UpsertModifier.h b/arangod/Aql/UpsertModifier.h index e4a6bcdd1c9f..840bf5d12cf2 100644 --- a/arangod/Aql/UpsertModifier.h +++ b/arangod/Aql/UpsertModifier.h @@ -38,6 +38,9 @@ namespace arangodb::aql { struct ModificationExecutorInfos; +// TODO Remove this state, and use a variant as in SimpleModifier. +// It makes most sense to do this when implementing async upsert operations, +// so I'm leaving it for now. enum class ModificationExecutorResultState { // State that is used when the Executor's modifier has not been // asked to produce a result. From e728b22b247530b7caa6aa1e06215cd9c256c096 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20G=C3=B6dderz?= Date: Mon, 16 Aug 2021 16:05:18 +0200 Subject: [PATCH 32/32] Added CHANGELOG entry --- CHANGELOG | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG b/CHANGELOG index bceb010eb6fd..f0c0b86a2f41 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,10 @@ v3.8.1 (XXXX-XX-XX) ------------------- +* Make AQL modification operations in a cluster asynchronous. This allows to + free the thread for other work until both the write and synchronous + replication are complete. + * Fixed various problems in GEO_INTERSECTS: wrong results, not implemented cases and numerically unstable behaviour. In particular, the case of the intersection of two polygons in which one is an S2LngLatRect is fixed