meilisearch / heed

A fully typed LMDB wrapper with minimum overhead 🐦
https://docs.rs/heed
MIT License
572 stars 53 forks source link

Change the lifetimes of the `Iter` types #111

Open Kerollmops opened 3 years ago

Kerollmops commented 3 years ago

A great idea from @Diggsey to fix https://github.com/Kerollmops/heed/pull/108, an issue where we were allowing the user to keep reference from inside the database while modifying it at the same time.

FWIW, you don't need to make the heed functions unsafe - you just need to make sure that all references returned from the database has a lifetime tied to the database, and that all database-modifying functions require a mutable borrow of the database in order to operate.

It could be a great solution to indeed have differences between the immutable iterators i.e. RoIter, RoPrefixIter, and the mutable ones i.e. RwIter, RwPrefixIter. Where the only differences would be with the lifetimes of the key and values returned, the read-only version would simply return entries with a lifetime of the initial transaction, while the read-write one would return entries with a lifetime that comes from the database itself and takes a new parameter a mutable reference of the database, this way we make sure that we can't keep values while also modifying the database.

// for the read-only iterator, nothing change.
fn Database::iter<'txn, T>(&self, txn: &'txn RoTxn<T>) -> Result<RoIter<'txn, KC, DC>>;

// but for the read-write iterator, we introduce a new lifetime.
fn Database::iter_mut<'txn, 'db, T>(&'db self, txn: &'txn mut RwTxn<T>) -> Result<RwIter<'txn, 'db, KC, DC>>;

// and we also change the del/put_current, and append methods,
// we now ask for a mutable reference of the database.
fn RwIter::put_current(&mut self, &mut db, key: &KC::EItem, data: &DC::EItem) -> Result<bool>;

// this is because the <RwIter as Iterator>::next method now
// returns entries that are tied to the database itself.
impl<'txn, 'db, KC, DC> Iterator for RwIter<'txn, 'db, KC, DC>
where
    KC: BytesDecode<'db>,
    DC: BytesDecode<'db>,
{
    type Item = Result<(KC::DItem, DC::DItem)>;
    fn next(&mut self) -> Option<Self::Item>;
}

I am not sure it will work as the initial Database::iter_mut method asks for a &Database and the RwIter::put_current can only be used with the &mut Database parameter, I am not sure that Rust will accept that. Will try when I got time.

Kerollmops commented 3 years ago

Hey @Diggsey,

As I told you, I am not sure it is possible to implement this API, at least in this exact way. The problem with it is that it asks for a mutable reference of the database when the cursor itself contains an immutable reference of it already.

What do you think ? Were you thinking about another API ?

Diggsey commented 3 years ago

I read through the LMDB docs, and their model seems to look like this:

This layout will avoid the need for all dynamic checks except when you first open a transaction, whilst also allowing the various Rust types to be thin wrappers around the underlying LMDB handles.