LMDB semantics dictate that a cursor must be valid for calls to mdb_cursor_get. In case of the rust wrappers, this means that the lmdb::RoCursor/lmdb::RwCursor must be valid the entire lifetime of an lmdb::Iter. In other words, cursors must not be dropped while an iterator built from it is alive. Unfortunately, the LMDB crate API does not express this through the type system at all, so we must enforce it somehow.
Before this PR, this worked because dead code wasn't eliminated. The Iter types for single/multi database wrappers owned a C generic type implementing BackendRoCursor, even though they didn't have to.
In case of a safe mode backend, this isn't required, and eliminating the dead code doesn't result in any SIGSEGV. However, in case of an LMDB backend, doing so results in signal: 11, SIGSEGV: invalid memory reference when running tests, even though everything compiles successfully and there's no unsafe code used.
These LMDB specifics shouldn't be expressed at this level, since they impose restrictions on other backends (namely safe mode). Ideally, they should be expressed in the LMDB crate (which, as it stands, allows a plethora of unsafe ways of using its API, without them being marked as such through the type system). However, if we don't want to fix the LMDB crate actually expose a safer API, we can do this at the backend level in the RKV crate.
With this PR:
Since cursors always open just one iterator, this is expressed through the type system now, which simplifies things a bit.
Safe mode is slightly better optimized memory wise, since a "cursor" is a misnomer in this particular case, and these types can be now dropped safely.
victorporof
commented
4 years ago
Signed-off-by: Victor Porof victor.porof@gmail.com
LMDB semantics dictate that a cursor must be valid for calls to
mdb_cursor_get. In case of the rust wrappers, this means that thelmdb::RoCursor/lmdb::RwCursormust be valid the entire lifetime of anlmdb::Iter. In other words, cursors must not be dropped while an iterator built from it is alive. Unfortunately, the LMDB crate API does not express this through the type system at all, so we must enforce it somehow.Before this PR, this worked because dead code wasn't eliminated. The
Itertypes for single/multi database wrappers owned aCgeneric type implementingBackendRoCursor, even though they didn't have to.In case of a safe mode backend, this isn't required, and eliminating the dead code doesn't result in any SIGSEGV. However, in case of an LMDB backend, doing so results in
signal: 11, SIGSEGV: invalid memory referencewhen running tests, even though everything compiles successfully and there's no unsafe code used.These LMDB specifics shouldn't be expressed at this level, since they impose restrictions on other backends (namely safe mode). Ideally, they should be expressed in the LMDB crate (which, as it stands, allows a plethora of unsafe ways of using its API, without them being marked as such through the type system). However, if we don't want to fix the LMDB crate actually expose a safer API, we can do this at the backend level in the RKV crate.
With this PR: