https://bugs.openldap.org/show_bug.cgi?id=10395
Issue ID: 10395 Summary: Support multiple readers on uncommitted changes Product: LMDB Version: unspecified Hardware: All OS: All Status: UNCONFIRMED Keywords: needs_review Severity: normal Priority: --- Component: liblmdb Assignee: bugs@openldap.org Reporter: renault.cle@gmail.com Target Milestone: ---
Created attachment 1086 --> https://bugs.openldap.org/attachment.cgi?id=1086&action=edit A patch to support multiple readers on uncommitted changes
Hello,
The attached patch is not meant to be merged immediately into LMDB. Still, it demonstrates how I added a helpful feature to the key-value store: reading uncommitted changes from multiple transactions. I am conscious that the patch still requires some work and uses non-C99 features, i.e., atomics are C11, which could be a blocker for it to be merged upstream. I would also be delighted to merge these changes under a flag/define to ensure we don't impact other users with non-C99 stuff.
The main feature we need at Meilisearch is to read uncommitted changes from multiple threads, compute parallel post-processing of different data structures [1], and speed up the search requests. We could have done the post-processing in a following transaction by opening multiple read transactions, but this would mean that the post-processed data structure would not include newly inserted or modified document IDs. Both data structures would be desync.
Regarding the design choice, I decided to follow the same design as the nested write transactions: use the parent argument of the mdb_txn_begin [2], and allow the MDB_RDONLY flag, which was disallowed when the parent argument was non-NULL [3]. I find it clear enough that, by calling the mdb_txn_begin function with these arguments, you can call it multiple times (I need to update the doc) to obtain nested read-only transactions from the parent write transaction.
ret = mdb_txn_begin(env, parent_txn, MDB_RDONLY, new_nested_rtxn);
Note that this early proposal lacks security and error handling. The generated transactions are fake-read-only and actually write transactions that share the underlying parent allocations and data structures. This is unsafe and must be changed or reviewed carefully, but most importantly, we need to add read-only capabilities to these transactions to disallow writes. Using a Rust wrapper on top of LMDB, I wrapped the fake read-only transactions into ReadTxn, which disallows any writes at compile time. However, I haven't checked the conflict database creations or openings.
The main issues I encountered were concurrent free of the main shared data structures when the different threads owning the transactions were dropping the transactions simultaneously. So, I decided to implement the equivalent of an ARC to free resources only when the last nested transaction was freed.
I can share numbers about how this feature improves the post-processing by 4x-9x or from 1200s to 120s [1]. You can look at this PR, which I would be happy to merge once an improved version of this patch lands on LMDB upstream.
I would be very happy if you could guide me a bit on how I could improve this patch to make it mergeable into LMDB. We want to contribute useful features like this to LMDB and not keep a deviant fork. LMDB works great; we are happy about it, and its performance is predictable.
Have a lovely week, kero
[1]: https://github.com/meilisearch/meilisearch/pull/5307 [2]: https://github.com/LMDB/lmdb/blob/14d6629bc8a9fe40d8a6bee1bf71c45afe7576b6/l... [3]: https://github.com/LMDB/lmdb/blob/14d6629bc8a9fe40d8a6bee1bf71c45afe7576b6/l...
https://bugs.openldap.org/show_bug.cgi?id=10395
--- Comment #1 from Howard Chu hyc@openldap.org --- I don't see intentionally breaking ACID as being a good idea. Reading uncommitted changes breaks Atomicity and Isolation at least, and may break Consistency in various situations.
The only thing that ought to be able to see uncommitted changes is the active write transaction. If anything else does, then you don't have a transactional DB engine any more. In ACID operation "newly inserted or modified document IDs" *don't exist* until their transaction commits.
Using multiple reader txns is the correct approach. When the writer commits, you can simply txn_reset() all the readers to get them all on the latest data.
https://bugs.openldap.org/show_bug.cgi?id=10395
--- Comment #2 from kero renault.cle@gmail.com ---
I don't see intentionally breaking ACID as being a good idea. Reading uncommitted changes breaks Atomicity and Isolation at least, and may break Consistency in various situations.
I may have been unclear about the changes I made, or I may have made an error in the code. The nested transactions do the same as the write transaction: they _read their own writes_. It is already supported. The only difference is that you can _read your writes_ in parallel, now. Additionally, the parent write transaction is unusable and only an abort can be performed, and only after the nested transactions are aborted. If it's not the case, I need to implement that as we mentioned in this Mastodon thread [1].
The goal of this feature is not related to parallel writes, but rather to allow parallel reads on uncommitted changes *from within the write transaction*. Nothing can go outside. To write into the write transaction, you have to abort the many nested read-only transactions and use the original parent transaction. You must store data somewhere (RAM, disk) before being able to write to LMDB. I don't want to break ACID, here.
The only thing that ought to be able to see uncommitted changes is the active write transaction. If anything else does, then you don't have a transactional DB engine any more. In ACID operation "newly inserted or modified document IDs" *don't exist* until their transaction commits.
Currently, IIRC, nested transactions can see uncommitted changes as well, as can the outer transaction. This PR proposes to do the same, but from multiple read-only transactions that are hooked to the parent transaction. You can only have one write-active nested transaction, and in the case of this very feature, there is not even the possibility to write while the nested read-only transactions are active.
Using multiple reader txns is the correct approach. When the writer commits, you can simply txn_reset() all the readers to get them all on the latest data.
That's the issue we have with Meilisearch: we would like to be able to read uncommitted changes to avoid desync issues, and in parallel, to do it quickly and scale with the number of CPUs.
[1]: https://fosstodon.org/@kero/113465979956049984
https://bugs.openldap.org/show_bug.cgi?id=10395
--- Comment #3 from kero renault.cle@gmail.com --- Currently, IIRC, nested transactions can see uncommitted changes as well, as can the *nested* transactions [..]
https://bugs.openldap.org/show_bug.cgi?id=10395
Quanah Gibson-Mount quanah@openldap.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Keywords|needs_review | Target Milestone|--- |1.0.0
https://bugs.openldap.org/show_bug.cgi?id=10395
kero renault.cle@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #1086|0 |1 is obsolete| |
--- Comment #4 from kero renault.cle@gmail.com --- Created attachment 1088 --> https://bugs.openldap.org/attachment.cgi?id=1088&action=edit Support nested RDONLY transactions
Hey Howard,
I took more time to work on this patch to introduce the possibility of creating multiple nested RDONLY transactions from a parent one. This was already the case with my previous patch, but now: - The nested transactions are RDONLY and disallow writes. - The parent transaction prevents creating nested transactions, unless they are all RDONLY. - Nested RDONLY transactions now avoid leaking me_pgstate/me_pghead by not allocating or copying them.
However, I still encounter a SIGBUS error when using a cursor on a dbi created by the current parent transaction but not yet committed. It seems that the issue originates from mc->mc_pg and MDB_page.mp2_flags not being properly initialized.
I wrote a reproducer at the end of this very message. Would you have a moment to review and help me identify the source of the issue?
Have a nice end of the week, kero
#include <stdio.h> #include <stdlib.h> #include "lmdb.h"
#define E(expr) CHECK((rc = (expr)) == MDB_SUCCESS, #expr) #define RES(err, expr) ((rc = expr) == (err) || (CHECK(!rc, #expr), 0)) #define CHECK(test, msg) ((test) ? (void)0 : ((void)fprintf(stderr, \ "%s:%d: %s: %s\n", __FILE__, __LINE__, msg, mdb_strerror(rc)), abort()))
int main(int argc,char * argv[]) { int rc; MDB_env *env; MDB_dbi dbi; MDB_val key, data; MDB_txn *txn, *nested; MDB_cursor *cursor;
E(mdb_env_create(&env)); E(mdb_env_set_maxdbs(env, 1)); E(mdb_env_set_maxreaders(env, 1)); E(mdb_env_set_mapsize(env, 10485760)); E(mdb_env_open(env, "./testdb", 0, 0664));
E(mdb_txn_begin(env, NULL, 0, &txn)); E(mdb_dbi_open(txn, "brand-new-db", MDB_CREATE, &dbi));
key.mv_data = "test-key\0"; key.mv_size = sizeof("test-key\0");
data.mv_data = "test-data\0"; data.mv_size = sizeof("test-data\0");
RES(MDB_KEYEXIST, mdb_put(txn, dbi, &key, &data, MDB_NOOVERWRITE));
E(mdb_txn_begin(env, txn, MDB_RDONLY, &nested)); E(mdb_cursor_open(nested, dbi, &cursor));
/* Crashes with a SIGBUS, a wrongly initialized mc->mc_pg */ RES(MDB_NOTFOUND, mdb_cursor_get(cursor, &key, &data, MDB_NEXT));
return 0; }
https://bugs.openldap.org/show_bug.cgi?id=10395
kero renault.cle@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #1088|0 |1 is obsolete| |
--- Comment #5 from kero renault.cle@gmail.com --- Created attachment 1089 --> https://bugs.openldap.org/attachment.cgi?id=1089&action=edit Attachments Support nested RDONLY transactions
Hello again,
I identified the SIGBUS error when reading a freshly created DBI from the parent transaction. The bug was located in the mdb_page_get function, specifically in the condition that specified whether we had to read the dirty list. I propose a new patch that addresses memory leaks and SIGBUS issues.
Have a nice end of the week, kero
https://bugs.openldap.org/show_bug.cgi?id=10395
kero renault.cle@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #1089|0 |1 is obsolete| |
--- Comment #6 from kero renault.cle@gmail.com --- Created attachment 1096 --> https://bugs.openldap.org/attachment.cgi?id=1096&action=edit Last patch to allow multiple nested read transactions from a write transaction
Hey Howard,
I have finalized the last version of this patch. Would you mind taking a look at it, please? We are using this patched version of LMDB in production with large datasets, and it performs exceptionally well. We are capable of multi-threading the reads we perform on uncommitted changes.
If you are not eager to merge this patch, could you please explain why? I am not quite sure I understand: We are not introducing concurrent modifications of the write transaction's content, nor allowing external read transactions to read uncommitted content. Only the program owning the write transaction can perform reads in parallel, which it was already allowed to do, but not in parallel before.
Side note: Would you mind updating the GitHub mirror of LMDB, please? There is a missing commit[1].
[1]: https://github.com/LMDB/lmdb/commit/1db2a61ade6ae087dc175b1f44303548a4aa7926
Have a nice end of the week, Thank you, kero
https://bugs.openldap.org/show_bug.cgi?id=10395
--- Comment #7 from Howard Chu hyc@openldap.org --- We can't use atomics in this code.
Haven't had time to review the rest.
https://bugs.openldap.org/show_bug.cgi?id=10395
kero renault.cle@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #1096|0 |1 is obsolete| |
--- Comment #8 from kero renault.cle@gmail.com --- Created attachment 1098 --> https://bugs.openldap.org/attachment.cgi?id=1098&action=edit Allow multiple nested read transactions from a write transaction using a mutex
Hello Howard, and thank you for the early review,
I found the time to replace the C11 atomics with C99 mutexes. However, I am not entirely sure if this is the correct way to use mutexes in this code, or if we should instead use macros or defines to ensure compatibility across all platforms. Anyway, this code is relatively small, and it would be easy to patch. It compiles and works on all platforms, as confirmed by the numerous CI tests we have on Heed [1] and Meilisearch [2].
Thank you very much, Have a nice day
[1]: https://github.com/meilisearch/heed/pull/307/commits/464a9c33f76e0679115eef7... [2]: https://github.com/meilisearch/meilisearch/pull/6042/checks
https://bugs.openldap.org/show_bug.cgi?id=10395
--- Comment #9 from kero renault.cle@gmail.com --- Hello Howard and Happy New Year,
We recently made use of this new deriving branch of LMDB in Meilisearch to reduce the indexing time of a large customer from 2 hours and 50 minutes to under 7 minutes. You can review the code here [1] if you are interested. It's really search-engine/inverted-index oriented, but still, the use of the `Env::nested_read_txn` method (creating a nested READ_ONLY transaction with a write transaction as a parent) makes it possible to read the current database in parallel.
Did you have any chance to review my LMDB patch? It's working fine in production at Meilisearch since the last working version of my patch on LMDB, and it hasn't broken either our usage or the LMDB API.
Have a nice begining of 2026, Looking forward for an answer, kero
[1]: https://github.com/meilisearch/meilisearch/pull/6100/changes
https://bugs.openldap.org/show_bug.cgi?id=10395
--- Comment #10 from Howard Chu hyc@openldap.org ---
+ /** The count of nested RDONLY txns under this txn also named child txns */ + unsigned int mt_rdonly_child_count;
I don't understand the "also named child txns" part of this comment.
+ txn->mt_rdonly_child_count = 0; +#ifdef _WIN32 + txn->mt_child_mutex = CreateMutex(NULL, FALSE, NULL); +#else + pthread_mutex_init(&txn->mt_child_mutex, NULL); +#endif
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.
+ /* Nested transactions: + * If RDONLY: Any number of children, writemap allowed + * If write: Max 1 child, no writemap
These comments are no longer unambiguous.
The meaning before: only write txns may have nested txns, the nested txns may only be write txns, and there may only be 1.
The meaning now should be: Only write txns may have nested txns; if the nested txn is a write txn there may only be 1; if the nested txn is a read txn there may be arbitrarily many.
+ /* Not useful when nested RDONLY but correctly freed in mdb_txn_end */
These allocations should be skipped for RDONLY txns, especially since there may be arbitrarily many of them.
+ if (F_ISSET(flags, MDB_RDONLY)) { + pthread_mutex_lock(&parent->mt_child_mutex); + parent->mt_rdonly_child_count++; + pthread_mutex_unlock(&parent->mt_child_mutex); + } else { + pthread_mutex_lock(&parent->mt_child_mutex); + parent->mt_rdonly_child_count = 0; + pthread_mutex_unlock(&parent->mt_child_mutex); + }
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.
+ if (!txn->mt_parent && F_ISSET(txn->mt_flags, MDB_TXN_RDONLY)) {
This appears to only be correct if you used MDB_NOTLS.
+ env->me_pgstate = ((MDB_ntxn *)txn)->mnt_pgstate;
pgstate should only exist in a writable child txn. In a read txn it should be unused, what is this value then?
https://bugs.openldap.org/show_bug.cgi?id=10395
kero renault.cle@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #1098|0 |1 is obsolete| |
--- Comment #11 from kero renault.cle@gmail.com --- 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
https://bugs.openldap.org/show_bug.cgi?id=10395
--- Comment #12 from kero renault.cle@gmail.com --- Actually, my patch is wrong. I am debugging and will provide a correct patch once it is fixed. In the meantime, patch [1098] is valid and available on comment #8.
https://bugs.openldap.org/show_bug.cgi?id=10395
kero renault.cle@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #1129|0 |1 is obsolete| |
--- Comment #13 from kero renault.cle@gmail.com --- Created attachment 1130 --> https://bugs.openldap.org/attachment.cgi?id=1130&action=edit Allow multiple nested read transactions from a write transaction review from comment #12
I have found the issue: I was no longer allocating a list for dirty and free pages, but assigning NULL to them was not the solution. I now share the parents' lists with the RDONLY nested children transactions. What do you think?
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.
I implemented the above proposal and updated the comment to clarify that it is only possible to create nested read transactions from NOTLS environments.
Thank you for the review again, and sorry for the little hiccup here. Have a nice week, kero
https://bugs.openldap.org/show_bug.cgi?id=10395
--- Comment #14 from Howard Chu hyc@openldap.org --- (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
https://bugs.openldap.org/show_bug.cgi?id=10395
kero renault.cle@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #1130|0 |1 is obsolete| |
--- Comment #15 from kero renault.cle@gmail.com --- Created attachment 1131 --> https://bugs.openldap.org/attachment.cgi?id=1131&action=edit Allow multiple nested read transactions from a write transaction review from comment #14
Hello Howard,
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.
Thanks for the info. I am already using pthread_mutex_lock. So, that's fine.
You can free a NULL pointer, that's standard C idiom. No need to conditionalize that.
Thanks for reminding me. However, I made sure not to allocate dirty_list and mt_free_pgs, and to use the parent's ones. I made sure not to free parents' lists when the transaction is a nested read-only one.
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).
Thanks for the deeper explanation of the problem. I now better understand. I checked, and it seems that maxreaders is already ignored by nested read transactions, as it is checked in mdb_txn_renew0, and only no-parent (+RDONLY) txns enter it. However, I see that we use calloc to allocate any transaction with a parent, which means that mt_u.reader is set to NULL for our nested read transaction. Am I right that everything is working fine, as the parent transaction is locked while child transactions are alive?
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.
I updated the mdb_txn_reset function to ignore nested read-only transactions and updated the documentation accordingly. I updated the documentation of mdb_txn_renew, as it throws an EINVAL because mdb_txn_reset skips nested read transactions.
- if (!txn->mt_parent && F_ISSET(txn->mt_flags, MDB_TXN_RDONLY)) {
Should I do anything with this? Is it correct, or should I handle that differently?
Also, the documentation for mdb_txn_begin in lmdb.h should be updated.
Done. I updated the documentation of mdb_txn_begin, mdb_txn_reset, and mdb_txn_renew accordingly.
Thank you and have a nice day, kero
https://bugs.openldap.org/show_bug.cgi?id=10395
--- Comment #16 from Howard Chu hyc@openldap.org --- It looked ok but doesn't apply cleanly to mdb.master3. Can you rebase?
https://bugs.openldap.org/show_bug.cgi?id=10395
--- Comment #17 from kero renault.cle@gmail.com --- Created attachment 1133 --> https://bugs.openldap.org/attachment.cgi?id=1133&action=edit Allow multiple nested read transactions from a write transaction for master3
Here is a patch that I rebased on top of mdb.master3. I haven't changed anything but rebased. Please let me know if it looks good or if I need to ensure it works correctly with other features, such as encryption-at-rest.
https://bugs.openldap.org/show_bug.cgi?id=10395
--- Comment #18 from Howard Chu hyc@openldap.org --- (In reply to kero from comment #17)
Created attachment 1133 [details] Allow multiple nested read transactions from a write transaction for master3
Here is a patch that I rebased on top of mdb.master3. I haven't changed anything but rebased. Please let me know if it looks good or if I need to ensure it works correctly with other features, such as encryption-at-rest.
This looks identical to the previous "after-comment-14" patch.
Yeah, I was wondering how this would interact with the encryption support. Definitely, if you can test it please do.
https://bugs.openldap.org/show_bug.cgi?id=10395
kero renault.cle@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #1131|0 |1 is obsolete| |
--- Comment #19 from kero renault.cle@gmail.com --- Created attachment 1134 --> https://bugs.openldap.org/attachment.cgi?id=1134&action=edit Allow multiple nested read transactions from a write transaction review from comment #18
Hello Howard,
Thank you for the review. I took a bit of time to get my patch working on master3 and found a potential issue when freeing the dirty_list. I previously ensured I didn't allocate it when beginning a nested read transaction, but I was deallocating it multiple times because it is owned by the parent transaction.
So, this patch is for mdb.master and ensures we free the dirty_list (with mdb_dlist_free) only once.
https://bugs.openldap.org/show_bug.cgi?id=10395
kero renault.cle@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #1133|0 |1 is obsolete| |
--- Comment #20 from kero renault.cle@gmail.com --- Created attachment 1135 --> https://bugs.openldap.org/attachment.cgi?id=1135&action=edit Allow multiple nested read transactions from a write transaction for master3 review from comment #18
This patch has been rebased on mdb.master3. I used `git format-patch` to generate it after resolving the merge conflicts, and `git apply --check` to verify its validity. There shouldn't be any merge conflicts when applying this patch.
When I initially applied the patch from mdb.master and tested it, I had double-free and segfault issues. The problem was coming from the call to mdb_dlist_free [1]. This is what made me realize that we should not free the dirty list when ending nested read transactions.
Have a nice day, kero
[1]: https://github.com/LMDB/lmdb/blob/be217bf254aae009a4683204bcd97f97ee4d5ad4/l...
https://bugs.openldap.org/show_bug.cgi?id=10395
--- Comment #21 from Howard Chu hyc@openldap.org --- (In reply to kero from comment #19)
Created attachment 1134 [details] Allow multiple nested read transactions from a write transaction review from comment #18
Hello Howard,
Thank you for the review. I took a bit of time to get my patch working on master3 and found a potential issue when freeing the dirty_list. I previously ensured I didn't allocate it when beginning a nested read transaction, but I was deallocating it multiple times because it is owned by the parent transaction.
So, this patch is for mdb.master and ensures we free the dirty_list (with mdb_dlist_free) only once.
There's no point basing on mdb.master, which is for 0.9. The 0.9 branch is closed for new features.
https://bugs.openldap.org/show_bug.cgi?id=10395
--- Comment #22 from kero renault.cle@gmail.com ---
There's no point basing on mdb.master, which is for 0.9. The 0.9 branch is closed for new features.
Actually, I didn't know that, or I forgot. Thanks for the reminder.
Please let me know if the patch rebased on mdb.master3 looks fine and can be applied correctly.
Just so you know, I have a bunch of CIs running [CIs running], and everything looks fine for them. There are tests using nested read transactions from a write one in an environment with an encryption-at-rest setup. Obviously, tests don't catch everything, but that's encouraging.
Have a nice day, kero
[CIs running]: https://github.com/meilisearch/heed/pull/307/checks
https://bugs.openldap.org/show_bug.cgi?id=10395
--- Comment #23 from Howard Chu hyc@openldap.org --- + /* mark parent txn has no longer having children if this is the last nested txn */ + int last_child = 0; + if (F_ISSET(flags, MDB_RDONLY)) { + pthread_mutex_lock(&txn->mt_parent->mt_child_mutex); + txn->mt_parent->mt_rdonly_child_count--; + last_child = (txn->mt_parent->mt_rdonly_child_count == 0); + pthread_mutex_unlock(&txn->mt_parent->mt_child_mutex); + } + if (!F_ISSET(flags, MDB_RDONLY) || last_child) { + txn->mt_parent->mt_child = NULL; + txn->mt_parent->mt_flags &= ~MDB_TXN_HAS_CHILD; + }
This is racy. A new child rtxn could come along after you set last_child but before you test it.
That whole sequence of `if (F_ISSET(flags, MDB_RDONLY))` tests needs to be consolidated.
https://bugs.openldap.org/show_bug.cgi?id=10395
kero renault.cle@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #1134|0 |1 is obsolete| | Attachment #1135|0 |1 is obsolete| |
--- Comment #24 from kero renault.cle@gmail.com --- Created attachment 1140 --> https://bugs.openldap.org/attachment.cgi?id=1140&action=edit Allow multiple nested read transactions from a write transaction for master3 review from comment #23
Hello Howard,
Thank you for the review. I took more time to check the different locks. The difficulty is around the fact that nested read transactions can be dropped while others can be created.
To make sure everything is safe I increased the scope the mutex were taking in mdb_txn_begin and especially when accessing parent->mt_child or parent->mt_flags. Now, it is also taken when initializing a nested write transaction. Please, tell me if you see anything spooky.
https://bugs.openldap.org/show_bug.cgi?id=10395
--- Comment #25 from Howard Chu hyc@openldap.org --- Created attachment 1141 --> https://bugs.openldap.org/attachment.cgi?id=1141&action=edit Small cleanups
Try this version. Just a few small cleanups.
https://bugs.openldap.org/show_bug.cgi?id=10395
--- Comment #26 from kero renault.cle@gmail.com --- Hello Howard,
Thank you very much for the much cleaner version of this patch. I created two dummy PRs to test the engine with it, as it extensively uses the nested read transaction feature.
One of the PRs [mdb.master3 PR] is to check the patch on mdb.master3, and it seems at least one thing is working differently from mdb.master. I haven't checked what it was, but I doubt it is related to this patch, since it's affecting an env that doesn't use the new feature.
[mdb.master3 PR]: https://github.com/meilisearch/meilisearch/pull/6309
The other PR [mdb.master PR] is testing your rebased patch against mdb.master. The reason I still need it on mdb.master is that we can't make a breaking change to the database format, and I prefer that Meilisearch rely on the mainstream LMDB with a single derivation rather than a completely different version (mdb.master3). I will maintain a fork and set up a notification system to be informed of new LMDB releases (emails are great; combining them with issues is better).
[mdb.master PR]: https://github.com/meilisearch/meilisearch/pull/6310
Anyway, the patch seems valid and doesn't trigger any issues on the mdb.master branch. On the mdb.master3 branch I would consider it valid too, the problem is unrelated to this very patch.
Have a good weekend, and thanks for your time, kero
https://bugs.openldap.org/show_bug.cgi?id=10395
Howard Chu hyc@openldap.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |TEST Status|UNCONFIRMED |RESOLVED
--- Comment #27 from Howard Chu hyc@openldap.org --- Thanks for confirming. Pushed to mdb.master3.
https://bugs.openldap.org/show_bug.cgi?id=10395
--- Comment #28 from Quanah Gibson-Mount quanah@openldap.org --- mdb.master3:
• 10161b20 by Kerollmops at 2026-04-03T16:26:58+01:00 ITS#10395 LMDB: Allow multiple nested read txns from a write txn
https://bugs.openldap.org/show_bug.cgi?id=10395
Howard Chu hyc@openldap.org changed:
What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.openldap.org/s | |how_bug.cgi?id=10518
https://bugs.openldap.org/show_bug.cgi?id=10395
Quanah Gibson-Mount quanah@openldap.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |VERIFIED
https://bugs.openldap.org/show_bug.cgi?id=10395
Quanah Gibson-Mount quanah@openldap.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|TEST |FIXED
-
openldap-its＠openldap.org