bzr commit into mysql-6.0-falcon branch (klewis:2756)

[Kevin Lewis]

#At file:///C:/Work/bzr/Merge/mysql-6.0-falcon/

 2756 Kevin Lewis	2008-07-25
      Made sure all functions that lock both committedTransactions.syncObject
      and activeTransactions.syncObject at the same time will lock
      committedTransactions first.  
      
      Note that findOldestActive used to lock activeTransactions.syncObject
      before calling findOldest, where it was locked again.
      
      Note also that validateDependencies used to lock committedTransactions
      when it should have locked activeTransactions.  But this function 
      does not currently have a caller.
modified:
  storage/falcon/TransactionManager.cpp

per-file messages:
  storage/falcon/TransactionManager.cpp
    Made sure all functions that lock both committedTransactions.syncObject
    and activeTransactions.syncObject at the same time will lock
    committedTransactions first.  
    
    Note that findOldestActive used to lock activeTransactions.syncObject
    before calling findOldest, where it was locked again.
    
    Note also that validateDependencies used to lock committedTransactions
    when it should have locked activeTransactions.  But this function 
    does not currently have a caller.
=== modified file 'storage/falcon/TransactionManager.cpp'
--- a/storage/falcon/TransactionManager.cpp	2008-07-24 08:45:03 +0000
+++ b/storage/falcon/TransactionManager.cpp	2008-07-25 22:09:18 +0000
@@ -71,28 +71,27 @@ TransactionManager::~TransactionManager(
 
 TransId TransactionManager::findOldestActive()
 {
+	Transaction *oldest = findOldest();
+
 	Sync syncCommitted(&committedTransactions.syncObject, "TransactionManager::findOldestActive(1)");
 	syncCommitted.lock(Shared);
-	TransId oldestActive = transactionSequence;
+	TransId oldestCommitted = transactionSequence;
 	
 	for (Transaction *trans = committedTransactions.first; trans; trans = trans->next)
-		oldestActive = MIN(trans->transactionId, oldestActive);
+		oldestCommitted = MIN(trans->transactionId, oldestCommitted);
 
 	syncCommitted.unlock();
-	Sync sync(&activeTransactions.syncObject, "TransactionManager::findOldestActive(2)");
-	sync.lock(Shared);
-	Transaction *oldest = findOldest();
 
 	if (oldest)
 		{
-		//Log::debug("Oldest transaction %d, oldest ancestor %d, oldest committed %d\n",  oldest->transactionId, oldest->oldestActive, oldestActive);
-					
-		return MIN(oldest->oldestActive, oldestActive);
+		//Log::debug("Oldest transaction %d, oldest ancestor %d, oldest committed %d\n",  oldest->transactionId, oldest->oldestActive, oldestCommitted);
+
+		return MIN(oldest->oldestActive, oldestCommitted);
 		}
 	
 	//Log::debug("No active, current %d, oldest committed %d\n", transactionSequence, oldestActive);
 	
-	return oldestActive;
+	return oldestCommitted;
 }
 
 Transaction* TransactionManager::findOldest(void)
@@ -218,16 +217,20 @@ void TransactionManager::rollbackByXid(i
 
 void TransactionManager::print(void)
 {
-	Sync sync (&activeTransactions.syncObject, "TransactionManager::print(1)");
-	sync.lock (Exclusive);
-	Sync committedTrans (&committedTransactions.syncObject, "TransactionManager::print(2)");
-	committedTrans.lock (Exclusive);
+	Sync SyncCommitted (&committedTransactions.syncObject, "TransactionManager::print(2)");
+	SyncCommitted.lock (Exclusive);
+
+	Sync syncActive (&activeTransactions.syncObject, "TransactionManager::print(1)");
+	syncActive.lock (Exclusive);
+
 	Transaction *transaction;
 	Log::debug("Active Transaction:\n");
 	
 	for (transaction = activeTransactions.first; transaction; transaction = transaction->next)
 		transaction->print();
-		
+
+	syncActive.unlock();
+
 	Log::debug("Committed Transaction:\n");
 	
 	for (transaction = committedTransactions.first; transaction; transaction = transaction->next)
@@ -237,15 +240,19 @@ void TransactionManager::print(void)
 
 void TransactionManager::getTransactionInfo(InfoTable* infoTable)
 {
-	Sync sync (&activeTransactions.syncObject, "TransactionManager::getTransactionInfo(1)");
-	sync.lock (Exclusive);
-	Sync committedTrans (&committedTransactions.syncObject, "TransactionManager::getTransactionInfo(2)");
-	committedTrans.lock (Exclusive);
+	Sync SyncCommitted (&committedTransactions.syncObject, "TransactionManager::getTransactionInfo(1)");
+	SyncCommitted.lock (Exclusive);
+
+	Sync syncActive (&activeTransactions.syncObject, "TransactionManager::getTransactionInfo(2)");
+	syncActive.lock (Exclusive);
+
 	Transaction *transaction;
 	
 	for (transaction = activeTransactions.first; transaction; transaction = transaction->next)
 		transaction->getInfo(infoTable);
-	
+
+	syncActive.unlock();
+
 	for (transaction = committedTransactions.first; transaction; transaction = transaction->next)
 		transaction->getInfo(infoTable);
 }
@@ -278,10 +285,10 @@ void TransactionManager::purgeTransactio
 
 void TransactionManager::getSummaryInfo(InfoTable* infoTable)
 {
-	Sync sync (&activeTransactions.syncObject, "TransactionManager::getSummaryInfo(1)");
-	sync.lock (Exclusive);
-	Sync committedTrans (&committedTransactions.syncObject, "TransactionManager::getSummaryInfo(2)");
-	committedTrans.lock (Exclusive);
+	Sync syncCommitted (&committedTransactions.syncObject, "TransactionManager::getSummaryInfo(1)");
+	syncCommitted.lock (Exclusive);
+	Sync syncActive (&activeTransactions.syncObject, "TransactionManager::getSummaryInfo(2)");
+	syncActive.lock (Exclusive);
 	int numberCommitted = committed;
 	int numberRolledBack = rolledBack;
 	int numberActive = 0;
@@ -298,14 +305,14 @@ void TransactionManager::getSummaryInfo(
 		if (transaction->state == Committed)
 			++numberPendingCommit;
 		}
+	syncActive.unlock();
 
 	for (transaction = committedTransactions.first; transaction; transaction = transaction->next)
 		if (transaction->writePending)
 			++numberPendingCompletion;
 	
-	committedTrans.unlock();
-	sync.unlock();
-	
+	syncCommitted.unlock();
+
 	int n = 0;
 	infoTable->putInt(n++, numberCommitted);
 	infoTable->putInt(n++, numberRolledBack);
@@ -373,15 +380,16 @@ void TransactionManager::expungeTransact
 
 Transaction* TransactionManager::findTransaction(TransId transactionId)
 {
-	Sync syncActiveTrans(&activeTransactions.syncObject, "TransactionManager::findTransaction(1)");
-	syncActiveTrans.lock(Shared);
+	Sync syncActive(&activeTransactions.syncObject, "TransactionManager::findTransaction(1)");
+	syncActive.lock(Shared);
 	Transaction *transaction;
 
 	for (transaction = activeTransactions.first; transaction; transaction = transaction->next)
 		if (transaction->transactionId == transactionId)
 			return transaction;
 	
-	syncActiveTrans.unlock();
+	syncActive.unlock();
+
 	Sync syncCommitted(&committedTransactions.syncObject, "TransactionManager::findTransaction(2)");
 	syncCommitted.lock(Shared);
 
@@ -389,20 +397,21 @@ Transaction* TransactionManager::findTra
 		if (transaction->transactionId == transactionId)
 			return transaction;
 	
-	return NULL;	
+	return NULL;
 }
 
 void TransactionManager::validateDependencies(void)
 {
-	Sync sync(&committedTransactions.syncObject, "TransactionManager::validateDepedendencies(1)");
-	sync.lock(Shared);
+	Sync syncActive(&activeTransactions.syncObject, "TransactionManager::validateDepedendencies(1)");
+	syncActive.lock(Shared);
 	Transaction *transaction;
 
 	for (transaction = activeTransactions.first; transaction; transaction = transaction->next)
 		if (transaction->isActive())
 			transaction->validateDependencies(false);
 			
-	sync.unlock();
+	syncActive.unlock();
+
 	Sync syncCommitted(&committedTransactions.syncObject, "TransactionManager::validateDepedendencies(2)");
 	syncCommitted.lock(Shared);