List:Commits« Previous MessageNext Message »
From:Lars-Erik Bjørk Date:March 13 2009 8:42am
Subject:Re: bzr commit into mysql-6.0-falcon-team branch
(lars-erik.bjork:3059) Bug#43488
View as plain text  
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)
>>
>>

Thread
bzr commit into mysql-6.0-falcon-team branch (lars-erik.bjork:3059) Bug#43488lars-erik.bjork10 Mar
  • Re: bzr commit into mysql-6.0-falcon-team branch(lars-erik.bjork:3059) Bug#43488Kevin Lewis11 Mar
    • Re: bzr commit into mysql-6.0-falcon-team branch(lars-erik.bjork:3059) Bug#43488Lars-Erik Bjørk13 Mar
      • Re: bzr commit into mysql-6.0-falcon-team branch(lars-erik.bjork:3059) Bug#43488Kevin Lewis13 Mar
        • Re: bzr commit into mysql-6.0-falcon-team branch(lars-erik.bjork:3059) Bug#43488Lars-Erik Bjørk15 Mar