Lars-Erik,
The candidate does not need to be released when fetchForUpdate was
called. But if there was a candidate and fetchForUpdate was not called,
it needs to be released before exiting this function.
Releasing a record twice is BAD. But that is not what this will do.
Kevin
Lars-Erik Bjørk wrote:
> Kevin,
>
> But you still think that candidate should be released, even though it is
> released before throwing the exception (at least in fetchForUpdate)?
>
> What happens if we release a record twice?
>
> Btw, releasing the candidate is the approach that is taken in other
> similar situations, so it is probably the way to go, just want to make
> sure it is correct :)
>
> /Lars-Erik
>
>
> Kevin Lewis wrote:
>> Lars-Erik,
>>
>> I think you had the test for releasing candidate correct and the
>> following comment may help explain why.
>> But if there was an exception thrown, we do not need to release
>> record since it has to be null still.
>>
>> + catch (SQLException& exception)
>> + {
>> + // {record} must be null if it threw an exception.
>> + // fetchForUpdate releases the {candidate} on error.
>> +
>> + if (candidate && !lockForUpdate)
>> + candidate->release(REC_HISTORY);
>> +
>> + // Re-throw the exception, catch it further up to return
>> + // the correct error
>> + throw;
>> + }
>>
>>
>>
>>
>> Lars-Erik.Bjork@stripped wrote:
>>> #At file:///home/lb200670/mysql/bugfest/ based on
>>> revid:christopher.powers@stripped
>>>
>>> 3059 lars-erik.bjork@stripped 2009-03-10
>>> This is a patch for Bug #43488
>>> Crash in Table::fetchForUpdate (update conflict) when using
>>> LIMIT query
>>> This crash was due to missing exception handling when
>>> calling fetchForUpdate
>>> when fetching records through the IndexWalker. The exception
>>> handling
>>> introduced in this patch is split in two parts. The first part
>>> does the necessary cleanup in IndexWalker::getValidatedRecord
>>> and re-throws the exception. The exception is re-caught in
>>> StorageDatabase::nextIndexed, where we return the correct error.
>>> I have not included any test files for this patch,
>>> because it is not
>>> easily reproducible without a larger framework as described in
>>> the bug
>>> report.
>>> Modified file 'storage/falcon/IndexWalker.cpp'
>>> ----------------------------------------------
>>> Added try - catch logic, doing the necessary cleanup in the case
>>> of an exception. This implementation is based on the similar cases
>>> in StorageDatabase.cpp. After the cleanup has been done, we
>>> re-throw
>>> the exception, so that we can return the appropriate error in
>>> StorageDatabase::nextIndexed
>>> Modified file 'storage/falcon/StorageDatabase.cpp'
>>> --------------------------------------------------
>>> Recatch the exception and return the appropriate error.
>>> modified:
>>> storage/falcon/IndexWalker.cpp
>>> storage/falcon/StorageDatabase.cpp
>>>
>>> === modified file 'storage/falcon/IndexWalker.cpp'
>>> --- a/storage/falcon/IndexWalker.cpp 2009-03-07 07:51:29 +0000
>>> +++ b/storage/falcon/IndexWalker.cpp 2009-03-10 08:33:34 +0000
>>> @@ -142,31 +142,51 @@ Record* IndexWalker::getValidatedRecord(
>>>
>>> // Fetch record. If it doesn't exist, that's ok.
>>>
>>> - Record *candidate = table->fetch(recordId);
>>> - if (!candidate)
>>> - return NULL;
>>> - RECORD_HISTORY(candidate);
>>> + Record *candidate = NULL;
>>> + Record *record = NULL;
>>> +
>>> + try
>>> + {
>>> + candidate = table->fetch(recordId);
>>>
>>> - // Get the correct version. If this is select for update, get a
>>> lock record
>>> + if (!candidate)
>>> + return NULL;
>>>
>>> - Record *record = (lockForUpdate) - ?
>>> table->fetchForUpdate(transaction, candidate, true)
>>> - : candidate->fetchVersion(transaction);
>>> + RECORD_HISTORY(candidate);
>>> +
>>> + // Get the correct version. If this is select for update,
>>> get a lock record
>>> + record = (lockForUpdate) + ?
>>> table->fetchForUpdate(transaction, candidate, true)
>>> + : candidate->fetchVersion(transaction);
>>> - if (!record)
>>> - {
>>> - if (!lockForUpdate)
>>> - candidate->release(REC_HISTORY);
>>> + if (!record)
>>> + {
>>> + if (!lockForUpdate)
>>> + candidate->release(REC_HISTORY);
>>> - return NULL;
>>> - }
>>> + return NULL;
>>> + }
>>> - // If we have a different record version, release the original
>>> + // If we have a different record version, release the original
>>> - if (!lockForUpdate && candidate != record)
>>> + if (!lockForUpdate && candidate != record)
>>> + {
>>> + record->addRef(REC_HISTORY);
>>> + candidate->release(REC_HISTORY);
>>> + }
>>> + }
>>> + catch (SQLException& exception)
>>> {
>>> - record->addRef(REC_HISTORY);
>>> - candidate->release(REC_HISTORY);
>>> +
>>> + if (record && record != candidate)
>>> + record->release(REC_HISTORY);
>>> +
>>> + if (candidate && !lockForUpdate)
>>> + candidate->release(REC_HISTORY);
>>> +
>>> + // Re-throw the exception, catch it further up to return the
>>> correct
>>> + // error
>>> + throw;
>>> }
>>> // Compute record key and compare against index key. If
>>> there' different, punt
>>>
>>> === modified file 'storage/falcon/StorageDatabase.cpp'
>>> --- a/storage/falcon/StorageDatabase.cpp 2009-02-26 20:04:31 +0000
>>> +++ b/storage/falcon/StorageDatabase.cpp 2009-03-10 08:33:34 +0000
>>> @@ -484,14 +484,45 @@ int StorageDatabase::nextIndexed(Storage
>>>
>>> int StorageDatabase::nextIndexed(StorageTable* storageTable,
>>> IndexWalker* indexWalker, bool lockForUpdate)
>>> {
>>> - Record *record = indexWalker->getNext(lockForUpdate);
>>>
>>> - if (!record)
>>> - return StorageErrorRecordNotFound;
>>> + try
>>> + {
>>> + Record *record = indexWalker->getNext(lockForUpdate);
>>> +
>>> + if (!record)
>>> + return StorageErrorRecordNotFound;
>>> +
>>> + storageTable->setRecord(record, lockForUpdate);
>>> + return record->recordNumber;
>>> + }
>>> + catch (SQLException& exception)
>>> + {
>>> + StorageConnection *storageConnection =
>>> storageTable->storageConnection;
>>> + storageConnection->setErrorText(&exception);
>>> + int errorCode = exception.getSqlcode();
>>> - storageTable->setRecord(record, lockForUpdate);
>>> - - return record->recordNumber;
>>> + switch (errorCode)
>>> + {
>>> + case UPDATE_CONFLICT:
>>> + return StorageErrorUpdateConflict;
>>> +
>>> + case OUT_OF_MEMORY_ERROR:
>>> + return StorageErrorOutOfMemory;
>>> +
>>> + case OUT_OF_RECORD_MEMORY_ERROR:
>>> + return StorageErrorOutOfRecordMemory;
>>> +
>>> + case DEADLOCK:
>>> + return StorageErrorDeadlock;
>>> +
>>> + case LOCK_TIMEOUT:
>>> + return StorageErrorLockTimeout;
>>> + + default:
>>> + ASSERT(false);
>>> + }
>>> +
>>> + }
>>> }
>>>
>>> int StorageDatabase::savepointSet(Connection* connection)
>>>
>>>
>
>
