SERVER-32721 Explain output should indicate when a backup plan is used #1231

d-d-ddl · 2018-03-27T15:56:20Z

No description provided.

…with an underscore

cswanson310

Lots of little changes requested, but overall this looks really good! The code is looking very clean and organized. You happened to copy some older code in the MultiPlanStage, so I understand some of the comments may be a surprise - sorry about that. We have different style standards today and the code you copied hadn't been updated in a while. I hope they most of them should be pretty easy to address.

Thanks for doing this!

cswanson310 · 2018-04-03T02:27:14Z

jstests/core/explain_use_backup_plan.js

+(function() {
+    "use strict";
+
+    db.foo.drop() let bulk = db.foo.initializeUnorderedBulkOp();


Looks like you're missing a semi-colon after .drop() here.

cswanson310 · 2018-04-03T02:28:27Z

jstests/core/explain_use_backup_plan.js

+    "use strict";
+
+    db.foo.drop() let bulk = db.foo.initializeUnorderedBulkOp();
+    for (let i = 0; i < 100000; ++i) {


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?

cswanson310 · 2018-04-03T02:28:33Z

jstests/core/explain_use_backup_plan.js

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


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

cswanson310 · 2018-04-03T02:29:46Z

src/mongo/db/exec/multi_plan.cpp

    _backupPlanIdx = kNoSuchPlan;
    if (bestSolution->hasBlockingStage && (0 == alreadyProduced.size())) {
        LOG(5) << "Winner has blocking stage, looking for backup plan...";
+


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.

cswanson310 · 2018-04-03T02:34:32Z

src/mongo/db/exec/multi_plan.h

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

+    /** Return the index of the backup plan chosen, for testing */


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.

cswanson310 · 2018-04-03T02:37:25Z

src/mongo/db/exec/multi_plan.h

    int _bestPlanIdx;

-    // index into _candidates, of the backup plan for sort
+    // index into _candidates, of the backup of the plan competition


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.

It looks like this still hasn't been addressed?

cswanson310 · 2018-04-03T02:41:53Z

src/mongo/db/query/explain.cpp


+
+/**
+ * Get PlanExecutor's original winning plan stats tree.


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'.

cswanson310 · 2018-04-03T02:44:11Z

src/mongo/db/query/explain.cpp

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


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.

cswanson310 · 2018-04-03T02:47:35Z

jstests/core/explain_use_backup_plan.js

+    // 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))


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.

cswanson310

A couple comments, mostly about the wording. I do think there is one serious problem with the global variable.

cswanson310 · 2018-05-01T01:53:06Z

jstests/core/explain_use_backup_plan.js

+// "executionStats" mode
+// This test was designed to reproduce SERVER-32721"
+
+load("jstests/libs/analyze_plan.js");


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

cswanson310 · 2018-05-01T01:54:13Z

jstests/core/explain_use_backup_plan.js

+
+"use strict";
+
+db.foo.drop();


Use a more unique collection name, how about

const coll = db.explain_backup_plan; coll.drop(); // Replace usages of 'db.foo' with 'coll' below.

cswanson310 · 2018-05-01T01:54:54Z

jstests/core/explain_use_backup_plan.js

+db.foo.ensureIndex({x: 1});
+
+// Configure log level and lower the sort bytes limit.
+db.setLogLevel(5, "query");


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.

cswanson310 · 2018-05-01T01:55:29Z

src/mongo/db/exec/multi_plan.h

    int _bestPlanIdx;

-    // index into _candidates, of the backup plan for sort
+    // index into _candidates, of the backup of the plan competition


It looks like this still hasn't been addressed?

cswanson310 · 2018-05-01T01:55:52Z

src/mongo/db/exec/multi_plan.h

    // 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


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

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

cswanson310 · 2018-05-01T01:57:21Z

src/mongo/db/query/explain.cpp

 using std::unique_ptr;
 using std::vector;

+bool backupPlanUsed = false;


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.

cswanson310 · 2018-05-01T01:58:42Z

src/mongo/db/query/explain.cpp

 }

+/**
+ * Gather the PlanStages when a backup plan is used


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'.

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."

cswanson310 · 2018-05-01T02:01:20Z

src/mongo/db/query/explain.cpp


+
+/**
+ * Returns the root of the roginal winning plan used by 'exec'.


original, not 'roginal'

cswanson310 · 2018-05-01T02:01:37Z

src/mongo/db/query/explain.cpp

+ * Returns the root of the roginal winning plan used by 'exec'.
+ * 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'


Finish with a period.

cswanson310 · 2018-05-01T02:02:23Z

src/mongo/db/query/explain.cpp

+
    auto winningPlanTrialStats = Explain::getWinningPlanTrialStats(exec);
-
+   


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

kelly-cs · 2025-10-03T21:44:36Z

I'm leaving this open since SERVER-32721 is still unresolved - could you double check this PR is up to date with master?

Addtionally, if you're still looking to merge this, can you:

Sign our Contributor's Agreement.

I checked in on this issue to see if it's something we're going to look at.

d-d-ddl added 8 commits March 20, 2018 13:05

SERVER-19904 Added command to check element's type

ed35209

SERVER-22480: Changed DocumentSource's member variable name to start …

a7c1914

…with an underscore

Added backup plan indicator

b9c7aed

Added backup plan incator to explain output

6316125

Added OriginalWinningPlan to explain output

8731c80

Removed changes for another ticket

3ec2451

Added if condition to prevent segmentation fault

9c66dab

Formatted Files

fa97a62

cswanson310 self-assigned this Apr 3, 2018

cswanson310 self-requested a review April 3, 2018 02:25

cswanson310 requested changes Apr 3, 2018

View reviewed changes

d-d-ddl added 5 commits April 24, 2018 11:58

Add Helper Function for jstest

d0f1174

Modified Testing Function

e2f2483

Modified Testing Function

22982fd

SERVER-32721

ca5e444

SERVER-32721 Almost Done?

ffd904b

cswanson310 reviewed May 1, 2018

View reviewed changes

Server-32721

d86caa4



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


		auto winningPlanTrialStats = Explain::getWinningPlanTrialStats(exec);

SERVER-32721 Explain output should indicate when a backup plan is used #1231

Are you sure you want to change the base?

SERVER-32721 Explain output should indicate when a backup plan is used #1231

Uh oh!

Conversation

d-d-ddl commented Mar 27, 2018

Uh oh!

cswanson310 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cswanson310 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kelly-cs commented Oct 3, 2025

Uh oh!

Uh oh!