From e03584974f06c9c7775a05eedca07b71aa4fcde5 Mon Sep 17 00:00:00 2001 From: Axel Viala Date: Wed, 5 Jul 2023 15:14:39 +0200 Subject: [PATCH] Reintroduce RoTxn::commit: fix database opening read-only database. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opening of databases with Env::open_database() without previous Env in memory with a RoTxn lead to the following error: `Io(Os { code: 22, kind: InvalidInput, message: "Invalid argument" })` It's due to the fact that RoTxn are not commited so the env->me_numdbs cannot be updated and will be set to CORE_DB so 2. It can trigger from multi-process setup, read-only opening, closing write access, and re-opening from the same process. Thanks to @kerollmops for the typo and review. Co-authored-by: Clément Renault --- heed/src/env.rs | 104 ++++++++++++++++++++++++++++++++++++++++++++++++ heed/src/txn.rs | 15 +++++++ 2 files changed, 119 insertions(+) diff --git a/heed/src/env.rs b/heed/src/env.rs index cf83d56e..48cd2992 100644 --- a/heed/src/env.rs +++ b/heed/src/env.rs @@ -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( &self, rtxn: &RoTxn, @@ -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}; @@ -970,4 +980,98 @@ mod tests { assert_eq!(10 * 4096, env.info().map_size); } + + /// Non-regression test for + /// + /// + /// 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::(&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::(&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::(&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(); + } + } } diff --git a/heed/src/txn.rs b/heed/src/txn.rs index 0f7f3bb6..deebd976 100644 --- a/heed/src/txn.rs +++ b/heed/src/txn.rs @@ -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<'_> {