bk commit into 6.0 tree (klewis:1.2580) BUG#34351

[klewis]

Below is the list of changes that have just been committed into a local
6.0 repository of klewis.  When klewis does a push these changes
will be propagated to the main repository and, within 24 hours after the
push, to the public repository.
For information on how to access the public repository
see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html

ChangeSet@stripped, 2008-02-28 12:19:19-06:00, klewis@klewis-mysql. +5 -0
  Bug#34351 - A new function, Transaction::needToUpdate, is used to
  determine if Table::fetchForUpdate() should attempt to lock the 
  record.  This function looks for ANY visible record version.  It 
  is able to avoid a lock attempt for ConsistentRead transactions
  that start before a transaction that inserts a record. It also 
  avoids locks then the committed version fo the record is deleted.
  
  Also, this changeset delets the Table::syncUpdate object which
  had no effect. In the process, Table::checkUniqueIndexes and 
  its two subfunctions are redesigned to loop when a wait occurs 
  instead of looping in the caller.

  storage/falcon/StorageDatabase.cpp@stripped, 2008-02-28 12:18:23-06:00, klewis@klewis-mysql.
+3 -5
    StorageDatabase::nextRow() is used to sequentially 
    traverse the table by record number. Previously, it would 
    increment the record number being searched on by only 1 when 
    there was a gap in the record slot due to recently deleted records.  
    This is not necessary because Table::fetchNext skips these gaps 
    and returns the next possible candidate.  In order to avoid 
    calling Table::fetchNext multiple times for each record number 
    in a gap of deleted record numbers, it needs to increment based 
    on the record number of the previous candidate.

  storage/falcon/Table.cpp@stripped, 2008-02-28 12:18:32-06:00, klewis@klewis-mysql. +73 -83
    I noticed once while debugging, that the Table::records pointer
    was NULL in the table destructor.  It is not very likely,
    but we might as well protect against deleting a null pointer.
    
    While investigating bug #33184, I discovered that the 
    Table::syncUpdate object was not really being used because it was
    only locked in shared mode.  It was intended to make 
    checkUniqueIndexes atomic so that concurrent threads do not insert 
    the same unuique key at the same time.  That may be evidenced by 
    Bug #33498.  When Table::syncUpdate was esed with exclusive locks
    it caused a performance problem.  So the problem it was intended 
    fix must be prevented another way.  Table::syncUpdate is deleted 
    here. In the process, checkUniqueIndexes and its two subfunctions
    are redesigned to loop when a wait occurs instead of looping in the 
    caller.  
    
    Bug#34351 - A new function, Transaction::needToUpdate, is used to
    determine if Table::fetchForUpdate() should attempt to lock the record.

  storage/falcon/Table.h@stripped, 2008-02-28 12:18:40-06:00, klewis@klewis-mysql. +3 -7
    Table::checkUniqueIndexes and its two subfunctions are redesigned
    to loop when a wait occurs instead of looping in the caller.  
    They now return true if a wait occurs, false if no duplicate was 
    found, and they throw an exception on error

  storage/falcon/Transaction.cpp@stripped, 2008-02-28 12:18:47-06:00, klewis@klewis-mysql. +30
-2
    Bug#34351 - A new function, Transaction::needToUpdate, is used to
    determine if Table::fetchForUpdate() should attempt to lock the 
    record.  This function looks for ANY visible record version.  It 
    is able to avoid a lock attempt for ConsistentRead transactions
    that start before a transaction that inserts a record. It also 
    avoids locks then the committed version fo the record is deleted.

  storage/falcon/Transaction.h@stripped, 2008-02-28 12:18:52-06:00, klewis@klewis-mysql. +1 -0
    Bug#34351 - A new function, Transaction::needToUpdate, is used to
    determine if Table::fetchForUpdate() should attempt to lock the 
    record.

diff -Nrup a/storage/falcon/StorageDatabase.cpp b/storage/falcon/StorageDatabase.cpp
--- a/storage/falcon/StorageDatabase.cpp	2008-02-20 15:16:03 -06:00
+++ b/storage/falcon/StorageDatabase.cpp	2008-02-28 12:18:23 -06:00
@@ -233,16 +233,13 @@ int StorageDatabase::nextRow(StorageTabl
 	Table *table = storageTable->share->table;
 	Transaction *transaction = connection->getTransaction();
 	Record *candidate;
-	Record *record;
+	Record *record = NULL;
 	
 	try
 		{
-		for (;; ++recordNumber)
+		for (;;)
 			{
-			record = NULL;
-			candidate = NULL;
 			candidate = table->fetchNext(recordNumber);
-			
 			if (!candidate)
 				return StorageErrorRecordNotFound;
 
@@ -255,6 +252,7 @@ int StorageDatabase::nextRow(StorageTabl
 				if (!lockForUpdate)
 					candidate->release();
 					
+				recordNumber = candidate->recordNumber + 1;
 				continue;
 				}
 			
diff -Nrup a/storage/falcon/Table.cpp b/storage/falcon/Table.cpp
--- a/storage/falcon/Table.cpp	2008-02-28 09:27:36 -06:00
+++ b/storage/falcon/Table.cpp	2008-02-28 12:18:32 -06:00
@@ -143,7 +143,8 @@ Table::~Table()
 
 	delete view;
 	delete backloggedRecords;
-	delete records;
+	if (records)
+		delete records;
 
 	if (recordBitmap)
 		recordBitmap->release();
@@ -342,9 +343,7 @@ void Table::insert(Transaction *transact
 		
 		if (indexes)
 			{
-			do
-				sync.lock(ATOMIC_UPDATE);
-			while (!checkUniqueIndexes(transaction, record, &sync));
+			checkUniqueIndexes(transaction, record);
 
 			FOR_INDEXES(index, this);
 				index->insert(record, transaction);
@@ -358,9 +357,6 @@ void Table::insert(Transaction *transact
 		insert(record, NULL, recordNumber);
 		inserted = true;
 		
-		if (indexes)
-			sync.unlock();
-			
 		updateInversion(record, transaction);
 		fireTriggers(transaction, PostInsert, NULL, record);
 		record->release();
@@ -1178,13 +1174,9 @@ void Table::update(Transaction * transac
 		
 		// Make insert/update atomic, then check for unique index duplicats
 
-		Sync sync(&syncUpdate, "Table::update");
-		
 		if (indexes)
 			{
-			do
-				sync.lock(ATOMIC_UPDATE);
-			while (!checkUniqueIndexes(transaction, record, &sync));
+			checkUniqueIndexes(transaction, record);
 
 			FOR_INDEXES(index, this);
 				index->update(oldRecord, record, transaction);
@@ -1196,9 +1188,6 @@ void Table::update(Transaction * transac
 		transaction->addRecord(record);
 		updated = true;
 
-		if (indexes)
-			sync.unlock();
-			
 		updateInversion(record, transaction);
 		fireTriggers(transaction, PostUpdate, oldRecord, record);
 
@@ -1839,7 +1828,7 @@ bool Table::insert(Record * record, Reco
 	if (record)
 		record->release();
 	
-	return false;	
+	return false;
 }
 
 void Table::expungeRecordVersions(RecordVersion *record, RecordScavenge *recordScavenge)
@@ -2341,37 +2330,46 @@ bool Table::isDuplicate(Index *index, Re
 @brief		Determine if the record we intend to write will have a duplicate conflict
 			with any pending or visible records.
 @details	For each index, call checkUniqueIndex.  
-			Return true if the search succeeded by not finding a duplicate.
-			Return false if a wait occurred and the caller neeeds to re-lock the sync object
-			and try again.  If a duplicate is found release the sync and throw an exception.
+			Return if the search succeeded by not finding a duplicate.
+			Retry if a wait occurred.
+			If a duplicate is found, an exception should be caught by the caller.
 **/
 
-bool Table::checkUniqueIndexes(Transaction *transaction, RecordVersion *record, Sync
*sync)
+void Table::checkUniqueIndexes(Transaction *transaction, RecordVersion *record)
 {
 	Record *oldRecord = record->priorVersion;
+	bool retry = true;
 
-	FOR_INDEXES(index, this);
-		if (INDEX_IS_UNIQUE(index->type) &&
-			(!oldRecord || index->changed(record, oldRecord)))
+	while (retry)
+		{
+		retry = false;
+		FOR_INDEXES(index, this);
 			{
-			bool noConflict = checkUniqueIndex(index, transaction, record, sync);
-			
-			if (!noConflict)
-				return false;
+			if (INDEX_IS_UNIQUE(index->type) &&
+				(!oldRecord || index->changed(record, oldRecord)))
+				{
+				retry = checkUniqueIndex(index, transaction, record);
+				if (retry)
+					break;
+				}
+			if (retry)
+				break;
 			}
-	END_FOR;
-	
-	return true;
+		END_FOR;
+		}
+
+	return;
 }
 
 /**
 @brief		Determine if the record we intend to write will have a duplicate conflict
 			with any pending or visible records within a single index.
 @details	For each record number found in a scanIndex, call checkUniqueRecordVersion.
-			Return same as checkUniqueIndexes.
+			Return true if a wait occured.
+			Return false if no duplicate was found.
 **/
 
-bool Table::checkUniqueIndex(Index *index, Transaction *transaction, RecordVersion
*record, Sync *sync)
+bool Table::checkUniqueIndex(Index *index, Transaction *transaction, RecordVersion
*record)
 {
 	Bitmap bitmap;
 	IndexKey indexKey(index);
@@ -2380,27 +2378,25 @@ bool Table::checkUniqueIndex(Index *inde
 
 	for (int32 recordNumber = 0; (recordNumber = bitmap.nextSet(recordNumber)) >= 0;
++recordNumber)
 		{
-		int rc = checkUniqueRecordVersion(recordNumber, index, transaction, record, sync);
+		int retry = checkUniqueRecordVersion(recordNumber, index, transaction, record);
 		
-		if (rc == checkUniqueWaited)
-			return false;  // restart the search with a new lock
-			
-		if (rc == checkUniqueIsDone)
-			return true;  // No need to search any more record versions.
-		// else rc == checkUniqueNext
+		if (retry)
+			return true;  // restart the search since a wait occurred.
 		}
 
-	return true; // Did not find a duplicate
+	return false; // Did not find a duplicate in this index
 }
 
 /**
 @brief		Determine if the record we intend to write will have a duplicate conflict
 			with any pending or visible recordVersions for a single index and record Number.
 @details	Search through the record version , call checkUniqueRecordVersion.
-			Return same as checkUniqueIndexes.
+			Return true if a wait occured.
+			Return false if no duplicate was found.
+			Throw an exception if a duplicate WAS found
 **/
 
-int Table::checkUniqueRecordVersion(int32 recordNumber, Index *index, Transaction
*transaction, RecordVersion *record, Sync *sync)
+bool Table::checkUniqueRecordVersion(int32 recordNumber, Index *index, Transaction
*transaction, RecordVersion *record)
 {
 	Record *rec;
 	Record *oldRecord = record->priorVersion;
@@ -2408,7 +2404,7 @@ int Table::checkUniqueRecordVersion(int3
 	State state = CommittedVisible;
 
 	if (oldRecord && recordNumber == oldRecord->recordNumber)
-		return checkUniqueNext;	 // Check next record number.
+		return false;	 // Check next record number.
 
 	// This flag is used to skip all records in the chain between the 
 	// first younger committed record and the first older committed record.
@@ -2416,7 +2412,7 @@ int Table::checkUniqueRecordVersion(int3
 	bool foundFirstCommitted = false;
 
 	if ( !(rec = fetch(recordNumber)) )
-		return checkUniqueNext;	 // Check next record number.
+		return false;	 // Check next record number.
 
 	for (Record *dup = rec; dup; dup = dup->getPriorVersion())
 		{
@@ -2452,7 +2448,7 @@ int Table::checkUniqueRecordVersion(int3
 					if (activeTransaction)
 						activeTransaction->release();
 						
-					return checkUniqueNext;	// Check next record number.
+					return false;	// Check next record number.
 
 				case CommittedInvisible:
 					// This state only happens for consistent read
@@ -2491,9 +2487,6 @@ int Table::checkUniqueRecordVersion(int3
 				{
 				// wait for that transaction, then restart checkUniqueIndexes()
 
-				if (sync)
-					sync->unlock();
-				
 				state = transaction->getRelativeState(dup, WAIT_IF_ACTIVE);
 
 				if (state != Deadlock)
@@ -2503,15 +2496,12 @@ int Table::checkUniqueRecordVersion(int3
 					if (activeTransaction)
 						activeTransaction->release();
 					
-					return checkUniqueWaited;
+					return true;  // retry after a wait
 					}
 				}
 
 			else if (activeTransaction)
 				{
-				if (sync)
-					sync->unlock();
-				
 				state = transaction->getRelativeState(activeTransaction,
 						activeTransaction->transactionId, WAIT_IF_ACTIVE);
 
@@ -2520,7 +2510,7 @@ int Table::checkUniqueRecordVersion(int3
 					activeTransaction->release();
 					rec->release();
 
-					return checkUniqueWaited;
+					return true;  // retry after a wait
 					}
 				}
 
@@ -2576,7 +2566,7 @@ int Table::checkUniqueRecordVersion(int3
 			if (activeTransaction)
 				activeTransaction->release();
 				
-			return checkUniqueNext;	 // Check next record number.
+			return false;	 // Check next record number.
 			}
 
 		if (state == CommittedInvisible)
@@ -2589,7 +2579,7 @@ int Table::checkUniqueRecordVersion(int3
 			if (activeTransaction)
 				activeTransaction->release();
 				
-			return checkUniqueNext;	// Check next record number
+			return false;	// Check next record number
 			}
 		}	// for each record version...
 
@@ -2599,7 +2589,7 @@ int Table::checkUniqueRecordVersion(int3
 	if (activeTransaction)
 		activeTransaction->release();
 
-	return checkUniqueNext;
+	return false;	// Check next record number
 }
 
 bool Table::dropForeignKey(int fieldCount, Field **fields, Table *references)
@@ -2913,25 +2903,20 @@ uint Table::insert(Transaction *transact
 		
 		if (indexes)
 			{
-			do
-				sync.lock(ATOMIC_UPDATE);
-			while (!checkUniqueIndexes(transaction, record, &sync));
+			checkUniqueIndexes(transaction, record);
 
 			FOR_INDEXES(index, this);
 				index->insert (record, transaction);
 			END_FOR;
 			}
 
-		// Do actual insert
-		
+		// Do the actual insert
+
 		transaction->addRecord(record);
 		bool ret = insert(record, NULL, recordNumber);
 		ASSERT(ret);
 		inserted = true;
-		
-		if (indexes)
-			sync.unlock();
-			
+
 		record->release();
 		}
 	catch (...)
@@ -3040,13 +3025,9 @@ void Table::update(Transaction * transac
 
 		// Make insert/update atomic, then check for unique index duplicats
 
-		Sync sync(&syncUpdate, "Table::update");
-		
 		if (indexes)
 			{
-			do
-				sync.lock(ATOMIC_UPDATE);
-			while (!checkUniqueIndexes(transaction, record, &sync));
+			checkUniqueIndexes(transaction, record);
 
 			FOR_INDEXES(index, this);
 				index->update(oldRecord, record, transaction);
@@ -3063,19 +3044,18 @@ void Table::update(Transaction * transac
 			validateAndInsert(transaction, record);
 			transaction->addRecord(record);
 			}
-			
-		updated = true;
 
+		updated = true;
 		//fireTriggers(transaction, PostUpdate, oldRecord, record);
 
 		// If this is a re-update in the same transaction and the same savepoint,
 		// carefully remove the prior version.
-		
+
 		record->scavenge(transaction->transactionId, transaction->curSavePointId);
-		
+
 		if (record)
 			record->release();
-			
+
 		oldRecord->release();	// This reference originated in this function.
 		}
 	catch (...)
@@ -3390,12 +3370,19 @@ Record* Table::fetchForUpdate(Transactio
 		return prior;
 		}
 
-	Sync sync(&syncObject, "Table::fetchForUpdate");
-	
-	// We need to lock the record
-		
 	for (;;)
 		{
+		// Try to avoid getting a lock if there is no way we will be updating this record.
+
+		if (!transaction->needToLock(record))
+			{
+			record->release();
+
+			return NULL;
+			}
+
+		// We may need to lock the record
+
 		State state = transaction->getRelativeState(record, WAIT_IF_ACTIVE);
 
 		switch (state)
@@ -3405,7 +3392,8 @@ Record* Table::fetchForUpdate(Transactio
 
 				ASSERT(IS_CONSISTENT_READ(transaction->isolationLevel));
 				record->release();
-				Log::debug("Table::fetchForUpdate: update conflict in table %s.%s", schemaName,
name);
+				Log::debug("Table::fetchForUpdate: Update Conflict: TransId=%d, RecordNumber=%d,
Table %s.%s", 
+					transaction->transactionId, record->recordNumber, schemaName, name);
 				throw SQLError(UPDATE_CONFLICT, "update conflict in table %s.%s", schemaName, name);
 
 			case CommittedVisible:
@@ -3413,15 +3401,18 @@ Record* Table::fetchForUpdate(Transactio
 				if (record->state == recDeleted)
 					{
 					record->release();
-					
+
 					return NULL;
 					}
 
 				// Lock the record
 
+				if (dbb->debug & DEBUG_RECORD_LOCKS)
+					Log::debug("Table::fetchForUpdate: TransactionId=%d, isolationLevel=%d,
recordNumber=%d\n", 
+					           transaction->transactionId, transaction->isolationLevel,
recordNumber);
+
 				RecordVersion *recordVersion = allocRecordVersion(NULL, transaction, record);
 				recordVersion->state = recLock;
-				//sync.lock(Exclusive);
 				
 				if (insert(recordVersion, record, recordNumber))
 					{
@@ -3436,7 +3427,6 @@ Record* Table::fetchForUpdate(Transactio
 					return record;
 					}
 		
-				//sync.unlock();
 				recordVersion->active = false;
 				recordVersion->release();
 				}
diff -Nrup a/storage/falcon/Table.h b/storage/falcon/Table.h
--- a/storage/falcon/Table.h	2008-02-15 16:29:49 -06:00
+++ b/storage/falcon/Table.h	2008-02-28 12:18:40 -06:00
@@ -40,10 +40,6 @@ static const int PostDelete = 32;
 static const int PreCommit	= 64;
 static const int PostCommit	= 128;
 
-static const int checkUniqueWaited	= 0;
-static const int checkUniqueIsDone	= 1;
-static const int checkUniqueNext	= 2;
-
 #define FORMAT_HASH_SIZE		20
 #define FOR_FIELDS(field,table)	{for (Field *field=table->fields; field; field =
field->next){
 #define FOR_INDEXES(index,table)	{for (Index *index=table->indexes; index; index =
index->next){
@@ -103,9 +99,9 @@ public:
 	bool		foreignKeyMember (ForeignKey *key);
 	void		makeNotSearchable (Field *field, Transaction *transaction);
 	bool		dropForeignKey (int fieldCount, Field **fields, Table *references);
-	bool		checkUniqueIndexes (Transaction *transaction, RecordVersion *record, Sync *sync);
-	bool		checkUniqueIndex(Index *index, Transaction *transaction, RecordVersion *record,
Sync *sync);
-	int			checkUniqueRecordVersion(int32 recordNumber, Index *index, Transaction
*transaction, RecordVersion *record, Sync *sync);
+	void		checkUniqueIndexes (Transaction *transaction, RecordVersion *record);
+	bool		checkUniqueIndex(Index *index, Transaction *transaction, RecordVersion *record);
+	bool		checkUniqueRecordVersion(int32 recordNumber, Index *index, Transaction
*transaction, RecordVersion *record);
 	bool		isDuplicate (Index *index, Record *record1, Record *record2);
 	void		checkDrop();
 	Field*		findField (const WCString *fieldName);
diff -Nrup a/storage/falcon/Transaction.cpp b/storage/falcon/Transaction.cpp
--- a/storage/falcon/Transaction.cpp	2008-02-15 16:43:54 -06:00
+++ b/storage/falcon/Transaction.cpp	2008-02-28 12:18:47 -06:00
@@ -363,7 +363,7 @@ void Transaction::commitNoUpdates(void)
 			thread = Thread::getThread("Transaction::commitNoUpdates");
 		}
 	
-	ASSERT(dependencies == 0);	
+	ASSERT(dependencies == 0);
 	sync.unlock();
 	delete [] xid;
 	xid = NULL;
@@ -720,6 +720,34 @@ bool Transaction::visible(Transaction * 
 	return true;
 }
 
+/***
+@brief		Determine if there is a need to lock this record for update.
+***/
+
+bool Transaction::needToLock(Record* record)
+{
+	// Find the first visible record version
+
+	for (Record* candidate = record; 
+		 candidate != NULL;
+		 candidate = candidate->getPriorVersion())
+		{
+		Transaction *transaction = candidate->getTransaction();
+		TransId transId = candidate->getTransactionId();
+
+		if (visible(transaction, transId, FOR_WRITING))
+			if (candidate->state == recDeleted)
+				if (!transaction || transaction->state == Committed)
+					return FALSE; // Committed and deleted
+				else
+					return TRUE; // Just in case this rolls back.
+			else
+				return TRUE;
+		}
+
+	return FALSE;
+}
+
 void Transaction::releaseDependencies()
 {
 	if (!numberStates)
@@ -1101,7 +1129,7 @@ void Transaction::rollbackSavepoint(int 
 	if (systemTransaction)
 		sync.lock(Exclusive);
 
-	// Be sure the target savepoint is valid before rollong them back.
+	// Be sure the target savepoint is valid before rolling them back.
 	
 	for (savePoint = savePoints; savePoint; savePoint = savePoint->next)
 		if (savePoint->id <= savePointId)
diff -Nrup a/storage/falcon/Transaction.h b/storage/falcon/Transaction.h
--- a/storage/falcon/Transaction.h	2008-02-15 16:32:11 -06:00
+++ b/storage/falcon/Transaction.h	2008-02-28 12:18:52 -06:00
@@ -94,6 +94,7 @@ public:
 	void		commitRecords();
 	void		releaseDependencies();
 	bool		visible (Transaction *transaction, TransId transId, int forWhat);
+	bool		needToLock(Record* record);
 	void		addRecord (RecordVersion *record);
 	void		prepare(int xidLength, const UCHAR *xid);
 	void		rollback();