2.3.12 transaction problem within skiplist DB->foreach()

[Bron Gondwana]

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

I've always hated the whole transaction management stuff.  My patch is still
a bit cut-and-pasty, but at least it has some good points:

a) tidptr name for all double pointers
b) tid name for the current tid in all functions where it's used enough to care
about dereferencing just once.
c) remove the stupid "malloc" flag and just always malloc, it's not that expensive.
d) following on from that, creation is initialisation.  Put all the transaction start
logic in one place.

I'd be even happier to merge a bunch more of the logic into a supporter function
rather than having 4 copies of it, but this will do for now.

By the way - I've only compile tested it so far... I'll do some more testing before
suggesting it for real :)

-- 
  Bron Gondwana
  brong at fastmail.fm

-------------- next part --------------
A non-text attachment was scrubbed...
Name: cyrus-skiplist-locking-rework-2.3.12.diff
Type: text/x-patch
Size: 12299 bytes
Desc: not available
Url : http://lists.andrew.cmu.edu/pipermail/cyrus-devel/attachments/20080829/f4c845b6/attachment-0001.bin