Re: bk commit into 5.1 tree (hezx:1.2632) BUG#30998

[Mats Kindahl]

He Zhenxing wrote:
> Hi Mats
>
> Thank you for the explanation and instructions.
>   

Hi Jason!

I assume you have read the Coding Guidelines?

    http://forge.mysql.com/wiki/MySQL_Internals_Coding_Guidelines

The rules are somewhat odd, but these are the ones we have. They are 
quite a few years old, and I think that Monty is the main influence for 
them.

Just my few cents,
Mats Kindahl

> On 2007-12-05 Wed 06:58 +0100，Mats Kindahl wrote：
>   
>> He Zhenxing wrote:
>>     
>>> Hi Mats
>>>
>>> Thank you for the review and good comments!
>>>
>>> About the comment style, I will following your style. But as to the
>>> brace usage of if statement, if it's not a big issue, I'd prefer to keep
>>> my style, because I think it's clearer, and I don't need to add or
>>> remove the braces next time if I add or remove some code of the if
>>> statement. But if it is a big issue, I will follow you style :)
>>>   
>>>       
>> Hi Jason!
>>
>> I'm merely commenting on the style, but since people have slightly 
>> different ideas about how to write code, I usually do not bother. You 
>> will in general find comments from me on all patches, but it is not 
>> necessarily the case that they prevent a push. I just comment so that 
>> people can improve in their programming skills.
>>
>>     
>>> And do I have to fix these before push? 
>>>   
>>>       
>> You need two reviews usually before pushing a patch, so once you have 
>> two people ticking the review boxes, you can set it to "Patch approved", 
>> merge and build (to check that nothing broke in the merge), and 
>> sometimes run the test suite after the merge as well. After that, the 
>> patch can be pushed to the team tree, and the state of the bug is set to 
>> "Patch queued".
>>
>> I usually comment on all patches, but if I say that a patch is approved 
>> or OK to push, you just need to read the comments and see if there is 
>> anything that you should consider. I try to be very clear about what I 
>> want to be done before approving the patch.
>>
>> Just my few cents,
>> Mats Kindahl
>>     
>>> On 2007-12-04 Tue 13:42 +0100，Mats Kindahl wrote：
>>>   
>>>       
>>>> Hi Jason!
>>>>
>>>> Patch is OK to push, but please read my comments below.
>>>>
>>>> Just my few cents,
>>>> Mats Kindahl
>>>>
>>>> hezx@stripped wrote:
>>>>     
>>>>         
>>>>> Below is the list of changes that have just been committed into a
> local
>>>>> 5.1 repository of hezx. When hezx does a push these changes will
>>>>> be propagated to the main repository and, within 24 hours after the
>>>>> push, to the public repository.
>>>>> For information on how to access the public repository
>>>>> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>>>>>
>>>>> ChangeSet@stripped, 2007-12-03 16:54:44+08:00, hezx@hezx.(none) +3 -0
>>>>>   Bug#30998 Drop View breaks replication if view does not exist
>>>>>   
>>>>>   When executing drop view statement on the master, the statement is
> not written into bin-log if any error occurs, this could cause master slave inconsistence
> if any view has been dropped. 
>>>>>   
>>>>>   If some error occured and no view has been dropped, don't bin-log
> the statement, if at least one view has been dropped the query is bin-logged possible with
> an error.
>>>>>
>>>>>   mysql-test/suite/rpl/r/rpl_drop_view.result@stripped, 2007-12-03
> 16:54:39+08:00, hezx@hezx.(none) +27 -0
>>>>>     Add test result for bug#30998
>>>>>
>>>>>   mysql-test/suite/rpl/r/rpl_drop_view.result@stripped, 2007-12-03
> 16:54:39+08:00, hezx@hezx.(none) +0 -0
>>>>>
>>>>>   mysql-test/suite/rpl/t/rpl_drop_view.test@stripped, 2007-12-03
> 16:54:39+08:00, hezx@hezx.(none) +31 -0
>>>>>     Add test case for bug#30998
>>>>>
>>>>>   mysql-test/suite/rpl/t/rpl_drop_view.test@stripped, 2007-12-03
> 16:54:39+08:00, hezx@hezx.(none) +0 -0
>>>>>
>>>>>   sql/sql_view.cc@stripped, 2007-12-03 16:54:39+08:00, hezx@hezx.(none)
> +18 -11
>>>>>     If at least one view has been dropped the query is bin-logged
> possible with an error.
>>>>>
>>>>> diff -Nrup a/mysql-test/suite/rpl/r/rpl_drop_view.result
> b/mysql-test/suite/rpl/r/rpl_drop_view.result
>>>>> --- /dev/null	Wed Dec 31 16:00:00 196900
>>>>> +++ b/mysql-test/suite/rpl/r/rpl_drop_view.result	2007-12-03 16:54:39
> +08:00
>>>>> @@ -0,0 +1,27 @@
>>>>> +stop slave;
>>>>> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
>>>>> +reset master;
>>>>> +reset slave;
>>>>> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
>>>>> +start slave;
>>>>> +drop table if exists t1, t2;
>>>>> +drop view if exists v1, v2, v3, not_exist_view;
>>>>> +create table t1 (a int);
>>>>> +create table t2 (b int);
>>>>> +create table t3 (c int);
>>>>> +create view v1 as select * from t1;
>>>>> +create view v2 as select * from t2;
>>>>> +create view v3 as select * from t3;
>>>>> +drop view not_exist_view;
>>>>> +ERROR 42S02: Unknown table 'not_exist_view'
>>>>> +drop view v1, not_exist_view;
>>>>> +ERROR 42S02: Unknown table 'not_exist_view'
>>>>> +select * from v1;
>>>>> +ERROR 42S02: Table 'test.v1' doesn't exist
>>>>> +drop view v2, v3;
>>>>> +select * from v1;
>>>>> +ERROR 42S02: Table 'test.v1' doesn't exist
>>>>> +select * from v2;
>>>>> +ERROR 42S02: Table 'test.v2' doesn't exist
>>>>> +select * from v3;
>>>>> +ERROR 42S02: Table 'test.v3' doesn't exist
>>>>> diff -Nrup a/mysql-test/suite/rpl/t/rpl_drop_view.test
> b/mysql-test/suite/rpl/t/rpl_drop_view.test
>>>>> --- /dev/null	Wed Dec 31 16:00:00 196900
>>>>> +++ b/mysql-test/suite/rpl/t/rpl_drop_view.test	2007-12-03 16:54:39
> +08:00
>>>>> @@ -0,0 +1,31 @@
>>>>> +# test case for bug#30998
>>>>> +# Drop View breaks replication if view does not exist
>>>>> +#
>>>>> +
>>>>> +source include/master-slave.inc;
>>>>> +--disable_warnings
>>>>> +drop table if exists t1, t2;
>>>>> +drop view if exists v1, v2, v3, not_exist_view;
>>>>> +--enable_warnings
>>>>> +create table t1 (a int);
>>>>> +create table t2 (b int);
>>>>> +create table t3 (c int);
>>>>> +create view v1 as select * from t1;
>>>>> +create view v2 as select * from t2;
>>>>> +create view v3 as select * from t3;
>>>>> +--error 1051
>>>>> +drop view not_exist_view;
>>>>> +--error 1051
>>>>> +drop view v1, not_exist_view;
>>>>> +--error 1146
>>>>> +select * from v1;
>>>>> +drop view v2, v3;
>>>>> +save_master_pos;
>>>>> +connection slave;
>>>>> +sync_with_master;
>>>>>   
>>>>>       
>>>>>           
>>>> These three lines can be replaced with sync_slave_with_master.
>>>>
>>>>
>>>>     
>>>>         
>>>>> +--error 1146
>>>>> +select * from v1;
>>>>> +--error 1146
>>>>> +select * from v2;
>>>>> +--error 1146
>>>>> +select * from v3;
>>>>> diff -Nrup a/sql/sql_view.cc b/sql/sql_view.cc
>>>>> --- a/sql/sql_view.cc	2007-10-31 01:08:13 +08:00
>>>>> +++ b/sql/sql_view.cc	2007-12-03 16:54:39 +08:00
>>>>> @@ -1465,6 +1465,8 @@ bool mysql_drop_view(THD *thd, TABLE_LIS
>>>>>    char *wrong_object_db= NULL, *wrong_object_name= NULL;
>>>>>    bool error= FALSE;
>>>>>    enum legacy_db_type not_used;
>>>>> +  bool some_views_deleted= FALSE;
>>>>> +  bool something_wrong= FALSE;
>>>>>    DBUG_ENTER("mysql_drop_view");
>>>>>  
>>>>>    VOID(pthread_mutex_lock(&LOCK_open));
>>>>> @@ -1506,6 +1508,8 @@ bool mysql_drop_view(THD *thd, TABLE_LIS
>>>>>      if (my_delete(path, MYF(MY_WME)))
>>>>>        error= TRUE;
>>>>>  
>>>>> +    some_views_deleted= TRUE;
>>>>> +
>>>>>      /*
>>>>>        For a view, there is only one table_share object which should
> never
>>>>>        be used outside of LOCK_open
>>>>> @@ -1523,29 +1527,32 @@ bool mysql_drop_view(THD *thd, TABLE_LIS
>>>>>      sp_cache_invalidate();
>>>>>    }
>>>>>  
>>>>> -  if (error)
>>>>> -  {
>>>>> -    VOID(pthread_mutex_unlock(&LOCK_open));
>>>>> -    DBUG_RETURN(TRUE);
>>>>> -  }
>>>>>    if (wrong_object_name)
>>>>>    {
>>>>> -    VOID(pthread_mutex_unlock(&LOCK_open));
>>>>>      my_error(ER_WRONG_OBJECT, MYF(0), wrong_object_db,
> wrong_object_name, 
>>>>>               "VIEW");
>>>>> -    DBUG_RETURN(TRUE);
>>>>>    }
>>>>>    if (non_existant_views.length())
>>>>>    {
>>>>> -    VOID(pthread_mutex_unlock(&LOCK_open));
>>>>>      my_error(ER_BAD_TABLE_ERROR, MYF(0),
> non_existant_views.c_ptr());
>>>>> -    DBUG_RETURN(TRUE);
>>>>>    }
>>>>>   
>>>>>       
>>>>>           
>>>> Braces not needed any more above.
>>>>
>>>>     
>>>>         
>>>>>  
>>>>> -  write_bin_log(thd, TRUE, thd->query, thd->query_length);
>>>>> +  something_wrong= error || wrong_object_name ||
> non_existant_views.length();
>>>>> +  if (some_views_deleted || !something_wrong)
>>>>> +  {
>>>>> +    /* if something goes wrong, bin-log with possible error code,
>>>>> +       otherwise bin-log with error code cleared.
>>>>> +     */
>>>>>   
>>>>>       
>>>>>           
>>>> Comments should be written:
>>>>
>>>>     /*
>>>>       ...
>>>>     */
>>>>
>>>>     
>>>>         
>>>>> +    write_bin_log(thd, !something_wrong, thd->query,
> thd->query_length);
>>>>> +  }
>>>>>  
>>>>> -  send_ok(thd);
>>>>>    VOID(pthread_mutex_unlock(&LOCK_open));
>>>>> +  
>>>>> +  if (something_wrong)
>>>>> +  {
>>>>> +    DBUG_RETURN(TRUE);
>>>>> +  }
>>>>>   
>>>>>       
>>>>>           
>>>> Braces not needed.
>>>>
>>>>     
>>>>         
>>>>> +  send_ok(thd);
>>>>>    DBUG_RETURN(FALSE);
>>>>>  }
>>>>>  
>>>>>
>>>>>   
>>>>>       
>>>>>           
>>>> -- 
>>>> MySQL Code Commits Mailing List
>>>> For list archives: http://lists.mysql.com/commits
>>>> To unsubscribe:    http://lists.mysql.com/commits?unsub=1
>>>>     
>>>>         
>>>   
>>>       
>> -- 
>> MySQL Code Commits Mailing List
>> For list archives: http://lists.mysql.com/commits
>> To unsubscribe:    http://lists.mysql.com/commits?unsub=1
>>     
>
>   


-- 
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com