bzr push into mysql-6.0-falcon branch (vvaintroub:2797 to 2798) Bug#38933

[Vladislav Vaintroub]

 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