List:Falcon Storage Engine« Previous MessageNext Message »
From:Olav Sandstaa Date:November 20 2008 2:30pm
Subject:Re: New dependency manager and Transaction::getRelativeState
View as plain text  
Kevin Lewis wrote:
> Olav, comments below;
>
>
> > Olav wrote;
>> The new dependency manager uses the "startEvent" (start time) and the 
>> "endEvent" (commit time) for each transaction to determine 
>> relationships (and thus dependency) between two transaction. The main 
>> test for this is:
>>
>>    if (transaction A's endEvent < transaction B's startEvent) { // 
>> Transaction A committed before B even started
>>       // A and B was never active on the same time
>>       // B would not have any dependencies on A (using the old 
>> dependency manage)
>>       // A can be clean/removed/deleted
>>  }
>
> Note that you cannot remove transaction A until it's endEvent is < the 
> startEvent of the 'oldestActiveTransaction'.  

Yes, I am aware of this. The pseudo code above was intended to 
illustrate how I could determine whether on transactions was active or 
not at the time another transaction started. Unfortunately, I 
accidentally added the comment about that it could be deleted. In the 
implementation of the new dependency manager the test for deleting old 
transaction is to check against the startEvent of the oldest active 
transaction (exactly as you have written).

> In fact, without the dependency pointers between transactions to 
> prevent this, Transaction::commitRecords() will probably not actually 
> be able to break that linkage between records and transaction at 
> Transaction::writeComplete().  It will have to be done later if there 
> are 'dependencies'.  The current code uses that dependency counter to 
> release these older transactions when the dependencies pointers 
> finally go away.  Fortunately, the current code also has a call to 
> TransactionManager::purgeTransactions() in the scavenger.  You will 
> need to rely on this to clean up these old committed transactions that 
> still have records attached to them.

Actually, in my prototype version of the dependency manager that is 
exactly where I have done this. TransactionManager::purgeTransaction() 
is the one that takes care of both calling  Transaction::commitRecords() 
and deleting old transaction objects. I have at least temporarily 
commented out the call to commitRecords both in Transaction::commit() 
(where I do not think it ever would be called due to !writeWritePending) 
and in Transaction::writeComplete(). I might try to get it back in in 
Transaction::writeComplete, but in Transaction::commit() I do not think 
we need it.

But I plan to move or call TransactionManager::purgeTransaction() from 
Transaction::commit() so that each time we commit a record and anyway 
have the lock on the transaction lists, then check if the one (or more) 
in front of the committedList is ready for being "purged" (but this is 
an optimization - I will wait until everything else is in place).

>
> That means that the work I am doing is now critical for the success of 
> your new dependency management code.  My new scavenger starts up on a 
> load based event and will call purgeTransactions for you much more 
> often.  The current code will only call it every 30 seconds by 
> default.  At least it does not wait until the scavenge threshold is 
> reached in the record cache like it does before releasing old records!

Maybe, but I as I wrote above, I might do this check for purging of 
transaction on every commit (when I have locked the two lists anyway). 
But right now with this taking place only in purgeTransactions I see 
very long transaction lists.

>
>> This test is basically replacing most of the tests for "dependencies 
>> == 0" in the old dependency manager.
>>
>> The problem with the code in Transaction::getRelativeState() is that 
>> we do not always have the transaction object for transaction A (only 
>> the transaction id of it). 
>
> This must change.  A record needs to keep the transaction pointer 
> until the transaction is visible to all.  The alternative is to add 
> the endEvent to each and every RecordVersion, which I agree is 
> generally not what we want.
>
> You have found an exception to this with Table::unlockRecord().  I am 
> working on a solution to this exception and will provide it to you 
> tomorrow.  Even if you add the endEvent to each Recordversion, we need 
> to fix this problem that a lock record can get state=recUnlock.  This 
> state is not expected by many places in the code.
>
>> I have a solution for this where B commits (in Transaction::commit()) 
>> it copies it "endEvent" number into all RecordVersion objects. This 
>> makes B's endEvent available in Transaction::getRelativeState() also 
>> when we do not have the pointer to the transaction object. This makes 
>> it possible to replace the for-loop that goes through the states 
>> array with a similar test using the endEvent for the "other" and the 
>> startEvent of "this" transaction for determining if the "other" 
>> transaction was active or not when "this" transaction started.
> >
>> It was actully by this approach that I found out that we had cases 
>> where the pointer to transaction was NULL while the transaction in 
>> fact was not committed. The way I saw this was that 
>> Transaction::getReletiveState got called and the endEvent was zero 
>> (e.g. the transaction had not committed). So I can actually by 
>> looking at the endEvent number I have introduced in the RecordVersion 
>> object both determine if the "other" transaction was active when 
>> "this" transaction started (test above) and if it has committed or 
>> not (endEvent equal to zero).
>>
>> But... I do not like having to extend the use of the endEvent 
>> transition event into the RecordVersion objects... So if we could fix 
>> the existing bug that we found yesterday it would have been great if 
>> we just could have declared the for-loop that tests the states array 
>> in Transaction::getRelativeState() as "dead code".
>
> There is another problem with putting the endEvent into each and every 
> record.  It takes a while to do. It would have to be done at 
> Transaction::commit.  Transaction::getRelativeState is supposed to 
> wait on another active transaction until it commits.  But if it sees a 
> number in endEvent, it will assume that the transaction is committed 
> already.  This may cause some race conditions.  It is much better to 
> go directly to the transaction object to see its state.

The prototype code that I have running does this right now: adds the 
endEvent in Transaction::commit(). It works but I do not like it. I am 
glad we agree that this is not a good solution and that we only use the 
transaction object to get the state.

The prototype code that uses an endEvent from the RecordVersion is only 
used in the part of Transaction::getRelativeState() where the code 
assumes the transaction is committed and there is no waiting (but as my 
testing showed, these transactions did not actually have to be 
committed) so I do not think it need to lead to any raise conditions - 
but we both agree that this is not how we want to implement this.

>
> I talked to Jim today about how to fix this.  He confirmed my solution 
> which is to get rid of the recUnlock state and leave detached lock 
> records attached to the transaction when they are 'unlocked'.  It will 
> still replace this record on the recordLeaf so that future 
> Table::fetch calls will find the lock record's priorVersion.
>
> In addition, I need to make sure that any code caught looking at one 
> of these lock records has the wherewithall to look at the prior.  And 
> if it waits, it MUST do another Table::fetch().
>
> I'll get this code to you as soon as I can.

I have been experimenting with the patch you sent out late last night. 
And it seems like it solves the problem, both the bug with the 
transaction null pointer even when the transaction was still active and 
the need for me to having to copy the endEvent to the RecordVersion 
object. This simplifies my code a lot in this area.

Thanks for providing a fix for this so quickly, Kevin!

BTW: If anyone want to see the current prototype implementation of the 
dependency manager let me know and I will make a patch (but it is full 
of debug code and #ifdefs to be able to run simultanous with both the 
old and/or the new).

Olav

Thread
New dependency manager and Transaction::getRelativeStateOlav Sandstaa18 Nov
  • Re: New dependency manager and Transaction::getRelativeStateKevin Lewis18 Nov
    • Re: New dependency manager and Transaction::getRelativeStateKevin Lewis18 Nov
      • Re: New dependency manager and Transaction::getRelativeStateOlav Sandstaa18 Nov
      • Re: New dependency manager and Transaction::getRelativeStateOlav Sandstaa18 Nov
        • Re: New dependency manager and Transaction::getRelativeStateKevin Lewis19 Nov
          • Re: New dependency manager and Transaction::getRelativeStateOlav Sandstaa19 Nov
            • Re: New dependency manager and Transaction::getRelativeStateKevin Lewis19 Nov
              • Re: New dependency manager and Transaction::getRelativeStateOlav Sandstaa20 Nov