Skip to content
Open
Show file tree
Hide file tree
Changes from 8 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
28 changes: 28 additions & 0 deletions jstests/core/explain_use_backup_plan.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// 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"
(function() {
"use strict";

db.foo.drop() let bulk = db.foo.initializeUnorderedBulkOp();
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 you're missing a semi-colon after .drop() here.

for (let i = 0; i < 100000; ++i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is great, but we tend to favor tests that don't need as much data to be inserted, since they will execute faster. In this case you configure the maxBlockingSortBytes, so do you need this many documents?

bulk.insert({_id: i, x: i, y: i});
}

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

Choose a reason for hiding this comment

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

Missing semi-colon, also on line 23 and 27.


// Configure log level and lower the sort bytes limit.
db.setLogLevel(5, "query");
db.adminCommand({setParameter: 1, internalQueryExecMaxBlockingSortBytes: 100});

// This query will not use the backup plan, hence it generates only two stages: winningPlan and
// rejectedPlans
assert
.commandWorked(db.foo.find({x: {$gte: 90}}).sort({_id: 1}).explain(true))
Copy link
Contributor

Choose a reason for hiding this comment

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

Your comment here mentions what we expect, but you just assert that the command "worked", which just means it returned a document with an ok field with a truthy value (usually 1).

Check out the helper functions defined here: https://github.com/mongodb/mongo/blob/r3.7.2/jstests/libs/analyze_plan.js

I think you should add a helper there which gets the 'originalWinningPlan', and assert that it is null here, and non-null below.


// This query will use backup plan, the exaplin output for this query will generate three
// stages: winningPlan, rejectedPlans and originalWinningPlan
assert.commandWorked(db.foo.find({x: {$gte: 90000}}).sort({_id: 1}).explain(true))
}());
9 changes: 9 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 @@ -245,6 +247,7 @@ Status MultiPlanStage::pickBestPlan(PlanYieldPolicy* yieldPolicy) {
_backupPlanIdx = kNoSuchPlan;
if (bestSolution->hasBlockingStage && (0 == alreadyProduced.size())) {
LOG(5) << "Winner has blocking stage, looking for backup plan...";

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal, but try not to include unnecessary changes like this in your patch. This one isn't particularly offensive, but I'd still recommend reverting this change.

for (size_t ix = 0; ix < _candidates.size(); ++ix) {
if (!_candidates[ix].solution->hasBlockingStage) {
LOG(5) << "Candidate " << ix << " is backup child";
Expand Down Expand Up @@ -452,6 +455,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
11 changes: 10 additions & 1 deletion src/mongo/db/exec/multi_plan.h
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,11 @@ class MultiPlanStage final : public PlanStage {
/** Return the index of the best plan chosen, for testing */
int bestPlanIdx() const;

/** Return the index of the backup plan chosen, for testing */
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you just copied this from above, but in new code we tend to always prefer

/**
 * This kind of syntax.
 */

instead of

/** This kind of syntax */

Also, these new methods are used for explain output, not just for testing. I'm pretty sure bestPlanIdx() is as well, so you can remove that part of it.

Please update these, and any others in this file while you're at it.

int backupPlanIdx() const;

/** Return the index of the backup plan chosen, for testing */
int originalWinningPlanIdx() const;
/**
* Returns the QuerySolution for the best plan, or NULL if no best plan
*
Expand Down Expand Up @@ -198,10 +203,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 competition
// uses -1 / kNoSuchPlan when best plan 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
29 changes: 29 additions & 0 deletions src/mongo/db/query/explain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,15 @@ unique_ptr<PlanStageStats> getWinningPlanStatsTree(const PlanExecutor* exec) {
: std::move(exec->getRootStage()->getStats());
}


/**
* Get PlanExecutor's original winning plan stats tree.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please elaborate here that this will only be different from the actual winning plan if a non-blocking backup plan was used. How about Returns the root of the original winning plan used by 'exec'. This may 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'.

*/
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 +653,16 @@ void Explain::generatePlannerInfo(PlanExecutor* exec,
BSONObjBuilder plannerBob(out->subobjStart("queryPlanner"));

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

const auto mps = getMultiPlanStage(exec->getRootStage());
int originaWinningPlanIdx = static_cast<size_t>(mps->originalWinningPlanIdx());

if (originaWinningPlanIdx > -1) {
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 you never use the actual index, so how about changing line 658 to be
const bool usedBackupPlan = (static_cast<size_t>(mps->originalWinningPlanIdx()) > -1);? That way you can collapse lines 660-664 into one line.

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 +707,16 @@ void Explain::generatePlannerInfo(PlanExecutor* exec,
}
allPlansBob.doneFast();

if (originaWinningPlanIdx > -1) {
// 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