Hi Andrei,
Thank you for your review!
Andrei Elkin wrote:
> He Zhenxing, hello.
>
> The work looks good.
>
> > #At file:///media/sdb2/hezx/work/mysql/bzrwork/b37145/5.0-bugteam/
> >
> > 2714 He Zhenxing 2009-03-11
> > BUG#37145 Killing a statement doing DDL may log binlog event with error
> code 1053
> >
>
[snip]
> > +# Aquire the debug lock again if used
> ~c~
> typo
>
will be fixed.
[snip]
> > === added file 'mysql-test/t/rpl_killed_ddl-master.opt'
> > --- a/mysql-test/t/rpl_killed_ddl-master.opt 1970-01-01 00:00:00 +0000
> > +++ b/mysql-test/t/rpl_killed_ddl-master.opt 2009-03-11 07:22:08 +0000
>
> > @@ -0,0 +1 @@
> > +--debug=d,debug_lock_before_query_log_event
> > \ No newline at end of file
>
> Why?
>
will add a newline to stop bzr complaining
[snip]
> > === modified file 'sql/log_event.cc'
> > --- a/sql/log_event.cc 2009-03-05 10:10:44 +0000
> > +++ b/sql/log_event.cc 2009-03-11 07:22:08 +0000
> > @@ -1358,6 +1358,17 @@ Query_log_event::Query_log_event(THD* th
> > {
> > time_t end_time;
> >
> > + /*
> > + XXXX: The call to DBUG_SYNC_POINT() will free all user locks,
> > + which can cause problem if user want to use user lock for other
> > + purposes. In order to overcome this problem, wrap it with a
> > + DBUG_EXECUTE_IF, so that it will only be activated if keyword
> > + 'debug_lock_before_query_log_event' is included in the 'debug'
> > + option, and will not fiddling user locks otherwise.
> > + */
>
> Wrt the comments, could you please remove XXXX?
> Second, it'd be good to mention the fact of freeing right in the
> absent heading comments for debug_sync_point(). I suggest to add
> a line like ` DBUG_SYNC_POINT() will free all user locks' to there.
>
good
> > + DBUG_EXECUTE_IF("debug_lock_before_query_log_event",
> > + DBUG_SYNC_POINT("debug_lock.before_query_log_event", 10););
> > +
> > if (killed_status_arg == THD::KILLED_NO_VALUE)
> > killed_status_arg= thd_arg->killed;
> > error_code=
> >
>
> > === modified file 'sql/log_event.h'
> > --- a/sql/log_event.h 2009-03-05 10:10:44 +0000
> > +++ b/sql/log_event.h 2009-03-11 07:22:08 +0000
> > @@ -815,7 +815,7 @@ public:
> >
> > Query_log_event(THD* thd_arg, const char* query_arg, ulong query_length,
> > bool using_trans, bool suppress_use,
> > - THD::killed_state killed_err_arg= THD::KILLED_NO_VALUE);
> > + THD::killed_state killed_err_arg);
>
> Okay, so Query_log_event() won't have the default arg.
> Therefore it makes important to document
> when it's supposed for
>
> killed_err_arg :=
>
> THD::KILLED_NO_VALUE
>
> and when it's
>
> THD::NOT_KILLED
>
> Could you please add a rule of thumb here in the interface file?
> Something like killed_err_arg ` THD::NOT_KILLED ' stands for if there
> was killing then it was not effective, otherwise use `THD::KILLED_NO_VALUE'
> (which I personally would preserve as the default as it represents a "normal"
> situation).
>
OK
[snip]
