From: Kevin Lewis
Date: November 18 2008 4:31pm
Subject: Re: New dependency manager and Transaction::getRelativeState
List-Archive: http://lists.mysql.com/falcon/194
Message-Id: <4922EDF0.3020500@sun.com>
MIME-Version: 1.0
Content-Type: text/plain; format=flowed; charset=ISO-8859-1
Content-Transfer-Encoding: 7BIT

Kevin Lewis wrote:
> Comments below;
> 
> Olav Sandstaa wrote:
>>
>> Jim and everybody else,
>>
>> As I mentioned in was working on removing the last parts of the old 
>> dependency manager and had one issue left (famous last word :-) ). 
>> This issue is related to Transaction::getRelativeState().
>>
>> Some background: The new dependency manager stores the event counter 
>> for (a) when the transaction became active and (b) when the 
>> transaction committed in the transaction object. These two event 
>> numbers is then used for comparing the relative state of active and 
>> committed transactions. Using these instead of the old dependency 
>> manager allows me to remove (a) the dependency counter for each 
>> transaction and (b) the states array with pointers to all transactions 
>> that where active at the start of a given transaction.
>>
>> The issue:
>>
>> The code for the first part of Transaction::getRelativeState() looks 
>> like this:
>>
>>
>> State Transaction::getRelativeState(Transaction *transaction, TransId 
>> transId, uint32 flags)
>> {
>>    if (transactionId == transId)
>>        return Us;
>>
>>    // A record may still have the transId even after the trans itself 
>> has been deleted.
>>      if (!transaction)
>>        {
>>        // All calls to getRelativeState are for the purpose of writing.
>>        // So only ConsistentRead can get CommittedInvisible.
>>
>>        if (IS_CONSISTENT_READ(isolationLevel))
>>            {
>>            // If the transaction is no longer around, and the record is,
>>            // then it must be committed.
>>
>>            if (transactionId < transId)
>>                return CommittedInvisible;
>>
>>            // Be sure it was not active when we started.
>>
>>            for (int n = 0; n < numberStates; ++n)
>>                if (states [n].transactionId == transId)
>>                    return CommittedInvisible;
>>            }
>>
>>        return CommittedVisible;
>>        }
>>
>>
>> The issue is related to when the pointer to the transaction argument 
>> is NULL:
>>
>> 1) Not having a reference to the transaction makes it hard(er) to 
>> compare the state of two different transactions since the one we are 
>> going to compare state against is "not there". For the old dependency 
>> manager this was easy since it has the states array of all active 
>> transactions available also after these transactions possibly are 
>> deleted (see the for loop above). But with the new dependency manager 
>> we should be able to remove this state array....
>>
>>     I have a working solution for this: By including the commit 
>> transition event number in every RecordVersion object (I set this in 
>> Transaction::commit()) makes it possible to compare this record 
>> against this transaction's start event. The code replacing the above 
>> for loop looks approximately like this:
>>
>>           if (transEndEvent_of_recordObject > startEvent)
>>                {
>>                fprintf(stderr, "T::getRelativeState: %d: NEW: 
>> CommittedInvisible: end=%d start=%d\n", transactionId, transEndEvent, 
>> startEvent);
>>                return CommittedInvisible;
>>                }
>>
>>    The test for this is simple but I do not like to having to include 
>> the commit event in record versions if I do not have to do so. Anyone 
>> having a suggestion for a better solution?
> 
> This all hinges on the design that the transaction is NULLed out during 
> Transaction::commitRecords().  This can happen at Transaction::commit() 
> or Transaction::writeComplete() or even later at 
> Transaction::releaseDependency().  But it always requires that 
> (dependencies == 0) && !writePending)
> 
> In fact, it is effectively impossible for the 
> Transaction::commitRecords() to be called in Transaction::commit() since 
> it requires !writePending.  I think that call is vestigial from the days 
> when commitNoUpdates() did not exist and some of the transaction going 
> through Transaction::commit() were read-only.
> 
> How are you managing the dependency counter? Without it, you may be 
> separating the transaction from its records too soon.  Before, if the 
> dependency counter went to zero, then this transaction ended before any 
> active currently transaction.  There would be no overlap because the 
> overlap was managed by the dependency counter and the states array.
> 
> In other words, if a record no longer had a transaction pointer, it 
> ended before any transaction that would be calling getRelativeState() 
> ever started.

This implies that the code you are worried about is no longer used or 
needed;

            // Be sure it was not active when we started.

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

Can you run a test to see if that return is EVER hit in the current code?

> 
> 
>> 2) Comments in the code above seems to suggest that the reason for not 
>> having the pointer to the transaction object is due to that the 
>> transaction object has been deleted and it assumes that the 
>> transaction has been committed. In my testing this seems not like it 
>> is an correct assumption, in most cases when the transaction pointer 
>> is NULL the transaction is actually still in Active state. Questions:
>>
>>     a) Are the comments in the code wrong? Or
>>     b) The code is wrong? And
>>     c) Why do we not have the transaction pointer pointing to the 
>> transaction if it is still active
>>
>> or (d) I have I introduced some error that causes this?
> 
> You have introduced an error that causes Transaction::commitRecords() to 
> be called while dependencies still exist.
> 
> Kevin
> 
> 
>