Hi Rafal,
> Thanks for review and being open in the discussion. I think
> we've come to an agreement. Please just check below if I'm
> not mistaken in thinking that the code for selecting backup
> engine is actually correct.
>
> I'll start working on the merge now - expect a merge patch
> from me by this Wednesday.
Great!
> > One last thing...while researching a bug, I found another
> bug in the
> > kernel code. Since I cannot apply your patches to the trees that I
> > have (long story
> > -- I'd rather wait until the merge patch), I'd like to ask you to
> > examine your code for this bug. Also, since you've already done
> > extensive modifications to the code it would be silly for
> me to open a
> > bug report on it. Here's your chance to prevent it! ;)
> >
> > @line#557 in old kernel.cc:
> >> /*
> >> Try default drivers in decreasing order of preferrence
> >> (first snapshot, then default)
> >> */
> >> for (no=1; no >=0; --no)
> >> {
> >> if (!m_snap[no]->accept(tbl,hton))
> >> continue;
> >>
> >> DBUG_RETURN(no);
> >> }
> >>
> >>
> report_error(ER_BACKUP_NO_BACKUP_DRIVER,tbl.describe(buf,sizeo
> f(buf)));
> >> DBUG_RETURN(-1);
> >
> > The problem is if the default drivers do not accept the table, this
> > loop terminates but does not report the error. Instead, it
> returns 0
> > (no == 0) and that causes code upstream to fail (the
> DBUG_PRINT goes
> > whacky first but other things get crazy from there). I have
> > implemented the following change to the old code so that I can
> > continue to work on the bug report, but it is best that you
> ensure that the error is corrected with your refactored code.
> >
>
> Please double-check my thinking on this. I think the above
> loop is corect, because the DBUG_RETURN(no) statement inside
> the loop is reached only if one of the two snapshots accepts
> the table. If not, then "continue" will be executed twice and
> the loop will terminate when 'no' drops below zero. Then the
> error will be reported and -1 returned to indicate the
> problem. The returned value is checked inside
> Backup_info::add_table() and the problem is reported up the
> call stack. If this doesn't work, the reason must be something else...
Ok, I agree there is nothing wrong with the loop, but there is still a
problem with the code. If you apply the BUG#33572 patch to the current tree
_minus_ the kernel.cc changes shown above, the code fails at line #1018 in
kernel.cc (in debug) because no is -1 and therefore the DBUG_PRINT's
sub-calls will fail.
Chuck
