MySQL Bugs: #40893: Falcon concurrent unlock can cause invalid fetches

Bug #40893	Falcon concurrent unlock can cause invalid fetches
Submitted:	20 Nov 2008 16:19	Modified:	13 Dec 2008 7:25
Reporter:	Kevin Lewis	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: Falcon storage engine	Severity:	S3 (Non-critical)
Version:	6.0.8	OS:	Any
Assigned to:	Kevin Lewis	CPU Architecture:	Any
Tags:	F_RECORD TREE

Description:
During the course of replacing the old transaction dependency linkages with a newer event counter system, Olav discovered that records being evaluated in Transaction::getRelativeState() were detached from transactions that were still active.  

A record needs to keep the transaction pointer until the transaction is visible to all.  This is especially important once the new dependency manager is used, since there is no longer a direct link between transactions.

The problem occured during Table::unlockRecord().  This function (1) changes the state of the record to recUnlock, (2) sets RecordVersion::transaction to null, and (3) puts it prior record back into the record tree (RecordLeaf).  Steps 1 and 2 are both problems that need to be fixed.  

In a concurrent environment, another thread can find this lock record in the record tree just before it is detached and replaced with the prior.  So while it has this pointer the record changes state and looses its pointer to the transaction.

1)  There are about 25 places in the code that checks for Record::state == recLock and only one that also checks for recUnlock.  We need to quit using recUnlock.  

2)  The pointer to the transaction needs to remain so that another thread in getrelativeState can do its job correctly.  The current code sees this null pointer and assumes that this lock record which is going away is actually a committed record.  In Falcon's default isolation of consistent-read, it is reported as committedInvisible, which causes the caller to look at the prior, which happens to be the right thing to do, but for the wrong reason.  If, however, the isolation mode is read-committed, the caller would definitely get the wrong record.  Maybe there would be an assert down the road, or wrong results from a query.  Either way this is serious for read-committed isolation.

How to repeat:
Put a breakpoint on this line in Transaction::getRelativeState() while running falcon_bug_22165.

    for (int n = 0; n < numberStates; ++n)
        if (states [n].transactionId == transId)
            return CommittedInvisible;           <-- break here 

If the record has state==recUnlock you see the problem.

Suggested fix:
Patch is on the way

Both Olav and Kevin have reproduced this.

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/59418

2914 Kevin Lewis	2008-11-20
      Code cleanup and a comment change related to Bug#40893

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/commits/59408

2912 Kevin Lewis	2008-11-19
       Do not use Record::state = recUnlock.  It is not 
       recognized by many parts of the code and can 
       cause the wrong behavior.
       
       Also, A lock record that is unlocked should stay 
       on the transaction.  It is still replaced on the 
       recordLeaf, but other threads with pointers to it 
       from a concurrent Table::fetch need to be able to 
       see the transaction.  It will be taken off the
       transaction later with all the other records on 
       the transaction.

Pushed into 6.0.9-alpha  (revid:klewis@mysql.com-20081120170550-p48a7rrjs4by8ag1) (version source revid:hky@sun.com-20081127084516-nbu7693932vcz2st) (pib:5)

No documentation needed.