2798 Vladislav Vaintroub 2008-08-25
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,except Transaction destructor, where there should be nobody
accessing the list concurrently. When the list is freed, firstRecord is
set to NULL to prevent crash in parallel Transaction::hasRecords().
modified:
storage/falcon/Transaction.cpp
storage/falcon/Transaction.h
2797 Vladislav Vaintroub 2008-08-22 [merge]
merge 6.0-falcon -> 6.0-falcon-team
removed:
mysql-test/include/wait_timeout_basic.inc
mysql-test/suite/rpl/t/rpl_row_err_daisychain-master.opt
mysql-test/suite/rpl/t/rpl_row_err_daisychain-slave.opt
mysql-test/suite/rpl_ndb/t/rpl_truncate_7ndb_2-master.opt
added:
mysql-test/include/have_nodebug.inc
mysql-test/r/have_nodebug.require
mysql-test/r/innodb-autoinc-optimize.result
mysql-test/r/sp-no-code.result
mysql-test/r/subselect_nulls.result
mysql-test/suite/sys_vars/inc/multi_range_count_basic.inc
mysql-test/suite/sys_vars/r/multi_range_count_basic_32.result
mysql-test/suite/sys_vars/r/multi_range_count_basic_64.result
mysql-test/suite/sys_vars/t/multi_range_count_basic_32.test
mysql-test/suite/sys_vars/t/multi_range_count_basic_64.test
mysql-test/t/innodb-autoinc-optimize.test
mysql-test/t/sp-no-code.test
mysql-test/t/subselect_nulls.test
modified:
client/Makefile.am
client/mysql_upgrade.c
client/mysqltest.c
config/ac-macros/character_sets.m4
configure.in
extra/comp_err.c
extra/perror.c
mysql-test/extra/binlog_tests/binlog_insert_delayed.test
mysql-test/extra/rpl_tests/rpl_row_basic.test
mysql-test/include/show_binlog_events.inc
mysql-test/r/constraints.result
mysql-test/r/func_if.result
mysql-test/r/perror.result
mysql-test/r/sp-error.result
mysql-test/r/subselect.result
mysql-test/r/subselect_no_mat.result
mysql-test/r/subselect_no_opts.result
mysql-test/r/subselect_no_semijoin.result
mysql-test/r/subselect_sj.result
mysql-test/r/subselect_sj2.result
mysql-test/suite/binlog/r/binlog_killed_simulate.result
mysql-test/suite/binlog/r/binlog_row_binlog.result
mysql-test/suite/binlog/r/binlog_row_mix_innodb_myisam.result
mysql-test/suite/binlog/r/binlog_statement_insert_delayed.result
mysql-test/suite/binlog/r/binlog_stm_binlog.result
mysql-test/suite/binlog/r/binlog_stm_mix_innodb_myisam.result
mysql-test/suite/funcs_1/r/falcon_storedproc.result
mysql-test/suite/funcs_1/r/falcon_trig_03.result
mysql-test/suite/funcs_1/r/falcon_trig_03e.result
mysql-test/suite/funcs_1/r/innodb_storedproc.result
mysql-test/suite/funcs_1/r/innodb_trig_03.result
mysql-test/suite/funcs_1/r/innodb_trig_03e.result
mysql-test/suite/funcs_1/r/is_columns_mysql.result
mysql-test/suite/funcs_1/r/is_user_privileges.result
mysql-test/suite/funcs_1/r/memory_storedproc.result
mysql-test/suite/funcs_1/r/memory_trig_03.result
mysql-test/suite/funcs_1/r/memory_trig_03e.result
mysql-test/suite/funcs_1/r/myisam_storedproc.result
mysql-test/suite/funcs_1/r/myisam_trig_03.result
mysql-test/suite/funcs_1/r/myisam_trig_03e.result
mysql-test/suite/funcs_1/r/ndb_storedproc.result
mysql-test/suite/funcs_1/r/ndb_trig_03.result
mysql-test/suite/funcs_1/r/ndb_trig_03e.result
mysql-test/suite/funcs_1/triggers/triggers_03.inc
mysql-test/suite/funcs_1/triggers/triggers_08.inc
mysql-test/suite/funcs_1/triggers/triggers_1011ext.inc
mysql-test/suite/ndb/r/ndb_basic.result
mysql-test/suite/ndb/t/disabled.def
mysql-test/suite/rpl/r/rpl_loaddata_map.result
mysql-test/suite/rpl/r/rpl_row_basic_2myisam.result
mysql-test/suite/rpl/r/rpl_row_basic_3innodb.result
mysql-test/suite/rpl/r/rpl_stm_log.result
mysql-test/suite/rpl/r/rpl_variables.result
mysql-test/suite/rpl/t/disabled.def
mysql-test/suite/rpl/t/rpl_incident.test
mysql-test/suite/rpl/t/rpl_loaddata_map.test
mysql-test/suite/rpl/t/rpl_trunc_temp.test
mysql-test/suite/rpl/t/rpl_variables.test
mysql-test/suite/rpl_ndb/t/rpl_truncate_7ndb_2.test
mysql-test/suite/rpl_ndb_big/r/rpl_row_basic_7ndb.result
mysql-test/t/constraints.test
mysql-test/t/disabled.def
mysql-test/t/func_if.test
mysql-test/t/perror.test
mysql-test/t/sp-error.test
mysql-test/t/subselect.test
mysql-test/t/subselect_sj.test
mysql-test/t/subselect_sj2.test
scripts/mysql_system_tables_fix.sql
sql/field.h
sql/handler.cc
sql/handler.h
sql/item.cc
sql/item.h
sql/item_cmpfunc.cc
sql/item_func.cc
sql/mysql_priv.h
sql/mysqld.cc
sql/share/errmsg.txt
sql/sp_head.cc
sql/sp_pcontext.cc
sql/sql_acl.cc
sql/sql_base.cc
sql/sql_binlog.cc
sql/sql_cursor.cc
sql/sql_error.cc
sql/sql_error.h
sql/sql_lex.h
sql/sql_list.h
sql/sql_parse.cc
sql/sql_select.cc
sql/sql_select.h
sql/sql_string.h
sql/sql_yacc.yy
sql/structs.h
sql/table.cc
sql/table.h
sql/thr_malloc.cc
support-files/build-tags
tests/mysql_client_test.c
=== 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-25 18:24:59 +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;
+ // We modify record list without locking.
+ // It is a destructor and if somebody accesses the list
+ // at this point, he is already lost.
for (RecordVersion *record; (record = firstRecord);)
{
- removeRecord(record);
+ removeRecordNoLock(record);
}
+ firstRecord = NULL;
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))
@@ -908,13 +914,10 @@ State Transaction::getRelativeState(Tran
void Transaction::dropTable(Table* table)
{
- Sync sync(&syncIndexes, "Transaction::dropTable");
- sync.lock(Exclusive);
-
releaseDeferredIndexes(table);
- // 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);
@@ -924,13 +927,9 @@ void Transaction::dropTable(Table* table
void Transaction::truncateTable(Table* table)
{
- Sync sync(&syncIndexes, "Transaction::truncateTable");
- sync.lock(Exclusive);
-
releaseDeferredIndexes(table);
-
- // 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 +939,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;
@@ -954,17 +952,12 @@ void Transaction::writeComplete(void)
{
ASSERT(writePending);
ASSERT(state == Committed);
- Sync sync(&syncIndexes, "Transaction::writeComplete");
- sync.lock(Exclusive);
releaseDeferredIndexes();
-
- // Keep the synIndexes lock to avoid a race condition with dropTable
if (dependencies == 0)
commitRecords();
writePending = false;
- sync.unlock();
}
bool Transaction::waitForTransaction(TransId transId)
@@ -1248,6 +1241,7 @@ void Transaction::rollbackSavepoint(int
savePoint = savePoints;
+
while (savePoint)
{
//validateRecords();
@@ -1256,6 +1250,8 @@ void Transaction::rollbackSavepoint(int
break;
// Purge out records from this savepoint
+ Sync syncRec(&syncRecords,"Transaction::rollbackSavepoint(2)");
+ syncRec.lock(Exclusive);
RecordVersion *record = *savePoint->records;
RecordVersion *stack = NULL;
@@ -1295,6 +1291,7 @@ void Transaction::rollbackSavepoint(int
rec->transaction = NULL;
rec->release();
}
+ syncRec.unlock();
// Handle any backlogged records
@@ -1340,6 +1337,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 +1347,7 @@ void Transaction::releaseRecordLocks(voi
}
else
ptr = &record->nextInTrans;
+ syncRec.unlock();
}
void Transaction::print(void)
@@ -1365,6 +1365,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 +1414,7 @@ void Transaction::printBlocking(int leve
record->recordNumber,
what);
}
-
+ syncRec.unlock();
database->transactionManager->printBlocking(this, level);
}
@@ -1450,14 +1452,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 +1497,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 +1556,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-25 18:24:59 +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
| Thread |
|---|
| • bzr push into mysql-6.0-falcon branch (vvaintroub:2797 to 2798) Bug#38933 | Vladislav Vaintroub | 25 Aug |
