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 2020/06/02 15:52:28 UTC
[couchdb] 01/01: Prevent eviction of newer handles from
fabric2_server cache
This is an automated email from the ASF dual-hosted git repository.
vatamane pushed a commit to branch fix-on-commit
in repository https://gitbox.apache.org/repos/asf/couchdb.git
commit 0eeab22b1f609dcfec1ea9468c4e4303cc70de7b
Author: Nick Vatamaniuc <va...@apache.org>
AuthorDate: Tue Jun 2 11:33:36 2020 -0400
Prevent eviction of newer handles from fabric2_server cache
Ensure fabric2_server operations do not evict newer handles by comparing the
metadata version to the what's already in the table. Also, remove on_commit
handler and instead update the fresh db handle right away to let concurrent
request see the updated handle as soon as possible.
---
src/fabric/include/fabric2.hrl | 1 -
src/fabric/src/fabric2_fdb.erl | 76 ++++++++++++-------------------
src/fabric/src/fabric2_server.erl | 62 +++++++++++++++++++++----
src/fabric/test/fabric2_db_misc_tests.erl | 17 +++++++
4 files changed, 100 insertions(+), 56 deletions(-)
diff --git a/src/fabric/include/fabric2.hrl b/src/fabric/include/fabric2.hrl
index 2e71787..29f3b02 100644
--- a/src/fabric/include/fabric2.hrl
+++ b/src/fabric/include/fabric2.hrl
@@ -70,7 +70,6 @@
-define(PDICT_CHECKED_MD_IS_CURRENT, '$fabric_checked_md_is_current').
-define(PDICT_TX_ID_KEY, '$fabric_tx_id').
-define(PDICT_TX_RES_KEY, '$fabric_tx_result').
--define(PDICT_ON_COMMIT_FUN, '$fabric_on_commit_fun').
-define(PDICT_FOLD_ACC_STATE, '$fabric_fold_acc_state').
% Let's keep these in ascending order
diff --git a/src/fabric/src/fabric2_fdb.erl b/src/fabric/src/fabric2_fdb.erl
index e8f6e0d..5857673 100644
--- a/src/fabric/src/fabric2_fdb.erl
+++ b/src/fabric/src/fabric2_fdb.erl
@@ -171,11 +171,7 @@ do_transaction(Fun, LayerPrefix) when is_function(Fun, 1) ->
true ->
get_previous_transaction_result();
false ->
- try
- execute_transaction(Tx, Fun, LayerPrefix)
- after
- erase({?PDICT_ON_COMMIT_FUN, Tx})
- end
+ execute_transaction(Tx, Fun, LayerPrefix)
end
end)
after
@@ -1311,18 +1307,14 @@ check_db_version(#{} = Db, CheckDbVersion) ->
} = Db,
AlreadyChecked = get(?PDICT_CHECKED_DB_IS_CURRENT),
- if not CheckDbVersion orelse AlreadyChecked == true -> Db; true ->
+ if not CheckDbVersion orelse AlreadyChecked == true -> current; true ->
DbVersionKey = erlfdb_tuple:pack({?DB_VERSION}, DbPrefix),
case erlfdb:wait(erlfdb:get(Tx, DbVersionKey)) of
DbVersion ->
put(?PDICT_CHECKED_DB_IS_CURRENT, true),
- Now = erlang:monotonic_time(millisecond),
- Db1 = Db#{check_current_ts := Now},
- on_commit(Tx, fun() -> fabric2_server:store(Db1) end),
- Db1;
+ current;
_NewDBVersion ->
- fabric2_server:remove(maps:get(name, Db)),
- throw({?MODULE, reopen})
+ stale
end
end.
@@ -1830,10 +1822,6 @@ fold_range_cb({K, V}, #fold_acc{} = Acc) ->
restart_fold(Tx, #fold_acc{} = Acc) ->
erase(?PDICT_CHECKED_MD_IS_CURRENT),
- % Not actually committing anyting so we skip on-commit handlers here. Those
- % are usually to refresh db handles in the cache. If the iterator runs for
- % a while it might be inserting a stale handle in there anyway.
- erase({?PDICT_ON_COMMIT_FUN, Tx}),
ok = erlfdb:reset(Tx),
@@ -1871,17 +1859,35 @@ ensure_current(Db) ->
ensure_current(#{} = Db0, CheckDbVersion) ->
require_transaction(Db0),
- Db2 = case check_metadata_version(Db0) of
- {current, Db1} -> Db1;
- {stale, Db1} -> check_db_version(Db1, CheckDbVersion)
+ Db3 = case check_metadata_version(Db0) of
+ {current, Db1} ->
+ Db1;
+ {stale, Db1} ->
+ % If db version is stale, then `reopen` will be thrown. If db
+ % version is current, make sure to update cache with the
+ % latest metadata so other requests can immediately see the
+ % refreshed db handle.
+ case check_db_version(Db1, CheckDbVersion) of
+ current ->
+ % If db version is current, update cache with the latest
+ % metadata so other requests can immediately see the
+ % refreshed db handle.
+ Now = erlang:monotonic_time(millisecond),
+ Db2 = Db1#{check_current_ts := Now},
+ fabric2_server:maybe_update(Db2),
+ Db2;
+ stale ->
+ fabric2_server:maybe_remove(Db1),
+ throw({?MODULE, reopen})
+ end
end,
- case maps:get(security_fun, Db2) of
+ case maps:get(security_fun, Db3) of
SecurityFun when is_function(SecurityFun, 2) ->
- #{security_doc := SecDoc} = Db2,
- ok = SecurityFun(Db2, SecDoc),
- Db2#{security_fun := undefined};
+ #{security_doc := SecDoc} = Db3,
+ ok = SecurityFun(Db3, SecDoc),
+ Db3#{security_fun := undefined};
undefined ->
- Db2
+ Db3
end.
@@ -1928,7 +1934,6 @@ execute_transaction(Tx, Fun, LayerPrefix) ->
erlfdb:set(Tx, get_transaction_id(Tx, LayerPrefix), <<>>),
put(?PDICT_TX_RES_KEY, Result)
end,
- ok = run_on_commit_fun(Tx),
Result.
@@ -1963,27 +1968,6 @@ get_transaction_id(Tx, LayerPrefix) ->
end.
-on_commit(Tx, Fun) when is_function(Fun, 0) ->
- % Here we rely on Tx objects matching. However they contain a nif resource
- % object. Before Erlang 20.0 those would have been represented as empty
- % binaries and would have compared equal to each other. See
- % http://erlang.org/doc/man/erl_nif.html for more info. We assume we run on
- % Erlang 20+ here and don't worry about that anymore.
- case get({?PDICT_ON_COMMIT_FUN, Tx}) of
- undefined -> put({?PDICT_ON_COMMIT_FUN, Tx}, Fun);
- _ -> error({?MODULE, on_commit_function_already_set})
- end.
-
-
-run_on_commit_fun(Tx) ->
- case get({?PDICT_ON_COMMIT_FUN, Tx}) of
- undefined ->
- ok;
- Fun when is_function(Fun, 0) ->
- Fun(),
- ok
- end.
-
with_span(Operation, ExtraTags, Fun) ->
case ctrace:has_span() of
true ->
diff --git a/src/fabric/src/fabric2_server.erl b/src/fabric/src/fabric2_server.erl
index 957efff..bf2c1d8 100644
--- a/src/fabric/src/fabric2_server.erl
+++ b/src/fabric/src/fabric2_server.erl
@@ -17,9 +17,15 @@
-export([
start_link/0,
+
fetch/2,
+
store/1,
+ maybe_update/1,
+
remove/1,
+ maybe_remove/1,
+
fdb_directory/0,
fdb_cluster/0
]).
@@ -66,27 +72,57 @@ start_link() ->
fetch(DbName, UUID) when is_binary(DbName) ->
case {UUID, ets:lookup(?MODULE, DbName)} of
{_, []} -> undefined;
- {undefined, [{DbName, #{} = Db}]} -> Db;
- {<<_/binary>>, [{DbName, #{uuid := UUID} = Db}]} -> Db;
- {<<_/binary>>, [{DbName, #{} = _Db}]} -> undefined
+ {undefined, [{DbName, _UUID, _, #{} = Db}]} -> Db;
+ {<<_/binary>>, [{DbName, UUID, _, #{} = Db}]} -> Db;
+ {<<_/binary>>, [{DbName, _UUID, _, #{} = _Db}]} -> undefined
end.
store(#{name := DbName} = Db0) when is_binary(DbName) ->
- Db1 = Db0#{
- tx := undefined,
- user_ctx := #user_ctx{},
- security_fun := undefined
- },
- true = ets:insert(?MODULE, {DbName, Db1}),
+ #{
+ uuid := UUID,
+ md_version := MDVer
+ } = Db0,
+ Db1 = sanitize(Db0),
+ case ets:insert_new(?MODULE, {DbName, UUID, MDVer, Db1}) of
+ true -> ok;
+ false -> maybe_update(Db1)
+ end,
ok.
+maybe_update(#{name := DbName} = Db0) when is_binary(DbName) ->
+ #{
+ uuid := UUID,
+ md_version := MDVer
+ } = Db0,
+ Db1 = sanitize(Db0),
+ Head = {DbName, UUID, '$1', '_'},
+ Guard = {'=<', '$1', MDVer},
+ Body = {DbName, UUID, MDVer, {const, Db1}},
+ try
+ 1 =:= ets:select_replace(?MODULE, [{Head, [Guard], [{Body}]}])
+ catch
+ error:badarg ->
+ false
+ end.
+
+
remove(DbName) when is_binary(DbName) ->
true = ets:delete(?MODULE, DbName),
ok.
+maybe_remove(#{name := DbName} = Db) when is_binary(DbName) ->
+ #{
+ uuid := UUID,
+ md_version := MDVer
+ } = Db,
+ Head = {DbName, UUID, '$1', '_'},
+ Guard = {'=<', '$1', MDVer},
+ 0 < ets:select_delete(?MODULE, [{Head, [Guard], [true]}]).
+
+
init(_) ->
ets:new(?MODULE, [
public,
@@ -229,3 +265,11 @@ set_option(Db, Option, Val) ->
Msg = "~p : Could not set fdb tx option ~p = ~p",
couch_log:error(Msg, [?MODULE, Option, Val])
end.
+
+
+sanitize(#{} = Db) ->
+ Db#{
+ tx := undefined,
+ user_ctx := #user_ctx{},
+ security_fun := undefined
+ }.
diff --git a/src/fabric/test/fabric2_db_misc_tests.erl b/src/fabric/test/fabric2_db_misc_tests.erl
index 9c95ca5..2fad73f 100644
--- a/src/fabric/test/fabric2_db_misc_tests.erl
+++ b/src/fabric/test/fabric2_db_misc_tests.erl
@@ -50,6 +50,7 @@ misc_test_() ->
?TDEF(ensure_full_commit),
?TDEF(metadata_bump),
?TDEF(db_version_bump),
+ ?TDEF(db_cache_doesnt_evict_newer_handles),
?TDEF(events_listener)
])
}
@@ -375,6 +376,22 @@ db_version_bump({DbName, _, _}) ->
?assertMatch(#{db_version := NewDbVersion}, Db2).
+db_cache_doesnt_evict_newer_handles({DbName, _, _}) ->
+ {ok, Db} = fabric2_db:open(DbName, [{user_ctx, ?ADMIN_USER}]),
+ CachedDb = fabric2_server:fetch(DbName, undefined),
+
+ StaleDb = Db#{md_version := <<0>>},
+
+ ok = fabric2_server:store(StaleDb),
+ ?assertEqual(CachedDb, fabric2_server:fetch(DbName, undefined)),
+
+ ?assert(not fabric2_server:maybe_update(StaleDb)),
+ ?assertEqual(CachedDb, fabric2_server:fetch(DbName, undefined)),
+
+ ?assert(not fabric2_server:maybe_remove(StaleDb)),
+ ?assertEqual(CachedDb, fabric2_server:fetch(DbName, undefined)).
+
+
events_listener({DbName, Db, _}) ->
Opts = [
{dbname, DbName},