#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
