From a579f1d19e87c0fbf5f86e3aacb39d710d8fb757 Mon Sep 17 00:00:00 2001 From: Projjal Chanda Date: Thu, 9 Feb 2023 11:57:09 +0000 Subject: SERVER-71525: Removed failOnPoisonedFieldLookup fail point and associated tests relying on it. Performs an invalid operation (divide-by-zero) instead of a "POISON" field lookup for short-circuit testing --- jstests/aggregation/expressions/filter.js | 46 +++++++---------- jstests/core/computed_projections.js | 71 +++++++++++++------------- src/mongo/db/exec/sbe/vm/vm.cpp | 7 --- src/mongo/db/query/sbe_stage_builder.cpp | 5 +- src/mongo/db/query/sbe_stage_builder_helpers.h | 13 ----- 5 files changed, 55 insertions(+), 87 deletions(-) diff --git a/jstests/aggregation/expressions/filter.js b/jstests/aggregation/expressions/filter.js index 2e7abe6b0cb..2f7014a6706 100644 --- a/jstests/aggregation/expressions/filter.js +++ b/jstests/aggregation/expressions/filter.js @@ -1,9 +1,4 @@ // Test $filter aggregation expression. -// -// @tags: [ -// # Can't set the 'failOnPoisonedFieldLookup' failpoint on mongos. -// assumes_against_mongod_not_mongos, -// ] load('jstests/aggregation/extras/utils.js'); // For assertErrorCode. load("jstests/libs/sbe_assert_error_override.js"); // Override error-code-checking APIs. @@ -405,29 +400,24 @@ runAndAssert(filterDoc, expectedResults); // Test short-circuiting in $and and $or inside $filter expression. coll.drop(); -assert.commandWorked(coll.insert({_id: 0, a: [-1, -2, -3, -4]})); -try { - // Lookup of '$POISON' field will always fail with this fail point enabled. - assert.commandWorked( - db.adminCommand({configureFailPoint: "failOnPoisonedFieldLookup", mode: "alwaysOn"})); - - // Create filter with $and expression containing '$POISON' in it. - expectedResults = [ - {_id: 0, b: []}, - ]; - filterDoc = {$filter: {input: '$a', cond: {$and: [{$gt: ['$$this', 0]}, '$POISON']}}}; - runAndAssert(filterDoc, expectedResults); - - // Create filter with $or expression containing '$POISON' in it. - expectedResults = [ - {_id: 0, b: [-1, -2, -3, -4]}, - ]; - filterDoc = {$filter: {input: '$a', cond: {$or: [{$lt: ['$$this', 0]}, '$POISON']}}}; - runAndAssert(filterDoc, expectedResults); -} finally { - assert.commandWorked( - db.adminCommand({configureFailPoint: "failOnPoisonedFieldLookup", mode: "off"})); -} +assert.commandWorked(coll.insert({_id: 0, a: [-1, -2, -3, -4], zero: 0})); +// Create filter with $and expression containing $divide by zero operation in it. +expectedResults = [ + {_id: 0, b: []}, +]; +filterDoc = { + $filter: {input: '$a', cond: {$and: [{$gt: ['$$this', 0]}, {$divide: [1, '$zero']}]}} +}; +runAndAssert(filterDoc, expectedResults); + +// Create filter with $or expression containing $divide by zero operation in it. +expectedResults = [ + {_id: 0, b: [-1, -2, -3, -4]}, +]; +filterDoc = { + $filter: {input: '$a', cond: {$or: [{$lte: ['$$this', 0]}, {$divide: [1, '$zero']}]}} +}; +runAndAssert(filterDoc, expectedResults); // Create filter with $and expression containing invalid call to $ln in it. expectedResults = [ diff --git a/jstests/core/computed_projections.js b/jstests/core/computed_projections.js index fd432771f44..9dd5eb7cab5 100644 --- a/jstests/core/computed_projections.js +++ b/jstests/core/computed_projections.js @@ -4,21 +4,10 @@ load("jstests/aggregation/extras/utils.js"); // For arrayEq and orderedArrayEq. load("jstests/libs/sbe_assert_error_override.js"); -// It is safe for other tests to run while this failpoint is active, so long as those tests do not -// use documents containing a field with "POISON" as their name. Note that this command can fail. -// This failpoint does not exist on mongos and on older version of mongod, which we may be talking -// to in sharding and/or multi-version passthrough suites. The test still functions but with reduced -// coverage. -const isPoisonFailPointEnabled = - db.adminCommand({configureFailPoint: "failOnPoisonedFieldLookup", mode: "alwaysOn"}).ok; -if (!isPoisonFailPointEnabled) { - print("Unable to set 'failOnPoisonedFieldLookup' failpoint."); -} - const coll = db.computed_projection; coll.drop(); const documents = [ - {_id: 0, a: 1, b: "x", c: 10}, + {_id: 0, a: 1, b: "x", c: 10, zero: 0}, {_id: 1, a: 2, b: "y", c: 11}, {_id: 2, a: 3, b: "z", c: 12}, {_id: 3, x: {y: 1}}, @@ -40,7 +29,17 @@ const documents = [ {_id: 19, i: {j: 5}, k: {l: 10}}, {_id: 20, x: [[{y: 1}, {y: 2}], {y: 3}, {y: 4}, [[[{y: 5}]]], {y: 6}]}, {_id: 21, x: [[{y: {z: 1}}, {y: 2}], {y: 3}, {y: {z: 2}}, [[[{y: 5}, {y: {z: 3}}]]], {y: 6}]}, - {_id: 22, tf: [true, false], ff: [false, false], t: true, f: false, n: null, a: 1, b: 0}, + { + _id: 22, + tf: [true, false], + ff: [false, false], + t: true, + f: false, + n: null, + a: 1, + b: 0, + zero: 0 + }, {_id: 23, i1: NumberInt(1), i2: NumberInt(-1), i3: NumberInt(-2147483648)}, {_id: 24, l1: NumberLong("12345678900"), l2: NumberLong("-12345678900")}, {_id: 25, s: "string", l: NumberLong("-9223372036854775808"), n: null}, @@ -646,35 +645,37 @@ const testCases = [ }, // // Test that short-circuited branches do not do any work, such as traversing an array. The - // 'failOnPoisonedFieldLookup' failpoint ensures that none of the "$POISON" lookups, which are - // in short-circuited branches, ever execute. If they were to execute, it would result in an - // error from the query that would cause the test to fail. + // short-circuited branches cointain divide by zero operation which if it were to execute + // would result in an error from the query that would cause the test to fail. // { - desc: "$POISON in short-circuited $and/$or branches", + desc: "$divide by zero in short-circuited $and/$or branches", expected: [{_id: 22, foo: false, bar: true}], query: {_id: 22, a: 1}, - proj: {foo: {$and: ["$f", "$POISON"]}, bar: {$or: ["$t", "$POISON"]}} + proj: { + foo: {$and: ["$f", {$divide: [1, "$zero"]}]}, + bar: {$or: ["$t", {$divide: [1, "$zero"]}]} + } }, { - desc: "$POISON in nested short-circuited $or branches", + desc: "$divide by zero in nested short-circuited $or branches", expected: [{_id: 22, foo: false, bar: true}], query: {_id: 22, a: 1}, proj: { - foo: {$and: ["$f", {$or: ["$f", "$POISON"]}, {$eq: ["$a", 1]}]}, - bar: {$and: ["$t", {$or: ["$t", "$POISON"]}, {$eq: ["$a", 1]}]} + foo: {$and: ["$f", {$or: ["$f", {$divide: [1, "$zero"]}]}, {$eq: ["$a", 1]}]}, + bar: {$and: ["$t", {$or: ["$t", {$divide: [1, "$zero"]}]}, {$eq: ["$a", 1]}]} } }, { - desc: "$POISON in untaken $switch cases", + desc: "$divide by zero in untaken $switch cases", expected: [{_id: 0, foo: "x"}, {_id: 22, foo: 0}], query: {a: 1}, proj: { foo: { $switch: { branches: [ - {case: {$eq: ["$a", 2]}, then: "$POISON"}, - {case: {$eq: ["$a", 3]}, then: "$POISON"} + {case: {$eq: ["$a", 2]}, then: {$divide: [1, "$zero"]}}, + {case: {$eq: ["$a", 3]}, then: {$divide: [1, "$zero"]}} ], default: "$b" } @@ -682,30 +683,32 @@ const testCases = [ } }, { - desc: "$POISON in unevaluated case condition and untaken $switch default branch", + desc: "$divide by zero in unevaluated case condition and untaken $switch default branch", expected: [{_id: 0, foo: "x"}, {_id: 22, foo: 0}], query: {a: 1}, proj: { foo: { $switch: { - branches: - [{case: {$eq: ["$a", 1]}, then: "$b"}, {case: "$POISON", then: "$POISON"}], - default: "$POISON" + branches: [ + {case: {$eq: ["$a", 1]}, then: "$b"}, + {case: {$divide: [1, "$zero"]}, then: {$divide: [1, "$zero"]}} + ], + default: {$divide: [1, "$zero"]} } } } }, { - desc: "$POISON in untaken $cond branch (else)", + desc: "$divide by zero in untaken $cond branch (else)", expected: [{_id: 0, foo: "x"}, {_id: 22, foo: 0}], query: {a: 1}, - proj: {foo: {$cond: {if: {$eq: ["$a", 1]}, then: "$b", else: "$POISON"}}} + proj: {foo: {$cond: {if: {$eq: ["$a", 1]}, then: "$b", else: {$divide: [1, "$zero"]}}}} }, { - desc: "$POISON in untaken $cond branch (then)", + desc: "$divide by zero in untaken $cond branch (then)", expected: [{_id: 0, foo: "x"}, {_id: 22, foo: 0}], query: {a: 1}, - proj: {foo: {$cond: {if: {$eq: ["$a", 2]}, then: "$POISON", else: "$b"}}} + proj: {foo: {$cond: {if: {$eq: ["$a", 2]}, then: {$divide: [1, "$zero"]}, else: "$b"}}} }, // // $let tests. @@ -1115,8 +1118,4 @@ testCases.forEach(function(test) { result, test.expectedErrorCode, Object.assign({result: result}, test)); } }); - -if (isPoisonFailPointEnabled) { - db.adminCommand({configureFailPoint: "failOnPoisonedFieldLookup", mode: "off"}); -} }()); diff --git a/src/mongo/db/exec/sbe/vm/vm.cpp b/src/mongo/db/exec/sbe/vm/vm.cpp index 35dab757964..99464310d2f 100644 --- a/src/mongo/db/exec/sbe/vm/vm.cpp +++ b/src/mongo/db/exec/sbe/vm/vm.cpp @@ -64,9 +64,6 @@ #define MONGO_LOGV2_DEFAULT_COMPONENT ::mongo::logv2::LogComponent::kQuery - -MONGO_FAIL_POINT_DEFINE(failOnPoisonedFieldLookup); - namespace mongo { namespace sbe { namespace vm { @@ -962,10 +959,6 @@ FastTuple ByteCode::getField(value::TypeTag FastTuple ByteCode::getField(value::TypeTags objTag, value::Value objValue, StringData fieldStr) { - if (MONGO_unlikely(failOnPoisonedFieldLookup.shouldFail())) { - uassert(4623399, "Lookup of $POISON", fieldStr != "POISON"); - } - if (objTag == value::TypeTags::Object) { auto [tag, val] = value::getObjectView(objValue)->getField(fieldStr); return {false, tag, val}; diff --git a/src/mongo/db/query/sbe_stage_builder.cpp b/src/mongo/db/query/sbe_stage_builder.cpp index 8fe87acf72b..cc8b4c7fa7c 100644 --- a/src/mongo/db/query/sbe_stage_builder.cpp +++ b/src/mongo/db/query/sbe_stage_builder.cpp @@ -2438,9 +2438,8 @@ std::pair, PlanStageSlots> SlotBasedStageBuilder } if (!groupNode->needWholeDocument) { - // Tracks whether we need to request kResult. One such case is lookup of the '$$POISON' - // field. - bool rootDocIsNeeded = containsPoisonTopLevelField(groupNode->requiredFields); + // Tracks whether we need to request kResult. + bool rootDocIsNeeded = false; auto referencesRoot = [&](const ExpressionFieldPath* fieldExpr) { rootDocIsNeeded = rootDocIsNeeded || fieldExpr->isROOT(); }; diff --git a/src/mongo/db/query/sbe_stage_builder_helpers.h b/src/mongo/db/query/sbe_stage_builder_helpers.h index acf111d5675..3ee2d1a2b35 100644 --- a/src/mongo/db/query/sbe_stage_builder_helpers.h +++ b/src/mongo/db/query/sbe_stage_builder_helpers.h @@ -1113,18 +1113,11 @@ inline StringData getTopLevelField(const T& path) { template inline std::vector getTopLevelFields(const T& setOfPaths) { - const bool testCommandsEnabled = getTestCommandsEnabled(); std::vector topLevelFields; StringSet topLevelFieldsSet; for (const auto& path : setOfPaths) { auto field = getTopLevelField(path); - // If test commands are enabled, then we need to skip any top-level field named - // "POISON" so that the "failOnPoisonedFieldLookup" fail point works correctly. - if (testCommandsEnabled && field == "POISON") { - continue; - } - if (!topLevelFieldsSet.count(field)) { topLevelFields.emplace_back(std::string(field)); topLevelFieldsSet.emplace(std::string(field)); @@ -1134,12 +1127,6 @@ inline std::vector getTopLevelFields(const T& setOfPaths) { return topLevelFields; } -template -inline bool containsPoisonTopLevelField(const T& setOfPaths) { - auto it = setOfPaths.lower_bound("POISON"); - return getTestCommandsEnabled() && it != setOfPaths.end() && getTopLevelField(*it) == "POISON"; -} - template inline std::vector filterVector(std::vector vec, FuncT fn) { std::vector result; -- cgit v1.2.1