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
--
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin Häring HRB München 161028
