From 2fbb728f496f3f6aa27afff534b562333db39f82 Mon Sep 17 00:00:00 2001
From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Tue, 15 Jul 2025 12:32:18 +0200
Subject: [PATCH] lmdb-safe: Remove the read-only transactions counter

LMDB requires:
- a transaction and its cursors must only be used by a single thread
- a thread may only have a single transaction at a time
- but, if `MDB_NOTLS` is in use, this does not apply to read-only transactions.

The first and second points are the reasons why we are keeping track
of transactions, but because of the third point we cannot be strict
with regard to read-only transactions, unless we keep track of whether
the `MDB_NOTLS` flag is in use.

I recently noticed that I made a mistake in c340aa91bf37d8105d2b2390eecbadfca88c1d27, and `MDBEnv::getROTX()` now returns the number of read-write
transactions instead of the number of read-only transactions. It
also noticed that `MDBEnv::getROTX()` is actually not used, and we
only enforce that a thread that already has a read-write transaction
cannot open another transaction.
Since `MDBEnv::getROTX()` is not used, it makes not much sense to
bear the cost of tracking read-only transactions so this commit removes
the read-only transactions counter.

Another option would be to make lmdb-safe stricter: if we kept track
of whether the `MDB_NOTLS` flag is in use, we could enforce:
- only one transaction when `MDB_NOTLS` is NOT in use. We then
 need to check both counters when opening a new transaction.
- only one read-write transaction but several read-only transaction
 allowed when `MDB_NOTLS` is in use.

Signed-off-by: Remi Gacogne <remi.gacogne@powerdns.com>
---
 ext/lmdb-safe/lmdb-safe.cc | 45 --------------------------------------
 ext/lmdb-safe/lmdb-safe.hh |  4 ----
 2 files changed, 49 deletions(-)

diff --git a/ext/lmdb-safe/lmdb-safe.cc b/ext/lmdb-safe/lmdb-safe.cc
index c73a9d34fe..24556b3033 100644
--- a/ext/lmdb-safe/lmdb-safe.cc
+++ b/ext/lmdb-safe/lmdb-safe.cc
@@ -133,35 +133,6 @@ Various other options may also need to be set before opening the handle, e.g. md
   }
 }
 
-void MDBEnv::incROTX()
-{
-  auto threadId = std::this_thread::get_id();
-  {
-    std::shared_lock<std::shared_mutex> lock(d_countmutex);
-    if (auto transactionsIt = d_ROtransactionsOut.find(threadId); transactionsIt != d_ROtransactionsOut.end()) {
-      ++transactionsIt->second;
-      return;
-    }
-  }
-
-  {
-    std::unique_lock<std::shared_mutex> lock(d_countmutex);
-    auto [transactionsIt, inserted] = d_ROtransactionsOut.emplace(threadId, 1);
-    if (!inserted) {
-      ++transactionsIt->second;
-    }
-  }
-}
-
-void MDBEnv::decROTX()
-{
-  auto threadId = std::this_thread::get_id();
-  {
-    std::shared_lock<std::shared_mutex> lock(d_countmutex);
-    d_ROtransactionsOut.at(threadId)--;
-  }
-}
-
 void MDBEnv::incRWTX()
 {
   auto threadId = std::this_thread::get_id();
@@ -203,18 +174,6 @@ int MDBEnv::getRWTX()
   return 0;
 }
 
-int MDBEnv::getROTX()
-{
-  auto threadId = std::this_thread::get_id();
-  {
-    std::shared_lock<std::shared_mutex> lock(d_countmutex);
-    if (auto transactionsIt = d_RWtransactionsOut.find(threadId); transactionsIt != d_RWtransactionsOut.end()) {
-      return transactionsIt->second.load();
-    }
-  }
-  return 0;
-}
-
 
 std::shared_ptr<MDBEnv> getMDBEnv(const char* fname, int flags, int mode, uint64_t mapsizeMB)
 {
@@ -382,8 +341,6 @@ MDB_txn *MDBROTransactionImpl::openROTransaction(MDBEnv *env, MDB_txn *parent, i
     throw std::runtime_error("Unable to start RO transaction: "+MDBError(retCode));
   }
 
-  env->incROTX();
-
   return result;
 }
 
@@ -414,7 +371,6 @@ void MDBROTransactionImpl::abort()
   closeROCursors();
   // if d_txn is non-nullptr here, either the transaction object was invalidated earlier (e.g. by moving from it), or it is an RW transaction which has already cleaned up the d_txn pointer (with an abort).
   if (d_txn) {
-    d_parent->decROTX();
     mdb_txn_abort(d_txn); // this appears to work better than abort for r/o database opening
     d_txn = nullptr;
   }
@@ -425,7 +381,6 @@ void MDBROTransactionImpl::commit()
   closeROCursors();
   // if d_txn is non-nullptr here, either the transaction object was invalidated earlier (e.g. by moving from it), or it is an RW transaction which has already cleaned up the d_txn pointer (with an abort).
   if (d_txn) {
-    d_parent->decROTX();
     mdb_txn_commit(d_txn); // this appears to work better than abort for r/o database opening
     d_txn = nullptr;
   }
diff --git a/ext/lmdb-safe/lmdb-safe.hh b/ext/lmdb-safe/lmdb-safe.hh
index 049311a502..b653d8fedf 100644
--- a/ext/lmdb-safe/lmdb-safe.hh
+++ b/ext/lmdb-safe/lmdb-safe.hh
@@ -110,14 +110,10 @@ public:
   int getRWTX();
   void incRWTX();
   void decRWTX();
-  int getROTX();
-  void incROTX();
-  void decROTX();
 private:
   std::mutex d_openmut;
   std::shared_mutex d_countmutex;
   std::unordered_map<std::thread::id, std::atomic<int>> d_RWtransactionsOut;
-  std::unordered_map<std::thread::id, std::atomic<int>> d_ROtransactionsOut;
 };
 
 std::shared_ptr<MDBEnv> getMDBEnv(const char* fname, int flags, int mode, uint64_t mapsizeMB);
-- 
2.47.2