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>

diff --git a/ext/lmdb-safe/lmdb-safe.cc b/ext/lmdb-safe/lmdb-safe.cc
index c73a9d34fe95d43c3440aad765c070649f7399d6..24556b3033f90ab509869ad60da008b4a7efc3d9 100644 (file)
--- 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 049311a502c1e911ba40e730cf71a0f9ab57f832..b653d8fedfebb49901b4ff9f95b03f3b702d7063 100644 (file)
--- 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);