wrap lmdb-rs transactions in rkv abstraction

[mykmelez]

@ncloudioj @rrichardson This branch re-wraps LMDB transactions in a Reader/Writer abstraction to fix #115.

The names aren't important, and I'm happy to use something else (even the same names as LMDB). I just want to achieve consistency in the return values of rkv methods, so they don't return sometimes Result<_, StoreError> and other times Result<_, lmdb::Error>.

In the process, I also added testing of MultiStore.get_first() and MultiStore.delete_all() to confirm that they work as expected.

Let me know what you think.

[ncloudioj]

Shall we make this function as crate(pub)? Looks like RoCursor is only internally used by the rkv iterators. If this makes sense, let's remove the pub use lmdb::RoCursor in the lib.rs.

[mykmelez]

I haven't found a way to do this, as it seems like the visibility of the function depends on the visibility of the trait, and it isn't possible to hide the trait, since public methods like SingleStore.get() have parameters that are bound to the trait. Do you know of any way around that?

[ncloudioj]

Ugh, right, it's a trait. Perhaps split this trait into two, and make the cursor one visible at the crate level?

My concern is that exposing RoCursor to the consumer is not very useful in rkv, and it might lead to lmdb:errors upon misuses.

[ncloudioj]

I think we can polish this in a followup patch once we come up with the better solution.

[mykmelez]

I don't think we can hide a trait that only declares an open_ro_cursor() function either, as we would still need to specify that trait in the public interfaces of SingleStore.iter_start(), SingleStore.iter_from(), and MultiStore.get() in order to call open_ro_cursor() on the Reader/Writer given to those functions.

[ncloudioj]

Yea, looks like hiding traits alone is just a deadend, perhpas apply the good old delegation trick to those iter_* functions? Need to stop myself from concerning this prematurely :)

[mykmelez]

Hehe, I filed #117 so we don't forget about it! 😉

[ncloudioj]

As you mentioned about the naming, I think Reader&Writer are reasonable names for read_only/write transactions. Though I find the Read trait a bit confusing here. At the first glance, I thought this is a reader specific trait. I believe that we wanted to express the fact that both read_only and write transactions can read the store.

Would it be better to name this trait as Readable, despite that it's not the preferred naming convention for traits in Rust?

[mykmelez]

Yes, it's a good point, the trait name is too confusing. I've renamed it to Readable in 1c7a144.

[mykmelez]

For what it's worth, it isn't clear to me what the preferred naming convention is for traits in Rust. milesmcc/ieql#2 suggests that it's "imperatives over modified words," in which case Read would be preferred to Readable.

But rust-lang/api-guidelines#28 suggests that the matter is more complicated, and it isn't clear to me that the answer is as simple as to use imperatives. In any case, it's plenty reasonable for us to use Readable, especially for a trait that consumers usually won't interact with directly.

[ncloudioj]

r=nanj Abstraction of raw LMDB transactions is indeed necessary for rkv!

Just two minor suggestions for your consideration. Also, looks like Clippy has found some nits in the code.

[mykmelez]

Also, looks like Clippy has found some nits in the code.

D'oh! I ran cargo clippy but not cargo clippy --all-targets --all-features -- -D warnings, which also checks the targets in examples/.

I fixed the problem in 18599e4. Interestingly, Clippy's suggestion was suboptimal, as it suggested:

error: identical conversion
  --> examples/iterator.rs:74:33
   |
74 |     writer.commit().map_err(|e| e.into())
   |                                 ^^^^^^^^ help: consider removing `.into()`: `e`
   |
   = note: `-D clippy::identity-conversion` implied by `-D warnings`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#identity_conversion

When in fact we can do better than removing the .into() and remove the entire .map_err() call.