Re: bzr commit into mysql-6.0-falcon-team branch (klewis:2912)

[Kevin Lewis]

Olave wrote;
------------------------
Hi Kevin,

I have read and tried to understand the patch. To me it seems correct 
and I also think it makes to code simpler and more readable.

My only comment is that you should update the following comment in:

  // If the record is locked or being unlocked keep looking for a dup.

in Table::checkUniqueRecordVersion.

If you plan to push the patch, let me know and I can respond to that I 
have reviewed it on the commit list.

Olav
----------------------

Thanks, Olav.  OK, I will push it.  I think I will also open a bug to 
track this, even though we do not yet know what effects this would have, 
and what other bugs it might fix.

Kevin

Kevin Lewis wrote:
> #At file:///C:/Work/bzr/Merge/mysql-6.0-falcon-team/
> 
>  2912 Kevin Lewis	2008-11-19
>       Do not use Record::state = recUnlock.  It is not 
>       recognized by many parts of the code and can 
>       cause the wrong behavior.
>       
>       Also, A lock record that is unlocked should stay 
>       on the transaction.  It is still replaced on the 
>       recordLeaf, but other threads with pointers to it 
>       from a concurrent Table::fetch need to be able to 
>       see the transaction.  It will be taken off the
>       transaction later with all the other records on 
>       the transaction.
> modified:
>   storage/falcon/Table.cpp
>   storage/falcon/Table.h
>   storage/falcon/Transaction.cpp
> 
> per-file messages:
>   storage/falcon/Table.cpp
>     Don't search for recUnlock.  It is no longer used.
>     The 'remove' parameter is no longer needed in
>     Table::unlockRecord() since lock records are no 
>     longer removed from the transaction.
>     Use superceded=true to keep from calling 
>     insert again if Table::unlockRecord() is called 
>     a second time.  This can now happen during a 
>     Transaction::rollback() after a rollbackSavepoint
>     since unlocked records now remain on the transaction.
>   storage/falcon/Table.h
>     The 'remove' parameter is no longer needed in
>     Table::unlockRecord() since lock records are no 
>     longer removed from the transaction.
>   storage/falcon/Transaction.cpp
>     The 'remove' parameter is no longer needed in
>     Table::unlockRecord() since lock records are no 
>     longer removed from the transaction.
>     
>     Transaction::releaseRecordLocks() needs to loop 
>     differently since the records are no longer
>     removed from the transaction.  They are removed 
>     later with all the other records.
> === modified file 'storage/falcon/Table.cpp'
> --- a/storage/falcon/Table.cpp	2008-10-24 05:35:38 +0000
> +++ b/storage/falcon/Table.cpp	2008-11-20 05:32:18 +0000
> @@ -2555,7 +2555,7 @@ bool Table::checkUniqueRecordVersion(int
>  			{
>  			// If the record is locked or being unlocked keep looking for a dup.
>  
> -			if ((dup->state == recLock) || (dup->state == recUnlocked))
> +			if (dup->state == recLock)
>  				continue;  // Next record version.
>  
>  			// The record has been deleted.
> @@ -3436,26 +3436,23 @@ void Table::unlockRecord(int recordNumbe
>  	if (record)
>  		{
>  		if (record->state == recLock)
> -			unlockRecord((RecordVersion*) record, true);
> +			unlockRecord((RecordVersion*) record);
>  		
>  		record->release();
>  		}
>  }
>  
> -void Table::unlockRecord(RecordVersion* record, bool remove)
> +void Table::unlockRecord(RecordVersion* record)
>  {
>  	//int uc = record->useCount;
>  	ASSERT(record->getPriorVersion());
> -	
> -	if (record->state == recLock)
> -		{
> -		record->state = recUnlocked;
>  
> +	// A lock record that has superceded=true is already unlocked
> +
> +	if ((record->state == recLock) && !record->isSuperceded())
> +		{
>  		if (insert(record->getPriorVersion(), record, record->recordNumber))
> -			{
> -			if (remove && record->transaction)
> -				record->transaction->removeRecord(record);
> -			}
> +			record->setSuperceded(true);
>  		else
>  			Log::debug("Table::unlockRecord: record lock not in record tree\n");
>  		}
> 
> === modified file 'storage/falcon/Table.h'
> --- a/storage/falcon/Table.h	2008-10-24 05:35:38 +0000
> +++ b/storage/falcon/Table.h	2008-11-20 05:32:18 +0000
> @@ -208,7 +208,7 @@ public:
>  	Record*			fetchForUpdate(Transaction* transaction, Record* record, bool
> usingIndex);
>  //	RecordVersion*	lockRecord(Record* record, Transaction* transaction);
>  	void			unlockRecord(int recordNumber);
> -	void			unlockRecord(RecordVersion* record, bool remove);
> +	void			unlockRecord(RecordVersion* record);
>  
>  	void			insert (Transaction *transaction, int count, Field **fields, Value
> **values);
>  	uint			insert (Transaction *transaction, Stream *stream);
> 
> === modified file 'storage/falcon/Transaction.cpp'
> --- a/storage/falcon/Transaction.cpp	2008-11-07 01:09:04 +0000
> +++ b/storage/falcon/Transaction.cpp	2008-11-20 05:32:18 +0000
> @@ -422,7 +422,7 @@ void Transaction::rollback()
>  		record->nextInTrans = NULL;
>  
>  		if (record->state == recLock)
> -			record->format->table->unlockRecord(record, false);
> +			record->format->table->unlockRecord(record);
>  		else
>  			record->rollback(this);
>  		
> @@ -1353,19 +1353,19 @@ bool Transaction::isXidEqual(int testLen
>  
>  void Transaction::releaseRecordLocks(void)
>  {
> -	RecordVersion **ptr;
> -	RecordVersion *record;
> -
>  	Sync syncRec(&syncRecords,"Transaction::releaseRecordLocks");
>  	syncRec.lock(Exclusive);
> -	for (ptr = &firstRecord; (record = *ptr);)
> +
> +	for (RecordVersion *record = firstRecord; record; record = record->nextInTrans)
>  		if (record->state == recLock)
>  			{
> -			record->format->table->unlockRecord(record, false);
> -			removeRecord(record);
> +			record->format->table->unlockRecord(record);
> +
> +			// Don't do  removeRecord(record); now.  Other threads might be 
> +			// pointing to it and need the transaction pointer to determine 
> +			// its relative state
>  			}
> -		else
> -			ptr = &record->nextInTrans;
> +
>  	syncRec.unlock();
>  }
>  
> 
>