Fix the invalid heed usage

[Kerollmops]

We were incorrectly using LMDB by keeping references into the database while modifying it, it was mostly due to heed not specifying anything about this behavior (blame me). I made sure in this PR that we always keep and owned version of the references we were previously using.

This PR partially fixes #261, we need to bump heed to v0.12 when it is available as it marks the del/put_current, and append methods as unsafe and explains why.

I would like to thank the whole Rust community for their help, especially:

• @Pointerbender 🎸 who tracked down the exact point where the memory write was altered,
• @Diggsey 💯 who found, in the LMDB documentation, the sentence that we forgot about,
• and @RalfJung 🥇 who track and kill undefined behavior in Rust programs in general.

[irevoire]

Tell me if I'm wrong, but we should totally fix these lines too, no?
https://github.com/meilisearch/milli/blob/main/milli/src/update/index_documents/mod.rs#L157
https://github.com/meilisearch/milli/blob/main/milli/src/update/index_documents/mod.rs#L219

[Kerollmops]

The k variable comes from the grenad iterator, we compare it with the key variable which, this one, comes from the LMDB database. So, no need to change anything here, am I wrong?

[RalfJung]

I obviously have no right to impose anything here, but I'd just suggest that when you make subtle changes like this, you leave a comment in the source code that at least briefly explains that this was a deliberate choice, and possibly links to an issue with more details. That reduces the risk of someone, in 2 years, going "ah I can eliminate some copies here" and re-introducing the bug. ;)

[irevoire]

Yes you're right, thank you!

And thanks again everyone 😄

[Kerollmops]

Thank you @RalfJung,

I am changing the heed functions to be unsafe, make a PR to this repo using the new version, and will add little comments above the functions in this PR. It will be just above an unsafe block, I hope this comment will be visible enough in 2 years 😄

[Diggsey]

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

[Kerollmops]

Thank you @Diggsey, I just opened an issue about your proposal, not sure if it will work but will try it out. I didn't publish the new v0.12 version of heed on crates.io so I can change anything.

Anyway, we will merge this PR for now as it fixes the bug and will use the potentially safe new version of heed if we update it!

[Diggsey]

@Kerollmops just a heads up that without implementing my proposal, you might have to worry about race conditions, since one thread might be reading a key from the DB while another thread tries to write to the DB. (I'm not sure, maybe everything is already forced to be single-threaded)

[Kerollmops]

@Diggsey, Yeah LMDB already ensures that there can be only one mutable transaction at a time. Thank you anyway!

[MarinPostma]

I think that have to move around a ref mut to the db is too much constraint, since we won't be able to clone it and use references to it anymore. This is redundant to lmdb's locking mechanism.

As soon as we get GATs, which so not be too long now, we should be able to create a Current type which represent a view on the current iterator entry, with a consuming self, and accessors for the value and the data, whose lifetime are bound to the lifetime of the Current object.