Hi Ingo,
Thank you for taking a look at it again and responding back so quickly!
I misunderstood the operation of the error injection in malloc, I will
follow your
suggestion and revert back.
Thanks Again,
Narayanan
Ingo Strüwing wrote:
> Hi VN,
>
> thanks for the clarifications. Please see below. I suspect a
> misunderstanding.
>
> V Narayanan, 25.06.2009 09:01:
>
> ...
>
>> Ingo Strüwing wrote:
>>
> ...
>
>>> V Narayanan, 24.06.2009 10:57:
>>>
>>> ...
>>>
>>>
>>>> 3391 V Narayanan 2009-06-24
>>>> Bug#43572 Handle failures from hash_init
>>>> The bug needs to be tested with error injection. This
>>>> patch adds error injection tests, for, testing failure
>>>> of hash_init, caused by memory allocation failure in
>>>> the dynamic array used in the hash.
>>>> @ mysql-test/r/hash_error_injection.result
>>>> Bug#43572 Handle failures from hash_init
>>>> Result file for error injection testing in hash_init
>>>> @ mysql-test/t/hash_error_injection.test
>>>> Bug#43572 Handle failures from hash_init
>>>> sets the debug flag to simulate a dynamic array
>>>> allocation
>>>> failure while initializing the hash. After setting the flag
>>>> create hash indexes on memory tables and test the queries to
>>>> ensure that, failure in the dynamic array allocation in
>>>> hash_init
>>>> does not causes these queries, that exercise the hash_init
>>>> function to fail.
>>>> @ mysys/hash.c
>>>> Bug#43572 Handle failures from hash_init
>>>> Contains the changes needed to add error injection
>>>> in the hash_init function. Test a debug flag to determine
>>>> if the dynamic array needs to be reset, thus, simulating
>>>> an memory allocation failure.
>>>>
>>>>
>>>>
>>> ...
>>>
>>>
>>>> === added file 'mysql-test/r/hash_error_injection.result'
>>>>
>>>>
>>> ...
>>>
>>>
>>>> +#
>>>> +# Reset the debug flag to stop error injection
>>>> +#
>>>> +#
>>>> +# Perform error injection
>>>> +#
>>>>
>>>>
>>> (I wonder about this comment. You stopped error injection above and
>>> claim to perform it here?)
>>>
>>>
>> Sorry, Mea Culpa!, Should have been "Perform Cleanup". Will correct it!
>>
>
>
> Aha, kind of a typo. I didn't think of this possibility. :-)
>
>
>>> I am puzzled that I don't see any result of the error injection in the
>>> result file. I understand that your original patch had the goal to
>>> circumvent a problem in allocation during hash_init. So it is fine that
>>> all statements succeed. But since there isn't even a warning, how can
>>> you be sure that the error injection has an effect at all?
>>>
>>>
>> Ingo, I verified that the error injection does indeed happen by single
>> stepping.
>> I single stepped to ensure that the control does indeed reach the error
>> injection
>> code.
>>
>
>
> Good!
>
>
>> Everything is supposed to work fine even if hash_init fails. The dynamic
>> array
>> will be initialized at a later stage during my_hash_insert. Hence not
>> seeing even
>> a warning is expected, it just shows that hash_init failure does not
>> affect normal
>> operation at all.
>>
>
>
> Yes, I know.
>
> ...
>
>> Ingo, *After this fix* causing a memory allocation failure in malloc
>> will not
>> cause hash_init to fail, instead it will cause my_hash_insert to fail.
>>
>> Pls see that I have raised Bug#45613 as for this.
>>
>
>
> Yes, I understand, what you mean and agree with it.
>
>
>> The errors that arise will not be due to hash_init but instead due to
>> my_hash_insert,
>> so we would in effect be writing a test case for a existing bug that has
>> not been fixed.
>>
>
>
> This may be a misunderstanding. Please see below.
>
> ...
>
>>>> === modified file 'mysys/hash.c'
>>>> --- a/mysys/hash.c 2009-06-22 12:38:37 +0000
>>>> +++ b/mysys/hash.c 2009-06-24 08:57:25 +0000
>>>> @@ -77,9 +77,16 @@ _my_hash_init(HASH *hash, uint growth_si
>>>> my_hash_get_key get_key,
>>>> void (*free_element)(void*), uint flags
>>>> CALLER_INFO_PROTO)
>>>> {
>>>> + /* Set to TRUE if error injection is enabled */
>>>> + my_bool hash_init_error_injection= FALSE;
>>>> +
>>>> DBUG_ENTER("my_hash_init");
>>>> DBUG_PRINT("enter",("hash: %p size: %u", hash, (uint) size));
>>>>
>>>> + /* Error injection for coverage testing. */
>>>> + DBUG_EXECUTE_IF("hash_init_allocation_failure_err_inj",
>>>> + hash_init_error_injection= TRUE;);
>>>> +
>>>> hash->records=0;
>>>> hash->key_offset=key_offset;
>>>> hash->key_length=key_length;
>>>> @@ -88,6 +95,19 @@ _my_hash_init(HASH *hash, uint growth_si
>>>> hash->free=free_element;
>>>> hash->flags=flags;
>>>> hash->charset=charset;
>>>> +
>>>> + /* + If error injection is enabled simulate failure to allocate
>>>> + the dynamic array
>>>> + */
>>>> + if (hash_init_error_injection)
>>>> + {
>>>> + my_init_dynamic_array_ci(&hash->array,
>>>> + sizeof(HASH_LINK), size, growth_size);
>>>> + hash->array.buffer= 0;
>>>> + hash->array.max_element= 0;
>>>> + DBUG_RETURN(FALSE);
>>>> + }
>>>> DBUG_RETURN(my_init_dynamic_array_ci(&hash->array,
>>>> sizeof(HASH_LINK), size,
>>>> growth_size));
>>>>
> ...
>
>>> OTOH, you could use the malloc error injection
>>> "my_malloc_error_inject=1". That would produce an allocation error
>>> inside of my_init_dynamic_array_ci(). And it would avoid to have an
>>> extra branch here. I suggest the following:
>>>
>>> DBUG_EXECUTE_IF("hash_init_allocation_failure_err_inj",
>>> my_malloc_error_inject=1);
>>>
>>> If you replace your similar statement with this, the extra branch isn't
>>> required at all (neither the hash_init_error_injection variable). The
>>> allocation will fail inside my_init_dynamic_array_ci() and you should
>>> get the same result.
>>>
>>>
>> The extra branch is required if you want to test hash_init failures alone.
>> One we fix the bug in my_hash_insert we can add error injection for a
>> malloc failure and ensure that failures during memory allocation for the
>> dynamic array do not affect any operation on the hash.
>>
>> The error injection case is useful for testing that *failures in
>> hash_init* do not
>> cause critical failures in the server operation (e.g. crashes). But this
>> testing
>> is limited to hash_init alone. Adding a case for malloc as you suggest
>> above
>> is a generalization for testing every place where malloc is used in an
>> hash operation.
>>
>
>
> I believe this is not the case. Please have a look into my_malloc.c and
> safemalloc.c. The error is injected by setting the global variable
> 'my_malloc_error_inject' to a non-zero value. Both malloc functions
> (my_malloc() and the safe version _mymalloc()) report an error and
> return NULL if they detect that value. 'my_malloc_error_inject' is
> immediately reset to zero. No further my_malloc() or _mymalloc() will
> fail due to the error injection (unless it is explicitly set again
> later). This has been arranged so, because otherwise so many things
> would fail in the rest of a statement execution that every test goal
> would be smashed.
>
> In summary: After setting 'my_malloc_error_inject' only the single next
> call to my_malloc() (or _mymalloc()) will fail. All further calls will
> succeed (unless there is really a lack of memory).
>
> So my suggestion should just disturb the malloc in hash_init, not any
> later malloc in my_hash_insert. Can you please consider again (or even
> try it) to follow my suggestion, please?
>
> Regards
> Ingo
>
