Chuck Bell wrote:
> Oystein,
>
>>> 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.
>
> I agree that we should, but in this case we have found an omission whereby
> we do not know the consequences of repairing the defect. We can only repair
> and guard against unforeseen problems with the correction.
>
> We should probably make it a point to ensure all such returns are validated
> during reviews.
>
>> 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.
>
> I think we could indeed find consensus on this topic but I would motion to
> move this outside of this bug report and take it up as an agenda item for a
> team meeting. The code in si_objects was originally supposed to be the
> responsibility of another team and I think we need discussion before suggest
> changes.
Unfortunately, I think both arguments make sense. I'm not sure if we
should scatter the code with thousands of DBUG_EXECUTE_IF's, but the
test did ensure me that the fix was correct.
Should I put this on hold until we have had the next team meeting? Since
team tree is currently frozen, there's no additional delay involved.
--
Jørgen Løland
