Re: bk commit into 5.0 tree (hezx:1.2557) BUG#30998

[Andrei Elkin]

Zhenxing, hi.


The patch is a good work.
I have some comments.

After fixing the problem description and highly desireble identifier
change the patch can go to further stages.

> Below is the list of changes that have just been committed into a local
> 5.0 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-11-30 12:27:25+08:00, hezx@hezx.(none) +3 -0
>   Bug#30998 Drop View breaks replication if view does not exist
>   

Please state the problem first. 

>   Writing binlog after check for errors, if have dropped some views, log the event
> with error code. 

Solution is okay although I'd say "if
at least one view has been dropped the query is bin-logged possible
with an error." 


>
>   mysql-test/r/rpl_drop_view.result@stripped, 2007-11-30 11:12:11+08:00, hezx@hezx.(none)
> +23 -0
>     BitKeeper file
> /media/hda5/work/mysql/bkwork/5.0/bug30998-drop-view-breaks-rpl/mysql-test/r/rpl_drop_view.result
>
>   mysql-test/r/rpl_drop_view.result@stripped, 2007-11-30 11:12:11+08:00, hezx@hezx.(none)
> +0 -0
>
>   mysql-test/t/rpl_drop_view.test@stripped, 2007-11-30 10:26:41+08:00, hezx@hezx.(none)
> +27 -0
>     BitKeeper file
> /media/hda5/work/mysql/bkwork/5.0/bug30998-drop-view-breaks-rpl/mysql-test/t/rpl_drop_view.test
>
>   mysql-test/t/rpl_drop_view.test@stripped, 2007-11-30 10:26:41+08:00, hezx@hezx.(none) +0
> -0
>
>   sql/sql_view.cc@stripped, 2007-11-30 10:38:16+08:00, hezx@hezx.(none) +19 -13
>     do binlog after check for errors, if no error, log the event, else if we have
> dropped some views, log the event with error code, otherwise ignore the event.
>
> diff -Nrup a/mysql-test/r/rpl_drop_view.result b/mysql-test/r/rpl_drop_view.result
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/r/rpl_drop_view.result	2007-11-30 11:12:11 +08:00
> @@ -0,0 +1,23 @@
> +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, 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
> diff -Nrup a/mysql-test/t/rpl_drop_view.test b/mysql-test/t/rpl_drop_view.test
> --- /dev/null	Wed Dec 31 16:00:00 196900
> +++ b/mysql-test/t/rpl_drop_view.test	2007-11-30 10:26:41 +08:00
> @@ -0,0 +1,27 @@
> +# 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, 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;
> +--error 1146
> +select * from v1;
> diff -Nrup a/sql/sql_view.cc b/sql/sql_view.cc
> --- a/sql/sql_view.cc	2007-09-28 03:45:36 +08:00
> +++ b/sql/sql_view.cc	2007-11-30 10:38:16 +08:00
> @@ -1424,6 +1424,8 @@ bool mysql_drop_view(THD *thd, TABLE_LIS
>    String non_existant_views;
>    char *wrong_object_db= NULL, *wrong_object_name= NULL;
>    bool error= FALSE;

> +  bool dropped_view= FALSE;
> +  bool something_wrong= FALSE;

You can find a similar handling of DROP table in
 mysql_rm_table_part2():

   if (some_tables_deleted || tmp_table_deleted || !error)

I suggest to stick to that style for identifiers (there is no temp
view for our pleasure :).

So

bool some_views_deleted;

>  
>    VOID(pthread_mutex_lock(&LOCK_open));
>    for (view= views; view; view= view->next_local)
> @@ -1462,33 +1464,37 @@ bool mysql_drop_view(THD *thd, TABLE_LIS
>      }
>      if (my_delete(path, MYF(MY_WME)))
>        error= TRUE;
> +    dropped_view= TRUE;
>      query_cache_invalidate3(thd, view, 0);
>      sp_cache_invalidate();
>    }
> -  if (mysql_bin_log.is_open())
> -  {
> -    thd->clear_error();
> -    Query_log_event qinfo(thd, thd->query, thd->query_length, 0, FALSE);
> -    mysql_bin_log.write(&qinfo);
> -  }
> -
> -  VOID(pthread_mutex_unlock(&LOCK_open));
>  

> -  if (error)
> -  {
> -    DBUG_RETURN(TRUE);
> -  }

Ok, you groupped this block with other failure exits.


>    if (wrong_object_name)
>    {
>      my_error(ER_WRONG_OBJECT, MYF(0), wrong_object_db, wrong_object_name, 
>               "VIEW");
> -    DBUG_RETURN(TRUE);
>    }
>    if (non_existant_views.length())
>    {
>      my_error(ER_BAD_TABLE_ERROR, MYF(0), non_existant_views.c_ptr());
> +  }
> +
> +  something_wrong= error || wrong_object_name || non_existant_views.length();
> +  if (dropped_view || !something_wrong)
> +  {
> +    if (!something_wrong)
> +      thd->clear_error();
> +    Query_log_event qinfo(thd, thd->query, thd->query_length, 0, FALSE);
> +    mysql_bin_log.write(&qinfo);
> +  }
> +
> +  VOID(pthread_mutex_unlock(&LOCK_open));
> +
> +  if (something_wrong)
> +  {
>      DBUG_RETURN(TRUE);
>    }
> +
>    send_ok(thd);
>    DBUG_RETURN(FALSE);
>  }
>

cheers,


/andrei