Fix query limit - #5515
Merged
Merged
Conversation
nickva
force-pushed
the
fix-query-limit-for-all-docs
branch
9 times, most recently
from
April 26, 2025 04:27
80ef735 to
370c1c0
Compare
nickva
commented
Apr 26, 2025
nickva
commented
Apr 26, 2025
nickva
commented
Apr 26, 2025
nickva
commented
Apr 26, 2025
nickva
force-pushed
the
fix-query-limit-for-all-docs
branch
from
April 27, 2025 18:32
370c1c0 to
a37253c
Compare
jaydoane
approved these changes
Apr 27, 2025
jaydoane
left a comment
jaydoane
left a comment
Contributor
There was a problem hiding this comment.
Nice improvement, but I'm a little worried that you've moved validation around without explicitly testing that validation. Is it worth adding a couple more tests to tighten that up?
| ?MAX_VIEW_LIMIT | ||
| ), | ||
| MaxLimit = | ||
| case config:get("query_server_config", LimitType, "infinity") of |
Contributor
There was a problem hiding this comment.
Nice ergonomic improvement!
Contributor
Author
|
Good idea, Jay, to add a few more tests. I assumed we tested those somewhere else already but it's apparently not the case. |
nickva
force-pushed
the
fix-query-limit-for-all-docs
branch
3 times, most recently
from
April 30, 2025 05:00
876a7a9 to
ef4354d
Compare
Previously, we set a default limit that was an effective infinity (2^28). It seems back in the 32 bit days that was the Erlang's largest small integer [1]. However, that turned out to be too low and it surprised a user when it truncated their all_docs output skipping some of the data. Fix that by increasing the limit to a larger "infinity" (highest 64 bit Erlang small integer [1]). We did have a "query_limit" config parameter to customize the limit, however that turned out to be broken and did not take effect when the user tried it for all_docs, so fix that as well. Fix that and use a test to ensure the limit gets reduced appropriately. To make the setting more user friendly, allow `infinity` as the value. Also, in the case of all_docs, we validated args and applied the limit check twice: once in the coordinator and another time on each worker, which wasted CPU resources and made things a bit confusing. To fix that, remove the validation from the common worker code in couch_mrview and validate once: either on the coordinator side, or local (port 5986) callback, right in the http callback. [1] https://www.erlang.org/doc/system/memory.html Fix #5176
nickva
force-pushed
the
fix-query-limit-for-all-docs
branch
from
April 30, 2025 05:08
ef4354d to
caa4f9b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, we set a default limit that was an effective infinity (2^28). It seems back in the 32 bit days that was the Erlang's largest small integer [1]. However, that turned out to be too low and it surprised a user when it truncated their all_docs output skipping some of the data. Fix that by increasing the limit to a larger "infinity" (highest 64 bit Erlang small integer [1]).
We did have a "query_limit" config parameter to customize the limit, however that turned out to be broken and did not take effect when the user tried it for all_docs, so fix that as well. Test to ensure the limit gets reduced appropriately. To make the setting more user friendly, allow
infinityas the value.Also, in the case of all_docs, we validated args and applied the limit check twice: once in the coordinator and another time on each worker, which wasted CPU resources and made things a bit confusing. To fix that, remove the validation from the common worker code in couch_mrview and validate once: either on the coordinator side, or local (port 5986) callback, right in the http callback.
[1] https://www.erlang.org/doc/system/memory.html
Fix #5176