Kevin , I think Transaction::syncRecordList could be appropriate name for
the new sync.
I can rewrite the patch to use that . Transaction::syncObject does not seem
an appropriate candidate to me, it looks like it is has something to do with
dependency checks , updates and releases , which is another story.
> -----Original Message-----
> From: Kevin Lewis [mailto:klewis@stripped]
> Sent: Thursday, August 21, 2008 5:26 PM
> To: 'Vladislav Vaintroub'; commits@stripped
> Subject: RE: bzr commit into mysql-6.0-falcon branch (vvaintroub:2792)
> Bug#38933
>
> Vlad,
>
> I agree that syncIndexes is being used inappropriately. It is supposed
> to
> just protect the list of deferredIndexes attached to the transaction.
>
> Sometime recently, syncIndexes got expanded in its use because it
> seemed
> useful.
> It is locked in Transaction::writeComplete with this comment;
>
> // Keep the syncIndexes lock to avoid a race condition with
> dropTable
>
> This was in Transaction::dropTable;
>
> // Keep exclusive lock to avoid race condition with writeComplete
>
>
> That has been there for a long time, because drop table needed to drop
> deferred indexes. It is probably no longer needed for that.
>
> In June, syncIndexes was locked in Transaction::releaseDependency with
> this
> comment;
>
> // The Sync is to avoid a race with writeComplete(). It looks
> whacko, but does the trick
>
> Then added to Transaction::truncateTable;
>
> // Keep exclusive lock to avoid race condition with writeComplete
>
> Then recently it was expanded to Transaction::hasRecords with this
> comment;
>
> // This lock is to avoid race with writeComplete
>
> The 'whacko' comment was dropped which started to give it a little more
> legitimacy. :)
>
> I think we have used this syncIndexes inappropriately for too long.
> There
> are other places where the Transaction::firstRecord or
> Transaction::lastRecord is accessed that are not explicitly protected
> by
> syncIndexes.
>
> I think that we either need two different SyncObjects or we should
> rename
> syncIndexes to reference what it is protecting.
>
> Note; Transaction::SyncObject is only used in
> Transaction::commitNoUpdates
> and Transaction::initialize, so I think it is a likely candidate. What
> do
> you think?
>
>
> >-----Original Message-----
> >From: Vladislav Vaintroub [mailto:vvaintroub@stripped]
> >Sent: Thursday, August 21, 2008 8:18 AM
> >To: commits@stripped
> >Subject: bzr commit into mysql-6.0-falcon branch (vvaintroub:2792)
> Bug#38933
> >
> >#At file:///C:/bzr/mysql-6.0-falcon-team/
> >
> > 2792 Vladislav Vaintroub 2008-08-21
> > Bug #38933 - crash if rollback and concurrent DDL operations
> >
> > Problem : Transaction record list is modified and entries of
> > the list are freed during Transaction::rollback.If there is
> > a parallel execution of DDL statement, DDL will traverse the
> > list and can crash accessing freed memory
> >
> > Fix: protect record list on Transaction::rollback. The
> syncObject
> > for this purpose has always been syncIndexes and it is used here
> > once again. syncIndexes usage is possibly a hack. TODO: have
> > dedicated syncObject for protecting the list.
> >modified:
> > mysql-test/suite/falcon/r/falcon_bug_22165.result
> > mysql-test/suite/falcon/t/falcon_bug_22165.test
> > storage/falcon/Transaction.cpp
> >
> >per-file messages:
> > mysql-test/suite/falcon/r/falcon_bug_22165.result
> > test rollback as well
> > mysql-test/suite/falcon/t/falcon_bug_22165.test
> > test rollback as well
> > storage/falcon/Transaction.cpp
> > protect record list during Transaction::rollback
> >=== modified file 'mysql-test/suite/falcon/r/falcon_bug_22165.result'
> >--- a/mysql-test/suite/falcon/r/falcon_bug_22165.result 2008-08-04
> >15:53:52 +0000
> >+++ b/mysql-test/suite/falcon/r/falcon_bug_22165.result 2008-08-21
> >13:18:06 +0000
> >@@ -16,7 +16,10 @@ while v < 10 do
> > START TRANSACTION;
> > INSERT INTO t1 (a, b) VALUES (v, 'a');
> > UPDATE t1 SET c = current_timestamp WHERE a = v;
> >-COMMIT;
> >+IF (v < 5)
> >+THEN COMMIT;
> >+ELSE ROLLBACK;
> >+END IF;
> > SET v = v + 1;
> > end while;
> > end//
> >
> >=== modified file 'mysql-test/suite/falcon/t/falcon_bug_22165.test'
> >--- a/mysql-test/suite/falcon/t/falcon_bug_22165.test 2008-08-04
> >15:53:52 +0000
> >+++ b/mysql-test/suite/falcon/t/falcon_bug_22165.test 2008-08-21
> >13:18:06 +0000
> >@@ -30,7 +30,10 @@ begin
> > START TRANSACTION;
> > INSERT INTO t1 (a, b) VALUES (v, 'a');
> > UPDATE t1 SET c = current_timestamp WHERE a = v;
> >- COMMIT;
> >+ IF (v < 5)
> >+ THEN COMMIT;
> >+ ELSE ROLLBACK;
> >+ END IF;
> > SET v = v + 1;
> > end while;
> > end//
> >@@ -57,7 +60,7 @@ while ($i)
> > --reap
> > connection conn2;
> > --reap
> >-
> >+
> > dec $i;
> > }
> > --enable_query_log
> >
> >=== modified file 'storage/falcon/Transaction.cpp'
> >--- a/storage/falcon/Transaction.cpp 2008-08-18 05:45:29 +0000
> >+++ b/storage/falcon/Transaction.cpp 2008-08-21 13:18:06 +0000
> >@@ -403,6 +403,9 @@ void Transaction::rollback()
> > // Rollback pending record versions from newest to oldest in case
> > // there are multiple record versions on a prior record chain
> >
> >+ Sync sync(&syncIndexes, "Transaction::rollback(1.5)");
> >+ sync.lock(Exclusive);
> >+
> > while (firstRecord)
> > {
> > record = firstRecord;
> >@@ -428,6 +431,8 @@ void Transaction::rollback()
> > record->transaction = rollbackTransaction;
> > record->release();
> > }
> >+ firstRecord = NULL;
> >+ sync.unlock();
> >
> > for (SavePoint *savePoint = savePoints; savePoint; savePoint =
> >savePoint->next)
> > if (savePoint->backloggedRecords)
> >
> >
> >--
> >MySQL Code Commits Mailing List
> >For list archives: http://lists.mysql.com/commits
> >To unsubscribe:
> http://lists.mysql.com/commits?unsub=1
