MySQL Lists are EOL. Please join:

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

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

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