Skip to content

Loading…

Save tombstones during partial merges. Fixes basho/bitcask#82 #102

Closed
wants to merge 1 commit into from

3 participants

@novalis

I decided to take a hack at this. I don't actually know Erlang, but Bitcask's code is pretty clear. I'm not super-happy with my solution for a couple reasons: First, because there are actually some cases where it is safe to remove tombstones during partial merges, and second, because I think there is still more cleanup that could be done. For instance, del_keydir is probably not necessary at all. But I do think that this is probably an improvement in correctness.

@evanmcc

Thanks! I'll take a look sometime in the coming week.

@slfritchie
Basho Technologies member

Hi, sorry about the delay. We've been pondering this issue on and off, sorry there hasn't been any visible action. I'm not 100% sure this is a good idea: if we end up preserving all tombstones forever, we will just have another problem. Finding a solution there isn't as easy as I'd hoped.

This issue hasn't been forgotten. Either I or Evan will update this ticket when we find a solution.

@novalis

It only keeps tombstones during partial merges, I think (if I can remember anything from when I looked at this). So a full merge should erase stale tombstones.

I actually think you should merge this, because it is better to keep too much data around than to return actually incorrect results.

@slfritchie
Basho Technologies member

Sorry, I disagree slightly. If you consume all RAM with tombstone data, then Bitcask won't be able to store a single thing for you. Requiring a full merge now and then for the sole purpose of deleting unnecessary tombstones from disk is not an option for Basho.

@slfritchie
Basho Technologies member

Hi, David. I'm going to close this PR. Sorry for not using it, but I believe that a better solution will be submitted for PR quite soon.

@slfritchie slfritchie closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Showing with 43 additions and 5 deletions.
  1. +43 −5 src/bitcask.erl
View
48 src/bitcask.erl
@@ -1032,6 +1032,20 @@ merge_files(#mstate { dirname = Dirname,
end,
merge_files(State2#mstate { input_files = Rest }).
+merge_single_entry(K, V, Tstamp, FileId, {_, _, Offset, _} = Pos, State) when State#mstate.partial, V =:= ?TOMBSTONE ->
+ %% In the case where we are doing a partial merge, we must keep
+ %% tombstones around, because otherwise, some existing non-merged
+ %% file might cause a key to be retained which ought to be
+ %% deleted.
+
+ ok = bitcask_nifs:keydir_put(State#mstate.del_keydir, K,
+ FileId, 0, Offset, Tstamp),
+ bitcask_nifs:keydir_remove(State#mstate.live_keydir, K,
+ Tstamp, FileId, Offset),
+ inner_merge_write(K, V, Tstamp,
+ FileId, Offset, State),
+ State;
+
merge_single_entry(K, V, Tstamp, FileId, {_, _, Offset, _} = Pos, State) ->
case out_of_date(K, Tstamp, FileId, Pos, State#mstate.expiry_time, false,
[State#mstate.live_keydir, State#mstate.del_keydir]) of
@@ -1055,11 +1069,7 @@ merge_single_entry(K, V, Tstamp, FileId, {_, _, Offset, _} = Pos, State) ->
bitcask_nifs:keydir_remove(State#mstate.live_keydir, K,
Tstamp, FileId, Offset),
- case State#mstate.partial of
- true -> inner_merge_write(K, V, Tstamp,
- FileId, Offset, State);
- false -> State
- end;
+ State;
false ->
ok = bitcask_nifs:keydir_remove(State#mstate.del_keydir, K),
inner_merge_write(K, V, Tstamp, FileId, Offset, State)
@@ -1800,6 +1810,34 @@ corrupt_file_test() ->
ok.
+merge_delete_test_() ->
+ {timeout, 60, fun merge_delete_test_body/0}.
+
+merge_delete_test_body() ->
+ Opts = [{max_file_size, 120}],
+ Dir = "/tmp/bc.test.merge-delete",
+ os:cmd("rm -rf /tmp/bc.test.merge-delete"),
+ B = bitcask:open(Dir, [read_write | Opts]),
+ PutSome = fun() ->
+ [bitcask:put(B, <<X:32>>, <<"yo this is a value">>) ||
+ X <- lists:seq(1,5)]
+ end,
+ PutSome(),
+ bitcask:delete(B, <<3:32>>),
+ bitcask:merge(Dir, Opts, ["/tmp/bc.test.merge-delete/2.bitcask.data"]),
+ bitcask:close(B),
+
+ B2 = bitcask:open(Dir, [read_write | Opts]),
+ V = bitcask:get(B2, <<3:32>>),
+ Result = if (V =:= not_found) ->
+ ok;
+ true ->
+ Error = lists:flatten(io_lib:format("expected not_found, got ~p", [V])),
+ erlang:error(Error)
+ end,
+ bitcask:close(B2),
+ Result.
+
invalid_data_size_test() ->
TestDir = "/tmp/bc.test.invalid_data_size_test",
TestDataFile = TestDir ++ "/1.bitcask.data",
Something went wrong with that request. Please try again.