https://bugs.openldap.org/show_bug.cgi?id=10395
kero <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #1098|0 |1 is obsolete| | --- Comment #11 from kero <[email protected]> --- Created attachment 1129 --> https://bugs.openldap.org/attachment.cgi?id=1129&action=edit Allow multiple nested read transactions from a write transaction review from comment #10 Hello Howard, Thank you very much for the time you spent reviewing my patch. I took a bit of time to fix the part you highlighted. > I don't understand the "also named child txns" part of this comment. I removed this part for clarity. > All of the platform-specific thread/mutex stuff should be #defined as macros > in the header block, e.g. around mdb.c:320. There should be no thread/mutex > ifdefs anywhere else in the code. Done. However, should I use LOCK_MUTEX rather than pthread_mutex_lock? The main difference seems to be that LOCK_MUTEX sets rc = result. > These comments are no longer unambiguous. Fixed, thanks. I took your comment and added the part about MDB_WRITEMAP. > These allocations should be skipped for RDONLY txns, especially since there > may be arbitrarily many of them. Done. I made sure not to allocate the dirty_list and mt_free_pgs in case this is a nested read txn. I also obviously made sure that we are not freeing them in mdb_txn_end. However, I am wondering if I need to do a conditional free or if I can free a NULL pointer; using mdb_midl_free on a NULL pointer is safe. > The else clause should be unnecessary; if your counting has been correct then > it should already be zero. If it's not correct then you have a bug. Agreed, thanks. Fixed. > pgstate should only exist in a writable child txn. In a read txn it should be > unused, what is this value then? You are right. If last_child is zero, we will enter this condition and set the me_pgstate again. It seems that the mnt_pgstate of txn is set to the env one in mdb.c:3318. However, I preferred setting it to NULL when the nested rtxn is created and only set it back to the env when we are destroying a nested write txn. > This appears to only be correct if you used MDB_NOTLS. I suppose you are talking about the relation to the following part of mdb_txn_renew0, which is called on a non-nested read transaction. MDB_reader *r = (env->me_flags & MDB_NOTLS) ? txn->mt_u.reader : pthread_getspecific(env->me_txkey); After examining the code, our nested read transaction has a parent and is RDONLY. We don't enter the condition, leaving a dirty mt_u.reader, mt_numdbs, and mt_flags not set with MDB_TXN_FINISHED, which isn't ideal. Am I right that the issue is about setting mt_u.reader to NULL? I'm unsure about allowing nested read transactions from non-NOTLS environments since they can't be moved between threads. It's already possible to read using a write transaction in a single thread. I'd prefer to keep the current code if it works for normal read transactions, and allow nested read transactions only from NOTLS environments, returning EINVAL or MDB_BAD_TXN otherwise. What do you think? Note that I'll have to update the no-longer-ambiguous comment to specify this restriction on the transactions part: environment must be opened with MDB_NOTLS to allow for nested read transactions. Have a nice day, kero -- You are receiving this mail because: You are on the CC list for the issue.
