Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve tombstone management #128

Closed
wants to merge 39 commits into from
Closed

Improve tombstone management #128

Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
eb182ba
Add regression test for GH #82
slfritchie Dec 15, 2013
1a9c994
Fix race with merge & bitcask:delete(): remove from keydir *first*
slfritchie Dec 15, 2013
d594f90
Change order of scan & fold: now oldest -> youngest
slfritchie Dec 15, 2013
8805790
New regression test (currently broken) for recent scan change of olde…
slfritchie Dec 15, 2013
a331587
Fix rough edges on test/bitcask_qc.erl & test/bitcask_qc_expiry.erl
slfritchie Dec 15, 2013
239ed71
Write tombstone on delete only if there's a keydir entry, plus race b…
slfritchie Dec 15, 2013
afc5c3f
Fix remaining bug when bitcask scan/fold order was changed to oldest-…
slfritchie Dec 17, 2013
1712650
FML: fix tombstone merging bug when K & K's tombstone are in same fileid
slfritchie Dec 17, 2013
dd1c8b4
WIP: new test new_20131217_c_test_() is broken, need to experiment wi…
slfritchie Dec 18, 2013
a99916f
Fix non-determinism in the fold_visits_unfrozen_test test ... now 100…
slfritchie Dec 18, 2013
2af49f0
Fix iterator freeze bug demonstrated by last commit: remove kh_put_wi…
slfritchie Dec 18, 2013
662fba8
Fix bug introduced by almost-bugfix in commit 1a9c99 that was suppose…
slfritchie Dec 18, 2013
990b5ca
Fix unexported function errors, thanks Buildbot
slfritchie Dec 18, 2013
4019fd0
Omnibus bugfixing, including:
slfritchie Dec 21, 2013
89711f3
Add UNIX epoch time to put_int NIF call, to avoid C use of time(3).
slfritchie Dec 21, 2013
dd068bf
Fix buildbot ?TESTDIR definition location problem
slfritchie Dec 21, 2013
0695ba1
Uff da: most PULSE non-determinism problems fixed?
slfritchie Dec 23, 2013
0c3699a
Fix merge race when keydir_put specifies old fileid & offset races wi…
slfritchie Dec 23, 2013
c0f8760
Fix bug in previous commit: offset=0 is valid, do not check it
slfritchie Dec 24, 2013
91a781b
Use larger retry # for open_fold_files() and keydir_wait_ready() when…
slfritchie Dec 24, 2013
3c4ffa5
Small bitcask_pulse changes: file size, pretty the keys used, slightl…
slfritchie Dec 24, 2013
f27c640
Adjust command frequency weightings for PULSE model
slfritchie Dec 25, 2013
a981d53
Add no_tombstones_after_reopen_test() to test desired condition
slfritchie Dec 26, 2013
2690354
Avoid storing tombstones in the keydir when possible
slfritchie Dec 26, 2013
cef207b
Add checks to the PULSE model to try to catch improper tombstones in …
slfritchie Dec 26, 2013
72d87aa
Dialyzer fixes
slfritchie Dec 26, 2013
78a2667
Add failing test: tombstones exist in keydir if hint files are destroyed
slfritchie Dec 26, 2013
7bb2db0
Add {tombstone,Key} wrapper when folding .data files; fixes test fail…
slfritchie Dec 26, 2013
01d4538
Fix intermittent bitcask_merge_worker intermittent timeout failure
slfritchie Jan 7, 2014
1a444dd
NIF review: avoid looking at argv[1] directly
slfritchie Jan 7, 2014
f0d5ffc
Remove unwanted oopsie foo.erl
slfritchie Jan 7, 2014
2518628
Add an 'epoch' counter to subdivide Bitcask timestamps
slfritchie Jan 22, 2014
1563feb
Add sibling->regular entry conversion sweeper, to GC keydir siblings
slfritchie Jan 24, 2014
9db9248
Add new EUnit test and fix PULSE model
slfritchie Jan 28, 2014
432f3b1
Fix last two Dialyzer errors by removing dead code
slfritchie Jan 28, 2014
a50a1c0
Add error filter for harmless race
slfritchie Jan 30, 2014
959ecce
Add gets() to PULSE model, analogus to puts() for multiple keys
slfritchie Jan 30, 2014
5f4d145
Add new NIF, bitcask_nifs_keydir_global_pending_frozen/0, to assist P…
slfritchie Jan 30, 2014
c9bf3a3
Review fixer-upper fixes, both small but worthwhile, thanks!
slfritchie Feb 20, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 72 additions & 12 deletions c_src/bitcask_nifs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,16 @@

#include <stdio.h>

void DEBUG2(const char *fmt, ...) { }
/* #include <stdarg.h> */
/* void DEBUG2(const char *fmt, ...) */
/* { */
/* va_list ap; */
/* va_start(ap, fmt); */
/* vfprintf(stderr, fmt, ap); */
/* va_end(ap); */
/* } */

#ifdef BITCASK_DEBUG
#include <stdarg.h>
void DEBUG(const char *fmt, ...)
Expand Down Expand Up @@ -159,7 +169,7 @@ typedef struct
uint32_t newest_folder; // Start time for the last keyfolder
uint64_t iter_generation;
uint64_t pending_updated;
uint64_t pending_start; // os:timestamp() as 64-bit integer
uint64_t pending_start; // UNIX epoch seconds (since 1970)
ErlNifPid* pending_awaken; // processes to wake once pending merged into entries
unsigned int pending_awaken_count;
unsigned int pending_awaken_size;
Expand Down Expand Up @@ -212,6 +222,11 @@ typedef struct
#define set_pending_tombstone(e) {(e)->tstamp = 0; \
(e)->offset = 0; }

// Use a magic number for signaling that a database is both in read-write
// mode and that we want to do a get while ignoring the iteration status
// of the keydir.
#define MAGIC_OVERRIDE_ITERATING_STATUS 0x42424242

// Atoms (initialized in on_load)
static ERL_NIF_TERM ATOM_ALLOCATION_ERROR;
static ERL_NIF_TERM ATOM_ALREADY_EXISTS;
Expand Down Expand Up @@ -291,7 +306,7 @@ static ErlNifFunc nif_funcs[] =
{"keydir_new", 0, bitcask_nifs_keydir_new0},
{"keydir_new", 1, bitcask_nifs_keydir_new1},
{"keydir_mark_ready", 1, bitcask_nifs_keydir_mark_ready},
{"keydir_put_int", 9, bitcask_nifs_keydir_put_int},
{"keydir_put_int", 10, bitcask_nifs_keydir_put_int},
{"keydir_get_int", 4, bitcask_nifs_keydir_get_int},
{"keydir_remove", 3, bitcask_nifs_keydir_remove},
{"keydir_remove_int", 6, bitcask_nifs_keydir_remove},
Expand All @@ -304,6 +319,7 @@ static ErlNifFunc nif_funcs[] =
{"keydir_trim_fstats", 2, bitcask_nifs_keydir_trim_fstats},

{"increment_file_id", 1, bitcask_nifs_increment_file_id},
{"increment_file_id", 2, bitcask_nifs_increment_file_id},

{"lock_acquire_int", 2, bitcask_nifs_lock_acquire},
{"lock_release_int", 1, bitcask_nifs_lock_release},
Expand Down Expand Up @@ -1058,6 +1074,7 @@ ERL_NIF_TERM bitcask_nifs_keydir_put_int(ErlNifEnv* env, int argc, const ERL_NIF
bitcask_keydir_handle* handle;
bitcask_keydir_entry_proxy entry;
ErlNifBinary key;
uint32_t nowsec;
uint32_t newest_put;
uint32_t old_file_id;
uint64_t old_offset;
Expand All @@ -1068,15 +1085,17 @@ ERL_NIF_TERM bitcask_nifs_keydir_put_int(ErlNifEnv* env, int argc, const ERL_NIF
enif_get_uint(env, argv[3], &(entry.total_sz)) &&
enif_get_uint64_bin(env, argv[4], &(entry.offset)) &&
enif_get_uint(env, argv[5], &(entry.tstamp)) &&
enif_get_uint(env, argv[6], &(newest_put)) &&
enif_get_uint(env, argv[7], &(old_file_id)) &&
enif_get_uint64_bin(env, argv[8], &(old_offset)))
enif_get_uint(env, argv[6], &(nowsec)) &&
enif_get_uint(env, argv[7], &(newest_put)) &&
enif_get_uint(env, argv[8], &(old_file_id)) &&
enif_get_uint64_bin(env, argv[9], &(old_offset)))
{
bitcask_keydir* keydir = handle->keydir;
entry.key = (char*)key.data;
entry.key_sz = key.size;

LOCK(keydir);
DEBUG2("LINE %d put\r\n", __LINE__);

DEBUG("+++ Put key = %d file_id=%d offset=%d total_sz=%d tstamp=%u old_file_id=%d\r\n",
(int)(key.data[3]),
Expand All @@ -1090,21 +1109,35 @@ ERL_NIF_TERM bitcask_nifs_keydir_put_int(ErlNifEnv* env, int argc, const ERL_NIF
// If conditional put and not found, bail early
if (!f.found && old_file_id != 0)
{
DEBUG2("LINE %d put -> already_exists\r\n", __LINE__);
UNLOCK(keydir);
return ATOM_ALREADY_EXISTS;
}

// If put would resize and iterating, start pending hash
if (kh_put_will_resize(entries, keydir->entries) &&
keydir->keyfolders != 0 &&
if (keydir->keyfolders != 0 &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to kill the new multiple folds improvement. Is it intentional?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we can't do this, given how reliant we now are on AAE.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand, sorry, both based on my sketchy knowledge of khash's internals, the multi-fold work, and the fact that if you restore that ky_put_will_resize() check then the PULSE test will start spewing failures very quickly because the keydir isn't properly frozen.

If the put is going to resize the hash, I certainly agree that a freeze is required in order to keep the folders' sorting order stable. However, this function is doing a mutation, and with a mutation of any kind (if there are any keyfolders) we must freeze. My brain can't think of a reason why you'd have keyfolders != 0 and doing a mutation where you would not want to freeze ... but then again, it's -18F here in Minneapolis today, my brain may be slushy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the work for multi-folds was aimed at making this possible: delaying the inevitable freeze until a put changed the number of khash slots. Iterators are linked to a timestamp and tolerate finding entries added after the iteration was started. Entries can now be simple entries or linked lists, which can contain multiple versions of an entry. Puts then do not replace the entry, but add another version to this list (or convert a plain entry to a list). Iterators will choose from this list of timestamped entries the one from the snapshot they belong to. When the pending hash is merged (freeze is over!), all entries are merged back to good ol' plain entries. It's a lot of fun, you should try it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a lot of fun, you should try it.

Ha, then how about a change to the PULSE model to deal with freezes that aren't really freezes? Or change all of the other logic to compare timestamps in the same way so that real frozenness returns?

Is the goal of delaying the freeze is simply a RAM-consumption optimization?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it is there to make sure the number of concurrent keyfolders is not limited by when the next write is coming, which is usually "there, it's already here". In fact, memory consumption could be higher with this, as you can end up with many versions of values. It depends on initial value of the khash table and typical growth during freeze I suppose. I have not looked at the implications for the PULSE model, but it does sound like we'll need to work on that soon to merge this work before 2.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apparently do not understand the goals of the multiple folds work. I'll try to review the original PR & comments today, because I'm stuck in a pit of befuddlement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright ... I put the will_resize check back in the code. The bitcask_pulse test finds this problem with a single folder (the fork_merge step in the counterexample):

[[{set,{var,11},{call,bitcask_pulse,bc_open,[true]}},
  {set,{var,13},
       {call,bitcask_pulse,puts,[{var,11},{1,16},<<0>>]}},
  {set,{var,18},{call,bitcask_pulse,bc_close,[{var,11}]}},
  {set,{var,23},{call,bitcask_pulse,incr_clock,[]}},
  {set,{var,24},{call,bitcask_pulse,bc_open,[true]}},
  {set,{var,26},
       {call,bitcask_pulse,puts,[{var,24},{4,13},<<0>>]}},
  {set,{var,31},{call,bitcask_pulse,fork_merge,[{var,24}]}},
  {set,{var,34},{call,bitcask_pulse,fold,[{var,24}]}},
  {set,{var,37},{call,bitcask_pulse,fold,[{var,24}]}}],
 {19747,13791,98974},
 [{events,[]}]]

The problem is that the fold in step # 37 sees the key 14 twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I've a different counterexample that skips keys: a fold doesn't see keys that have been put and have never been deleted.

7> C9c.
[[{set,{var,32},{call,bitcask_pulse,bc_open,[true]}},
  {set,{var,46},
       {call,bitcask_pulse,puts,[{var,32},{1,1},<<0,0,0>>]}},
  {set,{var,48},{call,bitcask_pulse,bc_close,[{var,32}]}},
  {set,{var,64},{call,bitcask_pulse,bc_open,[true]}},
  {set,{var,65},
       {call,bitcask_pulse,puts,[{var,64},{1,22},<<0>>]}},
  {set,{var,66},{call,bitcask_pulse,fork_merge,[{var,64}]}},
  {set,{var,81},{call,bitcask_pulse,incr_clock,[]}},
  {set,{var,82},{call,bitcask_pulse,bc_close,[{var,64}]}},
  {set,{var,85},{call,bitcask_pulse,bc_open,[true]}},
  {set,{var,87},{call,bitcask_pulse,fold,[{var,85}]}}],
 {86273,69841,50357},
 [{events,[]}]]

Here is an annotated timeline of the race:

folding proc:                      merging proc:
-------------------------------    ------------------------------------------

%% Fold starts.
{list_data_files,[1,2,3]}
{subfold,processing_file_number,1}

                                  %% Merge starts
                                  {merge_files,input_list,[2]}
                                  {inner_merge_write,fresh,new_file_is,4}
                                  {merge_single_entry,<<"kk02">>,
                                   old_location,file,2,offset,19}
                                  {merge_single_entry,<<"kk02">>,
                                   not_out_of_date}
                                  {inner_merge_write,<<"kk02">>,before_write}

{subfold,processing_file_number,2}

                                  {inner_merge_write,<<"kk02">>,
                                   new_location,file,4,offset,19,
                                   old_location,file,2,offset,19}
                                  {inner_merge_write,<<"kk02">>,keydir_put,ok}

%% The fold reaches the place in
%% file #2 where key <<"kk02">> is stored.
{fold,<<"kk02">>,keydir_get,not_exact,
 file_number,2,offset,19,
 keydir_location_is_now,file,4,offset,19
 fold_does_not_see_this_entry}

{subfold,processing_file_number,3}

%% key <<"kk02">> is not found in file #3, nor is it found in
%% any other file that the fold can process (in this case, file 3 is
%% the last file in the fold's list of files).

If the keydir is frozen, then the keydir update by the merge causes the keydir to freeze, and the folder sees <<"kk02">>'s entry in file # 2, the keydir's frozenness will allow the folder to see that the <<"kk02">>'s keydir entry is that same place in file # 2.

But I'm not seeing an easy way to fix this merge race without consuming more memory ... I need to sleep on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm, well, my comment about a fix without using more RAM may or may not survive.

For the 2nd problem mentioned above, the one with the annotated timeline. I am not understanding why the mutation made by the "merging proc" isn't visible by the "folding proc" at the place where the fold reaches that key in file # 2.

(keydir->pending == NULL))
{
keydir->pending = kh_init(entries);
keydir->pending_start = time(NULL);
keydir->pending_start = nowsec;
}

if (!f.found || f.is_tombstone)
{
if ((newest_put &&
(entry.file_id < keydir->biggest_file_id)) ||
old_file_id != 0) {
/*
* Really, it doesn't exist. But the atom 'already_exists'
* is also a signal that a merge has incremented the
* keydir->biggest_file_id and that we need to retry this
* operation after Erlang-land has re-written the key & val
* to a new location in the same-or-bigger file id.
*/
DEBUG2("LINE %d put -> already_exists\r\n", __LINE__);
UNLOCK(keydir);
return ATOM_ALREADY_EXISTS;
}

keydir->key_count++;
keydir->key_bytes += key.size;

Expand All @@ -1117,6 +1150,7 @@ ERL_NIF_TERM bitcask_nifs_keydir_put_int(ErlNifEnv* env, int argc, const ERL_NIF
DEBUG("+++ Put new\r\n");
DEBUG_KEYDIR(keydir);

DEBUG2("LINE %d put -> ok (!found || !tombstone)\r\n", __LINE__);
UNLOCK(keydir);
return ATOM_OK;
}
Expand All @@ -1127,6 +1161,7 @@ ERL_NIF_TERM bitcask_nifs_keydir_put_int(ErlNifEnv* env, int argc, const ERL_NIF
old_offset == f.proxy.offset))
{
DEBUG("++ Conditional not match\r\n");
DEBUG2("LINE %d put -> already_exists/cond bad match\r\n", __LINE__);
UNLOCK(keydir);
return ATOM_ALREADY_EXISTS;
}
Expand Down Expand Up @@ -1163,6 +1198,7 @@ ERL_NIF_TERM bitcask_nifs_keydir_put_int(ErlNifEnv* env, int argc, const ERL_NIF
}

put_entry(keydir, &f, &entry);
DEBUG2("LINE %d put -> ok\r\n", __LINE__);
UNLOCK(keydir);
DEBUG("Finished put\r\n");
DEBUG_KEYDIR(keydir);
Expand All @@ -1176,6 +1212,7 @@ ERL_NIF_TERM bitcask_nifs_keydir_put_int(ErlNifEnv* env, int argc, const ERL_NIF
update_fstats(env, keydir, entry.file_id, entry.tstamp,
0, 1, 0, entry.total_sz);
}
DEBUG2("LINE %d put -> already_exists end\r\n", __LINE__);
UNLOCK(keydir);
DEBUG("No update\r\n");
return ATOM_ALREADY_EXISTS;
Expand Down Expand Up @@ -1206,8 +1243,10 @@ ERL_NIF_TERM bitcask_nifs_keydir_get_int(ErlNifEnv* env, int argc, const ERL_NIF

DEBUG("+++ Get issued\r\n");

int iterating_status = (rw_p == MAGIC_OVERRIDE_ITERATING_STATUS) ?
0 : handle->iterating;
find_result f;
find_keydir_entry(keydir, &key, time, handle->iterating, &f);
find_keydir_entry(keydir, &key, time, iterating_status, &f);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make names clear, we should probably change some names. In find_keydir_entry, iterating should be called use_snapshot, get_from_snapshot or similar. This is not from your PR. I wrote that one. My bad :). A similar name should be given to iterating_status.


if (f.found && !f.is_tombstone && (rw_p || !f.no_snapshot))
{
Expand Down Expand Up @@ -1283,7 +1322,7 @@ ERL_NIF_TERM bitcask_nifs_keydir_remove(ErlNifEnv* env, int argc, const ERL_NIF_
{
UNLOCK(keydir);
DEBUG("+++Conditional no match\r\n");
return ATOM_OK;
return ATOM_ALREADY_EXISTS;
}

// Remove the key from the keydir stats
Expand All @@ -1297,12 +1336,14 @@ ERL_NIF_TERM bitcask_nifs_keydir_remove(ErlNifEnv* env, int argc, const ERL_NIF_
// If found an entry in the pending hash, convert it to a tombstone
if (fr.pending_entry)
{
DEBUG2("LINE %d pending put\r\n", __LINE__);
set_pending_tombstone(fr.pending_entry);
}
// If frozen, add tombstone to pending hash (iteration must have
// started between put/remove call in bitcask:delete.
else if (keydir->pending)
{
DEBUG2("LINE %d pending put\r\n", __LINE__);
bitcask_keydir_entry* pending_entry =
add_entry(keydir, keydir->pending, &fr.proxy);
set_pending_tombstone(pending_entry);
Expand Down Expand Up @@ -1401,6 +1442,7 @@ ERL_NIF_TERM bitcask_nifs_keydir_copy(ErlNifEnv* env, int argc, const ERL_NIF_TE
}
if (keydir->pending != NULL)
{
DEBUG2("LINE %d pending copy\r\n", __LINE__);
for (itr = kh_begin(keydir->pending); itr != kh_end(keydir->pending); ++itr)
{
// Allocate our entry to be inserted into the new table and copy the record
Expand Down Expand Up @@ -1450,14 +1492,17 @@ static int can_itr_keydir(bitcask_keydir* keydir, uint64_t ts, int maxage, int m
if (keydir->pending == NULL || // not frozen or caller wants to reuse
(maxage < 0 && maxputs < 0)) // the exiting freeze
{
DEBUG2("LINE %d can_itr\r\n", __LINE__);
return 1;
}
else if (ts == 0 || ts < keydir->pending_start)
{ // if clock skew (or forced wait), force key folding to wait
DEBUG2("LINE %d can_itr\r\n", __LINE__);
return 0; // which will fix keydir->pending_start
}
else
{
DEBUG2("LINE %d can_itr\r\n", __LINE__);
uint64_t age = ts - keydir->pending_start;
return ((maxage < 0 || age <= maxage) &&
(maxputs < 0 || keydir->pending_updated <= maxputs));
Expand Down Expand Up @@ -1500,6 +1545,7 @@ ERL_NIF_TERM bitcask_nifs_keydir_itr(ErlNifEnv* env, int argc, const ERL_NIF_TER
keydir->newest_folder = ts;
keydir->keyfolders++;
handle->iterator = kh_begin(keydir->entries);
DEBUG2("LINE %d itr started, keydir->pending = 0x%lx\r\n", __LINE__, keydir->pending);
UNLOCK(handle->keydir);
return ATOM_OK;
}
Expand All @@ -1521,6 +1567,7 @@ ERL_NIF_TERM bitcask_nifs_keydir_itr(ErlNifEnv* env, int argc, const ERL_NIF_TER
}
enif_self(env, &keydir->pending_awaken[keydir->pending_awaken_count]);
keydir->pending_awaken_count++;
DEBUG2("LINE %d itr\r\n", __LINE__);
UNLOCK(handle->keydir);
return ATOM_OUT_OF_DATE;
}
Expand Down Expand Up @@ -1553,6 +1600,7 @@ ERL_NIF_TERM bitcask_nifs_keydir_itr_next(ErlNifEnv* env, int argc, const ERL_NI
{
if (kh_exist(keydir->entries, handle->iterator))
{
DEBUG2("LINE %d itr_next\r\n", __LINE__);
bitcask_keydir_entry* entry = kh_key(keydir->entries, handle->iterator);
ErlNifBinary key;
bitcask_keydir_entry_proxy proxy;
Expand Down Expand Up @@ -1627,6 +1675,7 @@ ERL_NIF_TERM bitcask_nifs_keydir_itr_release(ErlNifEnv* env, int argc, const ERL
// If last iterator closing, unfreeze keydir and merge pending entries.
if (handle->keydir->keyfolders == 0 && handle->keydir->pending != NULL)
{
DEBUG2("LINE %d itr_release\r\n", __LINE__);
merge_pending_entries(env, handle->keydir);
handle->keydir->iter_generation++;
}
Expand Down Expand Up @@ -1716,12 +1765,22 @@ ERL_NIF_TERM bitcask_nifs_keydir_release(ErlNifEnv* env, int argc, const ERL_NIF
ERL_NIF_TERM bitcask_nifs_increment_file_id(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[])
{
bitcask_keydir_handle* handle;
uint32_t conditional_file_id = 0;

if (enif_get_resource(env, argv[0], bitcask_keydir_RESOURCE, (void**)&handle))
{

if (argv[1] != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really valid to test that a ERL_NIF_TERM is non-zero?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine that what you really want to do here is check argc > 1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a soon-to-be-poooshed commit.

enif_get_uint(env, argv[1], &(conditional_file_id));
}
LOCK(handle->keydir);
(handle->keydir->biggest_file_id)++;
if (conditional_file_id == 0) {
(handle->keydir->biggest_file_id)++;
} else {
if (conditional_file_id > handle->keydir->biggest_file_id) {
handle->keydir->biggest_file_id = conditional_file_id;
}
}
uint32_t id = handle->keydir->biggest_file_id;
UNLOCK(handle->keydir);
return enif_make_tuple2(env, ATOM_OK, enif_make_uint(env, id));
Expand Down Expand Up @@ -2348,6 +2407,7 @@ static void merge_pending_entries(ErlNifEnv* env, bitcask_keydir* keydir)

// Free all resources for keydir folding
kh_destroy(entries, keydir->pending);
DEBUG2("LINE %d keydir->pending = NULL\r\n", __LINE__);
keydir->pending = NULL;

keydir->pending_updated = 0;
Expand Down
13 changes: 10 additions & 3 deletions include/bitcask.hrl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@
fd, % File handle
hintfd, % File handle for hints
hintcrc=0,% CRC-32 of current hint
ofs }). % Current offset for writing
ofs, % Current offset for writing
l_ofs=0, % Last offset written to data file
l_hbytes=0,% Last # bytes written to hint file
l_hintcrc=0}). % CRC-32 of current hint prior to last write

-record(file_status, { filename,
fragmented,
Expand All @@ -26,8 +29,12 @@
-define(FMT(Str, Args), lists:flatten(io_lib:format(Str, Args))).

-define(TOMBSTONE, <<"bitcask_tombstone">>).
-define(TOMBSTONE2_STR, "bitcask_tombstone2").
-define(TOMBSTONE2,<<?TOMBSTONE2_STR>>).

-define(OFFSETFIELD, 64).
-define(OFFSETFIELD_v1, 64).
-define(TOMBSTONEFIELD_V2, 1).
-define(OFFSETFIELD_V2, 63).
-define(TSTAMPFIELD, 32).
-define(KEYSIZEFIELD, 16).
-define(TOTALSIZEFIELD, 32).
Expand All @@ -36,7 +43,7 @@
-define(HEADER_SIZE, 14). % 4 + 4 + 2 + 4 bytes
-define(MAXKEYSIZE, 2#1111111111111111).
-define(MAXVALSIZE, 2#11111111111111111111111111111111).
-define(MAXOFFSET, 16#ffffffffffffffff). % max 64-bit unsigned
-define(MAXOFFSET_V2, 16#7fffffffffffffff). % max 63-bit unsigned

%% for hintfile validation
-define(CHUNK_SIZE, 65535).
Expand Down
4 changes: 3 additions & 1 deletion rebar.config.script
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ case PulseBuild of
[ {bitcask_nifs, keydir_new, 0}
, {bitcask_nifs, keydir_new, 1}
, {bitcask_nifs, keydir_mark_ready, 1}
, {bitcask_nifs, keydir_put_int, 9}
, {bitcask_nifs, keydir_put_int, 10}
, {bitcask_nifs, keydir_get_int, 4}
, {bitcask_nifs, keydir_remove, 3}
, {bitcask_nifs, keydir_remove_int, 6}
Expand All @@ -25,6 +25,7 @@ case PulseBuild of
, {bitcask_nifs, keydir_trim_fstats, 2}

, {bitcask_nifs, increment_file_id, 1}
, {bitcask_nifs, increment_file_id, 2}

, {bitcask_nifs, lock_acquire_int, 2}
, {bitcask_nifs, lock_release_int, 1}
Expand All @@ -41,6 +42,7 @@ case PulseBuild of
, {bitcask_nifs, file_position_int, 2}
, {bitcask_nifs, file_seekbof_int, 1}

, {bitcask_file, '_', '_'}
, {bitcask_time, tstamp, 0}

, {prim_file, '_', '_'}
Expand Down
Loading