bzr commit into mysql-6.0-falcon-team branch (kevin.lewis:3105) Bug#41661

[Kevin Lewis]

#At file:///C:/Work/bzr/Merge/mysql-6.0-falcon-team/ based on revid:vvaintroub@stripped

 3105 Kevin Lewis	2009-04-07
      Bug#41661 - Table::insertIntoTree could fail during a Table::insert.  This should not happen since 
      Dbb::insertStub should have returned a record number that was free.  Nobody else should have 
      been messing with that slot in the RecordLeaf.
      
      The problem was in RecordLeaf::retireRecords.  This function called RecordVersion::retire which 
      then called expungeRecord. This makes the record number free to use again by any other thread.
      It did this with an exclusive lock on the RecordLeaf, but Table::insertIntoTree often does not get 
      any lock on RecordLeaf when it calls RecordLeaf::store.  So a free record number is reused in 
      Table::insert before the slot has been set to NULL by RecordLeaf::retireRecords.  That is why 
      Table::insertIntoTree fails.  It expects the slot to be NULL.
      
      Another problem fixed is that the first of the two Table::insert functions did not check the 
      return value of insertIntoTree at all.  This would have propogated the error further since 
      there would be nothing in the RecordTree slot after the insert.
      
      Another problem fixed in RecordLeaf::retireRecords is with this statement;
          if (record && recordScavenge->canBeRetired(record))
      The problem is with incrementing the 'count' integer.  If it is zero after stepping through the 
      RecordLeaf, then it is assumed that the RecordLeaf is empty and can be retired.
      But this 'if' statement gets the wrong result.  If the RecordLeaf is full of newer records that
      cannot yet be retired, then this count will remain zero.  The emptySections bitmap will be 
      set for this RecordLeaf. And later, in Table::retireRecords, it will attempt to retire these 
      sections.  But finding records in them, it will just waste a lot of time.  
      
      I changed this code to be accurate in determining if the RecordLeaf is indead empty 
      before setting the bit in Table::emptySections.
      
      Record::retire and RecordVersion::retire now only retire records.  The more thorough 
      tests for whether a record can be retired is collected into RecordScavenge::canBeRetired.
      
      Also, instead of just setting the slot in RecordLeaf to NULL directly, I now use a 
      COMPARE_EXCHANGE_POINTER() just like every other place where this location is changed.
      Then if it fails for any reason, we will not retire that record.  One legitimate reason that this 
      could fail is when this record is determined that it canBeRetired, but immediately after that, 
      a client thread does a fetchForUpdate, adding a lockRecord.
      
      
      Since the 'count' integer is incremented only within it, a RecordLeaf that does not have any

    modified:
      storage/falcon/Record.cpp
      storage/falcon/Record.h
      storage/falcon/RecordLeaf.cpp
      storage/falcon/RecordScavenge.cpp
      storage/falcon/RecordVersion.cpp
      storage/falcon/RecordVersion.h
      storage/falcon/Table.cpp
=== modified file 'storage/falcon/Record.cpp'
--- a/storage/falcon/Record.cpp	2009-03-27 04:29:33 +0000
+++ b/storage/falcon/Record.cpp	2009-04-07 14:31:04 +0000
@@ -497,23 +497,12 @@ bool Record::isVersion()
 	return false;
 }
 
-bool Record::retire(RecordScavenge *recordScavenge)
+void Record::retire(void)
 {
-	if (generation <= recordScavenge->scavengeGeneration)
-		{
-		recordScavenge->spaceRetired += getMemUsage();
-		++recordScavenge->recordsRetired;
-		SET_THIS_RECORD_ACTIVE(false);
-		RECORD_HISTORY(this);
-
-		release();
-		return true;
-		}
+	SET_THIS_RECORD_ACTIVE(false);
+	RECORD_HISTORY(this);
 
-	++recordScavenge->recordsRemaining;
-	recordScavenge->spaceRemaining += getMemUsage();
-
-	return false;
+	release();
 }
 
 void Record::scavenge(TransId targetTransactionId, int oldestActiveSavePointId)

=== modified file 'storage/falcon/Record.h'
--- a/storage/falcon/Record.h	2009-03-27 04:29:33 +0000
+++ b/storage/falcon/Record.h	2009-04-07 14:31:04 +0000
@@ -112,7 +112,7 @@ public:
 	virtual void		setSuperceded (bool flag);
 	virtual Record*		fetchVersion (Transaction * transaction);
 	virtual Record*		fetchVersionRecursive (Transaction *transaction);
-	virtual bool		retire(RecordScavenge *recordScavenge);
+	virtual void		retire(void);
 	virtual void		scavenge(TransId targetTransactionId, int oldestActiveSavePointId);
 	virtual bool		isVersion();
 	virtual bool		isSuperceded();

=== modified file 'storage/falcon/RecordLeaf.cpp'
--- a/storage/falcon/RecordLeaf.cpp	2009-03-20 19:28:10 +0000
+++ b/storage/falcon/RecordLeaf.cpp	2009-04-07 14:31:04 +0000
@@ -169,7 +169,7 @@ void RecordLeaf::pruneRecords (Table *ta
 
 void RecordLeaf::retireRecords (Table *table, int base, RecordScavenge *recordScavenge)
 {
-	int count = 0;
+	int slotsWithRecords = 0;
 	Record **ptr, **end;
 
 	Sync sync(&syncObject, "RecordLeaf::retireRecords(syncObject)");
@@ -198,19 +198,28 @@ void RecordLeaf::retireRecords (Table *t
 		{
 		Record *record = *ptr;
 		
-		if (record && recordScavenge->canBeRetired(record))
+		if (record)
 			{
-			if (record->retire(recordScavenge))
-				*ptr = NULL;		// This is like Table::insertIntoTree(NULL, ...)
+			if (   (recordScavenge->canBeRetired(record))
+				&& (COMPARE_EXCHANGE_POINTER(ptr, record, NULL)))
+				{
+				record->retire();
+				++recordScavenge->recordsRetired;
+				recordScavenge->spaceRetired += record->getMemUsage();
+				}
 			else
-				count++;
+				{
+				slotsWithRecords++;
+				++recordScavenge->recordsRemaining;
+				recordScavenge->spaceRemaining += record->getMemUsage();
+				}
 			}
 		}
 
 	// If this node is empty, store the base record number for use as an
 	// identifier when the leaf node is scavenged later.
 
-	if (!count && table->emptySections)
+	if ((slotsWithRecords == 0) && (table->emptySections))
 		table->emptySections->set(base);
 
 	return;

=== modified file 'storage/falcon/RecordScavenge.cpp'
--- a/storage/falcon/RecordScavenge.cpp	2009-03-27 13:43:10 +0000
+++ b/storage/falcon/RecordScavenge.cpp	2009-04-07 14:31:04 +0000
@@ -79,18 +79,21 @@ bool RecordScavenge::canBeRetired(Record
 
 	if (record->generation <= scavengeGeneration)
 		{
-		// Record objects are read from pages
+		// Record objects that are old enough can always be retired
 
 		if (!record->isVersion())
 			return true;
 
+		RecordVersion * recVer = (RecordVersion *) record;
+		TransactionState * transState = recVer->transactionState;
+
 		// This record version may be retired if it is
 		// currently not pointed to by a transaction.
 
-		RecordVersion * recVer = (RecordVersion *) record;
-		
-		//if (!recVer->transaction)
-		if (!recVer->transactionState->hasTransactionReference)
+		if (   recVer->useCount == 1
+			&& !recVer->getPriorVersion()
+			&& transState->committedBefore(oldestActiveTransaction)
+			&& (!transState->hasTransactionReference))
 			return true;
 		}
 

=== modified file 'storage/falcon/RecordVersion.cpp'
--- a/storage/falcon/RecordVersion.cpp	2009-03-27 15:16:56 +0000
+++ b/storage/falcon/RecordVersion.cpp	2009-04-07 14:31:04 +0000
@@ -243,36 +243,22 @@ bool RecordVersion::committedBefore(Tran
 
 // This is called with an exclusive lock on the recordLeaf
 
-bool RecordVersion::retire(RecordScavenge *recordScavenge)
+void RecordVersion::retire(void)
 {
-	bool neededByAnyActiveTrans = true;
-	
-	if (transactionState->committedBefore(recordScavenge->oldestActiveTransaction))
-		neededByAnyActiveTrans = false;
-
-	if (   generation <= recordScavenge->scavengeGeneration
-		&& useCount == 1
-		&& !priorVersion
-		&& !neededByAnyActiveTrans)
-		{
-		recordScavenge->recordsRetired++;
-		recordScavenge->spaceRetired += getMemUsage();
-		SET_THIS_RECORD_ACTIVE(false);
+	SET_THIS_RECORD_ACTIVE(false);
+	RECORD_HISTORY(this);
 
-		if (state == recDeleted)
-			expungeRecord();  // Allow this record number to be reused
+	if (state == recDeleted)
+		expungeRecord();  // Allow this record number to be reused
 
-		release();
+	release();
 
-		return true;
-		}
-
-	return false;
 }
 
 // Scavenge record versions replaced within a savepoint.
+// this record is staying and any prior records at 
+// the same savepoint are leaving
 
-//void RecordVersion::scavengeSavepoint(TransId targetTransactionId, int oldestActiveSavePointId)
 void RecordVersion::scavengeSavepoint(Transaction* targetTransaction, int oldestActiveSavePointId)
 {
 	if (!priorVersion)
@@ -313,12 +299,10 @@ void RecordVersion::scavengeSavepoint(Tr
 	Record *prior = priorVersion;
 	prior->addRef();
 
-	//syncPrior.unlock();
-	//syncPrior.lock(Exclusive);
+	// Set this record's priorVersion to point past the leaving record(s)
 
 	setPriorVersion(rec);
 	ptr->state = recEndChain;
-	//format->table->garbageCollect(prior, this, transaction, false);
 	format->table->garbageCollect(prior, this, targetTransaction, false);
 	prior->queueForDelete();
 }

=== modified file 'storage/falcon/RecordVersion.h'
--- a/storage/falcon/RecordVersion.h	2009-03-27 04:29:33 +0000
+++ b/storage/falcon/RecordVersion.h	2009-04-07 14:31:04 +0000
@@ -44,7 +44,7 @@ public:
 	virtual void		setSuperceded (bool flag);
 	virtual Record*		getPriorVersion();
 	virtual Record*		getGCPriorVersion(void);
-	virtual bool		retire(RecordScavenge *recordScavenge);
+	virtual void		retire(void);
 	virtual void		scavengeSavepoint(Transaction* targetTransaction, int oldestActiveSavePoint);
 	virtual bool		isVersion();
 	virtual void		rollback(Transaction *transaction);

=== modified file 'storage/falcon/Table.cpp'
--- a/storage/falcon/Table.cpp	2009-04-01 19:38:45 +0000
+++ b/storage/falcon/Table.cpp	2009-04-07 14:31:04 +0000
@@ -338,7 +338,8 @@ void Table::insert(Transaction *transact
 		findSections();
 
 	RecordVersion *record = NULL;
-	bool inserted = false;
+	bool insertedIntoTree = false;
+	bool addedToTransaction = false;
 	int32 recordNumber = -1;
 
 	try
@@ -390,9 +391,17 @@ void Table::insert(Transaction *transact
 
 		checkNullable(record);  // Verify that record is valid
 
+		if (insertIntoTree(record, NULL, recordNumber))
+			insertedIntoTree = true;
+		else 
+			{
+			bool cannotInsertIntoTree_A = false;
+			ASSERT(cannotInsertIntoTree_A);
+			}
+
 		transaction->addRecord(record);
-		insertIntoTree(record, NULL, recordNumber);
-		inserted = true;
+		addedToTransaction = true;
+
 		insertIndexes(transaction, record);
 		updateInversion(record, transaction);
 		fireTriggers(transaction, PostInsert, NULL, record);
@@ -401,11 +410,15 @@ void Table::insert(Transaction *transact
 		}
 	catch (...)
 		{
-		if (inserted)
-			{
+		if (insertedIntoTree)
+			if (!insertIntoTree(NULL, record, recordNumber))
+				{
+				bool cannotBackoutInsertIntoTree_A = false;
+				ASSERT(cannotBackoutInsertIntoTree_A);
+				}
+
+		if (addedToTransaction)
 			transaction->removeRecord(record);
-			insertIntoTree(NULL, record, recordNumber);
-			}
 
 		if (recordNumber >= 0)
 			{
@@ -3065,7 +3078,8 @@ uint Table::insert(Transaction *transact
 {
 	database->preUpdate();
 	RecordVersion *record = NULL;
-	bool inserted = false;
+	bool insertedIntoTree = false;
+	bool addedToTransaction = false;
 	int32 recordNumber = -1;
 
 	if (!dataSection)
@@ -3090,21 +3104,32 @@ uint Table::insert(Transaction *transact
 
 		// Do the actual insert
 
+		if (insertIntoTree(record, NULL, recordNumber))
+			insertedIntoTree = true;
+		else
+			{
+			bool cannotInsertIntoTree_A = false;
+			ASSERT(cannotInsertIntoTree_A);
+			}
+
 		transaction->addRecord(record);
-		bool ret = insertIntoTree(record, NULL, recordNumber);
-		inserted = true;
+		addedToTransaction = true;
+
 		insertIndexes(transaction, record);
-		ASSERT(ret);
 		record->state = recData;
 		record->release(REC_HISTORY);
 		}
 	catch (...)
 		{
-		if (inserted)
-			{
+		if (insertedIntoTree)
+			if (!insertIntoTree(NULL, record, recordNumber))
+				{
+				bool cannotBackoutInsertIntoTree_B = false;
+				ASSERT(cannotBackoutInsertIntoTree_B);
+				}
+
+		if (addedToTransaction)
 			transaction->removeRecord(record);
-			insertIntoTree(NULL, record, recordNumber);
-			}
 
 		if (recordNumber >= 0)
 			{


Attachment: [text/bzr-bundle] bzr/[e-mail]