Hi Mats
Yes, I have read that, and I agree with you on that some of the rules
are a little bit odd, but once you get used to them, it won't be a
problem any more.
On 2007-12-05 Wed 08:21 +0100，Mats Kindahl wrote：
> 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
> >>
> >
> >
>
>
