Vlad,
It seems that releaseDeferredIndexes still has a few places where
the caller will lock syncIndexes also.
It is hard for me to tell from this unified diff, but we should be
sure that there are no new race conditions using the two SyncObjects
instead of one.
I like the code otherwise.
Kevin
>-----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
