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

[Øystein Grøvlen]

Rafal Somla wrote:
> Hi Øystein,
> 
> I still try to defend some changes I introduced. Please see below.

Hi Rafal,

It seems like we are getting close to an agreement here.  See my 
comments 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."));
> 

Looks good.

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

I think the entire restore should execute as a single transaction, and 
data that is restore should not be accessible to other transactions 
until the entire restore has completed.  I fear that committing after 
data insertion, may cause locks to be released that make tables 
available to others before triggers have been created.  That may cause 
inconsistencies since other transactions may perform operations that 
normally should have caused triggers to fire before triggers have been 
created.

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

I follow your arguments.  Makes sense to make the abstractions that 
should have been made by the server locally.  This way missing 
abstractions does not pollute your code.

I must admit that I do not quite understand why two function calls are 
needed, and I also see that some places end_trans is used instead of 
end_active_trans.  Do you know what is the difference?

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

Great!

-- 
Øystein