2.3.12 transaction problem within skiplist DB->foreach()

[Bron Gondwana]

On Thu, 28 Aug 2008 18:17:44 -0400, "John Capo" <jc at irbs.com> said:
> . 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.

If you get this assert, it's an API violation.  Specifically, it's an API violation that
previously would have caused corruption.  I put the assert there to ensure that
it died instead of silently corrupting your skiplist file back when I was
debugging everything to do with skiplist files.

Traditionally in Cyrus code, you put the transaction inside your rock, and use
it for any subsidiary database calls.  Personally, I think the database access
interface blows goats, but it's what we have to work with unless we do a
pretty major refactor of a lot of code!  I did extend it so you can do read-only
queries without knowing the transaction, but if you're writing, you really
should be passed a copy of the outer transaction.

I've never seen the syncserver assert.  I'd be tempted to change those asserts
to syslog statements which include the database name and key name, and just
return DBERROR to the calling code.

Bron.
-- 
  Bron Gondwana
  brong at fastmail.fm