Vlad,
It looks like Transaction::dropTable still locks syncIndexes outside
releaseDeferredIndexes().
>-----Original Message-----
>From: Vladislav Vaintroub [mailto:vvaintroub@stripped]
>Sent: Friday, August 22, 2008 3:34 PM
>To: commits@stripped
>Subject: bzr commit into mysql-6.0-falcon branch (vvaintroub:2798)
Bug#38933
>
>#At file:///C:/bzr/mysql-6.0-falcon-team/
>
> 2798 Vladislav Vaintroub 2008-08-22
> Bug#38933 - Falcon crash in Transaction::hasRecords during concurrent
>DDL
>
> The problem is that Transaction::hasRecords traverses transaction's
>record
> list while a concurrent operation (rollback, writeComplete,addRecord)
>may
> concurrently modify it.
>
> This patch removed whacko-reuse of the Transaction::syncIndexes to
>protect
> the record list and introduces new lock Transaction::syncRecords for
>exactly
> this purpose.
>
> (hopefully) all places where the list is read or modified are now
>protected
> by this lock. When the list is freed, firstRecord is set to NULL to
>prevent
> crash in parallel Transaction::hasRecords() can crash.
>modified:
> storage/falcon/Transaction.cpp
> storage/falcon/Transaction.h
>
>=== modified file 'storage/falcon/Transaction.cpp'
>--- a/storage/falcon/Transaction.cpp 2008-08-22 15:01:12 +0000
>+++ b/storage/falcon/Transaction.cpp 2008-08-22 20:33:40 +0000
>@@ -86,6 +86,7 @@ Transaction::Transaction(Connection *cnc
> syncObject.setName("Transaction::syncObject");
> syncActive.setName("Transaction::syncActive");
> syncIndexes.setName("Transaction::syncIndexes");
>+ syncRecords.setName("Transaction::syncRecords");
> syncSavepoints.setName("Transaction::syncSavepoints");
> firstRecord = NULL;
> lastRecord = NULL;
>@@ -216,20 +217,19 @@ Transaction::~Transaction()
> delete backloggedRecords;
> chillPoint = &firstRecord;
>
>+ Sync syncRec(&syncRecords,"Transaction::~Transaction");
>+ syncRec.lock(Exclusive);
> for (RecordVersion *record; (record = firstRecord);)
> {
>- removeRecord(record);
>+ removeRecordNoLock(record);
> }
>+ firstRecord = NULL;
>+ syncRec.unlock();
>
> releaseSavepoints();
>
> if (deferredIndexes)
>- {
>- Sync sync(&syncIndexes, "Transaction::~Transaction");
>- sync.lock(Exclusive);
>-
> releaseDeferredIndexes();
>- }
> }
>
> void Transaction::commit()
>@@ -279,6 +279,8 @@ void Transaction::commit()
>
>
>
>+ Sync syncRec(&syncRecords,"Transaction::commit(1.5)");
>+ syncRec.lock(Shared);
> for (RecordVersion *record = firstRecord; record; record = record-
>>nextInTrans)
> {
> Table * table = record->format->table;
>@@ -295,7 +297,8 @@ void Transaction::commit()
> if (record->state == recDeleted && table->cardinality > 0)
> --table->cardinality;
> }
>-
>+ syncRec.unlock();
>+
> releaseDependencies();
> database->flushInversion(this);
>
>@@ -341,11 +344,7 @@ void Transaction::commitNoUpdates(void)
> ++transactionManager->committed;
>
> if (deferredIndexes)
>- {
>- Sync sync(&syncIndexes, "Transaction::commitNoUpdates(1)");
>- sync.lock(Exclusive);
> releaseDeferredIndexes();
>- }
>
> if (hasLocks)
> releaseRecordLocks();
>@@ -387,11 +386,7 @@ void Transaction::rollback()
> throw SQLEXCEPTION (RUNTIME_ERROR, "transaction is not
active");
>
> if (deferredIndexes)
>- {
>- Sync sync(&syncIndexes, "Transaction::rollback(1)");
>- sync.lock(Exclusive);
> releaseDeferredIndexes();
>- }
>
> releaseSavepoints();
> TransactionManager *transactionManager =
database->transactionManager;
>@@ -403,8 +398,8 @@ 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);
>+ Sync syncRec(&syncRecords, "Transaction::rollback(1.5)");
>+ syncRec.lock(Exclusive);
>
> while (firstRecord)
> {
>@@ -432,7 +427,7 @@ void Transaction::rollback()
> record->release();
> }
> firstRecord = NULL;
>- sync.unlock();
>+ syncRec.unlock();
>
> for (SavePoint *savePoint = savePoints; savePoint; savePoint =
>savePoint->next)
> if (savePoint->backloggedRecords)
>@@ -626,6 +621,8 @@ void Transaction::addRecord(RecordVersio
>
> record->addRef();
>
>+ Sync syncRec(&syncRecords,"Transaction::addRecord");
>+ syncRec.lock(Exclusive);
> if ( (record->prevInTrans = lastRecord) )
> lastRecord->nextInTrans = record;
> else
>@@ -633,6 +630,7 @@ void Transaction::addRecord(RecordVersio
>
> record->nextInTrans = NULL;
> lastRecord = record;
>+ syncRec.unlock();
>
> if (database->lowMemory && deletedRecords > MAX_LOW_MEMORY_RECORDS)
> backlogRecords();
>@@ -640,6 +638,12 @@ void Transaction::addRecord(RecordVersio
>
> void Transaction::removeRecord(RecordVersion *record)
> {
>+ Sync syncRec(&syncRecords,"Transaction::removeRecord");
>+ syncRec.lock(Exclusive);
>+ removeRecordNoLock(record);
>+}
>+void Transaction::removeRecordNoLock(RecordVersion *record)
>+{
> RecordVersion **ptr;
>
> if (record->nextInTrans)
>@@ -805,6 +809,8 @@ void Transaction::releaseDependencies()
>
> void Transaction::commitRecords()
> {
>+ Sync syncRec(&syncRecords,"Transaction::commitRecords");
>+ syncRec.lock(Exclusive);
> for (RecordVersion *recordList; (recordList = firstRecord);)
> {
> if (recordList && COMPARE_EXCHANGE_POINTER(&firstRecord,
>recordList, NULL))
>@@ -912,9 +918,11 @@ void Transaction::dropTable(Table* table
> sync.lock(Exclusive);
>
> releaseDeferredIndexes(table);
>+ sync.unlock();
>
>- // Keep exclusive lock to avoid race condition with writeComplete
>-
>+
>+ Sync syncRec(&syncRecords,"Transaction::dropTable(2)");
>+ syncRec.lock(Exclusive);
> for (RecordVersion **ptr = &firstRecord, *rec; (rec = *ptr);)
> if (rec->format->table == table)
> removeRecord(rec);
>@@ -928,9 +936,10 @@ void Transaction::truncateTable(Table* t
> sync.lock(Exclusive);
>
> releaseDeferredIndexes(table);
>+ sync.unlock();
>
>- // Keep exclusive lock to avoid race condition with writeComplete
>-
>+ Sync syncRec(&syncRecords,"Transaction::truncateTable(2)");
>+ syncRec.lock(Exclusive);
> for (RecordVersion **ptr = &firstRecord, *rec; (rec = *ptr);)
> if (rec->format->table == table)
> removeRecord(rec);
>@@ -940,9 +949,8 @@ void Transaction::truncateTable(Table* t
>
> bool Transaction::hasRecords(Table* table)
> {
>- // This lock is to avoid race with writeComplete
>- Sync sync(&syncIndexes, "Transaction::hasRecords");
>- sync.lock(Exclusive);
>+ Sync syncRec(&syncRecords, "Transaction::hasRecords");
>+ syncRec.lock(Shared);
> for (RecordVersion *rec = firstRecord; rec; rec = rec->nextInTrans)
> if (rec->format->table == table)
> return true;
>@@ -957,14 +965,12 @@ void Transaction::writeComplete(void)
> Sync sync(&syncIndexes, "Transaction::writeComplete");
> sync.lock(Exclusive);
> releaseDeferredIndexes();
>-
>- // Keep the synIndexes lock to avoid a race condition with dropTable
>+ sync.unlock();
>
> if (dependencies == 0)
> commitRecords();
>
> writePending = false;
>- sync.unlock();
> }
>
> bool Transaction::waitForTransaction(TransId transId)
>@@ -1248,6 +1254,7 @@ void Transaction::rollbackSavepoint(int
>
> savePoint = savePoints;
>
>+
> while (savePoint)
> {
> //validateRecords();
>@@ -1256,6 +1263,8 @@ void Transaction::rollbackSavepoint(int
> break;
>
> // Purge out records from this savepoint
>+ Sync
syncRec(&syncRecords,"Transaction::rollbackSavepoint(2)");
>+ sync.lock(Exclusive);
>
> RecordVersion *record = *savePoint->records;
> RecordVersion *stack = NULL;
>@@ -1295,6 +1304,7 @@ void Transaction::rollbackSavepoint(int
> rec->transaction = NULL;
> rec->release();
> }
>+ syncRec.unlock();
>
> // Handle any backlogged records
>
>@@ -1340,6 +1350,8 @@ void Transaction::releaseRecordLocks(voi
> RecordVersion **ptr;
> RecordVersion *record;
>
>+ Sync syncRec(&syncRecords,"Transaction::releaseRecordLocks");
>+ syncRec.lock(Exclusive);
> for (ptr = &firstRecord; (record = *ptr);)
> if (record->state == recLock)
> {
>@@ -1348,6 +1360,7 @@ void Transaction::releaseRecordLocks(voi
> }
> else
> ptr = &record->nextInTrans;
>+ syncRec.unlock();
> }
>
> void Transaction::print(void)
>@@ -1365,6 +1378,8 @@ void Transaction::printBlocking(int leve
> int deletes = 0;
> RecordVersion *record;
>
>+ Sync syncRec(&syncRecords,"Transaction::printBlocking");
>+ syncRec.lock(Shared);
> for (record = firstRecord; record; record = record->nextInTrans)
> if (record->state == recLock)
> ++locks;
>@@ -1412,7 +1427,7 @@ void Transaction::printBlocking(int leve
> record->recordNumber,
> what);
> }
>-
>+ syncRec.unlock();
> database->transactionManager->printBlocking(this, level);
> }
>
>@@ -1450,14 +1465,7 @@ void Transaction::releaseDependency(void
> INTERLOCKED_DECREMENT(dependencies);
>
> if ((dependencies == 0) && !writePending && firstRecord)
>- {
>- // The Sync is to avoid a race with writeComplete(). It
looks
>whacko, but does the trick
>-
>- Sync sync(&syncIndexes, "Transaction::releaseDependency");
>- sync.lock(Exclusive);
> commitRecords();
>- }
>-
> releaseCommittedTransaction();
> }
>
>@@ -1502,6 +1510,8 @@ void Transaction::printBlockage(void)
>
> void Transaction::releaseDeferredIndexes(void)
> {
>+ Sync sync(&syncIndexes, "Transaction::releaseDeferredIndexes");
>+ sync.lock(Exclusive);
> for (DeferredIndex *deferredIndex; (deferredIndex =
deferredIndexes);)
> {
> ASSERT(deferredIndex->transaction == this);
>@@ -1559,7 +1569,8 @@ void Transaction::backlogRecords(void)
> void Transaction::validateRecords(void)
> {
> RecordVersion *record;
>-
>+ Sync syncRec(&syncRecords,"Transaction::validateRecords");
>+ syncRec.lock(Shared);
> for (record = firstRecord; record && record->nextInTrans; record =
>record->nextInTrans)
> ;
>
>
>=== modified file 'storage/falcon/Transaction.h'
>--- a/storage/falcon/Transaction.h 2008-08-11 13:22:53 +0000
>+++ b/storage/falcon/Transaction.h 2008-08-22 20:33:40 +0000
>@@ -87,7 +87,8 @@ public:
>
> State getRelativeState(Record* record, uint32 flags);
> State getRelativeState (Transaction *transaction, TransId
>transId, uint32 flags);
>- void removeRecord (RecordVersion *record);
>+ void removeRecordNoLock (RecordVersion *record);
>+ void removeRecord(RecordVersion *record);
> void removeRecord (RecordVersion *record, RecordVersion
>**ptr);
> void expungeTransaction (Transaction *transaction);
> void commitRecords();
>@@ -171,6 +172,7 @@ public:
> SyncObject syncObject;
> SyncObject syncActive;
> SyncObject syncIndexes;
>+ SyncObject syncRecords;
> SyncObject syncSavepoints;
> uint64 totalRecordData; // total bytes of
record data
>for this transaction (unchilled + thawed)
> uint32 totalRecords; // total record
count
>
>
>--
>MySQL Code Commits Mailing List
>For list archives: http://lists.mysql.com/commits
>To unsubscribe: http://lists.mysql.com/commits?unsub=1
