Make view query limits configurable #1804

garrensmith · 2018-12-10T08:50:23Z

Overview

This allows us to set a maximun allowed number of documents
to be returned for a global or a partitioned query.

Testing recommendations

Set a view limit in the default.ini.

A query with a limit larger than the max will error
A query with the limit value below the max will run the query. A
A query with no limit will use the max limit set.

Related Issues or Pull Requests

Checklist

Code is written and works correctly;
Changes are covered by tests;
Documentation reflects the changes;

davisp

Mostly good but there's a few style issues.

src/couch_mrview/src/couch_mrview_util.erl

rel/overlay/etc/default.ini

davisp

Removing the clause checking for infinity is the main thing. Up to you on whether you want to relax the equality check. Otherwise this all looks good.

davisp · 2018-12-11T15:55:31Z

src/couch_mrview/src/couch_mrview_util.erl

+
+    % set the highest limit possible if a user has not 
+    % specified a limit
+    Args1 = case Args#mrargs.limit =:= ?MAX_VIEW_LIMIT


Super minor nit but I'd probably use == instead of =:= here. Only because I had to stop and think on whether we needed the stronger =:= for any reason or not.

davisp · 2018-12-11T15:56:21Z

src/couch_mrview/src/couch_mrview_util.erl

+    % set the highest limit possible if a user has not 
+    % specified a limit
+    Args1 = case Args#mrargs.limit =:= ?MAX_VIEW_LIMIT
+        andalso MaxLimit =/= infinity of


No need for this clause checking infinity anymore.

I changed this line from if Args1#mrargs.limit =:= MaxLimit -> Args1; true -> to
if Args1#mrargs.limit =< MaxLimit -> Args1; true ->

davisp

Everything looks alright but we have to figure out why those tests are failing before this can be merged. Reading through I'm not seeing anything so my best guess is an unrelated test leaves the config in a bad state.

davisp · 2019-01-21T16:54:06Z

src/couch_mrview/src/couch_mrview_util.erl

+    end,
+
+    if Args1#mrargs.limit =< MaxLimit -> Args1; true ->
+        Fmt = "Limit is too large, must not exceed ~p",


Something is wrong here but I'm not immediately seeing why. The view_multi_key_design.js test has failed with this error message on each worker. Perhaps we left a bad config from an unrelated test?

I found the issue. It was because of the fix_skip_and_limit function https://github.com/apache/couchdb/blob/master/src/fabric/src/fabric_view.erl#L380

We made the max limit greater than the defined max limit.

I see the issue there but I'm not seeing what changed to account for it.

davisp

Looks good overall except for the debug logging.

src/couch_mrview/src/couch_mrview_util.erl

davisp · 2019-01-22T16:30:05Z

src/couch_mrview/src/couch_mrview_util.erl

+    end,
+
+    if Args1#mrargs.limit =< MaxLimit -> Args1; true ->
+        Fmt = "Limit is too large, must not exceed ~p",


I see the issue there but I'm not seeing what changed to account for it.

davisp

Two other minor changes to make.

davisp · 2019-01-22T16:57:59Z

test/elixir/test/partition_all_docs_test.exs

+    %{:body => body} = resp
+
+    Enum.each(body["all_nodes"], fn node ->
+      resp = Couch.put("/_node/#{node}/_config/query_server_config/partition_query_limit", body: "\"2000\"")


Re-reading again, and just noticed this. You should use set_config which takes care of removing the config change after the test finishes. There's an example here:

couchdb/test/elixir/test/partition_size_limit_test.exs

Line 15 in 8d2af71

set_config({"couchdb", "max_partition_size", Integer.to_string(@max_size)})

And samesies for partition_view_test

davisp

So now that I've figured out how that interaction with fix_skip_and_limit works I realize that this is wrong. The current version appears to work but only because we're not testing with skip values that are large enough to cause a bad response.

The issue here is that we're calling validate_args on each RPC worker, however, due to clustering when a skip is set, each RPC worker has to return skip+limit rows which the coordinator then does the skip discard and then cuts off the response when limit rows. However, due to how this works we very often might be telling RPC workers to return more than the maximum allowed number of rows which gets flagged as an error.

The fix here is I think simple enough. We should just move the apply_limit code to fabric_util:validate_args which is only ever called from the coordinator layer so we don't have to worry about validating a system modified mrargs record.

davisp · 2019-01-22T17:56:53Z

Also, to force the bug, the skip would have to be set to Q * $max_rows because the current apply_limit throws away the skip bump to the limit we'd end up returning 0 rows when there was actual data to return.

That is of course an unlikely scenario and goes against the general recommendation to not use a large skip value but it'd still be an incorrect response if someone did get into that situation where maybe $max_rows was set low enough to be not super crazy.

davisp · 2019-01-22T21:16:41Z

Turns out fabric_util was too far up. However we can just move it up one call so the RPC workers don't call it again and that fixes the issue.

davisp

+1 but be sure and squerge this down into a single commit before merging. I left it unsquerged so you can see the changes I made separate from the rest of the PR.

This allows us to set a maximun allowed number of documents to be returned for a global or a partitioned view query and _all_docs query. Co-authored-by: Paul J. Davis <paul.joseph.davis@gmail.com>

davisp requested changes Dec 10, 2018

View reviewed changes

davisp requested changes Dec 11, 2018

View reviewed changes

davisp force-pushed the feature/database-partitions branch 11 times, most recently from 90be3b0 to 6c2785a Compare December 19, 2018 17:51

davisp force-pushed the feature/database-partitions branch 2 times, most recently from 005b442 to 60bbe3f Compare December 20, 2018 20:48

davisp force-pushed the feature/database-partitions branch 5 times, most recently from be1fe52 to 31a4a53 Compare January 8, 2019 17:35

davisp force-pushed the feature/database-partitions branch 7 times, most recently from 04146c3 to 51a482e Compare January 16, 2019 23:21

davisp force-pushed the feature/database-partitions branch 2 times, most recently from 8cd68be to 91af772 Compare January 18, 2019 18:35

davisp closed this Jan 18, 2019

garrensmith changed the base branch from feature/database-partitions to master January 21, 2019 08:08

garrensmith reopened this Jan 21, 2019

davisp requested changes Jan 21, 2019

View reviewed changes

davisp requested changes Jan 22, 2019

View reviewed changes

davisp approved these changes Jan 22, 2019

View reviewed changes

Make _all_docs and view query limits configurable

68d966b

This allows us to set a maximun allowed number of documents to be returned for a global or a partitioned view query and _all_docs query. Co-authored-by: Paul J. Davis <paul.joseph.davis@gmail.com>

garrensmith merged commit 1fdfe46 into apache:master Jan 23, 2019

garrensmith deleted the partition-view-limits branch December 12, 2019 17:20

laisuchoa mentioned this pull request Aug 14, 2024

Query exporting only 2ˆ28 documents (data have more) even after setting query limit to 2ˆ29 #5176

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make view query limits configurable #1804

Make view query limits configurable #1804

garrensmith commented Dec 10, 2018

davisp left a comment

davisp left a comment •

edited

Loading

davisp Dec 11, 2018

davisp Dec 11, 2018

garrensmith Jan 22, 2019

davisp left a comment

davisp Jan 21, 2019

garrensmith Jan 22, 2019

davisp Jan 22, 2019

davisp left a comment

davisp Jan 22, 2019

davisp left a comment

davisp Jan 22, 2019

davisp Jan 22, 2019

davisp left a comment

davisp commented Jan 22, 2019

davisp commented Jan 22, 2019

davisp left a comment

Make view query limits configurable #1804

Make view query limits configurable #1804

Conversation

garrensmith commented Dec 10, 2018

Overview

Testing recommendations

Related Issues or Pull Requests

Checklist

davisp left a comment

Choose a reason for hiding this comment

davisp left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davisp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davisp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davisp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davisp left a comment

Choose a reason for hiding this comment

davisp commented Jan 22, 2019

davisp commented Jan 22, 2019

davisp left a comment

Choose a reason for hiding this comment

davisp left a comment •

edited

Loading