MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Alfranio Correia Date:August 13 2009 4:38pm
Subject:Re: bzr commit into mysql-5.1-bugteam branch (alfranio.correia:3053)
Bug#46130
View as plain text  
Hi Jasonh,

I kept your original proposal and called ha_autocommit_or_rollback()

Cheers.

Alfranio Correia wrote:
> He Zhenxing wrote:
>   
>> Alfranio Correia wrote:
>>   
>>     
>>> He Zhenxing wrote:
>>>     
>>>       
>>>> Hi Alfranio,
>>>>
>>>> Thanks for the work, I have some comments, please see below!
>>>>   
>>>>       
>>>>         
>>> Hi Jasonh,
>>>
>>> Thank you for pointing out the close_thread_tables().
>>> I've read the comments but did not look into the body of the function.
>>> And the comments say nothing about rolling back the current statement.
>>>
>>> I partially agree with you. The fact that I did not see any rollback lead
>>> me to introduce all the rollbacks in this patch and write a bad description
>>> on the bug.
>>>
>>> The problem happens not only with concurrency errors but with any
>>> expected error that
>>> does not pop up on the slave. Note that the rollback is only called if
>>> THD::is_error() returns
>>> true. So instead of directly calling any rollback function, I will call
>>> my_error in such cases.
>>>
>>> What do you think?
>>>     
>>>       
>> I think this is good, one question is what error code do you plan to
>> use?
>>   
>>     
> Hi Jasonh,
>
> I am not sure yet. I thought about creating a new error.
> I will check if there is no side effect with this idea and
> try to commit a patch later on today .
>
> Cheers.
>   
>>   
>>     
>>> Cheers.
>>>     
>>>       
>>>> Alfranio Correia wrote:
>>>>   
>>>>       
>>>>         
>>>>> #At
> file:///home/acorreia/workspace.sun/repository.mysql/bzrwork/bug-46130/mysql-5.1-bugteam/
> based on revid:satya.bn@stripped
>>>>>
>>>>>  3053 Alfranio Correia	2009-08-04
>>>>>       BUG#46130 Slave does not correctly handle "expected errors"
>>>>>       
>>>>>       Slave does not correctly handle "expected errors" leading to
> inconsistencies
>>>>>       between the mater and slave. Specifically, when a statement
> changes both
>>>>>       transactional and non-transactional tables, the transactional
> changes are
>>>>>       automatically rolled back on the master but the slave ignores
> the error and
>>>>>       does not rollback them thus leading to inconsistencies.
>>>>>       
>>>>>       To fix the problem, we automatically rollback a statement that
> fails on
>>>>>       the slave but note that the transaction is not rolled back
> unless a "rollback"
>>>>>       command is in the relay log file.
>>>>>
>>>>>     modified:
>>>>>       mysql-test/suite/rpl/r/rpl_concurrency_error.result
>>>>>       mysql-test/suite/rpl/t/rpl_concurrency_error.test
>>>>>       sql/log_event.cc
>>>>> === modified file
> 'mysql-test/suite/rpl/r/rpl_concurrency_error.result'
>>>>> --- a/mysql-test/suite/rpl/r/rpl_concurrency_error.result	2009-07-18
> 20:07:56 +0000
>>>>> +++ b/mysql-test/suite/rpl/r/rpl_concurrency_error.result	2009-08-04
> 13:52:37 +0000
>>>>> @@ -7,7 +7,7 @@ start slave;
>>>>> 
> ########################################################################
>>>>>  #                             Environment
>>>>> 
> ########################################################################
>>>>> -CREATE TABLE t (i INT, PRIMARY KEY(i), f CHAR(8)) engine = Innodb;
>>>>> +CREATE TABLE t (i INT, PRIMARY KEY(i), f CHAR(32)) engine = Innodb;
>>>>>  CREATE TABLE n (d DATETIME, f CHAR(32)) engine = MyIsam;
>>>>>  CREATE TRIGGER tr AFTER UPDATE ON t FOR EACH ROW 
>>>>>  BEGIN 
>>>>> @@ -101,6 +101,8 @@ master-bin.000001	#	Query	#	#	use `test`
>>>>>  master-bin.000001	#	Query	#	#	use `test`; INSERT INTO t VALUES (6 +
> (1 * 10),"brown")
>>>>>  master-bin.000001	#	Query	#	#	use `test`; INSERT INTO n VALUES
> (now(),"brown")
>>>>>  master-bin.000001	#	Xid	#	#	COMMIT /* XID */
>>>>> +source include/diff_master_slave.inc;
>>>>> +source include/diff_master_slave.inc;
>>>>> 
> ########################################################################
>>>>>  #                                Cleanup
>>>>> 
> ########################################################################
>>>>>
>>>>> === modified file
> 'mysql-test/suite/rpl/t/rpl_concurrency_error.test'
>>>>> --- a/mysql-test/suite/rpl/t/rpl_concurrency_error.test	2009-07-18
> 20:07:56 +0000
>>>>> +++ b/mysql-test/suite/rpl/t/rpl_concurrency_error.test	2009-08-04
> 13:52:37 +0000
>>>>> @@ -26,7 +26,7 @@
>>>>>  --echo
> ########################################################################
>>>>>  connection master;
>>>>>  
>>>>> -CREATE TABLE t (i INT, PRIMARY KEY(i), f CHAR(8)) engine = Innodb;
>>>>> +CREATE TABLE t (i INT, PRIMARY KEY(i), f CHAR(32)) engine = Innodb;
>>>>>  CREATE TABLE n (d DATETIME, f CHAR(32)) engine = MyIsam;
>>>>>  
>>>>>  DELIMITER |;
>>>>> @@ -125,14 +125,13 @@ while ($type)
>>>>>  connection master;
>>>>>  sync_slave_with_master;
>>>>>  
>>>>> -# Re-enable this after fixing BUG#46130
>>>>> -#connection master;
>>>>> -#let $diff_statement= SELECT * FROM t order by i;
>>>>> -#source include/diff_master_slave.inc;
>>>>> -
>>>>> -#connection master;
>>>>> -#let $diff_statement= SELECT * FROM n order by d, f;
>>>>> -#source include/diff_master_slave.inc;
>>>>> +connection master;
>>>>> +let $diff_statement= SELECT * FROM t order by i;
>>>>> +source include/diff_master_slave.inc;
>>>>> +
>>>>> +connection master;
>>>>> +let $diff_statement= SELECT * FROM n order by d, f;
>>>>> +source include/diff_master_slave.inc;
>>>>>  
>>>>>  --echo
> ########################################################################
>>>>>  --echo #                                Cleanup
>>>>>
>>>>> === modified file 'sql/log_event.cc'
>>>>> --- a/sql/log_event.cc	2009-07-24 16:04:55 +0000
>>>>> +++ b/sql/log_event.cc	2009-08-04 13:52:37 +0000
>>>>> @@ -3157,7 +3157,6 @@ START SLAVE; . Query: '%s'", expected_er
>>>>>        general_log_write(thd, COM_QUERY, thd->query,
> thd->query_length);
>>>>>  
>>>>>  compare_errors:
>>>>> -
>>>>>       /*
>>>>>        If we expected a non-zero error code, and we don't get the
> same error
>>>>>        code, and none of them should be ignored.
>>>>> @@ -3181,6 +3180,7 @@ Default database: '%s'. Query: '%s'",
>>>>>                        actual_error,
>>>>>                        print_slave_db_safe(db), query_arg);
>>>>>        thd->is_slave_error= 1;
>>>>> +      ha_rollback_trans(thd, FALSE);
>>>>>     
>>>>>         
>>>>>           
>>>> I think this is not necessary, this case and all cases other than
>>>> concurrency errors have already been handled by the call to
>>>> close_thread_tables() later, which calls ha_autocommit_or_rollback().
>>>>
>>>> So I think the call to ha_rollback_trans should be removed here.
>>>>
>>>>   
>>>>       
>>>>         
>>>>>      }
>>>>>      /*
>>>>>        If we get the same error code as expected, or they should be
> ignored. 
>>>>> @@ -3190,14 +3190,19 @@ Default database: '%s'. Query: '%s'",
>>>>>   	     ignored_error_code(actual_error))
>>>>>      {
>>>>>        DBUG_PRINT("info",("error ignored"));
>>>>> +      if (actual_error != 0 &&
> !ignored_error_code(actual_error))
>>>>> +        ha_rollback_trans(thd, FALSE);
>>>>>     
>>>>>         
>>>>>           
>>>> See above, remove the call.
>>>>
>>>>   
>>>>       
>>>>         
>>>>>        clear_all_errors(thd,
> const_cast<Relay_log_info*>(rli));
>>>>>        thd->killed= THD::NOT_KILLED;
>>>>>      }
>>>>> +    else if (concurrency_error_code(expected_error))
>>>>> +      ha_rollback_trans(thd, FALSE);
>>>>>     
>>>>>         
>>>>>           
>>>> I think this is the only case that need to be handled, and I think
>>>> ha_autocommit_or_rollback() should be called instead of
>>>> ha_rollback_trans().
>>>>
>>>>   
>>>>       
>>>>         
>>>>>      /*
>>>>>        Other cases: mostly we expected no error and get one.
>>>>>      */
>>>>>      else if (thd->is_slave_error || thd->is_fatal_error)
>>>>>      {
>>>>> +      ha_rollback_trans(thd, FALSE);
>>>>>     
>>>>>         
>>>>>           
>>>> Not needed, remove the call.
>>>>
>>>>   
>>>>       
>>>>         
>>>>>        rli->report(ERROR_LEVEL, actual_error,
>>>>>                        "Error '%s' on query. Default database: '%s'.
> Query: '%s'",
>>>>>                        (actual_error ? thd->main_da.message() :
>>>>>
>>>>>     
>>>>>         
>>>>>           
>>>>   
>>>>       
>>>>         
>>>     
>>>       
>>   
>>     
>
>
>   

Thread
bzr commit into mysql-5.1-bugteam branch (alfranio.correia:3053)Bug#46130Alfranio Correia4 Aug
  • Re: bzr commit into mysql-5.1-bugteam branch (alfranio.correia:3053)Bug#46130He Zhenxing11 Aug
    • Re: bzr commit into mysql-5.1-bugteam branch (alfranio.correia:3053)Bug#46130Alfranio Correia12 Aug
      • Re: bzr commit into mysql-5.1-bugteam branch (alfranio.correia:3053)Bug#46130He Zhenxing13 Aug
        • Re: bzr commit into mysql-5.1-bugteam branch (alfranio.correia:3053)Bug#46130Alfranio Correia13 Aug
          • Re: bzr commit into mysql-5.1-bugteam branch (alfranio.correia:3053)Bug#46130Alfranio Correia13 Aug