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.

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

Change the lifetimes of the Iter types #111

Open
Kerollmops opened this issue Jun 28, 2021 · 2 comments
Open

Change the lifetimes of the Iter types #111

Kerollmops opened this issue Jun 28, 2021 · 2 comments
Labels
enhancement New feature or request help wanted Extra attention is needed
Milestone

Comments

@Kerollmops
Copy link
Member

Kerollmops commented Jun 28, 2021

A great idea from @Diggsey to fix #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 Kerollmops added enhancement New feature or request help wanted Extra attention is needed labels Jun 28, 2021
@Kerollmops
Copy link
Member Author

Kerollmops commented Jul 2, 2021

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
Copy link

Diggsey commented Jul 2, 2021

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

  • Database
    • Transaction (Up to one per thread per database).

      Rust currently has no way to enforce a one-per-thread constraint at compile-time, so this will need to be checked dynamically. This should work in the same way that my scoped_tls_hkt crate handles mutable thread-locals.

      Given that you'll need dynamic checking here anyway, there's no need for the transactions to borrow the database. Instead, they can have shared access to it via Arc or Weak (unless you want to avoid the allocation that would entail, in which an immutable borrow also works at the cost of more lifetime annotations).

      Transactions should have two parts which can be borrowed separately:

      • View

        This represents the transaction's view of the database.

        • Value

          Should immutably borrow its "view".

        • Operation

          All operations on the transaction should mutably or immutably borrow the transaction's "view", depending on whether the operation is read-only or not. Typically these would be methods on the "view" taking &self or &mut self.

      • Cursors

        This represents the set of cursors which are active for this transaction. It will have methods to create new cursors.

        • Cursor

          Should immutably borrow from the transaction's "cursors" field. The only methods on the cursor should be for modifying the cursor itself. Anything that needs to return a value from the cursor, or perform an update via the cursor should actually be a method on the transaction's "view" that takes a cursor as an argument. (See "Operation")

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.

@Kerollmops Kerollmops added this to the v0.20.0 milestone Jan 11, 2023
@Kerollmops Kerollmops modified the milestones: v0.20.0, v0.21.0 Feb 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants