From 770d52d9a415b2238f9cf319dc9936550ade098e Mon Sep 17 00:00:00 2001
From: Axel Viala <axel.viala@gandi.net>
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 <renault.cle@gmail.com>
---
 heed/src/env.rs | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
 heed/src/txn.rs | 16 +++++++++
 2 files changed, 102 insertions(+)

diff --git a/heed/src/env.rs b/heed/src/env.rs
index cf83d56e..0ddc9901 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 a database in a read-only manner from an other process that the one
+    /// where you wrote you might need to call manually `RoTxn::commit`,
+    /// to get metadata and databases handles opened shared with the global [Env] handle.
+    ///
+    /// If not done you might raise `Io(Os { code: 22, kind: InvalidInput, message: "Invalid argument" })`
+    /// known as `EINVAL` from LMDB on access functions.
     pub fn open_database<KC, DC>(
         &self,
         rtxn: &RoTxn,
@@ -970,4 +979,81 @@ mod tests {
 
         assert_eq!(10 * 4096, env.info().map_size);
     }
+
+    #[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 commited 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 commited the opening we get EINVAL.
+                let rtxn = env.read_txn().unwrap();
+                assert!(database0.get(&rtxn, "shared0").is_err());
+            }
+
+            env.prepare_for_closing().wait();
+        }
+    }
 }
diff --git a/heed/src/txn.rs b/heed/src/txn.rs
index 0f7f3bb6..49e36177 100644
--- a/heed/src/txn.rs
+++ b/heed/src/txn.rs
@@ -48,6 +48,22 @@ 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 readonly database opening.
+    /// After the transaction opening the a database is `drop`ed. the next transaction might return
+    /// `Io(Os { code: 22, kind: InvalidInput, message: "Invalid argument" })`
+    /// known as `EINVAL` on access functions.
+    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<'_> {