Hi Ingo,
Thank you for following up on this bug and giving your comments on the
test case attached! Please see some comments below.
Ingo Strüwing wrote:
> Hi VN,
>
> thank you for implementing error injection to test your bug fix. Please
> find below some thoughts for improvement.
>
> 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!
> 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.
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.
> If you follow my suggestion below, you would force my_malloc() to report
> an error. This should show up as a warning and not make the statements
> fail. I must admit that I'm not sure what will happen. It is possible
> that the error, reported by my_malloc() will make some statements fail
> with EE_OUTOFMEMORY or alike. But then it's something that might need to
> be solved. We could discuss that if it happens. The other possibility is
> that we see just warnings. And that is the way it should be, I think.
> The third possibility would be that we still don't see anything. But
> then I'd like to see, where the malloc error is suppressed. In this case
> we need to discuss again.
>
> Anyway, if the effect of error injection does not show up in the result
> file, we don't need to run the test in the test suite. It would do to
> run it manually once in a debugger and verify that it works as expected.
>
> However, I hope we can make warnings come up by the change I suggest below.
>
> ...
>
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.
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.
>> === added file 'mysql-test/t/hash_error_injection.test'
>>
> ...
>
>> +--echo #
>> +--echo # Reset the debug flag to stop error injection
>> +--echo #
>> +--disable_query_log
>> +eval SET debug= '$debug';
>> +--enable_query_log
>> +
>> +--echo #
>> +--echo # Perform error injection
>> +--echo #
>> +DROP INDEX c1_index ON t1;
>> +DROP INDEX c2_index ON t1;
>> +DROP INDEX c4_index ON t1;
>> +
>> +DROP TABLE t1;
>> +DROP TABLE t2;
>>
>
>
> Here is, what I meant above. After resetting "debug", why would we
> "perform error injection" here?
>
Mea Culpa! Sorry! Will fix it!
>
>> === 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));
>>
>
>
> IMHO it is not acceptable to insert this error injection code
> unconditionally. It would be present in a production server too.
>
> One way to avoid this would be to include it in "#ifndef DBUG_OFF ...
> #endif", or in IF_DBUG(...).
>
Agree! Will do!
> 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.
This case has not been handled yet and will be handled when Bug#45613 is
fixed.
> Regards
> Ingo
>
Thanks once again for the comments,
Narayanan
