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?)
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?
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.
...
> === 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?
>
> === 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(...).
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.
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
