You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@couchdb.apache.org by va...@apache.org on 2025/04/24 22:42:40 UTC
(couchdb) 01/01: Fix query limit
This is an automated email from the ASF dual-hosted git repository.
vatamane pushed a commit to branch fix-query-limit-for-all-docs
in repository https://gitbox.apache.org/repos/asf/couchdb.git
commit fbecb11b9fa23e2f4cfcf0b6a4e785022f24343f
Author: Nick Vatamaniuc <va...@gmail.com>
AuthorDate: Thu Apr 24 18:17:31 2025 -0400
Fix query limit
Previously, we set a default limit that was an effective "infinity" (2^28).
However, that turned out to be too low and it surprised a user when it
truncated their all_docs output skipping the rest of the data. Fix that by
increasing the limit to 100B.
We have a "query_limit" config parameter to increase the limit, however that
turned out to be broken and did not take effect, when the user tried, so fix
that as well.
Since we still support using the local API (localhost:5986 or _node/$node), and
the worker function is used for the clustered and local calls, the
`apply_limit/2` function is called twice for the same request: once on the
coordinator, and then on each worker. Since the coordinator sets limit as limit
+ skip, that means that we'd be failing all skip > 0 calls on the
coordinator (limit + skip > max_limit). To handle that use an extra option flag
to indicate that we already applied the limit and don't apply it again.
When running the tests also discoverd that our _dbs_info and list endpoints did
not validate query parameters, so ensure we validate them, which means users can
now configure query limits for them.
Fix #5176
---
rel/overlay/etc/default.ini | 4 +-
src/chttpd/src/chttpd_misc.erl | 3 +-
src/chttpd/src/chttpd_show.erl | 5 ++-
src/chttpd/test/eunit/chttpd_view_test.erl | 72 ++++++++++++++++++++++++++++--
src/couch_mrview/include/couch_mrview.hrl | 2 +-
src/couch_mrview/src/couch_mrview_util.erl | 48 +++++++++++++-------
6 files changed, 109 insertions(+), 25 deletions(-)
diff --git a/rel/overlay/etc/default.ini b/rel/overlay/etc/default.ini
index b7229d5e4..e220178a4 100644
--- a/rel/overlay/etc/default.ini
+++ b/rel/overlay/etc/default.ini
@@ -512,8 +512,8 @@ authentication_db = _users
; Timeout for how long a response from a busy view group server can take.
; "infinity" is also a valid configuration value.
;group_info_timeout = 5000
-;query_limit = 268435456
-;partition_query_limit = 268435456
+;query_limit = 100000000000
+;partition_query_limit = 100000000000
; Configure what to use as the db tag when selecting design doc couchjs
; processes. The choices are:
diff --git a/src/chttpd/src/chttpd_misc.erl b/src/chttpd/src/chttpd_misc.erl
index d0bf363f3..fe8270250 100644
--- a/src/chttpd/src/chttpd_misc.erl
+++ b/src/chttpd/src/chttpd_misc.erl
@@ -120,6 +120,7 @@ handle_all_dbs_req(Req) ->
handle_all_dbs_info_req(Req) ->
Args0 = couch_mrview_http:parse_params(Req, undefined),
Args1 = couch_mrview_util:set_extra(Args0, namespace, <<"_non_design">>),
+ Args2 = couch_mrview_util:validate_dbs_info_args(Args1),
ShardDbName = config:get("mem3", "shards_db", "_dbs"),
%% shard_db is not sharded but mem3:shards treats it as an edge case
%% so it can be pushed thru fabric
@@ -129,7 +130,7 @@ handle_all_dbs_info_req(Req) ->
{ok, Resp} = chttpd:etag_respond(Req, Etag, fun() ->
{ok, Resp} = chttpd:start_delayed_json_response(Req, 200, [{"ETag", Etag}]),
VAcc = #vacc{req = Req, resp = Resp},
- fabric:all_docs(ShardDbName, Options, fun all_dbs_info_callback/2, VAcc, Args1)
+ fabric:all_docs(ShardDbName, Options, fun all_dbs_info_callback/2, VAcc, Args2)
end),
case is_record(Resp, vacc) of
true -> {ok, Resp#vacc.resp};
diff --git a/src/chttpd/src/chttpd_show.erl b/src/chttpd/src/chttpd_show.erl
index ee27d38da..d2824f668 100644
--- a/src/chttpd/src/chttpd_show.erl
+++ b/src/chttpd/src/chttpd_show.erl
@@ -254,6 +254,7 @@ handle_view_list(Req, Db, DDoc, LName, {ViewDesignName, ViewName}, Keys) ->
{ok, VDoc} = ddoc_cache:open(DbName, <<"_design/", ViewDesignName/binary>>),
CB = fun list_cb/2,
QueryArgs = couch_mrview_http:parse_body_and_query(Req, Keys),
+ QueryArgs1 = couch_mrview_util:validate_all_docs_args(Db, QueryArgs),
Options = [{user_ctx, Req#httpd.user_ctx}],
couch_query_servers:with_ddoc_proc(Db, DDoc, fun(QServer) ->
Acc = #lacc{
@@ -264,7 +265,7 @@ handle_view_list(Req, Db, DDoc, LName, {ViewDesignName, ViewName}, Keys) ->
},
case ViewName of
<<"_all_docs">> ->
- fabric:all_docs(Db, Options, CB, Acc, QueryArgs);
+ fabric:all_docs(Db, Options, CB, Acc, QueryArgs1);
_ ->
fabric:query_view(
Db,
@@ -273,7 +274,7 @@ handle_view_list(Req, Db, DDoc, LName, {ViewDesignName, ViewName}, Keys) ->
ViewName,
CB,
Acc,
- QueryArgs
+ QueryArgs1
)
end
end).
diff --git a/src/chttpd/test/eunit/chttpd_view_test.erl b/src/chttpd/test/eunit/chttpd_view_test.erl
index e478e6640..beb71f823 100644
--- a/src/chttpd/test/eunit/chttpd_view_test.erl
+++ b/src/chttpd/test/eunit/chttpd_view_test.erl
@@ -48,7 +48,8 @@
setup() ->
Hashed = couch_passwords:hash_admin_password(?PASS),
- ok = config:set("admins", ?USER, ?b2l(Hashed), _Persist = false),
+ ok = config:set("admins", ?USER, ?b2l(Hashed), false),
+ ok = config:set("query_server_config", "query_limit", "100", false),
Db = ?tempdb(),
ok = create_db(Db),
ok = create_docs(Db),
@@ -57,7 +58,8 @@ setup() ->
teardown(Db) ->
ok = fabric:delete_db(Db),
- ok = config:delete("admins", ?USER, _Persist = false).
+ ok = config:delete("admins", ?USER, false),
+ ok = config:delete("query_server_config", "query_limit", false).
view_test_() ->
{
@@ -111,7 +113,11 @@ all_docs_test_() ->
?TDEF_FE(t_all_docs_with_key_non_existent_docs),
?TDEF_FE(t_all_docs_with_keys_non_existent_docs),
?TDEF_FE(t_all_docs_with_key_deleted_docs),
- ?TDEF_FE(t_all_docs_with_keys_deleted_docs)
+ ?TDEF_FE(t_all_docs_with_keys_deleted_docs),
+ ?TDEF_FE(t_all_docs_with_limit),
+ ?TDEF_FE(t_all_docs_with_skip),
+ ?TDEF_FE(t_all_docs_with_limit_and_skip),
+ ?TDEF_FE(t_all_docs_with_configured_query_limit)
]
}
}
@@ -370,6 +376,66 @@ t_view_map_reduce_with_keys_and_group(Db) ->
?assertEqual(Res, Res1),
?assertEqual(Code, Code1).
+t_all_docs_with_limit(Db) ->
+ {Code, Res} = req(get, url(Db, "_all_docs", "limit=2")),
+ ?assertEqual(200, Code),
+ ?assertMatch(
+ #{
+ <<"offset">> := 0,
+ <<"total_rows">> := 4,
+ <<"rows">> := [
+ #{<<"id">> := <<"_design/ddoc">>},
+ #{<<"id">> := <<"a">>}
+ ]
+ },
+ Res
+ ).
+
+t_all_docs_with_skip(Db) ->
+ {Code, Res} = req(get, url(Db, "_all_docs", "skip=2")),
+ ?assertEqual(200, Code),
+ ?assertMatch(
+ #{
+ <<"offset">> := 2,
+ <<"total_rows">> := 4,
+ <<"rows">> := [
+ #{<<"id">> := <<"b">>},
+ #{<<"id">> := <<"c">>}
+ ]
+ },
+ Res
+ ).
+
+t_all_docs_with_limit_and_skip(Db) ->
+ {Code, Res} = req(get, url(Db, "_all_docs", "skip=2&limit=1")),
+ ?assertEqual(200, Code),
+ ?assertMatch(
+ #{
+ <<"offset">> := 2,
+ <<"total_rows">> := 4,
+ <<"rows">> := [
+ #{<<"id">> := <<"b">>}
+ ]
+ },
+ Res
+ ).
+
+t_all_docs_with_configured_query_limit(Db) ->
+ config:set("query_server_config", "query_limit", "2", false),
+ {Code, Res} = req(get, url(Db, "_all_docs")),
+ ?assertEqual(200, Code),
+ ?assertMatch(
+ #{
+ <<"offset">> := 0,
+ <<"total_rows">> := 4,
+ <<"rows">> := [
+ #{<<"id">> := <<"_design/ddoc">>},
+ #{<<"id">> := <<"a">>}
+ ]
+ },
+ Res
+ ).
+
%%%%%%%%%%%%%%%%%%%% Utility Functions %%%%%%%%%%%%%%%%%%%%
url(Db) ->
Addr = config:get("chttpd", "bind_address", "127.0.0.1"),
diff --git a/src/couch_mrview/include/couch_mrview.hrl b/src/couch_mrview/include/couch_mrview.hrl
index b31463c53..b068c2dbb 100644
--- a/src/couch_mrview/include/couch_mrview.hrl
+++ b/src/couch_mrview/include/couch_mrview.hrl
@@ -54,7 +54,7 @@
view_states=nil
}).
--define(MAX_VIEW_LIMIT, 16#10000000).
+-define(MAX_VIEW_LIMIT, 100000000000).
-record(mrargs, {
view_type,
diff --git a/src/couch_mrview/src/couch_mrview_util.erl b/src/couch_mrview/src/couch_mrview_util.erl
index a478685da..3a34aac4e 100644
--- a/src/couch_mrview/src/couch_mrview_util.erl
+++ b/src/couch_mrview/src/couch_mrview_util.erl
@@ -26,7 +26,7 @@
-export([temp_view_to_ddoc/1]).
-export([calculate_external_size/1]).
-export([calculate_active_size/1]).
--export([validate_all_docs_args/2, validate_args/1, validate_args/3]).
+-export([validate_all_docs_args/2, validate_dbs_info_args/1, validate_args/1, validate_args/3]).
-export([maybe_load_doc/3, maybe_load_doc/4]).
-export([maybe_update_index_file/1]).
-export([extract_view/4, extract_view_reduce/1]).
@@ -484,7 +484,7 @@ fold_reduce({NthRed, Lang, View}, Fun, Acc, Options) ->
validate_args(Db, DDoc, Args0) ->
{ok, State} = couch_mrview_index:init(Db, DDoc),
- Args1 = apply_limit(State#mrst.partitioned, Args0),
+ Args1 = maybe_apply_limit(State#mrst.partitioned, Args0),
validate_args(State, Args1).
validate_args(#mrst{} = State, Args0) ->
@@ -512,9 +512,24 @@ validate_args(#mrst{} = State, Args0) ->
mrverror(Msg2)
end.
+maybe_apply_limit(ViewPartitioned, Args) ->
+ % This may be called twice: once from fabric and once from a worker because
+ % we still support node local API calls. Since fabric sets worker limit to
+ % limit + skip if we apply the limit the second time it will fail. So if we
+ % already applied the limit, we skip applying it again.
+ %
+ % When we remove the ability to call local APIs we can remove the
+ % limit_apply part as well and apply_limit in fabric only.
+ Options = Args#mrargs.extra,
+ LimitApplied = lists:keymember(limit_applied, 1, Options),
+ case LimitApplied of
+ true -> Args;
+ false -> apply_limit(ViewPartitioned, Args)
+ end.
+
apply_limit(ViewPartitioned, Args) ->
Options = Args#mrargs.extra,
- IgnorePQLimit = lists:keyfind(ignore_partition_query_limit, 1, Options),
+ IgnorePQLimit = lists:keymember(ignore_partition_query_limit, 1, Options),
LimitType =
case {ViewPartitioned, IgnorePQLimit} of
{true, false} -> "partition_query_limit";
@@ -522,23 +537,19 @@ apply_limit(ViewPartitioned, Args) ->
{false, _} -> "query_limit"
end,
- MaxLimit = config:get_integer(
- "query_server_config",
- LimitType,
- ?MAX_VIEW_LIMIT
- ),
-
- % Set the highest limit possible if a user has not
- % specified a limit
+ MaxLimit = config:get_integer("query_server_config", LimitType, ?MAX_VIEW_LIMIT),
+ % Set the highest limit possible if a user has not specified a limit
Args1 =
case Args#mrargs.limit == ?MAX_VIEW_LIMIT of
true -> Args#mrargs{limit = MaxLimit};
false -> Args
end,
-
+ LimitValid = Args1#mrargs.limit =< MaxLimit,
+ SkipValid = Args1#mrargs.skip =< MaxLimit,
if
- Args1#mrargs.limit =< MaxLimit ->
- Args1;
+ LimitValid andalso SkipValid ->
+ Options1 = lists:keystore(limit_applied, 1, Options, {limit_applied, true}),
+ Args1#mrargs{extra = Options1};
true ->
Fmt = "Limit is too large, must not exceed ~p",
mrverror(io_lib:format(Fmt, [MaxLimit]))
@@ -554,12 +565,17 @@ validate_all_docs_args(Db, Args0) ->
{false, <<_/binary>>} ->
mrverror(<<"`partition` parameter is not supported on this db">>);
{_, <<_/binary>>} ->
- Args1 = apply_limit(true, Args),
+ Args1 = maybe_apply_limit(true, Args),
apply_all_docs_partition(Args1, Partition);
_ ->
- Args
+ % maybe_apply_limit ensures query_limit config parameter takes effect
+ maybe_apply_limit(false, Args)
end.
+validate_dbs_info_args(Args) ->
+ Args1 = validate_args(Args),
+ maybe_apply_limit(false, Args1).
+
validate_args(Args) ->
GroupLevel = determine_group_level(Args),
Reduce = Args#mrargs.reduce,