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 2024/11/28 14:26:51 UTC
(couchdb) branch main updated: Fix attachment size calculation
This is an automated email from the ASF dual-hosted git repository.
vatamane pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/couchdb.git
The following commit(s) were added to refs/heads/main by this push:
new f2f6cd72d Fix attachment size calculation
f2f6cd72d is described below
commit f2f6cd72d45f3d73bd48484cf2527f08d57d867d
Author: Nick Vatamaniuc <va...@gmail.com>
AuthorDate: Thu Nov 28 00:10:34 2024 -0500
Fix attachment size calculation
Previously, we included attachment sizes in the active size totals even if they
attachments were deleted from the doc revision leafs.
The fix is, unsurprisingly, to do what the compactor does when it copies and
updates sizes: only update sizes for leaf revision during the rev-tree
traversal. We did this for doc bodies already in #4264, but forgot to consider
attachments then.
In general we're aiming for the active size to approximate the size of the file
after compaction. That is what drives smoosh (autocompaction) priority. If
we don't remove non-leaf attachments from the calculation, the active/file
ratio in smoosh may never trigger properly as ratio will stay low for shards
with large attachments.
Until a release goes out with this fix, a workaround could be to compact shards
with large attachments by hand, or use a lower threshold on `slack_dbs`
channel (and concurrency=1 to keep it in the background) to slowly cycle
through all the shards and try to compact them.
While the fix itself is small, to convince ourselves we did the right thing
update the tests to check a few corner cases.
Interestingly enough we had a disabled Elixir compaction test which we disabled
because it was "mysteriously" not reporting a lower "active" size after
compacting. So re-enable it and also improve it a bit to have both a attachment
that gets deleted and one that stays live.
Fix #5346
---
.../test/eunit/chttpd_db_attachment_size_tests.erl | 337 +++++++++++++++++----
src/couch/src/couch_db_updater.erl | 5 +-
test/elixir/test/compact_test.exs | 40 ++-
3 files changed, 303 insertions(+), 79 deletions(-)
diff --git a/src/chttpd/test/eunit/chttpd_db_attachment_size_tests.erl b/src/chttpd/test/eunit/chttpd_db_attachment_size_tests.erl
index 6e886935a..420291616 100644
--- a/src/chttpd/test/eunit/chttpd_db_attachment_size_tests.erl
+++ b/src/chttpd/test/eunit/chttpd_db_attachment_size_tests.erl
@@ -25,7 +25,6 @@ setup() ->
Hashed = couch_passwords:hash_admin_password(?PASS),
Persist = false,
ok = config:set("admins", ?USER, ?b2l(Hashed), Persist),
- ok = config:set("couchdb", "max_attachment_size", "50", Persist),
TmpDb = ?tempdb(),
Addr = config:get("chttpd", "bind_address", "127.0.0.1"),
Port = integer_to_list(mochiweb_socket_server:get(chttpd, port)),
@@ -51,72 +50,220 @@ attachment_size_test_() ->
fun setup/0,
fun teardown/1,
[
- fun put_inline/1,
- fun put_simple/1,
- fun put_simple_chunked/1,
- fun put_mp_related/1,
- fun put_chunked_mp_related/1
+ ?TDEF_FE(put_inline),
+ ?TDEF_FE(put_simple),
+ ?TDEF_FE(put_simple_chunked),
+ ?TDEF_FE(put_mp_related),
+ ?TDEF_FE(put_chunked_mp_related)
+ ]
+ }
+ }
+ }.
+
+attachment_size_db_active_size_test_() ->
+ {
+ "attachment sizes are accounted properly in db size",
+ {
+ setup,
+ fun chttpd_test_util:start_couch/0,
+ fun chttpd_test_util:stop_couch/1,
+ {
+ foreach,
+ fun setup/0,
+ fun teardown/1,
+ [
+ ?TDEF_FE(t_add_attachments),
+ ?TDEF_FE(t_remove_attachments),
+ ?TDEF_FE(t_delete_doc),
+ ?TDEF_FE(t_delete_doc_but_keep_attachment),
+ ?TDEF_FE(t_conflicting_attachments)
]
}
}
}.
put_inline(Url) ->
- ?_test(begin
- Status = put_inline(Url, "doc2", 50),
- ?assert(Status =:= 201 orelse Status =:= 202),
- ?assertEqual(413, put_inline(Url, "doc3", 51))
- end).
+ ok = config:set("couchdb", "max_attachment_size", "50", false),
+ Status = put_inline(Url, "doc2", 50),
+ ?assert(Status =:= 201 orelse Status =:= 202),
+ ?assertEqual(413, put_inline(Url, "doc3", 51)).
put_simple(Url) ->
- ?_test(begin
- Headers = [{"Content-Type", "app/binary"}],
- Rev1 = doc_rev(Url, "doc1"),
- Data1 = data(50),
- Status1 = put_req(Url ++ "/doc1/att2?rev=" ++ Rev1, Headers, Data1),
- ?assert(Status1 =:= 201 orelse Status1 =:= 202),
- Data2 = data(51),
- Rev2 = doc_rev(Url, "doc1"),
- Status2 = put_req(Url ++ "/doc1/att3?rev=" ++ Rev2, Headers, Data2),
- ?assertEqual(413, Status2)
- end).
+ ok = config:set("couchdb", "max_attachment_size", "50", false),
+ Headers = [{"Content-Type", "app/binary"}],
+ Rev1 = doc_rev(Url, "doc1"),
+ Data1 = data(50),
+ Status1 = put_req(Url ++ "/doc1/att2?rev=" ++ Rev1, Headers, Data1),
+ ?assert(Status1 =:= 201 orelse Status1 =:= 202),
+ Data2 = data(51),
+ Rev2 = doc_rev(Url, "doc1"),
+ Status2 = put_req(Url ++ "/doc1/att3?rev=" ++ Rev2, Headers, Data2),
+ ?assertEqual(413, Status2).
put_simple_chunked(Url) ->
- ?_test(begin
- Headers = [{"Content-Type", "app/binary"}],
- Rev1 = doc_rev(Url, "doc1"),
- DataFun1 = data_stream_fun(50),
- Status1 = put_req_chunked(Url ++ "/doc1/att2?rev=" ++ Rev1, Headers, DataFun1),
- ?assert(Status1 =:= 201 orelse Status1 =:= 202),
- DataFun2 = data_stream_fun(51),
- Rev2 = doc_rev(Url, "doc1"),
- Status2 = put_req_chunked(Url ++ "/doc1/att3?rev=" ++ Rev2, Headers, DataFun2),
- ?assertEqual(413, Status2)
- end).
+ ok = config:set("couchdb", "max_attachment_size", "50", false),
+ Headers = [{"Content-Type", "app/binary"}],
+ Rev1 = doc_rev(Url, "doc1"),
+ DataFun1 = data_stream_fun(50),
+ Status1 = put_req_chunked(Url ++ "/doc1/att2?rev=" ++ Rev1, Headers, DataFun1),
+ ?assert(Status1 =:= 201 orelse Status1 =:= 202),
+ DataFun2 = data_stream_fun(51),
+ Rev2 = doc_rev(Url, "doc1"),
+ Status2 = put_req_chunked(Url ++ "/doc1/att3?rev=" ++ Rev2, Headers, DataFun2),
+ ?assertEqual(413, Status2).
put_mp_related(Url) ->
- ?_test(begin
- Headers = [?CONTENT_MULTI_RELATED],
- Body1 = mp_body(50),
- Status1 = put_req(Url ++ "/doc2", Headers, Body1),
- ?assert(Status1 =:= 201 orelse Status1 =:= 202),
- Body2 = mp_body(51),
- Status2 = put_req(Url ++ "/doc3", Headers, Body2),
- ?assertEqual(413, Status2)
- end).
+ ok = config:set("couchdb", "max_attachment_size", "50", false),
+ Headers = [?CONTENT_MULTI_RELATED],
+ Body1 = mp_body(50),
+ Status1 = put_req(Url ++ "/doc2", Headers, Body1),
+ ?assert(Status1 =:= 201 orelse Status1 =:= 202),
+ Body2 = mp_body(51),
+ Status2 = put_req(Url ++ "/doc3", Headers, Body2),
+ ?assertEqual(413, Status2).
put_chunked_mp_related(Url) ->
- ?_test(begin
- Headers = [?CONTENT_MULTI_RELATED],
- Body = mp_body(50),
- Status = put_req_chunked(Url ++ "/doc4", Headers, Body),
- ?assert(Status =:= 201 orelse Status =:= 202)
- end).
+ ok = config:set("couchdb", "max_attachment_size", "50", false),
+ Headers = [?CONTENT_MULTI_RELATED],
+ Body = mp_body(50),
+ Status = put_req_chunked(Url ++ "/doc4", Headers, Body),
+ ?assert(Status =:= 201 orelse Status =:= 202).
+
+t_add_attachments(Url) ->
+ ?assertEqual(201, put_att(Url, "doc1", "att1", 10000)),
+ Active1 = active_size(Url),
+ ?assert(Active1 >= 10000 andalso Active1 < 12000),
+ ?assertEqual(201, put_att(Url, "doc1", "att2", 10000)),
+ Active2 = active_size(Url),
+ ?assert(Active2 >= 20000 andalso Active2 < 22000).
+
+t_remove_attachments(Url) ->
+ ?assertEqual(201, put_att(Url, "doc1", "att1", 10000)),
+ ?assertEqual(201, put_att(Url, "doc1", "att2", 10000)),
+ Active1 = active_size(Url),
+ ?assert(Active1 >= 20000 andalso Active1 < 22000),
+ ?assertEqual(201, remove_att(Url, "doc1", "att1")),
+ Active2 = active_size(Url),
+ ?assert(Active2 >= 10000 andalso Active2 < 12000),
+ ?assertEqual(201, remove_att(Url, "doc1", "att2")),
+ ?assert(active_size(Url) < 2000).
+
+t_delete_doc(Url) ->
+ ?assertEqual(201, put_att(Url, "doc1", "att1", 10000)),
+ ?assertEqual(201, put_att(Url, "doc1", "att2", 10000)),
+ Active1 = active_size(Url),
+ ?assert(Active1 >= 20000 andalso Active1 < 22000),
+ delete_doc(Url, "doc1"),
+ ?assert(active_size(Url) < 2000).
+
+t_delete_doc_but_keep_attachment(Url) ->
+ ?assertEqual(201, put_att(Url, "doc1", "att1", 10000)),
+ ?assertEqual(201, put_att(Url, "doc1", "att2", 10000)),
+ Active1 = active_size(Url),
+ ?assert(Active1 >= 20000 andalso Active1 < 22000),
+ % Here we are deleting the document but keeping the
+ % body and one of the attachments (att2) around
+ ?assertEqual(201, delete_remove_att(Url, "doc1", "att1")),
+ Active2 = active_size(Url),
+ ?assert(Active2 >= 10000 andalso Active2 < 12000).
+
+t_conflicting_attachments(Url) ->
+ ?assertEqual(201, put_att(Url, "doc1", "att1", 10000)),
+ % The rev tree should look like: (active ~= 10KB)
+ % 1-...
+ % |
+ % 2-... (att1)
+ Active1 = active_size(Url),
+ ?assert(Active1 >= 10000 andalso Active1 < 12000),
+
+ % Add two conflicting revisions to doc1
+ Rev = doc_rev(Url, "doc1"),
+ new_edits_false_update(Url, "doc1", Rev, <<"a">>),
+ new_edits_false_update(Url, "doc1", Rev, <<"b">>),
+ % Rev tree shape: (active still ~= 10KB):
+ % 1-...
+ % |
+ % 2-... (att1)
+ % / \
+ % / \
+ % / \
+ % 3-a 3-b
+ %
+ Active2 = active_size(Url),
+ ?assert(Active2 >= 10000 andalso Active2 < 12000),
+
+ ?assertEqual(201, put_att_rev(Url, "doc1", "3-a", "att2", 10000)),
+ % Updated 3-a with att2. Active size ~= 20KB
+ % 1-...
+ % |
+ % 2-... (att1)
+ % / \
+ % / \
+ % / \
+ % 3-a 3-b
+ % |
+ % 4-a (att2)
+ %
+ Active3 = active_size(Url),
+ ?assert(Active3 >= 20000 andalso Active3 < 22000),
+
+ % Update 3-b with att3 (active ~= 30KB)
+ %
+ % 1-...
+ % |
+ % 2-...(att1)
+ % / \
+ % / \
+ % / \
+ % 3-a 3-b
+ % | |
+ % 4-...(att2) 4-...(att3)
+ %
+ ?assertEqual(201, put_att_rev(Url, "doc1", "3-b", "att3", 10000)),
+ Active4 = active_size(Url),
+ ?assert(Active4 >= 30000 andalso Active4 < 32000),
+
+ delete_doc(Url, "doc1"),
+ % Delete the winning branch, whichever it is (active ~= 20KB)
+ %
+ % 1-...
+ % |
+ % 2-...(att1)
+ % / \
+ % / \
+ % / \
+ % 3-a 3-b
+ % | |
+ % 4-...(att2) 4-...(att3)
+ % |
+ % 5-...
+ % deleted
+ %
+ Active5 = active_size(Url),
+ ?assert(Active5 >= 20000 andalso Active5 < 22000),
+
+ delete_doc(Url, "doc1"),
+ % Delete the new (other) winning branch: (active < 10KB)
+ %
+ % 1-...
+ % |
+ % 2-...(att1)
+ % / \
+ % / \
+ % / \
+ % 3-a 3-b
+ % | |
+ % 4-...(att2) 4-...(att3)
+ % | |
+ % 5-... 5-...
+ % deleted deleted
+ %
+ ?assert(active_size(Url) < 2000).
% Helper functions
create_db(Url) ->
- Status = put_req(Url, "{}"),
+ Status = put_req(Url ++ "?q=1&n=1", "{}"),
?assert(Status =:= 201 orelse Status =:= 202).
add_doc(Url, DocId) ->
@@ -127,13 +274,15 @@ delete_db(Url) ->
{ok, 200, _, _} = test_request:delete(Url, [?AUTH]).
put_inline(Url, DocId, Size) ->
- Doc =
- "{\"_attachments\": {\"att1\":{"
- "\"content_type\": \"app/binary\", "
- "\"data\": \"" ++ data_b64(Size) ++
- "\""
- "}}}",
- put_req(Url ++ "/" ++ DocId, Doc).
+ Doc = #{
+ <<"_attachments">> => #{
+ <<"att1">> => #{
+ <<"content_type">> => <<"app/binary">>,
+ <<"data">> => data_b64(Size)
+ }
+ }
+ },
+ put_req(Url ++ "/" ++ DocId, [], Doc).
mp_body(AttSize) ->
AttData = data(AttSize),
@@ -167,15 +316,61 @@ mp_body(AttSize) ->
).
doc_rev(Url, DocId) ->
- {200, ResultProps} = get_req(Url ++ "/" ++ DocId),
- {<<"_rev">>, BinRev} = lists:keyfind(<<"_rev">>, 1, ResultProps),
- binary_to_list(BinRev).
+ {200, #{<<"_rev">> := Rev}} = get_req(Url ++ "/" ++ DocId),
+ binary_to_list(Rev).
+
+put_att(Url, DocId, AttName, Size) ->
+ put_att_rev(Url, DocId, doc_rev(Url, DocId), AttName, Size).
+
+put_att_rev(Url, DocId, Rev, AttName, Size) ->
+ Headers = [{"Content-Type", "app/binary"}],
+ Data = data(Size),
+ put_req(Url ++ "/" ++ DocId ++ "/" ++ AttName ++ "?rev=" ++ Rev, Headers, Data).
+
+remove_att(Url, DocId, AttName) ->
+ {200, #{} = Doc} = get_req(Url ++ "/" ++ DocId),
+ #{<<"_attachments">> := Atts} = Doc,
+ {#{}, Atts1} = maps:take(?l2b(AttName), Atts),
+ {Status, _} = req(post, Url, [], Doc#{<<"_attachments">> := Atts1}),
+ Status.
+
+delete_remove_att(Url, DocId, AttName) ->
+ {200, #{} = Doc} = get_req(Url ++ "/" ++ DocId),
+ #{<<"_attachments">> := Atts} = Doc,
+ {#{}, Atts1} = maps:take(?l2b(AttName), Atts),
+ Doc1 = Doc#{<<"_attachments">> => Atts1, <<"_deleted">> => true},
+ {Status, _} = req(post, Url, [], Doc1),
+ Status.
+
+new_edits_false_update(Url, DocId, OldRev, Rev) ->
+ {200, #{} = Doc} = get_req(Url ++ "/" ++ DocId ++ "?rev=" ++ OldRev ++ "&revs=true"),
+ #{<<"_revisions">> := Revisions} = Doc,
+ #{<<"ids">> := RevIds, <<"start">> := Start} = Revisions,
+ RevIds1 = [Rev] ++ RevIds,
+ Revisions1 = #{<<"ids">> => RevIds1, <<"start">> => Start + 1},
+ {_, Doc1} = maps:take(<<"_rev">>, Doc),
+ {_, Doc2} = maps:take(<<"_id">>, Doc1),
+ Doc3 = Doc2#{<<"_revisions">> => Revisions1},
+ {Status, Res} = req(put, Url ++ "/" ++ DocId ++ "?new_edits=false", [], Doc3),
+ ?assertEqual(201, Status),
+ ?assertMatch(#{<<"ok">> := true}, Res),
+ ok.
+
+delete_doc(Url, DocId) ->
+ Rev = doc_rev(Url, "doc1"),
+ {200, _} = req(delete, Url ++ "/" ++ DocId ++ "?rev=" ++ Rev, []),
+ ok.
+
+active_size(Url) ->
+ {200, #{<<"sizes">> := Sizes}} = get_req(Url),
+ #{<<"active">> := Active} = Sizes,
+ Active.
put_req(Url, Body) ->
put_req(Url, [], Body).
put_req(Url, Headers, Body) ->
- {ok, Status, _, _} = test_request:put(Url, Headers ++ [?AUTH], Body),
+ {Status, _} = req(put, Url, Headers, Body),
Status.
put_req_chunked(Url, Headers, Body) ->
@@ -184,9 +379,20 @@ put_req_chunked(Url, Headers, Body) ->
Status.
get_req(Url) ->
- {ok, Status, _, ResultBody} = test_request:get(Url, [?CONTENT_JSON, ?AUTH]),
- {[_ | _] = ResultProps} = ?JSON_DECODE(ResultBody),
- {Status, ResultProps}.
+ {ok, Status, _, Res} = test_request:get(Url, [?CONTENT_JSON, ?AUTH]),
+ {Status, json_decode(Res)}.
+
+req(Method, Url, Headers) ->
+ Headers1 = Headers ++ [?AUTH],
+ {ok, Code, _, Res} = test_request:request(Method, Url, Headers1),
+ {Code, json_decode(Res)}.
+
+req(Method, Url, Headers, #{} = Body) ->
+ req(Method, Url, Headers ++ [?CONTENT_JSON], jiffy:encode(Body));
+req(Method, Url, Headers, Body) ->
+ Headers1 = Headers ++ [?AUTH],
+ {ok, Code, _, Res} = test_request:request(Method, Url, Headers1, Body),
+ {Code, json_decode(Res)}.
% Data streaming generator for ibrowse client. ibrowse will repeatedly call the
% function with State and it should return {ok, Data, NewState} or eof at end.
@@ -201,4 +407,7 @@ data(Size) ->
string:copies("x", Size).
data_b64(Size) ->
- base64:encode_to_string(data(Size)).
+ base64:encode(data(Size)).
+
+json_decode(Bin) when is_binary(Bin) ->
+ jiffy:decode(Bin, [return_maps]).
diff --git a/src/couch/src/couch_db_updater.erl b/src/couch/src/couch_db_updater.erl
index 0747390b2..d1b4c1d2d 100644
--- a/src/couch/src/couch_db_updater.erl
+++ b/src/couch/src/couch_db_updater.erl
@@ -451,10 +451,9 @@ add_sizes(leaf, #leaf{sizes = Sizes, atts = AttSizes}, Acc) ->
NewESAcc = ExternalSize + ESAcc,
NewAttsAcc = lists:umerge(AttSizes, AttsAcc),
{NewASAcc, NewESAcc, NewAttsAcc};
-add_sizes(_, #leaf{atts = AttSizes}, Acc) ->
+add_sizes(_, #leaf{}, Acc) ->
% For intermediate nodes external and active contribution is 0
- {ASAcc, ESAcc, AttsAcc} = Acc,
- {ASAcc, ESAcc, lists:umerge(AttSizes, AttsAcc)}.
+ Acc.
upgrade_sizes(#size_info{} = SI) ->
SI;
diff --git a/test/elixir/test/compact_test.exs b/test/elixir/test/compact_test.exs
index 461a1d347..6edd77c01 100644
--- a/test/elixir/test/compact_test.exs
+++ b/test/elixir/test/compact_test.exs
@@ -8,14 +8,11 @@ defmodule CompactTest do
This is a port of compact.js
"""
- @att_doc_id "att_doc"
+ @att_doc_id1 "att_doc1"
+ @att_doc_id2 "att_doc2"
@att_name "foo.txt"
- @att_plaintext "This is plain text"
+ @att_plaintext String.duplicate("This is plain text", 100)
- # Need to investigate why compaction is not compacting (or compactor cannot complete)
- # Refer:- https://github.com/apache/couchdb/pull/2127
- @tag :pending
- @tag :skip_on_jenkins
@tag :with_db
test "compaction reduces size of deleted docs", context do
db = context[:db_name]
@@ -40,7 +37,9 @@ defmodule CompactTest do
retry_until(fn ->
assert get_info(db)["instance_start_time"] == start_time
+ assert Couch.get("/#{db}").body["doc_count"] == 1
assert_attachment_available(db)
+ assert_deleted_attachment_not_available(db)
info = get_info(db)
final_data_size = info["sizes"]["active"]
final_disk_size = info["sizes"]["file"]
@@ -51,10 +50,15 @@ defmodule CompactTest do
end
defp assert_attachment_available(db) do
- resp = Couch.get("/#{db}/#{@att_doc_id}/#{@att_name}")
+ resp = Couch.get("/#{db}/#{@att_doc_id1}/#{@att_name}")
assert resp.body == @att_plaintext
assert resp.headers["content-type"] == "text/plain"
- assert Couch.get("/#{db}").body["doc_count"] == 1
+ end
+
+ defp assert_deleted_attachment_not_available(db) do
+ resp = Couch.get("/#{db}/#{@att_doc_id2}/#{@att_name}")
+ assert resp.status_code == 404
+ assert resp.body == %{"error" => "not_found", "reason" => "deleted"}
end
defp populate(db) do
@@ -63,16 +67,28 @@ defmodule CompactTest do
assert resp.status_code in [201, 202]
docs = rev(docs, resp.body)
- doc = %{
- _id: "#{@att_doc_id}",
+ doc1 = %{
+ _id: "#{@att_doc_id1}",
+ _attachments: %{
+ "#{@att_name}": %{content_type: "text/plain", data: Base.encode64(@att_plaintext)}
+ }
+ }
+
+ resp = Couch.put("/#{db}/#{doc1._id}", body: doc1)
+ assert resp.status_code in [201, 202]
+
+ doc2 = %{
+ _id: "#{@att_doc_id2}",
_attachments: %{
"#{@att_name}": %{content_type: "text/plain", data: Base.encode64(@att_plaintext)}
}
}
- resp = Couch.put("/#{db}/#{doc._id}", body: doc)
+ resp = Couch.put("/#{db}/#{doc2._id}", body: doc2)
assert resp.status_code in [201, 202]
- docs
+
+ att_rev2 = resp.body["rev"]
+ docs ++ [%{"_id": "#{doc2._id}", "_rev": att_rev2}]
end
defp delete(db, docs) do