Kevin,
The patch looks fine and I think your changes make the code simpler to
read by reading the data.record once and also by doing the explicit
checks for the special recordversion/states directly instead of having
the conversion of these hidden in Record::findRecordData().
OK to push.
Olav
Kevin Lewis wrote:
> #At file:///C:/Work/bzr/Merge/mysql-6.0-falcon-team/ based on
> revid:kevin.lewis@stripped
>
> 2739 Kevin Lewis 2009-06-23
> Bug#45665 - Change the tests so that the Record::data.record is read once
> and tested multiple times instead of calling Record functions to test it
> multiple times in Table::checkUniqueRecordVersion().
> @ storage/falcon/Record.cpp
> Bug#45665 - findRecordData is simplified to an inline.
> isAllocated(void) and isRecordDataFreed() are not used.
> @ storage/falcon/Record.h
> Bug#45665 - findRecordData is simplified to an inline.
> isAllocated(void) and isRecordDataFreed() are not used.
> @ storage/falcon/RecordVersion.cpp
> Bug#45665 - findRecordData is simplified to an inline.
> @ storage/falcon/RecordVersion.h
> Bug#45665 - findRecordData is simplified to an inline.
> @ storage/falcon/SRLUpdateRecords.cpp
> Bug#45665 - Rename dataRecord to RecordData for consistency.
> SRLUpdateRecords::thaw() uses Record::findRecordData() to get
> Record::data.record. But it needs to return null if the record
> is not thawed. Record::findRecordData() is simplified to return
> exactly what is there so converting 'special' pointer values to
> null is not done here.
> @ storage/falcon/Table.cpp
> Bug#45665 - Change the tests so that the Record::data.record is read once
> and tested multiple times instead of calling Record functions to test it
> multiple times in Table::checkUniqueRecordVersion().
> @ storage/falcon/Transaction.cpp
> Bug#45665 - Use isRolledBack() in an assert at the bottom of
> Transaction::getRelativeState().
> At this point, the transaction can be no other.
> @ storage/falcon/TransactionState.h
> Use isRolledBack() in an assert at the bottom of
> Transaction::getRelativeState().
>
> modified:
> storage/falcon/Record.cpp
> storage/falcon/Record.h
> storage/falcon/RecordVersion.cpp
> storage/falcon/RecordVersion.h
> storage/falcon/SRLUpdateRecords.cpp
> storage/falcon/Table.cpp
> storage/falcon/Transaction.cpp
> storage/falcon/TransactionState.h
> === modified file 'storage/falcon/Record.cpp'
> --- a/storage/falcon/Record.cpp 2009-06-03 18:15:32 +0000
> +++ b/storage/falcon/Record.cpp 2009-06-23 21:16:25 +0000
> @@ -1051,11 +1051,6 @@ bool Record::isDeleted(void)
> return false; // Only RecordVersions are used to delete records!
> }
>
> -bool Record::isRecordDataFreed(void)
> -{
> - return (data.record == recordDataIsFreed);
> -}
> -
> void Record::print(void)
> {
> printf(" %p\tId %d, enc %d, state %d, use %d, grp " I64FORMAT "\n",
> @@ -1247,18 +1242,6 @@ char* Record::getRecordData(void)
> return recordData;
> }
>
> -char* Record::findRecordData(void)
> -{
> - char* recordData = data.record;
> - ASSERT(isAllocated(recordData));
> - return recordData;
> -}
> -
> -bool Record::isAllocated(void)
> -{
> - return isAllocated(data.record);
> -}
> -
> bool Record::isAllocated(char* recordData)
> {
> return ( recordData != NULL
>
> === modified file 'storage/falcon/Record.h'
> --- a/storage/falcon/Record.h 2009-06-10 15:05:08 +0000
> +++ b/storage/falcon/Record.h 2009-06-23 21:16:25 +0000
> @@ -130,7 +130,6 @@ public:
> virtual const char* getEncodedRecord();
> virtual char* setRecordData(const UCHAR *dataIn, int dataLength, int
> *bytesReallocated);
> virtual char* getRecordData(void);
> - virtual char* findRecordData(void);
> virtual void serialize(Serialize* stream);
> virtual int getSize(void);
> virtual SyncObject* getSyncThaw(void);
> @@ -168,8 +167,6 @@ public:
> void expungeRecord(void);
> int getDataMemUsage(void);
> int getMemUsage(void);
> - bool isRecordDataFreed();
> - bool isAllocated(void);
> bool isAllocated(char* recordData);
>
> Record (Table *table, Format *recordFormat);
> @@ -184,13 +181,19 @@ protected:
> char *record;
> } data;
>
> +public:
> +
> inline bool hasNoData(char* recordData)
> {
> ASSERT(recordData != NULL);
> return (recordData == recordIsALock || recordData == recordIsDeleted || recordData
> == recordDataIsFreed);
> }
> + inline char* findRecordData(void)
> + {
> + ASSERT(data.record != NULL);
> + return data.record;
> + }
>
> -public:
> uint64 generation;
> Format *format;
> volatile INTERLOCK_TYPE useCount;
>
> === modified file 'storage/falcon/RecordVersion.cpp'
> --- a/storage/falcon/RecordVersion.cpp 2009-06-04 12:55:34 +0000
> +++ b/storage/falcon/RecordVersion.cpp 2009-06-23 21:16:25 +0000
> @@ -570,16 +570,3 @@ char* RecordVersion::getRecordData()
>
> return recordData;
> }
> -
> -// The callers of this function want to see recordIsChilled
> -// instead of actually thawing the record like getRecordData does.
> -
> -char* RecordVersion::findRecordData(void)
> -{
> - char* recordData = data.record;
> -
> - if (hasNoData(recordData))
> - return NULL;
> -
> - return recordData;
> -}
>
> === modified file 'storage/falcon/RecordVersion.h'
> --- a/storage/falcon/RecordVersion.h 2009-06-03 01:36:57 +0000
> +++ b/storage/falcon/RecordVersion.h 2009-06-23 21:16:25 +0000
> @@ -58,7 +58,6 @@ public:
> virtual void serialize(Serialize* stream);
> virtual Transaction* findTransaction(void);
> virtual char* getRecordData();
> - virtual char* findRecordData();
> virtual bool hasRecord();
> virtual bool isChilled();
> virtual bool isALock();
>
> === modified file 'storage/falcon/SRLUpdateRecords.cpp'
> --- a/storage/falcon/SRLUpdateRecords.cpp 2009-06-10 23:54:03 +0000
> +++ b/storage/falcon/SRLUpdateRecords.cpp 2009-06-23 21:16:25 +0000
> @@ -64,10 +64,10 @@ bool SRLUpdateRecords::chill(Transaction
> char* SRLUpdateRecords::thaw(RecordVersion *record, int *bytesReallocated)
> {
> // Nothing to do if record is no longer chilled
> -
> - char* dataRecord = record->findRecordData();
> - if (dataRecord != recordIsChilled)
> - return dataRecord ;
> +
> + char* recordData = record->findRecordData();
> + if (recordData != recordIsChilled)
> + return recordData ;
>
> // Find the window where the record is stored using the record offset, then
> // activate the window, reading from disk if necessary
> @@ -121,9 +121,12 @@ char* SRLUpdateRecords::thaw(RecordVersi
> // setRecordData() handles race conditions with an interlocked compare and
> exchange,
> // but check the state anyway.
>
> - dataRecord = record->findRecordData();
> - if (dataRecord == recordIsChilled)
> - dataRecord = record->setRecordData(control->input, dataLength,
> bytesReallocated);
> + recordData = record->findRecordData();
> + if ((recordData == recordIsALock || recordData == recordIsDeleted || recordData ==
> recordDataIsFreed))
> + recordData = NULL;
> +
> + else if (recordData == recordIsChilled)
> + recordData = record->setRecordData(control->input, dataLength,
> bytesReallocated);
>
>
> if (*bytesReallocated > 0)
> @@ -134,7 +137,7 @@ char* SRLUpdateRecords::thaw(RecordVersi
> log->chilledRecords--;
> }
>
> - return dataRecord;
> + return recordData;
> }
>
> void SRLUpdateRecords::append(Transaction *transaction, RecordVersion *records, bool
> chillRecords)
>
> === modified file 'storage/falcon/Table.cpp'
> --- a/storage/falcon/Table.cpp 2009-06-12 18:17:19 +0000
> +++ b/storage/falcon/Table.cpp 2009-06-23 21:16:25 +0000
> @@ -29,6 +29,7 @@
> #include "Dbb.h"
> #include "PStatement.h"
> #include "Transaction.h"
> +#include "TransactionState.h"
> #include "Value.h"
> #include "Format.h"
> #include "RSet.h"
> @@ -2586,16 +2587,16 @@ bool Table::checkUniqueRecordVersion(int
>
> state = transaction->getRelativeState(dup, DO_NOT_WAIT);
>
> - // Check for a deleted record or a record lock
> + // Check for a record lock. No need to thaw here.
>
> - if (!dup->hasRecord())
> - {
> - // A record without a data buffer is either a lock record or a deleted record.
> + char *dataRecord = dup->findRecordData();
> + if ( (dataRecord == recordIsALock) || (dataRecord == recordDataIsFreed))
> + continue; // Next record version.
>
> - if (dup->isALock() || dup->isRecordDataFreed())
> - continue; // Next record version.
> + // Deleted records require special actions.
>
> - ASSERT(dup->isDeleted()); // Deleted records require special actions.
> + if (dataRecord == recordIsDeleted)
> + {
> switch (state)
> {
> case CommittedVisible:
>
> === modified file 'storage/falcon/Transaction.cpp'
> --- a/storage/falcon/Transaction.cpp 2009-06-03 18:15:32 +0000
> +++ b/storage/falcon/Transaction.cpp 2009-06-23 21:16:25 +0000
> @@ -912,7 +912,7 @@ State Transaction::getRelativeState(Tran
> return WasActive; // caller will need to re-fetch
> }
>
> - if (transState->state == Committed)
> + if (transState->isCommitted())
> {
> // Return CommittedVisible if the other trans has a lower TransId and
> // it was committed when we started.
> @@ -923,6 +923,7 @@ State Transaction::getRelativeState(Tran
> return CommittedInvisible;
> }
>
> + ASSERT(transState->isRolledBack());
> return (State) transState->state;
> }
>
>
> === modified file 'storage/falcon/TransactionState.h'
> --- a/storage/falcon/TransactionState.h 2009-03-27 13:43:10 +0000
> +++ b/storage/falcon/TransactionState.h 2009-06-23 21:16:25 +0000
> @@ -70,6 +70,11 @@ public:
> return state == Committed;
> }
>
> + inline bool isRolledBack()
> + {
> + return state == RolledBack;
> + }
> +
>
> public:
> TransId transactionId; // used also as startEvent by dep.mgr.
>
>
> ------------------------------------------------------------------------
>
>
