Skip to content
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions jstests/core/explain_use_backup_plan.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// Test that the explain will use backup plan if the original winning plan ran out of memory in the
// "executionStats" mode
// This test was designed to reproduce SERVER-32721"

load("jstests/libs/analyze_plan.js");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put the contents of this test in an immediately-invoked-function-expression (https://en.wikipedia.org/wiki/Immediately-invoked_function_expression)


"use strict";

db.foo.drop();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a more unique collection name, how about

const coll = db.explain_backup_plan;
coll.drop();

// Replace usages of 'db.foo' with 'coll' below.

let bulk = db.foo.initializeUnorderedBulkOp();

for (let i = 0; i < 100000; ++i) {
bulk.insert({_id: i, x: i, y: i});
}

bulk.execute();
db.foo.ensureIndex({x: 1});

// Configure log level and lower the sort bytes limit.
db.setLogLevel(5, "query");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to set the log level for the test, that was just for your personal info. Please remove this line so the test doesn't generate lots of logs. This will also change the setting for subsequent tests, so it would get quite noisy.

db.adminCommand({setParameter: 1, internalQueryExecMaxBlockingSortBytes: 100});

const test1 = db.foo.find({x: {$gte: 90}}).sort({_id: 1}).explain(true);
const test2 = db.foo.find({x: {$gte: 90000}}).sort({_id: 1}).explain(true);
// This query will not use the backup plan, hence it generates only two stages: winningPlan and
// rejectedPlans
assert.eq(backupPlanUsed(test1), false, "test1 did not use a backup plan");
// This query will use backup plan, the exaplin output for this query will generate three
// stages: winningPlan, rejectedPlans and originalWinningPlan
assert(backupPlanUsed(test2), "backup plan invoked in test2");
4 changes: 4 additions & 0 deletions jstests/libs/analyze_plan.js
Original file line number Diff line number Diff line change
Expand Up @@ -287,3 +287,7 @@ function assertCoveredQueryAndCount({collection, query, project, count}) {
"Winning plan for count was not covered: " + tojson(explain.queryPlanner.winningPlan));
assertExplainCount({explainResults: explain, expectedCount: count});
}

function backupPlanUsed(root) {
return root.queryPlanner.backupPlanUsed;
}
8 changes: 8 additions & 0 deletions src/mongo/db/exec/multi_plan.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ MultiPlanStage::MultiPlanStage(OperationContext* opCtx,
_query(cq),
_bestPlanIdx(kNoSuchPlan),
_backupPlanIdx(kNoSuchPlan),
_originalWinningPlanIdx(kNoSuchPlan),
_failure(false),
_failureCount(0),
_statusMemberId(WorkingSet::INVALID_ID) {
Expand Down Expand Up @@ -128,6 +129,7 @@ PlanStage::StageState MultiPlanStage::doWork(WorkingSetID* out) {
// cached plan runner to fall back on a different solution
// if the best solution fails. Alternatively we could try to
// defer cache insertion to be after the first produced result.
_originalWinningPlanIdx = _bestPlanIdx;

_collection->infoCache()->getPlanCache()->remove(*_query).transitional_ignore();

Expand Down Expand Up @@ -452,6 +454,12 @@ void MultiPlanStage::doInvalidate(OperationContext* opCtx,
bool MultiPlanStage::hasBackupPlan() const {
return kNoSuchPlan != _backupPlanIdx;
}
int MultiPlanStage::backupPlanIdx() const {
return _backupPlanIdx;
}
int MultiPlanStage::originalWinningPlanIdx() const {
return _originalWinningPlanIdx;
}

bool MultiPlanStage::bestPlanChosen() const {
return kNoSuchPlan != _bestPlanIdx;
Expand Down
20 changes: 18 additions & 2 deletions src/mongo/db/exec/multi_plan.h
Original file line number Diff line number Diff line change
Expand Up @@ -129,9 +129,21 @@ class MultiPlanStage final : public PlanStage {
/** Return true if a best plan has been chosen */
bool bestPlanChosen() const;

/** Return the index of the best plan chosen, for testing */
/*
* Return the index of the best plan chosen
*/
int bestPlanIdx() const;

/**
* Return the index of the backup plan chosen
*/
int backupPlanIdx() const;

/**
* Return the index of the original winning plan chosen
*/
int originalWinningPlanIdx() const;

/**
* Returns the QuerySolution for the best plan, or NULL if no best plan
*
Expand Down Expand Up @@ -198,10 +210,14 @@ class MultiPlanStage final : public PlanStage {
// uses -1 / kNoSuchPlan when best plan is not (yet) known
int _bestPlanIdx;

// index into _candidates, of the backup plan for sort
// index into _candidates, of the backup of the plan competition
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still doesn't read particularly well. How about re-phrasing this to
The index within '_candidates' of the non-blocking backup plan which can be used if a blocking plan fails. This is set to 'kNoSuchPlan' if there is no backup plan, or when it is not yet known.? I'd recommend a similar re-wording for the comment on '_originalWinningPlanIndex' below, with a mention that this is used to remember which plan one in the case that a backup plan took over.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this still hasn't been addressed?

// uses -1 / kNoSuchPlan when best plan is not (yet) known
int _backupPlanIdx;

// Index into _candidates, of the original winner of the plan which can be used if a blocking plan .fails
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use single-quotes to refer to the variable name: Index into '_candidates', ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it looks like you don't need the comma here, and you should move the "." before 'fails' to be after it.

// This is set to 'kNoSuchPlan' if there is no backup plan, or when it is not (yet) known
int _originalWinningPlanIdx;

// Set if this MultiPlanStage cannot continue, and the query must fail. This can happen in
// two ways. The first is that all candidate plans fail. Note that one plan can fail
// during normal execution of the plan competition. Here is an example:
Expand Down
63 changes: 61 additions & 2 deletions src/mongo/db/query/explain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ using std::string;
using std::unique_ptr;
using std::vector;

bool backupPlanUsed = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be passed as a parameter to the function. These helpers can be called by multiple threads, which would induce a race condition writing to the variable here.

/**
* Traverse the tree rooted at 'root', and add all tree nodes into the list 'flattened'.
*/
Expand Down Expand Up @@ -280,6 +281,30 @@ std::vector<std::unique_ptr<PlanStageStats>> getRejectedPlansTrialStats(PlanExec
return res;
}

/**
* Gather the PlanStages when a backup plan is used
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is confusing. What do we do when a backup plan is not used?

I'd actually recommend removing this method, and instead adding a 'bool backupPlanUsed' parameter to 'getRejectedPlansTrialStats'.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do that, you should then extend the comment on that method to include something like "If a backup plan was used, include the stats of the new winning plan instead of the original winning plan. The original winning plan is expected to already be known by the caller, since it needs to be snapshotted and stored before execution completes."

*/
std::vector<std::unique_ptr<PlanStageStats>> getRejectedPlansTrialStatsForAllPlanExecution(PlanExecutor* exec) {
const auto mps = getMultiPlanStage(exec->getRootStage());
std::vector<std::unique_ptr<PlanStageStats>> res;

if (backupPlanUsed) {
// Get the stats from the trial period for all the plans.
if (mps) {
const auto mpsStats = mps->getStats();
for (size_t i = 0; i < mpsStats->children.size(); ++i) {
if (i != static_cast<size_t>(mps->originalWinningPlanIdx())) {
res.emplace_back(std::move(mpsStats->children[i]));
}
}
}
} else {
res = getRejectedPlansTrialStats(exec);
}

return res;
}

/**
* Get PlanExecutor's winning plan stats tree.
*/
Expand All @@ -289,6 +314,18 @@ unique_ptr<PlanStageStats> getWinningPlanStatsTree(const PlanExecutor* exec) {
: std::move(exec->getRootStage()->getStats());
}


/**
* Returns the root of the roginal winning plan used by 'exec'.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

original, not 'roginal'

* This might be different than the final winning plan in the case that the MultiPlanStage selected a
* blocking plan which failed, and fell back to a non-blocking plan instead.
* If there is no MultiPlanStage in the tree, returns the root stage of 'exec'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finish with a period.

*/
unique_ptr<PlanStageStats> getOriginalWinningPlanStatsTree(const PlanExecutor* exec) {
MultiPlanStage* mps = getMultiPlanStage(exec->getRootStage());
return mps ? std::move(mps->getStats()->children[mps->originalWinningPlanIdx()])
: std::move(exec->getRootStage()->getStats());
}
} // namespace

namespace mongo {
Expand Down Expand Up @@ -644,6 +681,17 @@ void Explain::generatePlannerInfo(PlanExecutor* exec,
BSONObjBuilder plannerBob(out->subobjStart("queryPlanner"));

plannerBob.append("plannerVersion", QueryPlanner::kPlannerVersion);

const auto mps = getMultiPlanStage(exec->getRootStage());
// const bool backupPlanUsed = ((mps->originalWinningPlanIdx()) > -1);
backupPlanUsed = ((mps->originalWinningPlanIdx()) > -1);

if (backupPlanUsed) {
plannerBob.append("backupPlanUsed", true);
} else {
plannerBob.append("backupPlanUsed", false);
}

plannerBob.append("namespace", exec->nss().ns());

// Find whether there is an index filter set for the query shape. The 'indexFilterSet'
Expand Down Expand Up @@ -688,6 +736,16 @@ void Explain::generatePlannerInfo(PlanExecutor* exec,
}
allPlansBob.doneFast();

if (backupPlanUsed) {
// Generate array of original winning plan
BSONObjBuilder originalWinningPlanBob(plannerBob.subobjStart("originalWinningPlan"));
const auto originalWinnerStats = getOriginalWinningPlanStatsTree(exec);
statsToBSON(*originalWinnerStats.get(),
&originalWinningPlanBob,
ExplainOptions::Verbosity::kQueryPlanner);
originalWinningPlanBob.doneFast();
}

plannerBob.doneFast();
}

Expand Down Expand Up @@ -798,7 +856,7 @@ void Explain::generateExecutionInfo(PlanExecutor* exec,
planBob.doneFast();
}

const vector<unique_ptr<PlanStageStats>> rejectedStats = getRejectedPlansTrialStats(exec);
const vector<unique_ptr<PlanStageStats>> rejectedStats = getRejectedPlansTrialStatsForAllPlanExecution(exec);
for (size_t i = 0; i < rejectedStats.size(); ++i) {
BSONObjBuilder planBob(allPlansBob.subobjStart());
generateSinglePlanExecutionInfo(
Expand Down Expand Up @@ -855,8 +913,9 @@ void Explain::explainStages(PlanExecutor* exec,
const Collection* collection,
ExplainOptions::Verbosity verbosity,
BSONObjBuilder* out) {

auto winningPlanTrialStats = Explain::getWinningPlanTrialStats(exec);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a stray whitespace change (and also on line 916), can you please undo this?

Status executePlanStatus = Status::OK();

// If we need execution stats, then run the plan in order to gather the stats.
Expand Down