feat(mango): strict index selection

[pgj]

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, automated overrides can still happen.

Introduce the concept of "strict index selection" (h/t @willholley) which lets the user to request the exclusive use of a specific index. When it is not possible, give up on planning and return an HTTP 400 response right away. This way the user has the chance to learn about the missing index, request its creation and try again later.

The feature comes with a configuration toggle. By default, the feature is disabled to maintain backward compatibility, but the user may ask for this behavior via the new use_index_strict query parameter for a specific query. Note that, similarly, use_index_strict could be used to disable strict index selection temporarily when it is enabled on the server.

Fixes #4511

Checklist for the review

• Code is written and works correctly
• Changes are covered by tests
• Any new configurable parameters are documented in rel/overlay/etc/default.ini
• Documentation changes were made in the src/docs folder

[rnewson]

It would be better (RESTful, at least as much as anything else in CouchDB is) to enforce this through the url hierarchy, e.g,

/$dbname/_design/$ddoc/_find/$indexname

i.e, the same as search where a specific index must be specified.

[janl]

/$dbname/_design/$ddoc/_find/$indexname

we already have use_index as the in-JSON way to specify the index. afaict, this PR just makes it mandatory.

I have qualms with adding the URL-way on top of this and later possibly deprecating use_index, but this does not have to hold this particular PR for me.

[pgj]

I agree with @janl. I would be happy to work on making the Mango interface to be consistent with that of Search (I like the idea), but not in the scope of this PR.

[rnewson]

I disagree. The mistake when use_index was added, where it was allowed to not use the index requested, is embedded now. Having a "I really mean it" option, that is optional, and off by default, makes this interface worse, not better.

We should either fix the associated issue (prevent use_index from not using any index other than the one requested) or not. An optional fudge is worse.

[pgj]

I see. But we should keep use_index around as it is now for a while for backward compatibility, right?

[janl]

I see. But we should keep use_index around as it is now for a while for backward compatibility, right?

Yes.

An optional fudge is worse.

I wouldn't frame it these strict terms. I see this PR as a step towards making the Mango-query-index-selection API look similar to JS views and search APIs. IF this PR doesn't get us the whole step there, that’s fine by me as long as we arrive there eventually. IF we want to fix this all in one go, that’s fine by me, but for BC reasons we will need a toggle of some sort for a while and time for folks to migrate off of the no longer preferred APIs.

[rnewson]

the behaviour of use_index can't easily be changed (or, I think we'd all agree, fixed), short of a major version bump.

If there was a roadmap here for couchdb 4.0 (handwaving, but imagine that use_index as an option goes away entirely. you either use /dbname/_find and let couchdb choose, or you use /path/to/specific/index/_find to choose for yourself).

Without that, it's hard to decide whether adding more options is taking us somewhere better or just somewhere more confusing. As a user I would be legitimately baffled to be told I should use use_index_strict when I report that my use_index directive is being ignored.

I'm not sure if I'm -0 or -1 on this, I'm veering to -0 though. I don't like leaving use_index as-is (it should never have been merely a hint) and I don't like adding another argument to do what use_index should have done.

Hence I suggested having _find also reachable from a url that clearly is associated with a specific index resource, like we have for all the other kinds of index we have. That endpoint can return a 400 if the request parameters cannot be executed by the index.

That idea also gives users a migration path away from the flawed choosing algorithm. A future (major) release of CouchDB might then drop /dbname/_find entirely.

[janl]

In general, for API migrations we loosely agreed on: introduce a better API, eventually retire the old one, maybe.

In that light, adding the REST-URL-selects-index route is adding the better API and we can later remove raw _find when we think the time is right.

That is indeed cleaner than adding another toggle to change the behaviour of use_index.

I have no problems with that approach.

[rnewson]

@janl I don't see how this is a step towards "making the Mango-query-index-selection API look similar to JS views and search APIs". my suggestion of adding /path/to/specific/index/_find does that, but expanding /dbname/_find does not. Can you explain the step you see?

[rnewson]

@janl we commented concurrently. I agree with your last. :)

[rnewson]

noting use_index should be prohibited (400 Bad Request) for the /path/to/index/_find variant

[pgj]

That idea also gives users a migration path away from the flawed choosing algorithm. A future (major) release of CouchDB might then drop /dbname/_find entirely.

Why to drop /{db}/find entirely? Why not to fix the index selection algorithm instead? I understand that it might be a nuisance for the users that a less optimal index is selected, but that is why I started to work on #4662 for example.

[janl]

Why to drop /{db}/find entirely?

it was a mistake to allow unindexed queries. We are trying to reverse that mistake now :)

[pgj]

A revisited version of /{db}/find may refuse to run unindexed queries, it does not have to be necessarily removed because currently it makes that possible. Or am I missing something here?

[janl]

sure, but why leave it if we have a better API in place?

[pgj]

I am not sure I got you. In my understanding, /{db}/find provides the possibility of letting CouchDB choose the index for the query. Removing this endpoint would lose this feature. It is hard for me to compare this with explicitly naming the index in the URL — there is no "better" or "worse" API in that sense, I believe. These are two different answers to the same problem: manual vs. automated index selection.

[rnewson]

removing /dbname/_find is only a possibility (that could only happen in a major version bump). I mention it as part of framing any improvement here. Adding the index-specific_find is the couchdb-shaped way to achieve "strict index selection", as that's how it's done for map/reduce and search indexes today. If, in the future, we find that /dbname/_find is much less used / actively avoided / a source of confusion not joy, we could deprecate it.

[nickva]

More advanced query engines allow selecting multiple indices to answer a query. It's not impossible, for Mango to be improved to do it as well. For instance, one pain point of Mango is it can't use indices for general disjunctions today; but it might be possible to do that by doing a merge on two indices. If the new Mango query API is to specify only one index, that API choice will work against that improvement.

Another, more pessimistic view, I guess is to say Mango is never going to be anything but a layer on top of an mrview index and we shouldn't pretend otherwise. Then selecting an index and forcing that there be an index defined is probably reasonable.

[iilyak]

I agree with Nick that we would not want to put ourselves in the corner by embedding assumption about single index into endpoint name.

I also agree with Bob and Jan that adding more option to existing endpoint is not the best choice in this particular case. Specifically because the option would be off by default for compatibility reasons. Also adding new options to existing API endpoints complicates maintaining of client libraries.

I think we should come up with a new endpoint name but shouldn't embed index name into it.

I am bad at naming and naming is hard.

/{dbname}/_find_indexed
/{dbname}/_select - // inspired by SQL
/{dbname}/_find_select - // find using selection of indexes

Let's ask help from @ricellis who has an extensive experience in designing APIs.

The decision whether to keep _find in the future or remove it is out of the scope for this PR.

[rnewson]

The stated purpose of this PR is to allow a user to run a selector against a specific individual index or receive an error stating that the index is not compatible, rather than fallback to a less efficient index.

Let us focus on the best way to achieve that specific goal, and whether to then do so.

Speculation about the future of /dbname/_find has muddied the waters. I am sorry for my part in that.

For map/reduce views and search indexes (either dreyfus/clouseau or nouveau) this is done having the path to the index in the url.

Inventing a third mechanism (as Ilya proposes) for this is not acceptable to me; our API is already complex enough.

If we want a facility to run a selector against a user-selected index, then I only see one way: /db/_design/design-doc/_find/index-name.

If we don't want that, then we need do nothing.

[iilyak]

If we don't want that, then we need do nothing.

I don't have that strong of opinion. My only strong opinion is to involve Rich and his team in the design.

[rnewson]

Agreed, would be good to hear from Rich et al

[willholley]

Appreciating the difficulties with augmenting existing endpoints, I think it's worth considering adding "selector" and "fields" support to the existing _all_docs, view and search endpoints. This would be consistent with the _changes and _find endpoints and provides most of the benefits of a single-index _find query, albeit with a more explicit API. It would also enable the filtering functionality for existing view/search indexes whereas Mango makes assumptions about the index key structure that hand-crafted indexes would need to replicate if _find we're to be supported for them.

…

On Tue, 1 Aug 2023, 23:26 Robert Newson, ***@***.***> wrote: Agreed, would be good to hear from Rich et al — Reply to this email directly, view it on GitHub <#4710 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAX365O3ZXNF4RRRIJZHXLXTFYBHANCNFSM6AAAAAA25IW4QM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[rnewson]

oooh I like that.

[rnewson]

one further note here. when, in the future, you pass a selector to _view or _search we should send a 400 Bad Request if mango would do a startkey/endkey that covers the entire index (for _view that's when it does "start_key": [], "end_key": ["<MAX>"],.)

[pgj]

Okay, thanks for the note @rnewson.

[mikerhodes]

adding a note that for the new endpoints an index should not match a selector if it only has indexed fields where the selector does $regex or any other operator where neither startkey or endkey are changed from the defaults that cover the entire view.

I disagree that we should implement this restriction. The issue the user reported is "use_index doesn't do what it says". It wasn't "I don't like that the database does inefficient queries". Even if it was, I'd still disagree.

I do agree that a good way to solve the original bug is to introduce the URL framing for selecting an index. But I think the only reason we'd return a 400 would be that the index in the URL would be disqualified using the standard Mango index selection logic for the given selector (eg, selector doesn't contain the fields in an "indexable" way).

Specifically, I don't think the user should be prevented from passing in a selector that passes the index selection logic, but passes values which mean the entire view is read.

I think this because:

1. I think this is confusing for a user --- both views and search allow this, so why have this restriction only for Mango?
2. I don't think "because the user can shoot themselves in the foot" is a good reason either, because it's not what I'd want for CouchDB to do on my behalf. Databases provide many, many ways to shoot oneself in the foot. And they should do, that's why they are powerful.

In summary, I agree the URL method is a good solution to this problem, which is that use_index is bad because it doesn't do what it says. I feel that we should get use_index deprecated and a replacement into the codebase, and avoid trying to "fix" Mango in other ways at the same time.

[rnewson]

@mikerhodes I'm not sure we're disagreeing. The example I cite is when a field is indexed but is only in the selector as a $regex. $regex is brute-forced, it doesn't use any indexed field. aka, the field used for a $regex should not count, since whether it is indexed or not is irrelevant to evaluation.

whatever the formula currently is for /db/_find should remain untouched, sure, but we should take the opportunity to remove foot guns where we can.

_find is not powerful. It just pretends to be.

[rnewson]

as for comparisons to calling _view and _search with no restrictions, I don't think that's true, and the details matter.

Querying a view without startkey/endkey can make sense and can be efficient as it's quite common for the view to be a subset of the database (any if clause on doc properties, say). And it could be a reduce=true or group=true and the view-wide reduce value might be valuable (and is efficiently calculated).

Querying a search index with *:* is also sometimes reasonable (as it might be a subset of the database as above) and will certainly be more efficient, Lucene won't read the whole index to perform the query, it'll just return the first N results it finds (as a limit is not optional for Lucene as it is for views).

You can tell I am tired of dealing with users who have been let down by mango and its false promises.

[ricellis]

Controversial I'm sure, but can we go back to basics here?
Does anyone actually think use_index should be a hint?

The documentation says:

Instruct a query to use a specific index.

That doesn't even seem that ambiguous, given computers follow instructions.
Honestly it just seems like a long standing bug to me. Every user I've interacted with who has tried to use this option (including past me) does not think this instruction is optional for the server. I can't think of a good reason why it would ever be a "hint". If you want auto-selection, don't say use_index if you want it to use a specific index then specify use_index. 🤷

[big-r81]

You can tell I am tired of dealing with users who have been let down by mango and its false promises.

This is completely understandable and comprehensible and yes, you always answer the same questions and misunderstandings ;-). Maybe this is also one of the reasons that I have not used Mango much.

I think we shouldn't mix different things here though, nor personal inclinations / dislikes to various functions.

I think we can and should improve the current behavior as best as possible (in the sense of the user and operator) (and best of all, without introducing an additional configuration parameter).

If we want to preserve a backward-comatable behavior, can't we explicitly tell the user in the result if an index was used or not (more visible than just in _explain) and without interpreting the "execution statistics"?

Now I would also have no problem with marking this as backwards incompatible in the changlog! If we follow the documentation, then we just correct the behavior (which was wrong until now).

[rnewson]

@ricellis I agree that use_index should mean what it says and we should fix that.

@big-r81 how do you you propose to "explicitly tell the user in the result if an index was used"? We already send a warning in every _find response if it was executed inefficiently, and absolutely no one notices this. How could they? They get their results, though slower than if they had used a better index (and, for small datasets, not even that. The problem only appears when they add more data).

[rnewson]

@ricellis separately, what's your opinion on mango considering an index usable for a selector for $regex operator? I consider that a bug also. The index isn't usable for that query, it's no better than all_docs unless it was a partial index.

[big-r81]

@rnewson Mhhh, then we should not output a result if a user specifies an index that does not exist or cannot be used. Then we should just issue a warning as a result and specify "Use the correct index or do not use use_index at all". Then the user can decide what to do. Of course, this would not be backward compatible behavior either.

[nickva]

Good point, @ricellis. This looks more like a bug fix than adding a new feature. Documentation agrees too.

Not sure about $regex maybe that falls into a separate category of returning efficient results. The index itself could look silly and just index everything equivalently to an _all_docs anyway, even without a $regex involved.

[ricellis]

what's your opinion on mango considering an index usable for a selector for $regex operator?

That problem extends beyond $regex doesn't it? My undersanding is that (at least for json/view indexes) no (non-partial) index is more useful than _all_docs unless the selector has at least one "equality" operator ($eq, $gt, $gte, $lt, $lte) that can be used to narrow the set of docments to subsequently scan for the rest of the constraints. Or am I missing something particularly extra about regex?

when a field is indexed but is only in the selector as a $regex. $regex is brute-forced, it doesn't use any indexed field

Does brute-forced in this context mean it uses the keys from the view or does it actually pull the docs?
I think the "covering" work only applied to avoiding fetches where the projected fields were already in the key, but potentially a "covering" style improvement to regex where the keys from the index can used to evaluate the regex before fetching docs could make an index on the requested field more useful than an alternative.

[pgj]

Thank you all for the comments, it is much appreciated.

How about the following:

• Since changing the current semantics of use_index would break many existing integrations, even if it is indeed a bug, let us keep that for backwards compatibility and introduce index_hint for the current behavior (as an alias to use_index) and force_index for the desired behavior.

• Create another PR for the index_hint + force_index work and we leave this thread as the generic Mango discussion although it might be better to find a designated place for that.

• Spin off the parts about the suspected bug about $regex into the ticket and continue the related discussion there.

[willholley]

$regex gets converted into an $exists: true predicate for index selection, so an index can be used to optimize it in some cases, though it's still going to be a range scan on whatever index is selected. I don't think there's a bug there, though perhaps the performance could be improved as per @ricellis's suggestion.

[rnewson]

@pgj imo we either make use_index do what it says or we introduce the path-based _find endpoints that work like _view and _search in guaranteeing the request is served by that index (with the unique caveat that it's a 400 if the selector isn't covered by the index). I don't want to go the more confusing route of adding yet more parameters that mean 'really use the index'.

[pgj]

Curiously, use_index has always been considered a hint in the sources, see the comments in mango_cursor where the warnings are generated:

couchdb/src/mango/src/mango_cursor.erl

Lines 475 to 509 in 435db51

	% warn if user specified an index which doesn't exist or isn't valid
	% for the selector.
	% In this scenario, Mango will ignore the index hint and auto-select an index.
	invalid_index_warning(Index, Opts) ->
	UseIndex = lists:keyfind(use_index, 1, Opts),
	invalid_index_warning_int(Index, UseIndex).
	invalid_index_warning_int(Index, {use_index, [DesignId]}) ->
	Filtered = filter_indexes([Index], DesignId),
	if
	Filtered /= [] ->
	[];
	true ->
	couch_stats:increment_counter([mango, query_invalid_index]),
	Reason = fmt(
	"_design/~s was not used because it does not contain a valid index for this query.",
	[ddoc_name(DesignId)]
	),
	[Reason]
	end;
	invalid_index_warning_int(Index, {use_index, [DesignId, ViewName]}) ->
	Filtered = filter_indexes([Index], DesignId, ViewName),
	if
	Filtered /= [] ->
	[];
	true ->
	couch_stats:increment_counter([mango, query_invalid_index]),
	Reason = fmt(
	"_design/~s, ~s was not used because it is not a valid index for this query.",
	[ddoc_name(DesignId), ViewName]
	),
	[Reason]
	end;
	invalid_index_warning_int(_, _) ->
	[].

Is this a bug in the documentation itself?

couchdb/src/docs/src/api/database/find.rst

Lines 45 to 47 in 83e39b6

	:<json string|array use_index: Instruct a query to use a specific index.
	Specified either as ``"<design_document>"`` or
	``["<design_document>", "<index_name>"]``. *Optional*

The use of "instruct" there might be a poor choice which causes so much pain and misunderstanding. Personally, I can see the intention behind the wording but I can also understand that it is misleading at the first sight. (Being instructed does not necessarily warrant that one will obey. But I am not either a native speaker or a linguist.)

[rnewson]

Instruct

to order or tell someone to do something, especially in a formal way:

[pgj]

Submitted #4774 to fix the documentation.

[pgj]

Created #4775 for the problem with the $regex operator.

[rnewson]

another option occured to me. Similar to the initial proposal, we could introduce allow_fallback: true|false as a new option, defaulting to true to maintain current behaviour. We'd also update the documentation as in #4774 to clarify that use_index is a hint by default.

When allow_fallback is false, the behaviour changes as follows;

1. if use_index is set, and the index selected is not a match for the selector, we return a 400 Bad Request instead of falling back to a different index or all_docs.
2. if use_index is not set, and there is no index that can be used, we return a 400 Bad Request instead of falling back to all_docs.

If this exists we can encourage users to always set allow_fallback: false in _find requests, whether they use use_index or not, so they can avoid the worst of the fallback behaviour.

[ricellis]

I like this allow_fallback idea. I think it is a good compromise to allow use_index to behave like everyone expects without breaking backwards compatibility. Having purpose as an option beyond use_index cases makes it more useful than adding a new toggle that only impacts use_index.

[big-r81]

Do we really need backwards compatability here?

Let me give you a practical example:

There is a road with allowed max. speed of 50 km/h and a digital display board which shows your current speed and a 😄 if you drive less than or equal to 50 km/h and 😞 if you drive too fast. You drive this way for years and you drive always to fast because there are obviously no dangers. One day I got a speeding ticket because I was going too fast (like always)...

1. Should I declare to the fine office that I have always driven too fast up to now? (Who do you think will be proven right?) or
2. Should I just pay the ticket and drive only 50 km/h on this route from now on?

In this case - I think - we should fix use_index to use that index or fail if the index is not available / usable. No additional config setting. If we would introduce an additional config option for every functional change, we would

1. increase the complexity of the docs and
2. the complexity of the software in general and
3. (maybe) introduce additional dependencies between different settings and
4. is there a day when we would remove all deprecated and additional bc compatible settings?

So, just my 2 cents...

But, if we go the BC route, than allow_fallback is an elegant solution.

[rnewson]

@big-r81 the difference between your scenario and this one is that you know you are exceeding a limit. The problem with the fallback behaviour of _find is that you don't know.

[ricellis]

Perhaps some administrative config for both:

• the default setting of allow_fallback
• and whether to honor allow_fallback from requests

would provide enough control to allow admins to enforce no fallbacks when they have no compatibility concerns. That would also give a mechanism of flipping the default config value on the major version boundary.

[pgj]

@rnewson, @ricellis I like the proposed ideas and I am glad that we could utilize the original proposal but in a better way. Thanks for helping us with getting to this solution!

@big-r81 we need the backwards compatibility. Personally, I see at least two reasons for that:

• It is like that for about 7 years now, but there might be integrations that rely on this (good or bad, it does not matter) behavior. CouchDB already offers support for multi-version cluster upgrades for changes like that, that is basically mandatory.

• This is not a bug but a feature. The use_index parameter was introduced in 1b0426a to force the index selection to a specific index. Unfortunately, this did not work out well in practice, so 743bd88 added a fallback mechanism to make it less brittle.

I do not have plans to remove the fallback at the moment. I do not see it an obvious bad thing but at the same time I understand that this has many disadvantages that may confuse or surprise the users. Actually, more "noise" around this in the documentation and in the configuration toggles could help to raise the awareness. I do not see that much associated extra work either (it is a small change if you check the current version), but I am willing to take on those, if needed.

[willholley]

I agree with changing the behavior of use_index, but I'm less keen on overloading it to removing the use of the _all_docs index. There are legitimate reasons to use _all_docs and it would be a breaking change for some clients (thinking particularly things like Loopback which fetch docs by _id using _find). Making that breaking change a cluster-wide toggle then makes portability between CouchDB deployments difficult / unpredictable.

It may also be useful context that Mango did originally prevent the _all_docs fallback unless the query contained a clause that would require it. What we saw, certainly in Cloudant, was that lots of users just blindly added a { _id: { "$gt": 0 } } clause to their selectors to work around the restriction. I think if we want to limit the overhead of index / db scans (accidental or otherwise), there are better options available that would be less breaking (e.g. check the magnitude of the scan before executing the query).

[rnewson]

@willholley what cluster-wide toggle? I'm talking about allow_fallback: true|false in the _find request body. Opt-in. And we had (momentarily, it seems) reached consensus on not changing the default behaviour of use_index.

[willholley]

@rnewson #4710 (comment).

[rnewson]

@willholley aha! I missed it the first time. I agree that those two suggested toggles just complicate things. I suggest it's just the opt-in in the _find request body for now. couchdb could flip the default in code in a future major release (but I think it's unlikely).

[pgj]

Please note that #4792 has been opened as an alternative proposal in response to the comments in this conversation.