[SERVER-30476] Use of exception thrown by ChunkManager::findIntersectingChunk() to determine single shard targeting Created: 02/Aug/17 Updated: 06/Dec/22 |
|
| Status: | Backlog |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Improvement | Priority: | Major - P3 |
| Reporter: | James Wahlin | Assignee: | Backlog - Query Optimization |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | neweng | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Assigned Teams: |
Query Optimization
|
| Participants: |
| Description |
|
There are several places where we call ChunkManager::findIntersectingChunk() as a fast-path check to determine whether a single shard can be targeted for query. If it can't (one case being when shard key collation doesn't match the request) a uassert is triggered, which we then catch and ignore. We should refactor these call sites to not use exceptions as part of the non-error path. Here are a few examples. All callers of ChunkManager::findIntersectingChunk() and ChunkManager::findIntersectingChunkWithSimpleCollation() should be audited as part of this work. https://github.com/mongodb/mongo/blob/20c85d4848b4e4b3c88e1788eaff362143fffd20/src/mongo/s/chunk_manager.cpp#L126-L134 |
| Comments |
| Comment by Andy Schwerin [ 06/Aug/17 ] |
|
I don't think this should just be thought of as a performance problem. Swallowing exceptions without examining the code like that is risky at best. This implementation makes assumptions about all future implementations of findIntersectingChunk. |
| Comment by Kaloian Manassiev [ 03/Aug/17 ] |
|
Yes, 2 is a true error case due to user error so leaving uassert there is acceptable. For 1, I am not clear on the details - but is that a common scenario? Isn't that already slower because we need to cast the collection of the request or do we just resort to broadcast in this case (which is also slow)? |
| Comment by James Wahlin [ 02/Aug/17 ] |
|
The callers I audited have confirmed that they have the shard key before calling. The cases where findIntersectingChunk() will uassert are:
|
| Comment by Kaloian Manassiev [ 02/Aug/17 ] |
|
Is it the case that these methods expect the caller to have the shard key in the passed-in BSON or the check is more complex than that? If it is just a matter of checking for the presence of the shard key, we could prefix the hot paths with that. |