Re: [PR] apr_dbm_lmdb.c: better error handling and better [apr]

2023-10-24 Thread via GitHub


ylavic commented on code in PR #49:
URL: https://github.com/apache/apr/pull/49#discussion_r1370436413


##
dbm/apr_dbm_lmdb.c:
##
@@ -156,14 +168,16 @@ static void vt_lmdb_close(apr_dbm_t *dbm)
 {
 real_file_t *f = dbm->file;
 
-if (f->cursor) {
-mdb_cursor_close(f->cursor);
-f->cursor = NULL;
-}
-
+/* try to commit all transactions that haven't been commited yet on close 
*/
 if (f->txn) {
mdb_txn_commit(f->txn);
f->txn = NULL;
+   f->cursor = NULL;

Review 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:
   1. After an _open() this allows to truncate if APR_DBM_RWTRUNC was set.
   2. After a _store() and _del(), which do commit implicitly already, there is 
nothing to commit in _close() (supposedly?)
   3. 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?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@apr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] apr_dbm_lmdb.c: better error handling and better [apr]

2023-10-24 Thread via GitHub


asfgit closed pull request #49: apr_dbm_lmdb.c: better error handling and better
URL: https://github.com/apache/apr/pull/49


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@apr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] apr_dbm_lmdb.c: better error handling and better [apr]

2023-10-24 Thread via GitHub


notroj commented on code in PR #49:
URL: https://github.com/apache/apr/pull/49#discussion_r1370260493


##
dbm/apr_dbm_lmdb.c:
##
@@ -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, , , 0)) == 0) {
 /* commit transaction */
-if (((rv = mdb_txn_commit(f->txn)) == MDB_SUCCESS)
-&& ((rv = mdb_txn_begin(f->env, NULL, 0, >txn)) == 
MDB_SUCCESS)) {
+if ((rv = mdb_txn_commit(f->txn)) == MDB_SUCCESS){
 f->cursor = NULL;

Review Comment:
   Thanks, that makes sense to me then.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@apr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] apr_dbm_lmdb.c: better error handling and better [apr]

2023-10-24 Thread via GitHub


uhliarik commented on code in PR #49:
URL: https://github.com/apache/apr/pull/49#discussion_r1370095013


##
dbm/apr_dbm_lmdb.c:
##
@@ -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, , , 0)) == 0) {
 /* commit transaction */
-if (((rv = mdb_txn_commit(f->txn)) == MDB_SUCCESS)
-&& ((rv = mdb_txn_begin(f->env, NULL, 0, >txn)) == 
MDB_SUCCESS)) {
+if ((rv = mdb_txn_commit(f->txn)) == MDB_SUCCESS){
 f->cursor = NULL;

Review 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



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@apr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] apr_dbm_lmdb.c: better error handling and better [apr]

2023-10-24 Thread via GitHub


notroj commented on PR #49:
URL: https://github.com/apache/apr/pull/49#issuecomment-1776804513

   Also just a comment on code style - use `if (blah) {` rather than `if 
(blah){` without a space.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@apr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] apr_dbm_lmdb.c: better error handling and better [apr]

2023-10-24 Thread via GitHub


notroj commented on code in PR #49:
URL: https://github.com/apache/apr/pull/49#discussion_r1369834921


##
dbm/apr_dbm_lmdb.c:
##
@@ -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, , NULL)) == 0) {
 /* commit transaction */
-if (((rv = mdb_txn_commit(f->txn)) == MDB_SUCCESS)
-&& ((rv = mdb_txn_begin(f->env, NULL, 0, >txn)) == 
MDB_SUCCESS)) {
+if ((rv = mdb_txn_commit(f->txn)) == MDB_SUCCESS) {
 f->cursor = NULL;

Review Comment:
   same as above



##
dbm/apr_dbm_lmdb.c:
##
@@ -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, , , 0)) == 0) {
 /* commit transaction */
-if (((rv = mdb_txn_commit(f->txn)) == MDB_SUCCESS)
-&& ((rv = mdb_txn_begin(f->env, NULL, 0, >txn)) == 
MDB_SUCCESS)) {
+if ((rv = mdb_txn_commit(f->txn)) == MDB_SUCCESS){
 f->cursor = NULL;

Review Comment:
   Does this need an `mdb_cursor_close(f->cursor);` before setting cursor to 
`NULL`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@apr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



[PR] apr_dbm_lmdb.c: better error handling and better [apr]

2023-10-09 Thread via GitHub


uhliarik opened a new pull request, #49:
URL: https://github.com/apache/apr/pull/49

   checking of return values of mdb* functions.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@apr.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org