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
