|
The IndexBoundsChecker incorrectly computed the orientation of the index as being descending on "a", but sees that the index cursor is doing a forward scan of the index. The hang is an infinite loop that comes from the IndexBoundsChecker viewing the index cursor as positioned BEHIND where it should be to search the next interval; IndexBoundsChecker::checkKey() returns MUST_ADVANCE and causes the index cursor to seek to where it is currently positioned.
Consider the situation where the collection contains only the document {a: 'string'} and the index {a: NaN}. The query {a: {$in: [1, 3]}} will then try to search the intervals [1, 1] and [3, 3] of that index.
1. With the debugger, we can learn that key is {"" : "string"} in IndexBoundsChecker::checkKey(key, out).
2. The _expectedDirection of the index is incorrectly identified by the index bounds checker as -1 because NaN >= 0 returns false.
3. The index bounds checker compares the key to the first interval [3, 3]. (Where "first" is from the perspective of a descending index on the "a" field.) intervalCmp([3, 3], { "" : "string" }, -1) returns BEHIND because bsonWoCompare({"": "string"}, {"" : 3}) > 0 (mongo::NumberDouble comes before mongo::String).
4. The following IndexSeekPoint is generated:
keyPrefix = { "" : "string" }
|
prefixLen = 0
|
prefixExclusive = false
|
keySuffix = [ { "" : 3 } ]
|
suffixInclusive = [ true ]
|
5. The index cursor seeks to {"" : 3} and remains positioned at the document {a: 'string'} (rather than say EOF) because it's a forward cursor and an ascending index on the "a" field. The index bound checker sees the cursor still as behind the position of where it needs to be to search the next interval and tells it to see to the same IndexSeekPoint ad infinitum.
|
|
max.hirschhorn, that patch looks pretty sketchy to me, since
- we'd have to find all the places that use (elt.number() >= 0) as an ascending check, and
- it makes the correct way to check for ascending/descending even more subtle.
I'm tempted to fold this into SERVER-11064, schedule SERVER-11064, and then resolve this as a duplicate of SERVER-11064. We'd have to think carefully about what kind of values we can reject inside an index key pattern without breaking applications, but I think the following we could do safely:
- Reject NaN
- Reject undefined
- Reject JavaScript code
- Reject BinData
This would plug in nicely in index_key_validate.cpp and index_key_validate_test.cpp. Does that plan sound good to you?
One question: why does the planner's bounds error result in a server hang?
|
|
This appears to be a bug in index bounds generation. The Ordering class considers the index {a: NaN} to be ascending on "a", but bounds are generated as though the index was descending on "a".
> db.foo.drop()
|
> db.foo.insert({a: 1})
|
> db.foo.createIndex({a: NaN})
|
> db.foo.find({})
|
{ "_id" : ObjectId("567730db95459c76838b7c76"), "a" : 1 }
|
> db.foo.find({}).hint({a: NaN})
|
> db.foo.find({}).hint({a: NaN}).explain()
|
{
|
"queryPlanner" : {
|
"plannerVersion" : 1,
|
"namespace" : "test.foo",
|
"indexFilterSet" : false,
|
"parsedQuery" : {
|
"$and" : [ ]
|
},
|
"winningPlan" : {
|
"stage" : "FETCH",
|
"inputStage" : {
|
"stage" : "IXSCAN",
|
"keyPattern" : {
|
"a" : NaN
|
},
|
"indexName" : "a_NaN",
|
"isMultiKey" : false,
|
"isUnique" : false,
|
"isSparse" : false,
|
"isPartial" : false,
|
"indexVersion" : 1,
|
"direction" : "forward",
|
"indexBounds" : {
|
"a" : [
|
"[MaxKey, MinKey]"
|
]
|
}
|
}
|
},
|
"rejectedPlans" : [ ]
|
},
|
"serverInfo" : {
|
"host" : "gopher-blue",
|
"port" : 27017,
|
"version" : "3.2.0",
|
"gitVersion" : "45d947729a0315accb6d4f15a6b06be6d9c19fe7"
|
},
|
"ok" : 1
|
}
|
> db.foo.find({a: 1}).hint({a: NaN})
|
{ "_id" : ObjectId("567730db95459c76838b7c76"), "a" : 1 }
|
> db.foo.find({a: {$in: [2, 1, 3]}}).hint({a: NaN}).explain()
|
{
|
"queryPlanner" : {
|
"plannerVersion" : 1,
|
"namespace" : "test.foo",
|
"indexFilterSet" : false,
|
"parsedQuery" : {
|
"a" : {
|
"$in" : [
|
1,
|
2,
|
3
|
]
|
}
|
},
|
"winningPlan" : {
|
"stage" : "FETCH",
|
"inputStage" : {
|
"stage" : "IXSCAN",
|
"keyPattern" : {
|
"a" : NaN
|
},
|
"indexName" : "a_NaN",
|
"isMultiKey" : false,
|
"isUnique" : false,
|
"isSparse" : false,
|
"isPartial" : false,
|
"indexVersion" : 1,
|
"direction" : "forward",
|
"indexBounds" : {
|
"a" : [
|
"[3.0, 3.0]",
|
"[2.0, 2.0]",
|
"[1.0, 1.0]"
|
]
|
}
|
}
|
},
|
"rejectedPlans" : [ ]
|
},
|
"serverInfo" : {
|
"host" : "gopher-blue",
|
"port" : 27017,
|
"version" : "3.2.0",
|
"gitVersion" : "45d947729a0315accb6d4f15a6b06be6d9c19fe7"
|
},
|
"ok" : 1
|
}
|
david.storch, should we apply (something like) the following change to make the index bounds builder consistent with the Ordering class, or should we disallow the creation of indexes with a key pattern using NaN?
diff --git a/src/mongo/db/query/index_bounds.cpp b/src/mongo/db/query/index_bounds.cpp
|
index 85708c6..678affd 100644
|
--- a/src/mongo/db/query/index_bounds.cpp
|
+++ b/src/mongo/db/query/index_bounds.cpp
|
@@ -323,7 +323,7 @@ bool IndexBounds::isValidFor(const BSONObj& keyPattern, int direction) {
|
|
// Special indices are all inserted increasing. elt.number() will return 0 if it's
|
// not a number. Special indices are strings, not numbers.
|
- int expectedOrientation = direction * ((elt.number() >= 0) ? 1 : -1);
|
+ int expectedOrientation = direction * ((elt.number() < 0) ? -1 : 1);
|
|
if (!field.isValidFor(expectedOrientation)) {
|
return false;
|
@@ -343,7 +343,7 @@ IndexBoundsChecker::IndexBoundsChecker(const IndexBounds* bounds,
|
: _bounds(bounds), _curInterval(bounds->fields.size(), 0) {
|
BSONObjIterator it(keyPattern);
|
while (it.more()) {
|
- int indexDirection = it.next().number() >= 0 ? 1 : -1;
|
+ int indexDirection = it.next().number() < 0 ? -1 : 1;
|
_expectedDirection.push_back(indexDirection * scanDirection);
|
}
|
}
|
diff --git a/src/mongo/db/query/index_bounds_builder.cpp b/src/mongo/db/query/index_bounds_builder.cpp
|
index 364f957..2fcef73 100644
|
--- a/src/mongo/db/query/index_bounds_builder.cpp
|
+++ b/src/mongo/db/query/index_bounds_builder.cpp
|
@@ -860,9 +860,11 @@ void IndexBoundsBuilder::alignBounds(IndexBounds* bounds, const BSONObj& kp, int
|
size_t oilIdx = 0;
|
while (it.more()) {
|
BSONElement elt = it.next();
|
- // The canonical check as to whether a key pattern element is "ascending" or "descending" is
|
- // (elt.number() >= 0). This is defined by the Ordering class.
|
- int direction = (elt.number() >= 0) ? 1 : -1;
|
+ // Match the behavior of the Ordering class when considering whether the direction of a key
|
+ // pattern element 'elt' is "ascending" or "descending". The Ordering class defines the
|
+ // direction to be "descending" if 'elt.number() < 0', and "ascending" otherwise. In
|
+ // particular, {field: NaN} is considered to be an ascending index on "field".
|
+ int direction = (elt.number() < 0) ? -1 : 1;
|
direction *= scanDir;
|
if (-1 == direction) {
|
vector<Interval>& iv = bounds->fields[oilIdx].intervals;
|
|