apr_dbm_lmdb.c: better error handling and better #49
Conversation
checking of return values of mdb* functions.
| @@ -216,9 +230,14 @@ static apr_status_t vt_lmdb_store(apr_dbm_t *dbm, apr_datum_t key, | |||
|
|
|||
| if ((rv = mdb_put(f->txn, f->dbi, &ckey, &cvalue, 0)) == 0) { | |||
| /* commit transaction */ | |||
| if (((rv = mdb_txn_commit(f->txn)) == MDB_SUCCESS) | |||
| && ((rv = mdb_txn_begin(f->env, NULL, 0, &f->txn)) == MDB_SUCCESS)) { | |||
| if ((rv = mdb_txn_commit(f->txn)) == MDB_SUCCESS){ | |||
| f->cursor = NULL; | |||
There was a problem hiding this comment.
Does this need an mdb_cursor_close(f->cursor); before setting cursor to NULL
There was a problem hiding this comment.
I'm not sure about this. In the documentation[0] is said that after mdb_txn_commit(), cursor must not be used again. Therefore I thought that invalidating pointer is enough.
On the other hand, in the documentation is said: Earlier documentation incorrectly said all cursors would be freed. Only write-transactions free cursors.. So if I understand it correctly, read-only transaction would leave cursor not freed and so we must free it using mdb_cursor_close(f->cursor);?
But in mdb_txn_commit function, there is mdb_cursors_close() [1] which looks like it closes all cursors [2].
[0] http://www.lmdb.tech/doc/group__mdb.html#ga846fbd6f46105617ac9f4d76476f6597
[1] https://github.com/LMDB/lmdb/blob/mdb.master/libraries/liblmdb/mdb.c#L4089C2-L4089C19
[2] https://github.com/LMDB/lmdb/blob/mdb.master/libraries/liblmdb/mdb.c#L2927C1-L2927C18
There was a problem hiding this comment.
Thanks, that makes sense to me then.
| @@ -237,9 +256,14 @@ static apr_status_t vt_lmdb_del(apr_dbm_t *dbm, apr_datum_t key) | |||
|
|
|||
| if ((rv = mdb_del(f->txn, f->dbi, &ckey, NULL)) == 0) { | |||
| /* commit transaction */ | |||
| if (((rv = mdb_txn_commit(f->txn)) == MDB_SUCCESS) | |||
| && ((rv = mdb_txn_begin(f->env, NULL, 0, &f->txn)) == MDB_SUCCESS)) { | |||
| if ((rv = mdb_txn_commit(f->txn)) == MDB_SUCCESS) { | |||
| f->cursor = NULL; | |||
|
Also just a comment on code style - use |
| if (f->txn) { | ||
| mdb_txn_commit(f->txn); | ||
| f->txn = NULL; | ||
| f->cursor = NULL; |
There was a problem hiding this comment.
Thinking more about whether or not we should commit or rollback an existing transaction in _close()..
Looking at where a ->txn != NULL can come from here:
- After an _open() this allows to truncate if APR_DBM_RWTRUNC was set.
- After a _store() and _del(), which do commit implicitly already, there is nothing to commit in _close() (supposedly?)
- After anything else there is nothing to commit either since they are read operations (supposedly?)
So if mdb_txn_commit() does nothing for an "empty" ->tnx (2. and 3.), which seems likely, this commit in close only addresses 1. right?
IOW, it avoids an implicit commit in _open() if APR_DBM_RWTRUNC is set, defering to the next _store() or _del() or _close(), which looks fine/sensible.
In the other dbm drivers there does not seem to be any commit/rollback possible or involved, _store() and _del() probably have immediate effect.
For now we get the same in lmdb with the implicit commits in _store() and _del(), and I don't think we can avoid that since there is no commit/rollback API in apr_dbm that the user can play with explicitely.
So, IIUC, unless we provide this new API (at some point) it seems that the implicit commit in _close() is what we want, as the only possible optimization with transactions for now...
Do I get this right?
checking of return values of mdb* functions.