MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:He Zhenxing Date:September 19 2009 5:55am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:2973)
Bug#37148
View as plain text  
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