Hi,
My replies below with some deletion.
> (I first thought that you wanted me to address commit after
> backup also in this patch. Can you confirm that this is not
> the case?)
No, that was my mistake. Sorry. I had like 4 things going at one time -- my
multitasking buffer was overflowing. :P
> > 1) running autocommit=0 with only the CS driver, both CS
> and default
> > driver, and a native driver (see below)
> > 2) running tests with autocommit=1 after the autocommit=0 tests
> > 3) running backup with autocommit=0 but turn it back on
> before restore
> > (maybe). Basically, mix and match autocommit settings to
> make sure it
> > all works as expected.
> >
>
> OK. Is this still in the context of verifying that a rollback
> after restore does not undo the restore operation, or are you
> thinking of a broader test scope?
No, I wanted to make sure we had complete test coverage for the bug
(rollback after restore). The coverage I suggested may have been too broad
-- feel free to adjust it.
> > if (thd->options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN))
> > {
> > ha_autocommit_or_rollback(m_thd, 0);
> > end_active_trans(m_thd);
> > }
>
> I will do this. (I just ask myself what not all this could
> have been done within the end_active_trans function). What
> about testing the return values of the functions I call for
> errors. Seems like if the commit fails, this will silently
> be ignored.
Yes, it appears that the errors for these calls are sometimes ignored. If we
place the guard around it I think we can eliminate most of the 'safe'
errors. However, examining the ha_autocommit_or_rollback() method reveals it
either returns 1 or what was passed in as the second argument so checking
for errors here isn't very helpful. However, that is not the case for
end_active_trans(). I agree we should check for errors on that method and it
wouldn't hurt to check for errors on both calls.
> > --- 1.20/sql/backup/data_backup.cc 2008-05-05 11:03:15 -04:00
> > +++ edited/data_backup.cc 2008-05-28 11:45:37 -04:00
> > @@ -1595,7 +1595,19 @@
> > Close all tables if default or snapshot driver used.
> > */
> > if (table_list)
> > - close_thread_tables(::current_thd);
> > + {
> > + THD *thd= ::current_thd;
> > + close_thread_tables(thd);
> > + /*
> > + If autocommit is turned off, be sure to commit the
> transaction.
> > + */
> > + if (thd->options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN))
> > + {
> > + ha_autocommit_or_rollback(thd, 0);
> > + end_active_trans(thd);
> > + }
> > + }
> > +
> >
> > One more thing: "What if a restore uses only a native
> driver (thus no
> > default or CS driver and thus no call to ha_auto...)? Does
> this bug still
> > apply?"
>
> Why not move the new code outside the "if (table_list)"? Is
> there any
> problems with that?
I don't think there would be any problems because it seems the calls are
'safe' but I don't want to take the chance. I think we should leave it
inside the "if" because I think this problem will not be an issue with
native drivers and only applies to the default and CS driver.
> > We should check it with the mysql-6.0-backup-myisam code.
>
> Since MyISAM does not support transaction, I guess testing
> will not show
> any problems.
That's correct. Too bad we don't have a native driver that supports
transactions. Ok, skip that for now but I'd put a note in the test file just
in case.
Chuck
