[BTS-2268] Fix query plan caching with subqueries (#22146)

jbajic · web-flow · commit 99ff1fa3402a · 2025-12-01T15:39:35.000+01:00
* Invalidate cost in splice-subqueries

* Fix changelog

* Always recalculate cost estimates for plan in optimizer

* Address review comments
diff --git a/CHANGELOG b/CHANGELOG
@@ -1,6 +1,10 @@
 devel
 -----
 
+* Fix BTS-2268: Fixed an issue where query plan for queries with subquery
+  is causing a crash when being stored in query plan cache. The issue
+  is a missing invalidateCost from the splice-subqueries rule.
+
 * FE-905: Update tooltip text for maxSkewThreshold  & minDeletionRatio. 
 
 * COR-45: Added support for index hints in vector search queries.
diff --git a/arangod/Aql/Optimizer.cpp b/arangod/Aql/Optimizer.cpp
@@ -277,8 +277,7 @@ void Optimizer::initializeRules(ExecutionPlan* plan,
 
 // @brief the actual optimization
 void Optimizer::createPlans(std::unique_ptr<ExecutionPlan> plan,
-                            QueryOptions const& queryOptions,
-                            bool estimateAllPlans) {
+                            QueryOptions const& queryOptions) {
   _runOnlyRequiredRules = false;
   ExecutionPlan* initialPlan = plan.get();
 
@@ -416,7 +415,7 @@ void Optimizer::createPlans(std::unique_ptr<ExecutionPlan> plan,
 
   finalizePlans();
 
-  estimateCosts(queryOptions, estimateAllPlans);
+  estimateCosts(queryOptions);
 
   // Best plan should not have forced hints left.
   // note: this method will throw in case the plan still contains
@@ -467,29 +466,18 @@ void Optimizer::checkForcedIndexHints() {
   TRI_ASSERT(!_plans.list.empty());
 }
 
-void Optimizer::estimateCosts(QueryOptions const& queryOptions,
-                              bool estimateAllPlans) {
-  if (estimateAllPlans || _plans.size() > 1 ||
-      queryOptions.profile >= ProfileLevel::Blocks) {
-    // if profiling is turned on, we must do the cost estimation here
-    // because the cost estimation must be done while the transaction
-    // is still running
-    for (auto& plan : _plans.list) {
-      plan.first->invalidateCost();
-      plan.first->getCost();
-      // this value is cached in the plan, so formally this step is
-      // unnecessary, but for the sake of cleanliness...
-    }
-
-    if (_plans.size() > 1) {
-      // only sort plans when necessary
-      std::sort(_plans.list.begin(), _plans.list.end(),
-                [](PlanList::Entry const& a, PlanList::Entry const& b) -> bool {
-                  return a.first->getCost().estimatedCost <
-                         b.first->getCost().estimatedCost;
-                });
-    }
+void Optimizer::estimateCosts(QueryOptions const& queryOptions) {
+  // Invalidate all plans and get new estimates before doing anything
+  for (auto& plan : _plans.list) {
+    plan.first->invalidateCost();
+    plan.first->getCost();
   }
+
+  std::sort(_plans.list.begin(), _plans.list.end(),
+            [](PlanList::Entry const& a, PlanList::Entry const& b) -> bool {
+              return a.first->getCost().estimatedCost <
+                     b.first->getCost().estimatedCost;
+            });
 }
 
 void Optimizer::disableRule(ExecutionPlan* plan, int level) {
diff --git a/arangod/Aql/Optimizer.h b/arangod/Aql/Optimizer.h
@@ -113,7 +113,7 @@ class Optimizer {
   /// If you need to extract the plans from the optimizer use stealBest or
   /// stealPlans.
   void createPlans(std::unique_ptr<ExecutionPlan> p,
-                   QueryOptions const& queryOptions, bool estimateAllPlans);
+                   QueryOptions const& queryOptions);
 
   /// @brief add a plan to the optimizer
   void addPlan(std::unique_ptr<ExecutionPlan>, OptimizerRule const&,
@@ -173,7 +173,7 @@ class Optimizer {
 
   void checkForcedIndexHints();
 
-  void estimateCosts(QueryOptions const& queryOptions, bool estimateAllPlans);
+  void estimateCosts(QueryOptions const& queryOptions);
 
   /// @brief optimizer statistics
   Stats _stats;
diff --git a/arangod/Aql/OptimizerRules.cpp b/arangod/Aql/OptimizerRules.cpp
@@ -8854,6 +8854,9 @@ void arangodb::aql::spliceSubqueriesRule(Optimizer* opt,
     TRI_ASSERT(sq->getParents().empty());
   }
 
+  if (modified) {
+    plan->root()->invalidateCost();
+  }
   opt->addPlan(std::move(plan), rule, modified);
 }
 
diff --git a/arangod/Aql/Query.cpp b/arangod/Aql/Query.cpp
@@ -691,7 +691,7 @@ std::unique_ptr<ExecutionPlan> Query::preparePlan() {
   enterState(QueryExecutionState::ValueType::PLAN_OPTIMIZATION);
   Optimizer opt(*_resourceMonitor, _queryOptions.maxNumberOfPlans);
   // get enabled/disabled rules
-  opt.createPlans(std::move(plan), _queryOptions, false);
+  opt.createPlans(std::move(plan), _queryOptions);
   // Now plan and all derived plans belong to the optimizer
   plan = opt.stealBest();  // Now we own the best one again
 
@@ -1473,7 +1473,7 @@ QueryResult Query::explain() {
     enterState(QueryExecutionState::ValueType::PLAN_OPTIMIZATION);
     Optimizer opt(*_resourceMonitor, _queryOptions.maxNumberOfPlans);
     // get enabled/disabled rules
-    opt.createPlans(std::move(plan), _queryOptions, true);
+    opt.createPlans(std::move(plan), _queryOptions);
 
     enterState(QueryExecutionState::ValueType::FINALIZATION);
 
diff --git a/arangod/IResearch/IResearchAqlAnalyzer.cpp b/arangod/IResearch/IResearchAqlAnalyzer.cpp
@@ -499,7 +499,7 @@ bool AqlAnalyzer::reset(std::string_view field) noexcept {
       optimizer.disableRules(plan.get(), [](OptimizerRule const& rule) -> bool {
         return rule.canBeDisabled() || rule.isClusterOnly();
       });
-      optimizer.createPlans(std::move(plan), _query->queryOptions(), false);
+      optimizer.createPlans(std::move(plan), _query->queryOptions());
 
       _plan = optimizer.stealBest();
       TRI_ASSERT(
diff --git a/tests/js/client/aql/aql-query-plan-cache.js b/tests/js/client/aql/aql-query-plan-cache.js
@@ -697,6 +697,18 @@ function QueryPlanCacheTestSuite () {
       assertEqual(1, entries.filter(buildFilter({ query: query2, bindVars: {"@collection": cn2}, fullCount: false, dataSources: [cn2] })).length, entries);
       assertEqual(1, entries.filter(buildFilter({ query: query3, bindVars: {}, fullCount: true, dataSources: [cn2] })).length, entries);
     },
+
+    // Fixes BTS-2268
+    testPlanCacheEstimatesCrash : function () {
+      const query = `${uniqid()} FOR doc IN ${cn1} LET var = (FOR i IN 0..10 FILTER i == 2 RETURN i) FILTER doc.value1 IN var RETURN doc.value`;
+      const options = { optimizePlanForCaching: true, usePlanCache: true };
+
+      let res = db._query(query, {}, options);
+      assertFalse(res.hasOwnProperty("planCacheKey"));
+
+      res = db._query(query, {}, options);
+      assertTrue(res.hasOwnProperty("planCacheKey"));
+    },
   };
 }
 

Original file line number	Diff line number	Diff line change
`@@ -8854,6 +8854,9 @@ void arangodb::aql::spliceSubqueriesRule(Optimizer* opt,`
`8854`	`8854`	`TRI_ASSERT(sq->getParents().empty());`
`8855`	`8855`	`}`
`8856`	`8856`
	`8857`	`+ if (modified) {`
	`8858`	`+ plan->root()->invalidateCost();`
	`8859`	`+ }`
`8857`	`8860`	`opt->addPlan(std::move(plan), rule, modified);`
`8858`	`8861`	`}`
`8859`	`8862`