2.3.12 transaction problem within skiplist DB->foreach()

[John Capo]

Quoting Bron Gondwana (brong at fastmail.fm):
> 
> On Thu, 28 Aug 2008 19:52:19 -0400, "John Capo" <jc at irbs.com> said:
> > 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.
> 
> Oooh, I see what you mean.  Yes - of course.  The double pointer 'txn **tid'
> gets passed in, and may be needed DURING the foreach by subsidiary code,
> but that's not possible because it doesn't actually get set until the end of
> the function.  How annoying.

Best I can tell, the annotation code is the only place that updates
during a foreach().  We use annotations to select name space per
user, duplicate delivery per user, and per folder expire times, so
the bug bites bad here.  Replication has been unstable now that all
servers are running 2.3.12.  I suspect this bug is part of it.

> Your patch looks viable, though it causes unreachable code later in the
> function, and I'd probably update myfetch to be matching in behaviour
> just for clarity.  I'll put that together now.

I wasn't willing to remove that code but I didn't think it would
ever be hit.

John Capo



> 
> Bron.
> 
> > 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
> > > 
> > > 
> > > 
> -- 
>   Bron Gondwana
>   brong at fastmail.fm
>