MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:He Zhenxing Date:September 21 2009 3:34am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:2973)
Bug#37148
View as plain text  
Alfranio Correia wrote:
> Hi all,
> 
> I agree with Jasonh however I think we should do two things as soon as
> possible:
> 
> 1 - *Create* a worklog with the intend to write a test case for this patch.
> 

OK, will create one.

> 2 - Discuss and define what is the pattern regarding error propagation:
> return error
> code or use thd.
> 

OK

> Cheers
> 
> He Zhenxing wrote:
> > Andrei Elkin wrote:
> >   
> >> Zhen Xing, hello.
> >>
> >>     
> >>> #At file:///media/sdb2/hezx/work/mysql/bzrwork/b37148/5.1-bugteam/ based
> on revid:aelkin@stripped
> >>>
> >>>  2973 He Zhenxing	2009-08-20
> >>>       BUG#37148 Most callers of mysql_bin_log.write ignore the return
> result
> >>>       
> >>>       The return value of mysql_bin_log.write was ignored by most
> callers,
> >>>       which may lead to inconsistent on master and slave if the
> transaction
> >>>       was committed while the binlog was not correctly written.
> >>>       
> >> I think it's important either to provide a test case proving the
> >> complaining lines are not longer valid after your patch, or to
> >> describe specifically how incorectness can be gained.
> >>
> >>     
> >
> > I think we already have decided on this in the conference meeting that a
> > test case is not needed for this.
> >
> > Please check this:
> >  
> https://intranet.mysql.com/~lthalmann/ltplan.pl?file=mom-rpl.org&section=MOM-RPL-090902
> >
> >   
> >> Before to dwell into details of your patch I would like to 
> >> to follow our chat on #rep help on Sep 7th,
> >>
> >>    Sep 07 15:57:06 <andrei>	jasonh, as it looks to me, we actually
> should be able to send any error to the client as well as log an incident to the error
> log, the latter applies to the slave as well.
> >>    Sep 07 15:59:48 <andrei>	jasonh, i mean the error log of the slave.
> As the reporter claimed
> >>    Sep 07 16:00:00 <andrei>	"It is a bad idea to rely on clients to
> >>    Sep 07 16:00:00 <andrei>	log and report all error messages
> especially if the client is the SQL slave thread when
> >>    Sep 07 16:00:00 <andrei>	the slave is also writing a binlog.
> >>    Sep 07 16:00:02 <andrei>	"
> >>    Sep 07 16:01:09 <andrei>	but there are sql_print_error() or
> something all around.
> >>    Sep 07 16:01:50 <andrei>	jasonh, maybe we should first identify
> what are cases when the slave sql thread does not report to the slave server error log?
> >>
> >> Analysis of some cases, I don't claim that all cases, confirms no need
> >> for propagating an error gained inside of MYSQL_BIN_LOG::write() via
> >> the return value.
> >> Once an error has happened for instance in the bottom of the following
> stack,
> >>
> >> THD::binlog_query(
> >>   THD::binlog_flush_pending_rows_event(
> >>     MYSQL_BIN_LOG::flush_and_set_pending_rows_event(
> >>      set_write_error(thd);
> >>
> >> the bottom line function is to call
> >>  
> >>     my_error(ER_ERROR_ON_WRITE, MYF(MY_WME), name, errno) or friends
> >>
> >> which is really takes place.
> >> On the following execution path the error will be found -
> >>
> >>     net_end_statement(thd) -
> >>
> >> and the client be notified.
> >>
> >> As it stands the whole idea of the bug complaint is to optimize the error
> >> path in that to react on an error condition sooner and not to do some
> >> extra work.
> >>     
> >
> > Yes, I think it's good to react on the error sooner, for example if we
> > are doing a LOAD DATA statement to load a big hunk of data and if
> > binlogging fails at the beginning, it would be much better to check for
> > this error and quit the statement early then wait until the end of the
> > statement. And I also think it would make the code clearer and less
> > error prone.
> >
> >   
> >> Per the quoted logging to the error log of the slave, we indeed have
> >> to make sure that an error incident like in the quoted stack is really
> >> reported to the error log of the slave server.
> >>
> >> Does this setup make sense from your point of view?
> >>
> >>     
> >>>       
> >>>       This fixed the problem by let the caller to check and handle the
> >>>       return value of mysql_bin_log.write. This patch only adresses the
> >>>       simple cases.
> >>>       
> >> Could you please list what has been fixed and what is leaving out?
> >>
> >>     
> >
> > Usually this patch only addresses the direct caller of
> > mysql_bin_log.write or thd->binlog_query, it does not try to address the
> > caller of the caller unless it's very easy to fix that.
> >
> >   
> >>     
> >>>     M  sql/events.cc
> >>>     M  sql/ha_ndbcluster_binlog.cc
> >>>     M  sql/log.cc
> >>>     M  sql/log_event.cc
> >>>     M  sql/log_event_old.cc
> >>>     M  sql/mysql_priv.h
> >>>     M  sql/rpl_injector.cc
> >>>     M  sql/sp.cc
> >>>     M  sql/sp_head.cc
> >>>     M  sql/sql_acl.cc
> >>>     M  sql/sql_base.cc
> >>>     M  sql/sql_class.h
> >>>     M  sql/sql_db.cc
> >>>     M  sql/sql_delete.cc
> >>>     M  sql/sql_insert.cc
> >>>     M  sql/sql_load.cc
> >>>     M  sql/sql_parse.cc
> >>>     M  sql/sql_partition.cc
> >>>     M  sql/sql_rename.cc
> >>>     M  sql/sql_repl.cc
> >>>     M  sql/sql_table.cc
> >>>     M  sql/sql_tablespace.cc
> >>>     M  sql/sql_trigger.cc
> >>>     M  sql/sql_udf.cc
> >>>     M  sql/sql_update.cc
> >>>     M  sql/sql_view.cc
> >>>       
> >> Even though changes are straightforward I suggest to attach per-file
> >> comments. For many in your patch that would be a copy-paste "changed
> >> signature with ..." or "propagated the error..."
> >>
> >>     
> >
> > OK
> >
> >   
> >> I don't comment on anything more before we have set our minds on the same
> >> page about top level issue.
> >>
> >> cheers,
> >>
> >> Andrei
> >>
> >>
> >>     
> >>> === modified file 'sql/events.cc'
> >>> --- a/sql/events.cc	2009-04-09 06:22:06 +0000
> >>> +++ b/sql/events.cc	2009-08-20 09:53:53 +0000
> >>> @@ -439,7 +439,7 @@ Events::create_event(THD *thd, Event_par
> >>>      {
> >>>        /* Binlog the create event. */
> >>>        DBUG_ASSERT(thd->query && thd->query_length);
> >>> -      write_bin_log(thd, TRUE, thd->query, thd->query_length);
> >>> +      ret= write_bin_log(thd, TRUE, thd->query,
> thd->query_length);
> >>>      }
> >>>    }
> >>>       
> >> ...
> >>
> >>     
> >
> >   
> 
> 

Thread
bzr commit into mysql-5.1-bugteam branch (zhenxing.he:2973) Bug#37148He Zhenxing20 Aug
  • Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:2973)Bug#37148Andrei Elkin17 Sep
    • Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:2973)Bug#37148He Zhenxing19 Sep
      • Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:2973)Bug#37148Alfranio Correia20 Sep
        • Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:2973)Bug#37148He Zhenxing21 Sep
      • Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:2973)Bug#37148Andrei Elkin21 Sep
        • Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:2973)Bug#37148He Zhenxing22 Sep
          • Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:2973)Bug#37148Andrei Elkin22 Sep
            • Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:2973)Bug#37148He Zhenxing23 Sep
              • Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:2973)Bug#37148Andrei Elkin23 Sep
                • Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:2973)Bug#37148Andrei Elkin23 Sep
                  • Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:2973)Bug#37148He Zhenxing24 Sep
        • Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:2973)Bug#37148Alfranio Correia22 Sep
          • Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:2973)Bug#37148He Zhenxing22 Sep