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

[Øystein Grøvlen]

Rafal Somla wrote:
> 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.

Sounds reasonable to me.  I agree that this test belongs inside the 
commit function and should not be the responsibility of the caller.

> 
>>
>>  - 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;
>  }

Looks good, but if you an return error code, you should also need to 
handle that where the function is called.

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

I do not understand this.  If everything is executed in the same 
connection, you should not need to commit to make the data visible to 
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.

I am not sure it makes sense to fire triggers during restore.  And how 
could a trigger be fired if they are not created until all data have 
been inserted?

> 
>>  - 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?

My point is that there are many places in the code (outside backup) 
where these two functions are called in sequence.  It seems strange to 
single out one of these occurrences and make a helper function for just 
this one.

> 
>>  - 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?

I think it is best if the test accompanies the bug fix.  Makes it easier 
  later to understand the connection between the test and the bug fix. 
Also, if the test is completed after the bug is fixed, one will not 
necessarily detect if the test still would have provoked the bug.  On 
the other hand, we should avoid having several tests that test the same, 
so I guess it is OK if Hema makes a test case that explicit tests for 
this.  In other words, that the error not just occurs as a side effect 
of something else she is testing.


-- 
Øystein