From: Olav Sandstaa
Date: December 3 2008 1:25pm
Subject: Re: Request for review: New dependency manager - patch 0
List-Archive: http://lists.mysql.com/falcon/232
Message-Id: <493688AD.8030005@sun.com>
MIME-Version: 1.0
Content-Type: text/plain; format=flowed; charset=ISO-8859-1
Content-Transfer-Encoding: 7BIT

Hi,

Based on comments from Kevin I have made an updated version of the 
dependency manager patch:

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

which replaces the previous committed patch.

Changes in this patch compared to the previous patch are only minor:

  -uses the transactionId as "start event" instead of having a separate 
start event
  -renamed commitEvent to commitId.

Olav

Olav Sandstaa wrote:
> Hi,
>
> [Sorry to those who have gotten an almost identical email about this 
> already.
> I have now run this patch in pushbuild and think it is ready for an 
> official review.
> Thanks to Kevin who already have provided feedback on the patch. His 
> comments
> will be included in follow-up patches.]
>
> I have committed the initial implementation of a new transaction
> dependency manager for Falcon. The patch is available here:
>
> http://lists.mysql.com/commits/60279
>
> I have opened a worklog for the new dependency manager. This currently
> only contains a high-level description of the dependency manager but
> should still be read before reading the patch:
>
>  http://forge.mysql.com/worklog/task.php?id=4654
>
> The patch includes the basic implementation of the new dependency
> manager. I have tried to not do too much changes in how it is used. It
> should be fully functional and hopefully both correct and
> robust. Still there are some changes and optimizations that I plan to
> commit in separate patches.
>
> A description of the patch:
>
> New datastructures (three new integers):
> ========================================
>
> 1. The transaction manager has gotten a "transition event counter" 
> (named transitionEventSequence). This is incremented each time a 
> transaction either starts or commits (not for rollback).
>
> 2. The transaction object have gotten two new members. startEvent and 
> commitEvent. These are assigned values from the 
> transictionEventSequence when the transaction starts and when it commits.
>
> These replace the old transaction manager's states array and
> dependencies counter.
>
>
> Handling of read-only transaction:
> ==================================
>
> Much of the complexity of the old dependency manager was caused due to
> having to optimize for read-only transactions. This will be much
> simpler with the new dependency manager since there is no data
> structures "pointing between transactions". Still, in this patch the
> optimization for read-only transactions is not included and all
> transaction goes through the regular commit code path (which is good
> for testing of it).
>
> Thus: Transaction::commitNoUpdate() is currently not called.
>
>
> Transaction::commitRecords and removal of transaction objects:
> ==============================================================
>
> Calling Transaction::commitRecords and checking for deleting of old
> transaction objects was done at least 4-5 places in the old
> implementation. In the new implementation this is done in only one
> place (I think). The only place this is done now is in
> TransactionManager::purgeTransaction() which is run by the scavenger
> thread. The reason for having it in purgeTransaction() is that this is
> also the place where the old dependency manager had a similar test. I
> plan to move this to Transaction::commit() in the next patch (if it
> does not have negative effect on performance to do this check on every
> commit).
>
> Comment: several of the existing places where the old dependency
> manager called Transaction::commitRecords is still in the code, but
> just commented out. Some of them might be re-enabled if we think that
> this would seriously help on getting "rid of" record versions much
> earlier. Some of these will likely just be deleted since I do not
> think they every was called (e.g. Transaction::commit() is unlikely to
> call commitRecords() I guess, but I will test it).
>
>
> Transaction::visible() and Transaction::getRelativeState()
> ==========================================================
>
> These have been updated to use the new dependency manager for
> determining the correct state for transactions.
>
>
> Transaction::getInfo()
> ======================
>
> This still reports information about number of dependencies (equal to
> zero). I did not remove this yet because this required some changes to
> the information schema definiiton for this (I think). Will be removed
> in a follow-up patch.
>
> Question: Do we want to include information about the start and commit
> event for transactions in this table?
>
>
> Debug code:
> ===========
>
> I have left a few lines of debug code that traces the number of
> allocated and deleted transaction objects. This code will eventually 
> be removed but I have left them in in order to
> detect whether there are any memory leaks introduced during this work.
>
>
> Best regards,
> Olav
>