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

[Mats Kindahl]

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


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