Chuck Bell wrote:
> I've had a look at this myself. There is no way to reproduce this error at a
> lower level without additional heroics. The error from open_tables() is
> beyond the scope of backup. We are not testing what happens when
> open_tables() fails, we are testing how backup would handle such a failure.
> The real issue is that the original code never returned an error status. We
> are asking, "what happens if this method returns true?" In that case, debug
> insertion is perfectly valid.
So why do we not ask our self that question for all methods that may
return true? The only thing special about this method is that someone
at one point in time made a typo when handling the result from calling
this method.
The main reason for adding tests as part of a bug fix is that the
occurrence of the bug has revealed that our test suite is missing a test
case that could make us aware of this bug at a earlier stage, and that
we should plug that hole to ensure that we don't have a regression at a
later stage. For this particular issue, the added test case will not
detect if someone reversed the fix later. (To detect that you would
have to put the DBUG_EXECUTE_IF statement inside open_tables.)
However, one can argue that this test tests what will happen if
TriggerObj::do_serialize fails. However, if we think that is important,
it follows that we should add the same type of statement to all
do_serialize methods.
>
>> Fix look good, but I am not sure I like the mechanism used in
>> the second test case:
>>
>>> + DBUG_EXECUTE_IF("backup_fail_add_trigger", DBUG_RETURN(TRUE););
>>> if (open_tables(thd, &lst, &num_tables, 0))
>>> - DBUG_RETURN(FALSE);
>>> + DBUG_RETURN(TRUE);
>>>
>>> DBUG_ASSERT(num_tables == 1);
>>> Table_triggers_list *triggers= lst->table->triggers;
>> Any particular reason we should test that the code works if
>> TRUE is returned from this particular point in this function,
>> and not from all the other places that may return TRUE? I do
>> not think it makes sense to add such "error triggers" for all
>> potential errors in the code, so what makes this code exceptional?
>
>
--
Øystein
