Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MONGOID-5030 - Skip contradicting queries #5734

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

johnnyshields
Copy link
Contributor

@johnnyshields johnnyshields commented Oct 18, 2023

(This PR is currently a Proof-of-Concept.)

What?

This PR implements short-circuit logic for the $in, $nin, and equality operators, in cases where the query condition deterministically results in a "none" result.

Why?

When we upgraded to MongoDB 7.0.2 from 6.x, we discovered the hard way (~4 hours of downtime) that the new Slot-Based Query Execution Engine (SBQEE) behaves differently than the Classic Engine.

In particular, we had a query with a shape like this in a collection with 250M+ records:

Reservation.where(start_time: t1..t2).lt(end_time: t2).any_in(table_ids: [n1, n2]).sort(shop_id: 1, start_time: 1)

Previously, when table_ids: { "$in" => [] } (empty array) was present, the Classic Engine was optimized to skip the query. However, the SBQEE does a full collection scan. If these queries pile-up, database CPU goes to 100% and its game over. We believe this behavior is a bug; SERVER ticket coming soon.

Solution

In MONGOID-5030 which I raised in 2020, I proposed to short-circuit these queries. ActiveRecord 6.1 did this in this PR. At the time the MongoDB answered that the query engine was optimized to skip the queries, but I think the introduction of the SBQEE makes this more urgent.

I considered 3 possible approaches:

  1. Implement the skip in Criteria.in method, at the time conditions are added to the selector.
  2. Implement a filter in Mongoid::Contextual::Mongo before the query goes to the driver, and detect contradictions in the final query.
  3. Similar to #2 but do in Mongo Ruby Driver.

Option #1 has the disadvantage that it doesn't cover cases like Person.where(name: { "$in" => [] }), so I ruled it out. Option #3 I investigated, but it would require introducing the concept of a "Null Operation" to the driver, which increases complexity. Option #2 we already have a precedent with Mongoid::Contextual::None so it might be the best approach.

This PR is currently a POC to illustrate where I will insert the logic. I will add a feature flag for this, however, I believe it is safe to implement.

Truth Tables

For reference. These tables illustrate the logic that can be short circuited. This PR will only cover the none case.

$in

Expression Boolean Equivalent Can Short-circuit?
$in => [1, 2] ? No
$in => [] F Yes (same as none)
$in => [1, 2], $in => [] ? && F == F Yes (same as none)
$in => [], $in => [] F && F == F Yes (same as none)
$not => { $in => [1, 2], $in => [] } !(? && F) == T Yes (same as all)
$not => { $in => [], $in => [] } !(F && F) == T Yes (same as all)

$nin

Expression Boolean Equivalent Can Short-circuit?
$nin => [1, 2] ? No
$nin => [] T Yes (same as all)
$nin => [1, 2], $nin => [] ? && T == ? No
$nin => [], $nin => [] T && T == T Yes (same as all)
$not => { $nin => [] } !T == F Yes (same as none)
$not => { $nin => [1, 2], $nin => [] } !(? && T) == ? No
$not => { $nin => [], $nin => [] } !(T && T) == F Yes (same as none)

@johnnyshields
Copy link
Contributor Author

@jamis I'd appreciate your review on what I'm trying to achieve here. I'm glad to code this to completion.

@jamis
Copy link
Contributor

jamis commented Oct 18, 2023

@johnnyshields Thanks for raising this, Johnny. We're discussing it internally and will let you know. In the meantime, as mentioned in the HELP ticket you raised, it sounds like this server bug was fixed in the 7.0.3 patch release.

@johnnyshields
Copy link
Contributor Author

@jamis any update on the direction here? We consider this issue pretty critical after our experience with SERVER-79088

@alexbevi
Copy link
Contributor

alexbevi commented Nov 7, 2023

Hi @johnnyshields, and thanks for raising this PR. The initial motivation for MONGOID-5030 was to recreate an optimization from AR6.1 that would prevent certain query shapes from being sent to the server.

Mongoid attempts to match Rails' APIs wherever possible, and incorporate optimizations where appropriate. As there were users reporting issues with the approach in AR6.1, we want to ensure we account for similar potential challenges before incorporating short-circuit logic.

Though we appreciate there was a recent issue (SERVER-79088), this PR shouldn't be considered a "bug fix" as the underlying bug should already be addressed (and will be included in the upcoming 7.0.3 server release).

This PR/ticket is still on our radar, however we want to be extremely careful as to how we approach logic changes that silently alter the intent of a user operation.

@johnnyshields
Copy link
Contributor Author

johnnyshields commented Nov 7, 2023

@alexbevi It is fine to primarily consider this a "latency optimization" and also "ActiveRecord parity".

(That being said, no query == zero possibility of a server bug 😄)

@johnnyshields
Copy link
Contributor Author

johnnyshields commented Nov 8, 2023

@alexbevi

We want to ensure we account for similar potential challenges before incorporating short-circuit logic.

The AR issue you referenced wouldn't happen from the proposed change in this PR. Here we will leverage the existing Mongo::Contextual::None abstraction which represents a non-executable query, and things like count, sum etc. are already well-defined/tested to return the appropriate result (e.g. 0 in the case of count and sum.)

Of course we always need to be careful with every change, but would like to paint the picture that this change is less risky than you might think.

@johnnyshields
Copy link
Contributor Author

johnnyshields commented Nov 11, 2023

Another case which can be handled client side is $in with a nested $ operator -- currently it raises a server-side error; this error could be moved client side.

Mongo::Error::OperationFailure 
[2:BadValue]: cannot nest $ under $in

@alexbevi
Copy link
Contributor

@johnnyshields I've added more detail to MONGOID-5030 regarding where this feature fits on the roadmap at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants