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

[He Zhenxing]

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

And do I have to fix these before push? 

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