MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Kevin Lewis Date:April 1 2009 7:39pm
Subject:bzr commit into mysql-6.0-falcon-team branch (kevin.lewis:3093)
Bug#43958
View as plain text  
#At file:///C:/Work/bzr/Merge/mysql-6.0-falcon-team/ based on revid:kevin.lewis@stripped

 3093 Kevin Lewis	2009-04-01
      Bug#43958 - An alternative solution to deleting the assert for the existence of the TransState object is to re-organize the calls to Transaction::waitForTransaction so that they ALWAYS use a TransactionState pointer.  This can be done because the only place where the transId is sent in by itself is in Table::validateAndInsert().  This function calls waitForTransaction when a new base record is found superceding the record that was known as the prior.  This new base record MUST be a RecordVersion, so it MUST have a TransactionState pointer.  So instead of sending the TransId, the patch now sends the TransactionState pointer.  This also allows us to eliminate a costly and obnoxious search for a Transaction based on a TransId.
      
      Note that Table::validateAndInsert() holds its own useCount on the TransactionState that it gets from the new pending base record, just in case that record is rolledBack.  Although, with the new CycleManager, that rolledBack record will not be deleted until validateAndInsert() is done.
      
      In addition to waitForTransaction, the various calls to getRelativeState are also changed to use TransactionState and not use TransId. waitForTransaction does not need to increment its own useCount on the TransactionState object sent in.  It either comes from validateAndInsert or getRelativeState. Table::validateAndInsert holds a useCount itself.  Most of the getRelativeState calls hold a useCount on the record associated with the TransactionState, which by extention, holds the useCount on the TransactionState.  An addRef is added to getrelativeState(Record*) so that waitForTransaction does not need to.

    modified:
      storage/falcon/Table.cpp
      storage/falcon/Transaction.cpp
      storage/falcon/Transaction.h
=== modified file 'storage/falcon/Table.cpp'
--- a/storage/falcon/Table.cpp	2009-04-01 18:29:00 +0000
+++ b/storage/falcon/Table.cpp	2009-04-01 19:38:45 +0000
@@ -2566,7 +2566,7 @@ bool Table::checkUniqueRecordVersion(int
 	Record *rec;
 	Record *oldRecord = record->getPriorVersion();
 	//Transaction *activeTransaction = NULL;
-	TransactionState *activeTransaction = NULL;
+	TransactionState *activeTransState = NULL;
 	State state = CommittedVisible;
 
 	if (oldRecord && recordNumber == oldRecord->recordNumber)
@@ -2614,8 +2614,8 @@ bool Table::checkUniqueRecordVersion(int
 					// No conflict with a visible deleted record.
 					rec->release(REC_HISTORY);
 					
-					if (activeTransaction)
-						activeTransaction->release();
+					if (activeTransState)
+						activeTransState->release();
 					
 					return false;	// Check next record number.
 
@@ -2631,8 +2631,8 @@ bool Table::checkUniqueRecordVersion(int
 					// Keep looking for a possible duplicate conflict,
 					// either visible, or pending at a savepoint.
 
-					activeTransaction = dup->getTransactionState();
-					activeTransaction->addRef();
+					activeTransState = dup->getTransactionState();
+					activeTransState->addRef();
 					
 					continue;
 
@@ -2667,24 +2667,23 @@ bool Table::checkUniqueRecordVersion(int
 					{
 					rec->release(REC_HISTORY);
 					
-					if (activeTransaction)
-						activeTransaction->release();
+					if (activeTransState)
+						activeTransState->release();
 					
 					return true;  // retry after a wait
 					}
 				}
 
-			else if (activeTransaction)
+			else if (activeTransState)
 				{
 				//syncPrior.unlock(); // release lock before wait
 				syncUnique->unlock(); // release lock before wait
 
-				state = transaction->getRelativeState(activeTransaction,
-						activeTransaction->transactionId, WAIT_IF_ACTIVE);
+				state = transaction->getRelativeState(activeTransState, WAIT_IF_ACTIVE);
 
 				if (state != Deadlock)
 					{
-					activeTransaction->release();
+					activeTransState->release();
 					rec->release(REC_HISTORY);
 
 					return true;  // retry after a wait
@@ -2695,8 +2694,8 @@ bool Table::checkUniqueRecordVersion(int
 
 			rec->release(REC_HISTORY);
 
-			if (activeTransaction)
-				activeTransaction->release();
+			if (activeTransState)
+				activeTransState->release();
 
 			const char *text = "duplicate values for key %s in table %s.%s";
 			int code = UNIQUE_DUPLICATE;
@@ -2724,12 +2723,12 @@ bool Table::checkUniqueRecordVersion(int
 			// Only wait on this record if the duplicate is visible or pending
 			// at a savepoint.
 
-			if (!activeTransaction)
+			if (!activeTransState)
 				{
-				activeTransaction = dup->getTransactionState();
+				activeTransState = dup->getTransactionState();
 				
-				if (activeTransaction)
-					activeTransaction->addRef();
+				if (activeTransState)
+					activeTransState->addRef();
 				}
 
 			continue;  // check next record version
@@ -2742,8 +2741,8 @@ bool Table::checkUniqueRecordVersion(int
 			{
 			rec->release(REC_HISTORY);
 			
-			if (activeTransaction)
-				activeTransaction->release();
+			if (activeTransState)
+				activeTransState->release();
 				
 			return false;	 // Check next record number.
 			}
@@ -2755,8 +2754,8 @@ bool Table::checkUniqueRecordVersion(int
 			{
 			rec->release(REC_HISTORY);
 			
-			if (activeTransaction)
-				activeTransaction->release();
+			if (activeTransState)
+				activeTransState->release();
 				
 			return false;	// Check next record number
 			}
@@ -2765,8 +2764,8 @@ bool Table::checkUniqueRecordVersion(int
 	if (rec)
 		rec->release(REC_HISTORY);
 		
-	if (activeTransaction)
-		activeTransaction->release();
+	if (activeTransState)
+		activeTransState->release();
 
 	return false;	// Check next record number
 }
@@ -3376,11 +3375,12 @@ void Table::validateAndInsert(Transactio
 					// an update conflict.  If not, wait on that trans and, if it is not
 					// committed, try again.
 
-					TransId transId = current->getTransactionId();
+					TransactionState *transState = current->getTransactionState();
+					transState->addRef();
 					current->release(REC_HISTORY);
 					syncTable.unlock();
 
-					if (transaction->waitForTransaction(transId))
+					if (transaction->waitForTransaction(transState))
 						{
 						current = fetch(record->recordNumber);
 						
@@ -3388,10 +3388,13 @@ void Table::validateAndInsert(Transactio
 							current->release(REC_HISTORY);
 						else
 							{
-							transaction->blockedBy = transId;
+							transaction->blockedBy = transState->transactionId;
+							transState->release();
 							throw SQLError(UPDATE_CONFLICT, "update conflict in table %s.%s record %d", schemaName, name, record->recordNumber);
 							}
 						}
+
+					transState->release();
 					}
 				}
 			}

=== modified file 'storage/falcon/Transaction.cpp'
--- a/storage/falcon/Transaction.cpp	2009-03-31 09:25:28 +0000
+++ b/storage/falcon/Transaction.cpp	2009-04-01 19:38:45 +0000
@@ -856,13 +856,19 @@ void Transaction::commitRecords()
 State Transaction::getRelativeState(Record* record, uint32 flags)
 {
 	// If this is a Record object it has no assosiated transaction
-	// and is always visible
+	// and is always visible.
 	
 	if (!record->isVersion())
 		return CommittedVisible;
 
+	// This RecordVersion MUST have a TransState with a reference count.
+	// The caller has a reference count on record, but we will add 
+	// another useCount to the transState just to be careful.
+
 	blockingRecord = record;
-	State state = getRelativeState(record->getTransactionState(), record->getTransactionId(), flags);
+	TransactionState * transactionState = record->getTransactionState();
+	ASSERT(transactionState);
+	State state = getRelativeState(transactionState, flags);
 	blockingRecord = NULL;
 
 	return state;
@@ -872,11 +878,9 @@ State Transaction::getRelativeState(Reco
 @brief		Get the relative state between this transaction and another.
 ***/
 
-State Transaction::getRelativeState(TransactionState* transState, TransId transId, uint32 flags)
+State Transaction::getRelativeState(TransactionState* transState, uint32 flags)
 {
-	// Note: The structure of this method is still based on the
-	// original getRelativeState() - should be carefully rewritten
-
+	TransId transId = transState->transactionId;
 	if (transactionId == transId)
 		return Us;
 
@@ -910,7 +914,7 @@ State Transaction::getRelativeState(Tran
 			return Active;
 
 		bool isDeadlock;
-		waitForTransaction(transState, 0 , &isDeadlock);
+		waitForTransaction(transState, &isDeadlock);
 
 		if (isDeadlock)
 			return Deadlock;
@@ -991,11 +995,11 @@ void Transaction::writeComplete(void)
 	writePending = false;
 }
 
-bool Transaction::waitForTransaction(TransId transId)
+bool Transaction::waitForTransaction(TransactionState *transState)
 {
 	bool deadlock;
-	State state = waitForTransaction(NULL, transId, &deadlock);
-	
+	State state = waitForTransaction(transState, &deadlock);
+
 	return (deadlock || state == Committed || state == Available);
 }
 
@@ -1012,8 +1016,7 @@ bool Transaction::waitForTransaction(Tra
 // use inline assembly or intrinsics to generate memory barrier instead of 
 // volatile. 
 
-State Transaction::waitForTransaction(TransactionState* transState, TransId transId,
-										bool *deadlock)
+State Transaction::waitForTransaction(TransactionState* transState, bool *deadlock)
 {
 	*deadlock = false;
 	State state;
@@ -1021,46 +1024,19 @@ State Transaction::waitForTransaction(Tr
 	// Increase the use count on the transaction state object to ensure
 	// the object the waitingFor pointer refers to does not get deleted
 
-	if (transState)
-		transState->addRef();
+	ASSERT(transState != NULL);
 
 	Sync syncActiveTransactions(&transactionManager->activeTransactions.syncObject,
 								"Transaction::waitForTransaction(1)");
 	syncActiveTransactions.lock(Shared);
 
-	// If a transaction state object is not given, locate it by searching
-	// for the transaction in the active list
-
-	if (!transState)
-		{
-		Transaction* transaction; 
-
-		// transaction parameter is not given, find transaction using its ID.
-		for (transaction = transactionManager->activeTransactions.first; transaction;
-			 transaction = transaction->next)
-			if (transaction->transactionId == transId)
-				break;
-
-		// If the transaction is not found in the active list it is committed
-
-		if (!transaction)
-			return Committed;
-
-		transState = transaction->transactionState;
-		transState->addRef();
-		}
-
-	ASSERT(transState != NULL);
-
 	if (transState->state == Available || transState->state == Committed)
 		{
 		state = (State) transState->state;
-		transState->release();
-		
+
 		return state;
 		}
 
-
 	if (!COMPARE_EXCHANGE_POINTER(&transactionState->waitingFor, NULL, transState))
 		FATAL("waitingFor was not NULL");
 
@@ -1085,7 +1061,7 @@ State Transaction::waitForTransaction(Tr
 			{
 			CycleLock *cycleLock = CycleLock::unlock();
 			transState->waitForTransaction();
-			
+
 			if (cycleLock)
 				cycleLock->lockCycle();
 			}
@@ -1097,8 +1073,6 @@ State Transaction::waitForTransaction(Tr
 			// See comments about this locking further down
 
 			syncActiveTransactions.lock(Exclusive);
-			transState->release();
-				
 			throw;
 			}
 		}
@@ -1106,21 +1080,16 @@ State Transaction::waitForTransaction(Tr
 	if (!COMPARE_EXCHANGE_POINTER(&transactionState->waitingFor, transState, NULL))
 		FATAL("waitingFor was not %p", transState);
 
-	// Before releasing the reference count on the transaction state object we
-	// need to aquire a exclusive lock on the active transaction list. The 
-	// cause for this is that another thread might have used this 
-	// transaction state object's waitingFor pointer to navigate to the next 
-	// transaction state object - and this thread might be the only one that 
-	// has a reference count on that transaction state object. So to avoid that 
-	// the transaction state object is released and deleted while another 
-	// thread is transversing the waitingFor list (code above) with a shared
-	// lock on it we lock it exclusively.
+	// Before returning we need to aquire an exclusive lock on 
+	// syncActiveTransactions. This acts as a 'pump' to make sure that
+	// no thread is still using the object that waitingFor was pointing to.
+	// Traversals of the waitingFor list are done with shared locks on
+	// syncActiveTransactions.  So this exclusive lock waits for those 
+	// threads to finish using those pointers.
 
 	syncActiveTransactions.lock(Exclusive);
 
 	state = (State) transState->state;
-	transState->release();
-
 	return state;
 }
 

=== modified file 'storage/falcon/Transaction.h'
--- a/storage/falcon/Transaction.h	2009-03-25 22:11:35 +0000
+++ b/storage/falcon/Transaction.h	2009-04-01 19:38:45 +0000
@@ -60,7 +60,7 @@ public:
 	Transaction(Connection *connection, TransId seq);
 
 	State		getRelativeState(Record* record, uint32 flags);
-	State		getRelativeState (TransactionState* ts, TransId transId, uint32 flags);
+	State		getRelativeState (TransactionState* ts, uint32 flags);
 	void		removeRecordNoLock (RecordVersion *record);
 	void		removeRecord(RecordVersion *record);
 	void		removeRecord (RecordVersion *record, RecordVersion **ptr);
@@ -74,9 +74,8 @@ public:
 	void		commit();
 	void		release();
 	void		addRef();
-	//void		waitForTransaction();
-	bool		waitForTransaction (TransId transId);
-	State		waitForTransaction (TransactionState* ts, TransId transId, bool *deadlock);
+	bool		waitForTransaction (TransactionState* ts);
+	State		waitForTransaction (TransactionState* ts, bool *deadlock);
 	void		dropTable(Table* table);
 	void		truncateTable(Table* table);
 	bool		hasRecords(Table* table);


Attachment: [text/bzr-bundle] bzr/kevin.lewis@sun.com-20090401193845-97k8cnd4d0zlzyll.bundle
Thread
bzr commit into mysql-6.0-falcon-team branch (kevin.lewis:3093)Bug#43958Kevin Lewis1 Apr
  • Re: bzr commit into mysql-6.0-falcon-team branch (kevin.lewis:3093)Bug#43958Olav Sandstaa2 Apr