Hi Kevin,
Thanks for the explanation and confirming my observation. I agree with
your suggestion and think the code looks better and is more robust when
testing explicitly for the situation where we have a record object and
no associated transaction.
I have run some tests with your suggested change and it seems to work
very well. Do you want to check in this change yourself or should I just
include this in one of my patches?
Olav
Kevin Lewis wrote:
> Olav,
>
> I'm glad you noticed this. The relative state of a Record object is
> always CommittedVisible. It was never associated wioth a transaction.
> Just a committed record read from the database. We might as well save
> time and report that directly. Try this;
>
> State Transaction::getRelativeState(Record* record, uint32 flags)
> {
> + if (!record->isVersion())
> + return CommittedVisible;
> +
> blockingRecord = record;
> State state = getRelativeState(record->getTransaction(),
> record->getTransactionId(), flags);
> blockingRecord = NULL;
>
> return state;
> }
>
>
> Olav Sandstaa wrote:
>> Hi Kevin,
>>
>> After applying your patch for Bug #40893 "Falcon concurrent unlock
>> can cause invalid fetches" I found another issue with the use of
>> Transaction::getRelativeState() that I first thought had to be wrong
>> but which might be correct anyway. It would be good to get your
>> comment on it.
>>
>> It is related to the following code:
>>
>> State Transaction::getRelativeState(Transaction *transaction, TransId
>> transId, uint32 flags)
>> {
>> if (transactionId == transId)
>> return Us;
>>
>> // A record may still have the transId even after the trans itself
>> has been deleted.
>> if (!transaction)
>> {
>> // All calls to getRelativeState are for the purpose of writing.
>> // So only ConsistentRead can get CommittedInvisible.
>>
>> if (IS_CONSISTENT_READ(isolationLevel))
>> {
>> // If the transaction is no longer around, and the record is,
>> // then it must be committed.
>>
>> if (transactionId < transId)
>> return CommittedInvisible;
>>
>> // Be sure it was not active when we started.
>>
>> for (int n = 0; n < numberStates; ++n)
>> if (states [n].transactionId == transId)
>> return CommittedInvisible;
>> }
>>
>> return CommittedVisible;
>> }
>>
>>
>> After your fix I had hoped that we no longer should see cases where
>> we use the states array in the last for-loop. But with the debug code
>> I had in my code tree we were still executing that code (but not the
>> return statement). So I wondered why....
>>
>> It turns out that the case where this code is executed happens when
>> we call Transaction::getRelativeState(Transaction*...) and the
>> transaction argument is NULL and the transId is 0. This seems to
>> happen when we calls Transaction::getReleativeState(Record* ) and the
>> record argument is a Record object, not a RecordVersion object.
>>
>> When this is the case, Transaction::getRelativeState() will always
>> return CommittedVisible (which is likely the correct value). Still
>> from reading the above code it is a bit hard to imagine that it is
>> supposed to work when both transaction==NULL and transId==0 (and
>> could easily be a source for introducing unintended changes if
>> someone decided to change the order of the logic).
>>
>> At least this puzzled me, so is it correct use of
>> Transaction::getTransaction(NULL, 0, flags) in this case?
>>
>> Olav
>>
>>
>>
