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

Read-only database opening does not work on a read-only context #176

Merged
merged 1 commit into from
Jul 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
104 changes: 104 additions & 0 deletions heed/src/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,15 @@ impl Env {
/// LMDB have an important restriction on the unnamed database when named ones are opened,
/// the names of the named databases are stored as keys in the unnamed one and are immutable,
/// these keys can only be read and not written.
///
/// ## Lmdb read-only access of existing database
///
/// In the case of accessing a database in a read-only manner from another process
/// where you wrote you might need to call manually `RoTxn::commit` to get metadata
/// and the databases handles opened and shared with the global [Env] handle.
///
/// If not done you might raise `Io(Os { code: 22, kind: InvalidInput, message: "Invalid argument" })`
/// known as `EINVAL`.
pub fn open_database<KC, DC>(
&self,
rtxn: &RoTxn,
Expand Down Expand Up @@ -791,6 +800,7 @@ impl fmt::Debug for EnvClosingEvent {

#[cfg(test)]
mod tests {
use std::io::ErrorKind;
use std::time::Duration;
use std::{fs, thread};

Expand Down Expand Up @@ -970,4 +980,98 @@ mod tests {

assert_eq!(10 * 4096, env.info().map_size);
}

/// Non-regression test for
/// <https://github.com/meilisearch/heed/issues/183>
///
/// We should be able to open database Read-Only Env with
/// no prior Read-Write Env opening. And query data.
#[test]
fn open_read_only_without_no_env_opened_before() {
let expected_data0 = "Data Expected db0";
let dir = tempfile::tempdir().unwrap();

{
// We really need this env to be dropped before the read-only access.
let env = EnvOpenOptions::new()
.map_size(16 * 1024 * 1024 * 1024) // 10MB
.max_dbs(32)
.open(dir.path())
.unwrap();
let mut wtxn = env.write_txn().unwrap();
let database0 = env.create_database::<Str, Str>(&mut wtxn, Some("shared0")).unwrap();

wtxn.commit().unwrap();
let mut wtxn = env.write_txn().unwrap();
database0.put(&mut wtxn, "shared0", expected_data0).unwrap();
wtxn.commit().unwrap();
// We also really need that no other env reside in memory in other thread doing tests.
env.prepare_for_closing().wait();
}

{
// Open now we do a read-only opening
let env = EnvOpenOptions::new()
.map_size(16 * 1024 * 1024 * 1024) // 10MB
.max_dbs(32)
.open(dir.path())
.unwrap();
let database0 = {
let rtxn = env.read_txn().unwrap();
let database0 =
env.open_database::<Str, Str>(&rtxn, Some("shared0")).unwrap().unwrap();
// This commit is mandatory if not committed you might get
// Io(Os { code: 22, kind: InvalidInput, message: "Invalid argument" })
rtxn.commit().unwrap();
database0
};

{
// If we didn't committed the opening it might fail with EINVAL.
let rtxn = env.read_txn().unwrap();
let value = database0.get(&rtxn, "shared0").unwrap().unwrap();
assert_eq!(value, expected_data0);
}

env.prepare_for_closing().wait();
}

// To avoid reintroducing the bug let's try to open again but without the commit
{
// Open now we do a read-only opening
let env = EnvOpenOptions::new()
.map_size(16 * 1024 * 1024 * 1024) // 10MB
.max_dbs(32)
.open(dir.path())
.unwrap();
let database0 = {
let rtxn = env.read_txn().unwrap();
let database0 =
env.open_database::<Str, Str>(&rtxn, Some("shared0")).unwrap().unwrap();
// No commit it's important, dropping explicitly
drop(rtxn);
database0
};

{
// We didn't committed the opening we will get EINVAL.
let rtxn = env.read_txn().unwrap();
// The dbg!() is intentional in case of a change in rust-std or in lmdb related
// to the windows error.
let err = dbg!(database0.get(&rtxn, "shared0"));

// The error kind is still ErrorKind Uncategorized on windows.
// Behind it's a ERROR_BAD_COMMAND code 22 like EINVAL.
if cfg!(windows) {
assert!(err.is_err());
} else {
assert!(
matches!(err, Err(Error::Io(ref e)) if e.kind() == ErrorKind::InvalidInput)
);
}
}

env.prepare_for_closing().wait();
}
}
}
15 changes: 15 additions & 0 deletions heed/src/txn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,21 @@ impl<'e> RoTxn<'e> {
pub(crate) fn env_mut_ptr(&self) -> *mut ffi::MDB_env {
self.env.env_mut_ptr()
}

/// Commit a read transaction.
///
/// Synchronizing some [Env] metadata with the global handle.
///
/// ## Lmdb
///
/// It's mandatory in a multi-process setup to call [RoTxn::commit] upon read-only database opening.
/// After the transaction opening, the database is `drop`ed. The next transaction might return
/// `Io(Os { code: 22, kind: InvalidInput, message: "Invalid argument" })` known as `EINVAL`.
pub fn commit(mut self) -> Result<()> {
let result = unsafe { mdb_result(ffi::mdb_txn_commit(self.txn)) };
self.txn = ptr::null_mut();
result.map_err(Into::into)
}
}

impl Drop for RoTxn<'_> {
Expand Down