Re: bzr commit into mysql-6.0-backup branch (Rafal.Somla:2741) Bug#32702 WL#4644

[Chuck Bell]

STATUS
------
Patch approved.

REQUESTS
--------
None

COMMENTARY
----------
Thanks for the extra effort to make reviewing this large patch easier.
I accept your explanation for not moving functions to si_objects.
I noted the patch acceptance state in the worklog and bug report.

DETAILS
-------
> I have updated my patch to the latest 6.0-backup tree 
> (revid:jorgen.loland@stripped).
> 
> I have addressed the issues you raised. As discussed in Stockholm I 
> created a branch with my patch - you can pull changes from there and 
> then all renames should be correctly handled by bzr. The url of the 
> branch is:
> 
> bzr+ssh://bk-internal.mysql.com/bzrroot/server/mysql-6.0-backup-bug32702
> 
> I'm waiting for your review of the updated patch.
> 
> Chuck Bell wrote:
>> REQUESTS
>> --------
>> 1. The patch does not apply cleanly to bml.h or backup_logs.result.
> 
> You can pull it now from the branch I created.
> 
>> 2. The backup_errors test fails.
> 
> Now the result file should be correct. On my box all backup tests pass 
> fine.
> 
>> 3. Inconsistent use of rename 'ddl_blocker' to 'bml_blocker'.
> 
> Fixed.
> 
>> 4. Unclear benefits of new 'prepared' state.
> 
> I removed the new state.
> 
>> 5. Move BML utility functions to si_objects.
> 
> I disagree - see my reply below.
> 
>> DETAILS
>> -------
> ...
>>> === modified file 'sql/sql_parse.cc'
>>> --- a/sql/sql_parse.cc    2008-12-04 23:14:30 +0000
>>> +++ b/sql/sql_parse.cc    2008-12-17 14:32:46 +0000
>>
>> ...
>>
>>> @@ -351,6 +392,13 @@ bool is_log_table_write_query(enum enum_
>>>    return (sql_command_flags[command] & CF_WRITE_LOGS_COMMAND) != 0;
>>>  }
>>>  
>>> +static
>>> +bool is_bml_blocked(const enum enum_sql_command command)
>>> +{
>>> +  DBUG_ASSERT(command <= SQLCOM_END);
>>> +  return (sql_command_flags[command] & CF_BLOCKED_BY_BML) != 0;
>>> +}
>>> +
>>
>> [5] I think this belongs in si_objects. It could be useful in other 
>> parts of backup code.
>>
> 
> This function is analogous to two other functions introduced here: 
> is_update_query() and is_log_table_write_query() therefore I think it 
> should stay together with them.
> 
> Whether it belongs to si_objects depends on how you look at it. I see it 
> as auxiliary function for reading the sql_command_flags[] table (hiding 
> details such as the exact flag used to mark these commands). It is not 
> part of the public interface of the BML service (as per design in the 
> WL). It is rather part of the internal implementation of this service.
> 
> This implementation is spread across si_objects.cc where public 
> interface methods bml_get(), bml_release() etc. are implemented), bml.cc 
> where the BML_class is implemented and sql_parse.cc where the parser is 
> modified to perform registering/de-registering of the DDL statements 
> with the BML_class instance. Recognizing which statements are to be 
> registered belongs, in my opinion, to the sql_parse.cc part of the 
> implementation.

Chuck