Re: bzr commit into mysql-6.0-backup branch (rsomla:2672)

[Rafal Somla]

Hi Øystein,

Thanks for looking at this. Here are my replies.

Øystein Grøvlen wrote:
> Rafal,
> 
> Here are my comments/questions to this patch:
> 
>  - You have removed the test:
>        if (m_thd->options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN))
>    Is not that a necessary test?

I see this test as some optimization, so that we do not perform commit always 
but only when some options are set in m_thd. But, assuming that 
ha_outocommit_or_rollback() is no-op if there is nothing to commit, It should be 
ok to call it always. Also, it should be more safe in case we missinterpret 
thd->options or their meaning changes in the future.

> 
>  - When compiling, I get this warning:
>         kernel.cc:943: warning: control reaches end of non-void function
>

Good point. I have changed this as follows

@@ -938,8 +938,9 @@ int Backup_restore_ctx::restore_triggers
  */
  int Backup_restore_ctx::commit_changes()
  {
-  ha_autocommit_or_rollback(m_thd, 0);
-  end_active_trans(m_thd);
+  int ret1= ha_autocommit_or_rollback(m_thd, 0);
+  int ret2= end_active_trans(m_thd);
+  return ret2 ? ret2 : ret1;
  }

Do you agree with these changes?

>  - Why commit twice?  I do not understand why you want to commit
>    before creating triggers and events.
>

I was thinknig that if a created event or trigger fires, its correct operation 
might depend on all table data being present in the tables. This is why I want 
to commit the data inserted into tables before I create events and triggers.

Again, in case a trigger or event fires during restore and modifies some data, I 
want to commit all these changes at the end.

>  - There is nothing backup/restore specific about the code in
>    commit_changes.  Hence, it does not seem to me that
>    Backup_restore_ctx is the right class for this code.
> 

I would say that commit_changes() is related to restore. Restore operation 
(Backup_restore_ctx::do_restore()) modifies data in the server and such changes 
should be committed before they take effect. Thus commit_changes() is a helper 
method for do_restore() and this is why I added it to Backup_restore_ctx class. 
Do you see a better place for it?

>  - That this issue was not detected before the code was pushed,
>    indicates that a test is missing.  I think such a test should be
>    added as part of this patch.
> 

I think Hema is working on tests which test that functionality (this is how the 
problem was detected). Is it ok to cover this by Hema's test?

Rafal