2.3.12 transaction problem within skiplist DB->foreach()

[John Capo]

I'm convinced this is a foreach() bug.  myforeach() is a copy of
myfetch() with a loop in the middle for the callback.  This bug has
been there since day one but showed up as just another unexplained
IOERROR message untill the asserts were added in 2.3.12.

Patch attached that's running on two test boxes.

John Capo
Tuffmail.com



Quoting John Capo (jc at irbs.com):
> . OK User logged in
> . rename user/abox user/bbox
> * BYE Fatal error: Internal error: assertion failed: cyrusdb_skiplist.c: 627: db->lock_status == UNLOCKED
> 
> This results from attempting to update annotations.db in a DB->foreach() callback.
> 
>     assert(db->lock_status == UNLOCKED)  in write_lock()
> 
> cmd_rename() -> annotatemore_rename() -> annotatemore_findall() -> foreach() -> rename_cb()
> 
> /* foreach allows for subsidary mailbox operations in 'cb'.
>    if there is a txn, 'cb' must make use of it.
> */
> 
> That comment makes sense but the transaction structure created in
> foreach() is not made available to the callback.  foreach() does
> provide the transaction structure it created when foreach() returns
> but that's too late for the callback to use the transaction.
> 
> Sometimes the same assert() is hit when syncserver tries to create
> a new mailbox.  I haven't look into this one yet.
> 
> Aug 28 14:04:24 m4 syncserver[77241]: Failed to access inbox for yadda
> Aug 28 14:04:25 m4 syncserver[77241]: Fatal error: Internal error: assertion failed: cyrusdb_skiplist.c: 622: db-> lock_status == UNLOCKED
> Aug 28 14:04:25 m4 syncserver[77241]: skiplist: closed while still locked
> 
> Foreach bug, annotation code DB API violation, or ???
> 
> If the comment above is correct, the bug is foreach() not providing
> the transaction to the callback.   Annotation code maybe the only place
> where updates are done from a foreach() callback.  Finding all the rocks
> and procs is tedious.
> 
> John Capo
> 
> 
> 
-------------- next part --------------
Index: lib/cyrusdb_skiplist.c
===================================================================
RCS file: /usr/local/CVS/src/cyrus-imapd/lib/cyrusdb_skiplist.c,v
retrieving revision 1.5
diff -u -r1.5 cyrusdb_skiplist.c
--- lib/cyrusdb_skiplist.c	25 Apr 2008 14:29:52 -0000	1.5
+++ lib/cyrusdb_skiplist.c	28 Aug 2008 23:02:38 -0000
@@ -1079,6 +1079,12 @@
 	if ((r = newtxn(db, &t))) return r;
 
 	tp = &t;
+
+	*tid = xmalloc(sizeof(struct txn));
+	memcpy(*tid, tp, sizeof(struct txn));
+	(*tid)->ismalloc = 1;
+	db->current_txn = *tid;
+
     } else {
 	assert(db->current_txn == *tid);
 	tp = *tid;
@@ -1152,6 +1158,8 @@
     }
 
     if (tid) {
+	/* Will this ever be true when *tid is allocated above???
+	*/
 	if (!*tid) {
 	    /* return the txn structure */