https://bugs.openldap.org/show_bug.cgi?id=10395
--- Comment #14 from Howard Chu <[email protected]> --- (In reply to kero from comment #11) > Created attachment 1129 [details] > 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. LOCK_MUTEX is used for the process-shared mutexes, which may not always be pthread mutexes. You should just use pthread_mutex_lock here since you don't need an interprocess mutex. > > > 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. You can free a NULL pointer, that's standard C idiom. No need to conditionalize that. > > > 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? It occurs to me that the reader table is irrelevant in this case. The reader table's purpose is to prevent a write txn from reusing pages that read txns may still need. But since in this case, the write txn is the parent and it cannot do anything while it has child read txns, there's nothing to worry about there. Which means these read txns can be used freely, independent of the readers table and any maxreaders setting. But it could get confusing if these read txns are kept around (using txn_reset/txn_renew) and used outside of a parent write txn, because then they would need their slot in the reader table (assuming a default environment, without NOTLS). > > 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. As above, it's probably fine to use nested read txns regardless. But unless specially handled otherwise, readtxns created as a child txn probably can't be reused later as independent readtxns. Also, the documentation for mdb_txn_begin in lmdb.h should be updated. > Have a nice day, > kero -- You are receiving this mail because: You are on the CC list for the issue.
