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.
Chuck
