bzr commit into mysql-6.0-falcon branch (vvaintroub:2772) Bug#22165

[Vladislav Vaintroub]

#At file:///C:/bzr/mysql-6.0-falcon-team/

 2772 Vladislav Vaintroub	2008-08-11
      Bug#22165 - crash while doing ALTER in parallel with another transactions
      Eliminated races.
      
      Race condition 1.
      ALTER/DROP/TRUNCATE have to ensure that table has no committed records
      that are not yet "write complete". Otherwise crash is possible if table is 
      deleted, but then accessed during Transaction::writeComplete() in Record::poke()
      
      It is now solved, Storage::external_lock will wait for all transaction on 
      the table  to become write complete, if there is no uncommited records.
      
      Race condition 2.
      In Transaction::commit(), Transaction state is set to Committed to early
      while the transaction is still in the active list. This can produce incorrect
      results in Transaction::hasUncommitedRecords, DROP is not blocked.
      When later Transaction::commit() traverses the record list, it can crash 
      accessing freed memory in Table::updateRecord().
      
      This is fixed and transaction state is changed during the transition from 
      active list to committed list.
modified:
  mysql-test/suite/falcon/t/disabled.def
  storage/falcon/Database.cpp
  storage/falcon/Database.h
  storage/falcon/StorageTable.cpp
  storage/falcon/StorageTable.h
  storage/falcon/Table.cpp
  storage/falcon/Table.h
  storage/falcon/Transaction.cpp
  storage/falcon/Transaction.h
  storage/falcon/TransactionManager.cpp
  storage/falcon/TransactionManager.h
  storage/falcon/ha_falcon.cpp

per-file messages:
  storage/falcon/Database.cpp
    new function waitForWriteComplete
  storage/falcon/Database.h
    new function waitForWriteComplete
  storage/falcon/StorageTable.cpp
    new function waitForWriteComplete
  storage/falcon/StorageTable.h
    new function waitForWriteComplete
  storage/falcon/Table.cpp
    new function waitForWriteComplete
  storage/falcon/Table.h
    new function waitForWriteComplete
  storage/falcon/Transaction.cpp
    Transaction state was set to Committed to early, allowing for DROP to process in
paralell
    moved state change to later point, when transaction is removed from active list and
added 
    to committed list.
    
    Also, removed unnecessary loops : record list is now traversed 
    only once in Transaction::commit() ,not 2 or 3 times
    
    Additionally, Transaction::hasRecords is now lock-protected to
    avoid races with writeComplete() and dropTable()
    
    Renamed hasUncommittedRecords() to hasRecords() - we're sometimes interested 
    in committed no-yet-write-complete transactions
  storage/falcon/Transaction.h
    hasUncommittedRecords->hasRecords
  storage/falcon/TransactionManager.cpp
    new function TransactionManager::waitForWriteComplete() to
    wait for all transactions referencing records from given
    table to become "write complete"
  storage/falcon/TransactionManager.h
    new function waitForWriteComplete()
  storage/falcon/ha_falcon.cpp
    in StorageInterface::external_lock, if there is no uncommitted records
    for the table, also wait while committed records will become write 
    complete. This is done to avoid race with Transaction::writeComplete()
=== modified file 'mysql-test/suite/falcon/t/disabled.def'
--- a/mysql-test/suite/falcon/t/disabled.def	2008-08-07 21:11:55 +0000
+++ b/mysql-test/suite/falcon/t/disabled.def	2008-08-11 13:22:53 +0000
@@ -12,4 +12,3 @@
 
 falcon_bug_28095_I  : Bug#xxxxx 2008-04-22 hakank Disabled until soft restart is in main
tree
 falcon_bug_28095_II : Bug#xxxxx 2008-03-22 hakank Disabled until soft restart is in main
tree
-falcon_bug_22165 : Bug#22165 2008-08-07 wlad Disabled temporarily, because it crashes.
Work in progress

=== modified file 'storage/falcon/Database.cpp'
--- a/storage/falcon/Database.cpp	2008-07-17 13:52:17 +0000
+++ b/storage/falcon/Database.cpp	2008-08-11 13:22:53 +0000
@@ -2282,6 +2282,11 @@ bool Database::hasUncommittedRecords(Tab
 	return transactionManager->hasUncommittedRecords(table, transaction);
 }
 
+void Database::waitForWriteComplete(Table *table)
+{
+	transactionManager->waitForWriteComplete(table);
+}
+
 void Database::commitByXid(int xidLength, const UCHAR* xid)
 {
 	serialLog->commitByXid(xidLength, xid);

=== modified file 'storage/falcon/Database.h'
--- a/storage/falcon/Database.h	2008-05-09 19:58:50 +0000
+++ b/storage/falcon/Database.h	2008-08-11 13:22:53 +0000
@@ -200,6 +200,7 @@ public:
 	virtual void	createDatabase (const char *filename);
 	void			renameTable(Table* table, const char* newSchema, const char* newName);
 	bool			hasUncommittedRecords(Table* table, Transaction *transaction);
+	void			waitForWriteComplete(Table *table);
 	void			validateCache(void);
 	int				recoverGetNextLimbo(int xidSize, unsigned char *xid);
 	void			commitByXid(int xidLength, const UCHAR* xid);

=== modified file 'storage/falcon/StorageTable.cpp'
--- a/storage/falcon/StorageTable.cpp	2008-08-04 15:53:52 +0000
+++ b/storage/falcon/StorageTable.cpp	2008-08-11 13:22:53 +0000
@@ -565,6 +565,11 @@ int StorageTable::alterCheck(void)
 	return 0;
 }
 
+void StorageTable::waitForWriteComplete(void)
+{
+	share->table->waitForWriteComplete();
+}
+
 void StorageTable::unlockRow(void)
 {
 	if (recordLocked)

=== modified file 'storage/falcon/StorageTable.h'
--- a/storage/falcon/StorageTable.h	2008-08-04 15:53:52 +0000
+++ b/storage/falcon/StorageTable.h	2008-08-11 13:22:53 +0000
@@ -61,6 +61,7 @@ public:
 	void			transactionEnded(void);
 	void			setRecord(Record* record, bool locked);
 	int				alterCheck(void);
+	void			waitForWriteComplete();
 	void			clearAlter(void);
 	bool			setAlter(void);
 	

=== modified file 'storage/falcon/Table.cpp'
--- a/storage/falcon/Table.cpp	2008-08-07 21:11:55 +0000
+++ b/storage/falcon/Table.cpp	2008-08-11 13:22:53 +0000
@@ -3331,6 +3331,11 @@ bool Table::hasUncommittedRecords(Transa
 	return database->hasUncommittedRecords(this, transaction);
 }
 
+void Table::waitForWriteComplete()
+{
+	return database->waitForWriteComplete(this);
+}
+
 RecordVersion* Table::lockRecord(Record* record, Transaction* transaction)
 {
 	Record *current = fetch(record->recordNumber);

=== modified file 'storage/falcon/Table.h'
--- a/storage/falcon/Table.h	2008-04-22 21:10:23 +0000
+++ b/storage/falcon/Table.h	2008-08-11 13:22:53 +0000
@@ -185,6 +185,7 @@ public:
 	int			getFormatVersion();
 	void		validateAndInsert(Transaction *transaction, RecordVersion *record);
 	bool		hasUncommittedRecords(Transaction* transaction);
+	void		waitForWriteComplete();
 	void		checkAncestor(Record* current, Record* oldRecord);
 	int64		estimateCardinality(void);
 	void		optimize(Connection *connection);

=== modified file 'storage/falcon/Transaction.cpp'
--- a/storage/falcon/Transaction.cpp	2008-07-31 10:04:30 +0000
+++ b/storage/falcon/Transaction.cpp	2008-08-11 13:22:53 +0000
@@ -275,38 +275,43 @@ void Transaction::commit()
 		releaseRecordLocks();
 
 	database->serialLog->preCommit(this);
-	state = Committed;
 	syncActive.unlock();
 
+	
+
 	for (RecordVersion *record = firstRecord; record; record = record->nextInTrans)
-		if (!record->isSuperceded() && record->state != recLock)
-			record->format->table->updateRecord (record);
+	{
+		Table * table = record->format->table;
 
-	if (commitTriggers)
-		for (RecordVersion *record = firstRecord; record; record = record->nextInTrans)
-			if (!record->isSuperceded() && record->state != recLock)
-				record->format->table->postCommit (this, record);
+		if (!record->isSuperceded() && record->state != recLock)
+			{
+			table->updateRecord (record);
+			if (commitTriggers)
+				table->postCommit (this, record);
+			}
 
+		if (!record->getPriorVersion())
+			++table->cardinality;
+		if (record->state == recDeleted && table->cardinality > 0)
+			--table->cardinality;
+	}
+	
 	releaseDependencies();
 	database->flushInversion(this);
 
+	// Transfer transaction from active list to committed list, set committed state
 	Sync syncCommitted(&transactionManager->committedTransactions.syncObject,
"Transaction::commit(2)");
-	syncCommitted.lock(Exclusive);
-
 	Sync syncActiveTransactions(&transactionManager->activeTransactions.syncObject,
"Transaction::commit(3)");
+	syncCommitted.lock(Exclusive);
 	syncActiveTransactions.lock(Exclusive);
+
 	transactionManager->activeTransactions.remove(this);
-	syncActiveTransactions.unlock();
-	
-	for (RecordVersion *record = firstRecord; record; record = record->nextInTrans)
-		{
-		if (!record->getPriorVersion())
-			++record->format->table->cardinality;
-		if (record->state == recDeleted &&
record->format->table->cardinality > 0)
-			--record->format->table->cardinality;
-		}
 	transactionManager->committedTransactions.append(this);
+	state = Committed;
+
+	syncActiveTransactions.unlock();
 	syncCommitted.unlock();
+
 	database->commit(this);
 
 	delete [] xid;
@@ -925,8 +930,11 @@ void Transaction::truncateTable(Table* t
 			ptr = &rec->nextInTrans;
 }
 
-bool Transaction::hasUncommittedRecords(Table* table)
+bool Transaction::hasRecords(Table* table)
 {
+	// This lock is to avoid race with writeComplete
+	Sync sync(&syncIndexes, "Transaction::releaseDependency");
+	sync.lock(Exclusive);
 	for (RecordVersion *rec = firstRecord; rec; rec = rec->nextInTrans)
 		if (rec->format->table == table)
 			return true;

=== modified file 'storage/falcon/Transaction.h'
--- a/storage/falcon/Transaction.h	2008-07-24 08:45:03 +0000
+++ b/storage/falcon/Transaction.h	2008-08-11 13:22:53 +0000
@@ -105,7 +105,7 @@ public:
 	State		waitForTransaction (Transaction *transaction, TransId transId, bool *deadlock);
 	void		dropTable(Table* table);
 	void		truncateTable(Table* table);
-	bool		hasUncommittedRecords(Table* table);
+	bool		hasRecords(Table* table);
 	void		writeComplete(void);
 	void		releaseDependency(void);
 	int			createSavepoint();

=== modified file 'storage/falcon/TransactionManager.cpp'
--- a/storage/falcon/TransactionManager.cpp	2008-07-24 08:45:03 +0000
+++ b/storage/falcon/TransactionManager.cpp	2008-08-11 13:22:53 +0000
@@ -27,6 +27,7 @@
 #include "Log.h"
 #include "LogLock.h"
 #include "Synchronize.h"
+#include "Thread.h"
 
 static const int EXTRA_TRANSACTIONS = 10;
 
@@ -168,12 +169,40 @@ bool TransactionManager::hasUncommittedR
 	syncTrans.lock (Shared);
 	
 	for (Transaction *trans = activeTransactions.first; trans; trans = trans->next)
-		if (trans != transaction && trans->isActive() &&
trans->hasUncommittedRecords(table))
+		if (trans != transaction && trans->isActive() &&
trans->hasRecords(table))
 			return true;
 
 	return false;
 }
 
+// Wait until all committed records for a table are purged by gophers
+// (their transaction become write complete)
+void TransactionManager::waitForWriteComplete(Table* table)
+{
+	for(;;)
+		{
+		bool again = false;
+		Sync committedTrans (&committedTransactions.syncObject,
+			"waitForWriteComplete");
+		committedTrans.lock (Shared);
+
+		for (Transaction *trans = committedTransactions.first; trans; 
+			 trans = trans->next)
+			{
+				if (trans->hasRecords(table)&& trans->writePending)
+				{
+				again = true;
+				break;
+				}
+			}
+
+		if(!again)
+			return;
+
+		committedTrans.unlock();
+		Thread::getThread("waitForWriteComplete")->sleep(500);
+		}
+}
 void TransactionManager::commitByXid(int xidLength, const UCHAR* xid)
 {
 	Sync sync (&activeTransactions.syncObject, "TransactionManager::commitByXid");

=== modified file 'storage/falcon/TransactionManager.h'
--- a/storage/falcon/TransactionManager.h	2008-04-24 17:26:34 +0000
+++ b/storage/falcon/TransactionManager.h	2008-08-11 13:22:53 +0000
@@ -35,6 +35,7 @@ public:
 	void			dropTable(Table* table, Transaction* transaction);
 	void			truncateTable(Table* table, Transaction* transaction);
 	bool			hasUncommittedRecords(Table* table, Transaction* transaction);
+	void			waitForWriteComplete(Table *table);
 	void			commitByXid(int xidLength, const UCHAR* xid);
 	void			rollbackByXid(int xidLength, const UCHAR* xid);
 	void			print(void);

=== modified file 'storage/falcon/ha_falcon.cpp'
--- a/storage/falcon/ha_falcon.cpp	2008-08-04 15:53:52 +0000
+++ b/storage/falcon/ha_falcon.cpp	2008-08-11 13:22:53 +0000
@@ -1893,6 +1893,7 @@ int StorageInterface::external_lock(THD 
 					DBUG_RETURN(error(ret));
 					}
 				}
+				storageTable->waitForWriteComplete();
 				break;
 			default:
 				break;