mnesia: add select_reverse/1-4 #9475
Conversation
|
|
CT Test Results 2 files 59 suites 18m 56s ⏱️ Results for commit 711a508. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts
// Erlang/OTP Github Action Bot |
Bentheburrito
force-pushed
the
mnesia-select-reverse
branch
2 times, most recently
from
3767c4f to
4d892c5
Compare
There was a problem hiding this comment.
Hi, I've executed code coverage on this change, and we're missing things. Some cases of select_reverse are not covered by tests.
While not everything in existing code is covered by tests, we should keep it at least as good as before.
You can run code coverage like this (change path to mnesia.cover file):
make mnesia_test ARGS="-cover /workspaces/otp/lib/mnesia/test/mnesia.cover"
The other problematic thing is documentation:
You can see that your select_reverse functions in mnesia can execute Mod:select_reverse functions, so they can use mnesia_access callback module.
In at least those 2 places you should document the select_reverse functions:
- https://www.erlang.org/doc/apps/mnesia/mnesia_chap4.html#activity-concept-and-various-access-contexts
- https://www.erlang.org/doc/apps/mnesia/mnesia_app_b
Another thing is that mnesia_access behavior is implemented in mnesia_frag.erl so you should add select_reverse functions there as well.
Fragmented tables do not support ordering, so it should be fine to do:
select_reverse(...) -> select(...). in mnesia_frag.erl, but it should be covered by a test case as well.
|
Hi, thanks for the review, I believe i've updated the docs + While making these changes, I noticed there's a
odd record_name_dirty_access_xets failure
git patch that causes above error
diff --git a/lib/mnesia/test/ext_test.erl b/lib/mnesia/test/ext_test.erl
index 7d57955511..9e2b07589d 100644
--- a/lib/mnesia/test/ext_test.erl
+++ b/lib/mnesia/test/ext_test.erl
@@ -47,7 +47,8 @@
insert/3, update_counter/4,
lookup/3,
delete/3, match_delete/3,
- select/1, select/3, select/4, repair_continuation/2
+ select/1, select/3, select/4, select_reverse/1,
+ select_reverse/3, select_reverse/4, repair_continuation/2
]).
semantics(ext_ram_copies, storage) -> ram_copies;
@@ -246,6 +247,18 @@ select(Alias, Tab, Ms, Limit) ->
?DBG({Alias, ext_test_server:tab_to_list(Tab), Ms, Limit}),
call({?FUNCTION_NAME, Alias, Tab, Ms, Limit}).
+select_reverse(Continuation) ->
+ ?DBG(Continuation),
+ call({?FUNCTION_NAME, Continuation}).
+
+select_reverse(Alias, Tab, Ms) ->
+ ?DBG({Alias, ext_test_server:tab_to_list(Tab), Ms}),
+ call({?FUNCTION_NAME, Alias, Tab, Ms}).
+
+select_reverse(Alias, Tab, Ms, Limit) ->
+ ?DBG({Alias, ext_test_server:tab_to_list(Tab), Ms, Limit}),
+ call({?FUNCTION_NAME, Alias, Tab, Ms, Limit}).
+
repair_continuation(Cont, Ms) ->
?DBG({Cont, Ms}),
call({?FUNCTION_NAME, Cont, Ms}).
diff --git a/lib/mnesia/test/ext_test_server.erl b/lib/mnesia/test/ext_test_server.erl
index 22a1ec8f0a..6f3e7e5007 100644
--- a/lib/mnesia/test/ext_test_server.erl
+++ b/lib/mnesia/test/ext_test_server.erl
@@ -121,28 +121,37 @@ receive_data(Data, Alias, Tab, Sender, {Name, Sender} = MnesiaState, State) ->
?DBG({Data, Alias, tab_to_list(Tab), State}),
receive_data(Data, Alias, Tab, Sender, {Name, Tab, Sender}, State).
-select(Alias, Tab, Ms, State) ->
+select(Alias, Tab, Ms, State, Dir) ->
Res = select(Alias, Tab, Ms, 100000, State),
- select_1(Alias, Res).
+ select_1(Alias, Res, Dir).
-select_1(_Alias, '$end_of_table') -> [];
-select_1(ext_ram_copies, {Acc, C}) ->
+select_1(_Alias, '$end_of_table', _Dir) -> [];
+select_1(ext_ram_copies, {Acc, C}, forward) ->
case ets:select(C) of
'$end_of_table' -> Acc;
{New, Cont} ->
- select_1(ext_ram_copies, {New ++ Acc, Cont})
+ select_1(ext_ram_copies, {New ++ Acc, Cont}, forward)
end;
-select_1(ext_disc_only_copies, {Acc, C}) ->
+select_1(ext_ram_copies, {Acc, C}, reverse) ->
+ case ets:select_reverse(C) of
+ '$end_of_table' -> Acc;
+ {New, Cont} ->
+ select_1(ext_ram_copies, {New ++ Acc, Cont}, reverse)
+ end;
+select_1(ext_disc_only_copies, {Acc, C}, Dir) ->
case dets:select(C) of
'$end_of_table' -> Acc;
{New, Cont} ->
- select_1(ext_disc_only_copies, {New ++ Acc, Cont})
+ select_1(ext_disc_only_copies, {New ++ Acc, Cont}, Dir)
end.
-select(ext_ram_copies, Tab, Ms, Limit, State) when is_integer(Limit); Limit =:= infinity ->
+select(ext_ram_copies, Tab, Ms, Limit, State, forward) when is_integer(Limit); Limit =:= infinity ->
?DBG({ext_ram_copies, tab_to_list(Tab), Ms, Limit}),
ets:select(tab_to_tid(Tab, State), Ms, Limit);
-select(ext_disc_only_copies, Tab, Ms, Limit, State) when is_integer(Limit); Limit =:= infinity ->
+select(ext_ram_copies, Tab, Ms, Limit, State, reverse) when is_integer(Limit); Limit =:= infinity ->
+ ?DBG({ext_ram_copies, tab_to_list(Tab), Ms, Limit}),
+ ets:select_reverse(tab_to_tid(Tab, State), Ms, Limit);
+select(ext_disc_only_copies, Tab, Ms, Limit, State, _Dir) when is_integer(Limit); Limit =:= infinity ->
?DBG({ext_disc_only_copies, tab_to_list(Tab), Ms, Limit}),
dets:select(tab_to_tid(Tab, State), Ms, Limit).
@@ -375,12 +384,34 @@ handle_call({select, C}, _From, State) ->
handle_call({select, Alias, Tab, Ms}, _From, State) ->
?DBG({select, Alias, tab_to_list(Tab), Ms}),
- Res = ?TRY(select(Alias, Tab, Ms, State)),
+ Res = ?TRY(select(Alias, Tab, Ms, State, forward)),
{reply, Res, State};
handle_call({select, Alias, Tab, Ms, Limit}, _From, State) ->
?DBG({select, Alias, tab_to_list(Tab), Ms, Limit}),
- Res = ?TRY(select(Alias, Tab, Ms, Limit, State)),
+ Res = ?TRY(select(Alias, Tab, Ms, Limit, State, forward)),
+ {reply, Res, State};
+
+handle_call({select_reverse, '$end_of_table' = End}, _From, State) ->
+ ?DBG({select_reverse, End}),
+ {reply, End, State};
+handle_call({select_reverse, C}, _From, State) when element(1, C) == dets_cont ->
+ ?DBG({select_reverse, {ext_disc_only_copies, C}}),
+ Res = ?TRY(dets:select(C)),
+ {reply, Res, State};
+handle_call({select_reverse, C}, _From, State) ->
+ ?DBG({select_reverse, {ext_ram_copies, C}}),
+ Res = ?TRY(ets:select_reverse(C)),
+ {reply, Res, State};
+
+handle_call({select_reverse, Alias, Tab, Ms}, _From, State) ->
+ ?DBG({select_reverse, Alias, tab_to_list(Tab), Ms}),
+ Res = ?TRY(select(Alias, Tab, Ms, State, reverse)),
+ {reply, Res, State};
+
+handle_call({select_reverse, Alias, Tab, Ms, Limit}, _From, State) ->
+ ?DBG({select_reverse, Alias, tab_to_list(Tab), Ms, Limit}),
+ Res = ?TRY(select(Alias, Tab, Ms, Limit, State, reverse)),
{reply, Res, State};
handle_call({repair_continuation, '$end_of_table' = Cont, _Ms}, _From, State) ->
@@ -425,4 +456,4 @@ tab_to_filename(Tab) ->
tab_to_tid(Tab, #state{tables = Tables}) ->
Table = maps:get(Tab, Tables),
- Table#table.tid.
\ No newline at end of file
+ Table#table.tid.
By the way, do you know if it's possible to configure the test cases to use a specific data dir? The tests are a bit harsh (and slow) on my hard drive, so being able to point it at some tmpfs in memory would be helpful. Also, is there a way to run the tests in parallel? (I didn't see anything in TESTING.md about it) |
|
Hi the patch you sent does not apply for me, you seem to have pushed commit to your branch today, could you send updated patch file? |
Bentheburrito
force-pushed
the
mnesia-select-reverse
branch
from
60e8147 to
774c2eb
Compare
ah sorry, it was something silly, you can disregard the patch/error -
thanks! That and |
|
Seems like an ephemeral error in CI? The mnesia check looks good on my fork |
Hi, there was some configuration changed quite recently and I've had similar issues on my PR's as well. You can try to rebase on latest master and force push to your branch again. I've closed some conversations when the issue was resolved, and added comments in existing ones, where issue is still present. |
|
Hi @Bentheburrito are you planning on continuing this PR? |
Yes, just need to make time to address the rest of your comments |
Adds mnesia:select_reverse/1-4 and supporting functions in mnesia_lib, bringing the mnesia API closer to ets's. Related GH issue: erlang#8993
also fix an apparent bug in ext_test_server's repair_continuation handle_call clause
Bentheburrito
force-pushed
the
mnesia-select-reverse
branch
from
774c2eb to
9f5a4f5
Compare
| {?DEFAULT_ACCESS, Tid, Ts} -> | ||
| select_reverse(Tid, Ts, Tab, Pat, NObjects, LockKind); | ||
| {Mod, Tid, Ts} -> | ||
| Mod:select_reverse(Tid, Ts, Tab, Pat, NObjects, LockKind); |
There was a problem hiding this comment.
I see this line is still not covered.
There was a problem hiding this comment.
This (and the other coverage comments below) appear to be covered for me running mnesia_evil_coverage_test + mnesia_frag_test. Can you share the test command you're running that shows the missing coverage in these spots?
for reference, what I'm doing is temporarily adding {mnesia_frag_test, all}, to the all/0 list in mnesia_evil_coverage_test.erl, then I run the evil tests with
make mnesia_test ARGS="-mnesia dir '"/mnesia_test_tmp"' -logdir /mnesia_test_tmp -suite mnesia_SUITE -group evil -cover $ERL_TOP/lib/mnesia/test/mnesia.cover"(the -mnesia dir and -logdir probably aren't relevant, but just including for completeness)
There was a problem hiding this comment.
I will check then, maybe some test that covers this is disabled or something.
The command I'm using is:
make mnesia_test ARGS="-cover /workspaces/otp/lib/mnesia/test/mnesia.cover"
There was a problem hiding this comment.
Hi, I've checked this, and there are some tests which should be running, but they're not for some reason.
I will try to fix this, but we don't have to wait for the fix with this PR.
I will check this PR again, and also run this through our CI, and let you know if it's okay to merge.
There was a problem hiding this comment.
Ahh I see, sounds good, happy to rebase if you happen to fix the tests before then
Mikaka27
added
the
testing
There was a problem hiding this comment.
Hi,
I've consulted with another team member, and there is still a problem here.
For example in select_reverse/1 and select/1 we're trying to pass direction, but it doesn't matter at all.
I've tried this in a test like this:
test(Config) when is_list(Config) ->
[Node1] = Nodes = ?acquire_nodes(1, Config),
Tab = select_reverse_tab,
Schema = [{name, Tab}, {attributes, [k, v]}, {ram_copies, [Node1]}, {type, ordered_set}],
?match({atomic, ok}, mnesia:create_table(Schema)),
OneRec = {Tab, 1, 2},
TwoRec = {Tab, 2, 3},
ThreeRec = {Tab, 3, 4},
AllPat = [{'_', [], ['$_']}],
?match({atomic, ok},
mnesia:transaction(fun() -> mnesia:write(OneRec) end)),
?match({atomic, ok},
mnesia:transaction(fun() -> mnesia:write(TwoRec) end)),
?match({atomic, ok},
mnesia:transaction(fun() -> mnesia:write(ThreeRec) end)),
% {[ThreeRec], Cont1} = ets:select_reverse(Tab, AllPat, 1),
% {[TwoRec], Cont2} = ets:select(Cont1),
% {[ThreeRec], _} = ets:select(Cont2),
?match({atomic, ThreeRec},
mnesia:transaction(fun() ->
{[ThreeRec], Cont1} = mnesia:select_reverse(Tab, AllPat, 1, read),
{[TwoRec], Cont2} = mnesia:select_reverse(Cont1),
{[ThreeRec], _} = mnesia:select(Cont2),
ThreeRec
end)),
?verify_mnesia(Nodes, []).
You can see that this will fail, because you cannot change direction after you got your continuation (at least in this simple case). You can uncomment the ets case, and it also does not allow to change direction.
We think we can do the following here:
- get rid of
mnesia:select_reverse/1implementation and just make it callmnesia:select/1 - store direction alongside continuation in mnesia_select record, and don't allow to try to change direction
Sorry for not noticing this earlier.
Mikaka27
removed
the
testing
|
That makes sense, I also noticed this at some point and didn't mention it since it matches how ETS works. I'll look at removing |
|
Hi, We only thought that you can get rid of implementation of select_reverse/1, and just do select_reverse(Cont) -> select(Cont). Regarding the hot code upgrades, yes it will break old continuations. I think in your case it would be easiest to do something like this: At least I would try to do it like this, I hope I didn't make a mistake, I didn't check if that example would compile, sorry :( |
Thanks for clarifying, I misread that before. I pushed this change Actually could you elaborate on the need to add |
|
Hi @Mikaka27, just wanted to follow up to see if my last comment makes sense. To elaborate a little more: my thinking is, by not storing a I guess one assumption is that the backend documents this behavior difference from |
lib/mnesia/test/ext_test.erl
Outdated
| select_reverse(Continuation) -> | ||
| ?DBG(Continuation), | ||
| call({?FUNCTION_NAME, Continuation}). |
There was a problem hiding this comment.
Since you removed mnesia_lib:db_select_rev_cont this will never be called, I think you can remove it.
There was a problem hiding this comment.
I've removed it, you can check @Bentheburrito
We've talked about this in the team, and we shouldn't do it, it might be confusing to the users. It's better to stick to the behavior of ets. |
Mikaka27
added
the
testing
Hello, this PR adds
mnesia:select_reverse/1-4and supporting functions inmnesia_lib, bringing the mnesia API closer toets's.Related issue: #8993
Much of this is copy/paste of
select/1-6, with the needed tweaks to call the newmnesia_lib:*_rev_*functions andlists:sortin descending order. The tests are also nearly identical toselect's tests, only they assert on the expected reverse ordering.