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

feat(mango): add allow_fallback for user-specified indexes on _find #4792

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pgj
Copy link
Contributor

@pgj pgj commented Oct 4, 2023

This is an alternative to #4710 based on the results of the related discussion.

It is not always beneficial for the performance if the Mango query planner tries to assign an index to the selector. User-specified indexes may save the day, but since they are only hints for the planner, fallbacks may still happen.

Introduce the allow_fallback flag which can be used to tell if falling back to other indexes is acceptable when an index is explicitly specified by the user. When set to false, give up on planning and return an HTTP 400 response right away. This way the user has the chance to learn about the requested but missing index, optionally create it and try again.

By default, fallbacks are allowed to maintain backwards compatibility. It is possible to set allow_fallback to true but currently it coincides with the default behavior hence becomes a no-op in practice.

Fixes #4511

Testing recommendations

The changes could be verified by the following commands:

make eunit apps=mango
make mango-test

This is a non-breaking change, so there should be no observable difference in the behavior for the users. When allow_fallback is set to false and use_index is set to some index, it may return HTTP 400 depending on the suitability of the given index.

Checklist

  • Code is written and works correctly
  • Changes are covered by tests
  • Documentation changes were made in the src/docs folder

@pgj pgj mentioned this pull request Oct 4, 2023
4 tasks
@pgj pgj force-pushed the feat/mango/find/use_index_allow_fallback branch from fcb727c to f13501e Compare October 4, 2023 20:48
It is not always beneficial for the performance if the Mango query
planner tries to assign an index to the selector.  User-specified
indexes may save the day, but since they are only hints for the
planner, fallbacks may still happen.

Introduce the `allow_fallback` flag which can be used to tell if
falling back to other indexes is acceptable when an index is
explicitly specified by the user.  When set to `false`, give up
on planning and return an HTTP 400 response right away.  This way
the user has the chance to learn about the requested but missing
index, optionally create it and try again.

By default, fallbacks are allowed to maintain backwards
compatibility.  It is possible to set `allow_fallback` to `true`
but currently it coincides with the default behavior hence becomes
a no-op in practice.

Fixes apache#4511
The `binary:split/2` function will split the input only at the
first occurrence of the pattern which is not suitable for
identifying each part of the index name, therefore validating it.
Add the `global` option to fully break the index name into parts
and let the related validation logic work as expected.
@pgj pgj force-pushed the feat/mango/find/use_index_allow_fallback branch from f13501e to 449edf0 Compare October 5, 2023 15:10
Copy link
Contributor

@nickva nickva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Nicely done, @pgj!

Perhaps we could get a few additional approvals from anyone involved in the original discussion #4710

@pgj
Copy link
Contributor Author

pgj commented Oct 5, 2023

Yes, I agree. @rnewson, @willholley, @ricellis please comment if you have the time.

@ricellis
Copy link

ricellis commented Oct 5, 2023

I was under the impression that allow_fallback was going to apply to all _find requests not just those with use_index. I know @willholley disagreed with that overloading, but I thought that was only related to the cluster toggles. I didn't think there was an objection to disallowing all_docs fallback as long as applied consistently.

@pgj
Copy link
Contributor Author

pgj commented Oct 9, 2023

I do not want to respond on behalf of @willholley but in my understanding, in his respective comment he opposed to extending the allow_fallback mechanism to _all_docs: "I'm less keen on overloading it to removing the use of the _all_docs index". He also included a historical reference to suggest that it would not give us much: "Mango did originally prevent the _all_docs fallback unless the query contained a clause that would require it. [..] lots of users just blindly added a { _id: { "$gt": 0 } } clause to [..] work around the restriction."

@rnewson
Copy link
Member

rnewson commented Oct 11, 2023

if allow_fallback: false will allow fallback, then it will not be merged.

@pgj
Copy link
Contributor Author

pgj commented Oct 11, 2023

@rnewson do you mean that we shall extend this change to disabling the _all_docs fallback as well?

@rnewson
Copy link
Member

rnewson commented Oct 11, 2023

Yes. Remember this an opt-in in the request body by the user. They are asking us to not fallback; they want an error instead of inefficient execution. I do not understand @willholley's objection to this.

@willholley
Copy link
Member

I don't think this issue / PR has anything to do with query efficiency - only fixing the unclear semantics of use_index.

I'm open to a discussion about if we want to / how to prevent use of inefficient indexes or, but that's much broader problem space in which I don't see _all_docs as any different to user-defined indexes.

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.

Mango queries should not use other index when use_index is specified
5 participants