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

[Rafal Somla]

Hi Øystein,

I still try to defend some changes I introduced. Please see below.

Øystein Grøvlen wrote:
>>>
>>>  - 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.
> 

Indeed, I forgot about this. Now I added.

@@ -999,7 +1000,8 @@ int Backup_restore_ctx::do_restore()
      DBUG_RETURN(ER_BACKUP_RESTORE);

    // Commit new table data before creating triggers and events.
-  commit_changes();
+  if(commit_changes())
+    DBUG_RETURN(ER_BACKUP_RESTORE);

    /*
     Re-create all triggers and events (it was not done in @c bcat_create_item()).
@@ -1012,7 +1014,8 @@ int Backup_restore_ctx::do_restore()
       DBUG_RETURN(ER_BACKUP_RESTORE);

    // Final commit (perhaps redundant but should not hurt).
-  commit_changes();
+  if (commit_changes())
+    DBUG_RETURN(ER_BACKUP_RESTORE);

    DBUG_PRINT("restore",("Done."));

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

Yes, theoretically I should not, but who knows all the details of how events and 
triggers are executed? Perhaps a separate thread is used for that and then an 
event will not see the new data. Instead of spending time on digging the server 
code, it is safe to just commit the new data after it has been inserted. This 
will certainly not hurt (do you agree?) and will keep us on the safe side.

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

An event could be fired by event scheduler. Such event can modify data and then 
fire a trigger. So I think it is theoretically possible that one of the restored 
events/triggers fires.

It is also true that the code doesn't handle this very well. Ideally all 
restored events should be kept disabled until all objects are re-created. But 
implementing this would require extending the server, so it was accepted that we 
will use a simplistic approach for the moment.

In further discussion about that (if any) I propose to separate two issues.

1. Is doing additional commit really needed and why?
2. Is doing additional commit correct, that is it does not break anything.

If answer for 2 is YES, then perhaps it doesn't pay off to spend much time on 
discussing 1.

But if you are sure the extra commit is not needed I can remove it - just tell me.

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

You should know that our long-term plan is to make backup system tree 
independent from the server tree. Ideally, we would like to have backup 
implemented as a set of loadable components and be able to compile these 
components without need for having a complete server tree around.

To this end we try to define an API for accessing server functionality that is 
needed in the backup code and consequently use it there. This API is not 
complete, not even designed, but I try to write new code having this issue in mind.

For that reason, the backup code can not look like the rest of the server code. 
For one thing, server code can freely use all the functions defined in the 
server while backup code should not access them directly but only through an API.

Now, committing and terminating the current transaction is certainly a server 
job. We haven't defined or even though much about API for that functionality, 
but a good step in that direction would be to put it in a separate 
function/method, so that when API is ready the code can be easily updated to use it.

So, I would keep the commit_changes() method which is representing a 
to-be-included-in-the-API server service. It is also good to have if we want to 
commit the changes in 2 places - which is being discussed above.

Again, if you insist, I can remove this helper method and call server functions 
directly, especially if you also insist on committing in only one place.

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

I think detecting this was a side effect of testing by Hema how backup/restore 
deals with transactions. So, I'll look at the backup_commit_restore test you 
created for BUG#34210 and make sure that it also tests that BACKUP does not 
implicitly commit transactions.

An updated patch will follow this email when it is ready.

Rafal