List:Commits« Previous MessageNext Message »
From:He Zhenxing Date:September 2 2009 10:08am
Subject:Re: bzr commit into mysql-5.1 branch (Dao-Gang.Qu:3095) Bug#45677
View as plain text  
Hi Daogang,

Thank you for the work!

I have some comments, please see below!

Dao-Gang.Qu@stripped wrote:
> #At file:///home/daogangq/mysql/bzrwork/bug45677/mysql-5.1-bugteam/ based on
> revid:azundris@stripped
> 
>  3095 Dao-Gang.Qu@stripped	2009-09-02
>       Bug #45677  	Slave stops with Duplicate entry for key PRIMARY when using
> trigger
>       
>       Concurrent transactions that cause a trigger or call a function to insert or 

I don't think this is related to concurrent transactions, just one query
is enough to fire this problem.

>       update > 1 values into an autoinc column will generate wrong auto_increment
> 

update > 1 autoinc fields is safe (but we do not distinguish this
because it is no possible), and the autoinc value is correct, the
problem is that there is one value associated with the query. 

>       values in statement-based replication, because we just write one specific 
>       'SET INSERT_ID=n' sentence to binlog for the first insert sentence, 
>       the second insert sentence will generate its auto_increment value 
>       base on the current auto_increment value of the table instead of the value  
>       of the last INSERT_ID. So the duplicate entry error will be caused for the 
>       key PRIMARY. In this case, the duplicate entry error can't be avoided in 
>       statement-based replication base on current architecture.
>       

I think dup key is only one scenario of this problem, I'd suggest to
use: data inconsistency between master and slave, which I think is more
general on the idea.

>       In mixed mode, the problem has been resolved by switching to row-based 
>       binlogging.

I'd suggest: The problem is resolved by mark all statements that ... as
unsafe, and will switching to row-format in Mixed mode.

>      @ mysql-test/suite/rpl/r/rpl_auto_increment_update_failure.result
>         Test result for bug#45677
>      @ mysql-test/suite/rpl/t/rpl_auto_increment_update_failure.test
>         Added test to verify if concurrent transactions that cause a trigger or call
>         a function to insert or update > 1 values into an autoinc column can
> cause
>         mixed based replication to break with slave reporting error 1062 (Error
>         'Duplicate entry 'x' for key 'PRIMARY'').
>      @ sql/sql_base.cc
>         Added function 'has_table_with_auto_increment_in_sub_statements' to check 
>         if one (or more) tables have auto_increment columns in sub-statements.
>         
>         Removed function 'has_two_write_locked_tables_with_auto_increment', 
>         because the function is included in function 
>         'has_table_with_auto_increment_in_sub_statements'.
> 
>     added:
>       mysql-test/suite/rpl/r/rpl_auto_increment_update_failure.result
>       mysql-test/suite/rpl/t/rpl_auto_increment_update_failure.test
>     modified:
>       sql/sql_base.cc
> === added file 'mysql-test/suite/rpl/r/rpl_auto_increment_update_failure.result'
> --- a/mysql-test/suite/rpl/r/rpl_auto_increment_update_failure.result	1970-01-01
> 00:00:00 +0000
> +++ b/mysql-test/suite/rpl/r/rpl_auto_increment_update_failure.result	2009-09-02
> 05:23:51 +0000
> @@ -0,0 +1,209 @@
> +stop slave;
> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> +reset master;
> +reset slave;
> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> +start slave;
> +drop table if exists t10,t11,t12;
> +DROP FUNCTION IF EXISTS f1_simple_insert;
> +use test;
> +set autocommit=0;
> +#Test case1: INSERT with AFTER/BEFOR TRIGGER
> +create table t1(f1 int) engine=innodb;
> +create table t2(i1 int not null auto_increment, f1 int, primary key(i1))
> engine=innodb;
> +create trigger tr1 after insert on t1 for each row insert into t2(f1)
> values(new.f1);
> +insert into t1(f1) values(1),(2);
> +insert into t1(f1) values(3);
> +#Test case2: UPDATE with AFTER/BEFOR TRIGGER
> +create table t3(f1 int, f2 int) engine=innodb;
> +create table t4(i1 int not null auto_increment, f1 int, primary key(i1))
> engine=innodb;
> +create trigger tr2 before update on t3 for each row insert into t4(f1)
> values(new.f1);
> +insert into t3(f1,f2) values(1,1);
> +insert into t3(f1,f2) values(2,1);
> +update t3 set f1 = f1 + 5 where f2 = 1;
> +#Test case3: DELETE with AFTER/BEFOR TRIGGER
> +create table t5(f1 int, f2 int) engine=innodb;
> +create table t6(i1 int not null auto_increment, f1 int, primary key(i1))
> engine=innodb;
> +create trigger tr3  before delete on t5 for each row insert into t6(f1) values(6);
> +insert into t5(f1,f2) values(1,1);
> +insert into t5(f1,f2) values(2,1);
> +insert into t5(f1,f2) values(3,1);
> +delete from t5 where f2 = 1;
> +#Test case4: INVOKES A TRIGGER TO INSERT/UPDATE TWO OR MORE VALUES INTO AN AUTOINC
> COLUMN
> +create table t7(f1 int) engine=innodb;
> +create table t8(i1 int not null auto_increment, f1 int, primary key(i1))
> engine=innodb;
> +create trigger tr4 before insert on t7 for each row begin
> +insert into t8(f1) values(new.f1);
> +insert into t8(f1) values(new.f1);
> +end |
> +insert into t7(f1) values(1),(2);
> +insert into t7(f1) values(3);
> +#Test case5: INVOKES A FUNCTION TO INSERT/UPDATE TWO OR MORE VALUES INTO AN AUTOINC
> COLUMN
> +create table t9(f1 int) engine=innodb;
> +create table t10(i1 int not null auto_increment, f1 int, primary key(i1))
> engine=innodb;
> +CREATE FUNCTION f1_two_inserts() RETURNS INTEGER
> +BEGIN
> +INSERT INTO t10(f1) values(2);
> +INSERT INTO t10(f1) values(2);
> +RETURN 1;
> +END//
> +insert into t1(f1) values(f1_two_inserts());
> +#Test case6: Two (or more) different tables to update with auto_inc columns
> +create table t11(i int not null auto_increment, f1 int, primary key(i))
> engine=innodb;
> +create table t12(i1 int not null auto_increment, f1 int, primary key(i1))
> engine=innodb;
> +create trigger tr6 after insert on t11 for each row insert into t12(f1)
> values(new.f1);
> +insert into t11(f1) values(1),(2);
> +insert into t11(f1) values(3);
> +#Test case1: INSERT
> +insert into t2(f1) values(4),(5);
> +#Test case2: UPDATE
> +insert into t4(f1) values(3);
> +#Test case3: DELETE
> +insert into t6(f1) values(3);
> +#Test case4: INVOKES A TRIGGER TO INSERT/UPDATE TWO OR MORE VALUES
> +insert into t8(f1) values(4),(5);
> +#Test case5: INVOKES A FUNCTION TO INSERT/UPDATE TWO OR MORE VALUES
> +insert into t10(f1) values(4),(5);
> +#Test case6: Two (or more) different tables to update with auto_inc columns
> +insert into t12(f1) values(4),(5);
> +commit;
> +#Test case1: INSERT
> +insert into t1(f1) values(6);
> +#Test case2: UPDATE
> +insert into t3(f1,f2) values(4,2);
> +update t3 set f1 = f1 + 5 where f2 = 2;
> +#Test case3: DELETE
> +insert into t5(f1,f2) values(4,2);
> +delete from t5 where f2 = 2;
> +#Test case4: INVOKES A TRIGGER TO INSERT/UPDATE TWO OR MORE VALUES
> +insert into t7(f1) values(6);
> +#Test case5: INVOKES A FUNCTION TO INSERT/UPDATE TWO OR MORE VALUES
> +insert into t9(f1) values(f1_two_inserts());
> +#Test case6: Two (or more) different tables to update with auto_inc columns
> +insert into t11(f1) values(6);
> +commit;
> +#Test result for INSERT with AFTER/BEFOR TRIGGER on master 
> +select * from t2;
> +i1	f1
> +1	1
> +2	2
> +3	3
> +4	1
> +5	4
> +6	5
> +7	6
> +#Test result for UPDATE with AFTER/BEFOR TRIGGER on master
> +select * from t4;
> +i1	f1
> +1	6
> +2	7
> +3	3
> +4	9
> +#Test result for DELETE with AFTER/BEFOR TRIGGER on master
> +select * from t6;
> +i1	f1
> +1	6
> +2	6
> +3	6
> +4	3
> +5	6
> +#Test result for INVOKES A TRIGGER TO INSERT/UPDATE TWO OR MORE VALUES on master
> +select * from t8;
> +i1	f1
> +1	1
> +2	1
> +3	2
> +4	2
> +5	3
> +6	3
> +7	4
> +8	5
> +9	6
> +10	6
> +#Test result for INVOKES A FUNCTION TO INSERT/UPDATE TWO OR MORE VALUES on master
> +select * from t10;
> +i1	f1
> +1	2
> +2	2
> +3	4
> +4	5
> +5	2
> +6	2
> +#Test result for Two (or more) different tables to update with auto_inc columns on
> master
> +select * from t12;
> +i1	f1
> +1	1
> +2	2
> +3	3
> +4	4
> +5	5
> +6	6
> +#Test result for INSERT with AFTER/BEFOR TRIGGER on slave
> +select * from t2;
> +i1	f1
> +1	1
> +2	2
> +3	3
> +4	1
> +5	4
> +6	5
> +7	6
> +#Test result for UPDATE with AFTER/BEFOR TRIGGER on slave
> +select * from t4;
> +i1	f1
> +1	6
> +2	7
> +3	3
> +4	9
> +#Test result for DELETE with AFTER/BEFOR TRIGGER on slave
> +select * from t6;
> +i1	f1
> +1	6
> +2	6
> +3	6
> +4	3
> +5	6
> +#Test result for INVOKES A TRIGGER TO INSERT/UPDATE TWO OR MORE VALUES on slave
> +select * from t8;
> +i1	f1
> +1	1
> +2	1
> +3	2
> +4	2
> +5	3
> +6	3
> +7	4
> +8	5
> +9	6
> +10	6
> +#Test result for INVOKES A FUNCTION TO INSERT/UPDATE TWO OR MORE VALUES on slave
> +select * from t10;
> +i1	f1
> +1	2
> +2	2
> +3	4
> +4	5
> +5	2
> +6	2
> +#Test result for Two (or more) different tables to update with auto_inc columns on
> slave
> +select * from t12;
> +i1	f1
> +1	1
> +2	2
> +3	3
> +4	4
> +5	5
> +6	6
> +DROP TABLE t1;
> +DROP TABLE t2;
> +DROP TABLE t3;
> +DROP TABLE t4;
> +DROP TABLE t5;
> +DROP TABLE t6;
> +DROP TABLE t7;
> +DROP TABLE t8;
> +DROP TABLE t9;
> +DROP TABLE t10;
> +DROP TABLE t11;
> +DROP TABLE t12;
> +DROP FUNCTION f1_two_inserts;
> 
> === added file 'mysql-test/suite/rpl/t/rpl_auto_increment_update_failure.test'
> --- a/mysql-test/suite/rpl/t/rpl_auto_increment_update_failure.test	1970-01-01
> 00:00:00 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_auto_increment_update_failure.test	2009-09-02
> 05:23:51 +0000
> @@ -0,0 +1,159 @@
> +#
> +# Bug45677
> +# This test verifies if concurrent transactions that cause a trigger or call 

Here too, concurrent transactions is not related to this problem.

> +# a function to insert or update > 1 values into an autoinc column can cause 
> +# mixed based replication to break with slave reporting error 1062 (Error 
> +# 'Duplicate entry 'x' for key 'PRIMARY'').
> +#

Please rewrite this, please describe what this test.

> +
> +source include/have_binlog_format_mixed.inc;
> +source include/have_innodb.inc;
> +source include/master-slave.inc;
> +
> +connection master;
> +--disable_warnings
> +drop table if exists t10,t11,t12;
> +DROP FUNCTION IF EXISTS f1_simple_insert;
> +--enable_warnings
> +
> +use test;
> +#The transaction can't be committed untill the commit statement is executed 
> +set autocommit=0;
> +
> +--echo #Test case1: INSERT with AFTER/BEFOR TRIGGER
> +create table t1(f1 int) engine=innodb;
> +create table t2(i1 int not null auto_increment, f1 int, primary key(i1))
> engine=innodb;
> +create trigger tr1 after insert on t1 for each row insert into t2(f1)
> values(new.f1);
> +insert into t1(f1) values(1),(2);
> +insert into t1(f1) values(3);
> +

call sync_slave_with_master here;

please also add test for BEFORE INSERT trigger.

Please check if the binlog-format is switched to row-format.

> +--echo #Test case2: UPDATE with AFTER/BEFOR TRIGGER
> +create table t3(f1 int, f2 int) engine=innodb;
> +create table t4(i1 int not null auto_increment, f1 int, primary key(i1))
> engine=innodb;
> +create trigger tr2 before update on t3 for each row insert into t4(f1)
> values(new.f1);
> +insert into t3(f1,f2) values(1,1);
> +insert into t3(f1,f2) values(2,1);
> +update t3 set f1 = f1 + 5 where f2 = 1;
> +

add test for AFTER UPDATE trigger

> +--echo #Test case3: DELETE with AFTER/BEFOR TRIGGER
> +create table t5(f1 int, f2 int) engine=innodb;
> +create table t6(i1 int not null auto_increment, f1 int, primary key(i1))
> engine=innodb;
> +create trigger tr3  before delete on t5 for each row insert into t6(f1) values(6);
> +insert into t5(f1,f2) values(1,1);
> +insert into t5(f1,f2) values(2,1);
> +insert into t5(f1,f2) values(3,1);
> +delete from t5 where f2 = 1;
> +

Here too, AFTER DELETE trigger

> +--echo #Test case4: INVOKES A TRIGGER TO INSERT/UPDATE TWO OR MORE VALUES INTO AN
> AUTOINC COLUMN
> +create table t7(f1 int) engine=innodb;
> +create table t8(i1 int not null auto_increment, f1 int, primary key(i1))
> engine=innodb;
> +delimiter |;
> +create trigger tr4 before insert on t7 for each row begin
> +    insert into t8(f1) values(new.f1);
> +    insert into t8(f1) values(new.f1);
> +end |
> +delimiter ;|
> +insert into t7(f1) values(1),(2);
> +insert into t7(f1) values(3);
> +

test UPDATE trigger too.

> +--echo #Test case5: INVOKES A FUNCTION TO INSERT/UPDATE TWO OR MORE VALUES INTO AN
> AUTOINC COLUMN
> +create table t9(f1 int) engine=innodb;
> +create table t10(i1 int not null auto_increment, f1 int, primary key(i1))
> engine=innodb;
> +delimiter //;
> +CREATE FUNCTION f1_two_inserts() RETURNS INTEGER
> +BEGIN
> +   INSERT INTO t10(f1) values(2);
> +   INSERT INTO t10(f1) values(2);
> +   RETURN 1;
> +END//
> +delimiter ;//
> +insert into t1(f1) values(f1_two_inserts());
> +

test function that do UPDATE, which should also cause the switch to row
format.

> +--echo #Test case6: Two (or more) different tables to update with auto_inc columns
> +create table t11(i int not null auto_increment, f1 int, primary key(i))
> engine=innodb;
> +create table t12(i1 int not null auto_increment, f1 int, primary key(i1))
> engine=innodb;
> +create trigger tr6 after insert on t11 for each row insert into t12(f1)
> values(new.f1);
> +insert into t11(f1) values(1),(2);
> +insert into t11(f1) values(3);
> +

Please also test the case that insert into more than one tables with
autoinc fields in the trigger.

I think it's probably be good to test statement that invokes more than
one trigger or functions.

> +
> +connection master1;
> +#The default autocommit is set to 1, so the transaction is auto committed
> +--echo #Test case1: INSERT
> +insert into t2(f1) values(4),(5);
> +--echo #Test case2: UPDATE
> +insert into t4(f1) values(3);
> +--echo #Test case3: DELETE
> +insert into t6(f1) values(3);
> +--echo #Test case4: INVOKES A TRIGGER TO INSERT/UPDATE TWO OR MORE VALUES
> +insert into t8(f1) values(4),(5);
> +--echo #Test case5: INVOKES A FUNCTION TO INSERT/UPDATE TWO OR MORE VALUES
> +insert into t10(f1) values(4),(5);
> +--echo #Test case6: Two (or more) different tables to update with auto_inc columns
> +insert into t12(f1) values(4),(5);
> +
> +
> +connection master;
> +commit;
> +--echo #Test case1: INSERT
> +insert into t1(f1) values(6);
> +--echo #Test case2: UPDATE
> +insert into t3(f1,f2) values(4,2);
> +update t3 set f1 = f1 + 5 where f2 = 2;
> +--echo #Test case3: DELETE
> +insert into t5(f1,f2) values(4,2);
> +delete from t5 where f2 = 2;
> +--echo #Test case4: INVOKES A TRIGGER TO INSERT/UPDATE TWO OR MORE VALUES
> +insert into t7(f1) values(6);
> +--echo #Test case5: INVOKES A FUNCTION TO INSERT/UPDATE TWO OR MORE VALUES
> +insert into t9(f1) values(f1_two_inserts());
> +--echo #Test case6: Two (or more) different tables to update with auto_inc columns
> +insert into t11(f1) values(6);
> +commit;
> +
> +
> +connection master;
> +--echo #Test result for INSERT with AFTER/BEFOR TRIGGER on master 
> +select * from t2;
> +--echo #Test result for UPDATE with AFTER/BEFOR TRIGGER on master
> +select * from t4;
> +--echo #Test result for DELETE with AFTER/BEFOR TRIGGER on master
> +select * from t6;
> +--echo #Test result for INVOKES A TRIGGER TO INSERT/UPDATE TWO OR MORE VALUES on
> master
> +select * from t8;
> +--echo #Test result for INVOKES A FUNCTION TO INSERT/UPDATE TWO OR MORE VALUES on
> master
> +select * from t10;
> +--echo #Test result for Two (or more) different tables to update with auto_inc
> columns on master
> +select * from t12;
> +
> +sync_slave_with_master;
> +connection slave;
> +--echo #Test result for INSERT with AFTER/BEFOR TRIGGER on slave
> +select * from t2;
> +--echo #Test result for UPDATE with AFTER/BEFOR TRIGGER on slave
> +select * from t4;
> +--echo #Test result for DELETE with AFTER/BEFOR TRIGGER on slave
> +select * from t6;
> +--echo #Test result for INVOKES A TRIGGER TO INSERT/UPDATE TWO OR MORE VALUES on
> slave
> +select * from t8;
> +--echo #Test result for INVOKES A FUNCTION TO INSERT/UPDATE TWO OR MORE VALUES on
> slave
> +select * from t10;
> +--echo #Test result for Two (or more) different tables to update with auto_inc
> columns on slave
> +select * from t12;
> +
> +connection master;
> +DROP TABLE t1;
> +DROP TABLE t2;
> +DROP TABLE t3;
> +DROP TABLE t4;
> +DROP TABLE t5;
> +DROP TABLE t6;
> +DROP TABLE t7;
> +DROP TABLE t8;
> +DROP TABLE t9;
> +DROP TABLE t10;
> +DROP TABLE t11;
> +DROP TABLE t12;
> +DROP FUNCTION f1_two_inserts;
> +sync_slave_with_master;
> +
> 
> === modified file 'sql/sql_base.cc'
> --- a/sql/sql_base.cc	2009-08-28 16:21:54 +0000
> +++ b/sql/sql_base.cc	2009-09-02 05:23:51 +0000
> @@ -106,7 +106,7 @@ static bool open_new_frm(THD *thd, TABLE
>  static void close_old_data_files(THD *thd, TABLE *table, bool morph_locks,
>                                   bool send_refresh);
>  static bool
> -has_two_write_locked_tables_with_auto_increment(TABLE_LIST *tables);
> +has_table_with_auto_increment_in_sub_statements(TABLE_LIST *tables);
>  
> 
>  extern "C" uchar *table_cache_key(const uchar *record, size_t *length,
> @@ -5278,14 +5278,16 @@ int lock_tables(THD *thd, TABLE_LIST *ta
>        thd->options|= OPTION_TABLE_LOCK;
>        /*
>          If we have >= 2 different tables to update with auto_inc columns,
> -        statement-based binlogging won't work. We can solve this problem in
> -        mixed mode by switching to row-based binlogging:
> +        statement-based binlogging won't work. 
> +        Concurrent transactions that cause a trigger or call a function 
> +        to insert or update > 1 values into an autoinc column will generate 
> +        wrong auto_increment values in statement-based replication. 
> +        We can solve these problems in mixed mode by switching to row-based 
> +        binlogging:
>        */
> -      if (thd->variables.binlog_format == BINLOG_FORMAT_MIXED &&
> -          has_two_write_locked_tables_with_auto_increment(tables))
> +      if (has_table_with_auto_increment_in_sub_statements(tables))
>        {
>          thd->lex->set_stmt_unsafe();
> -        thd->set_current_stmt_binlog_row_based_if_mixed();
>        }
>      }
>  
> @@ -8815,47 +8817,33 @@ void mysql_wait_completed_table(ALTER_PA
>  
> 
>  /*
> -  Tells if two (or more) tables have auto_increment columns and we want to
> -  lock those tables with a write lock.
> +  Check if one (or more) tables have auto_increment columns in sub-statements.
>  
> -  SYNOPSIS
> -    has_two_write_locked_tables_with_auto_increment
> -      tables        Table list
> +  PARAMETERS
> +      tables[in]  Table list
> +
> +  RETURN
> +      0           No
> +      1           Yes
>  
>    NOTES:
>      Call this function only when you have established the list of all tables
>      which you'll want to update (including stored functions, triggers, views
>      inside your statement).
> -
> -  RETURN
> -    0  No
> -    1  Yes
>  */
>  
>  static bool
> -has_two_write_locked_tables_with_auto_increment(TABLE_LIST *tables)
> +has_table_with_auto_increment_in_sub_statements(TABLE_LIST *tables)

I'd suggest: has_write_table_with_auto_increment

>  {
> -  char *first_table_name= NULL, *first_db;
> -  LINT_INIT(first_db);
> -
> -  for (TABLE_LIST *table= tables; table; table= table->next_global)
> +  for (TABLE_LIST *table= tables->next_global; table; table=
> table->next_global)
>    {
>      /* we must do preliminary checks as table->table may be NULL */
>      if (!table->placeholder() &&
>          table->table->found_next_number_field &&
>          (table->lock_type >= TL_WRITE_ALLOW_WRITE))
> -    {
> -      if (first_table_name == NULL)
> -      {
> -        first_table_name= table->table_name;
> -        first_db= table->db;
> -        DBUG_ASSERT(first_db);
> -      }
> -      else if (strcmp(first_db, table->db) ||
> -               strcmp(first_table_name, table->table_name))
> -        return 1;
> -    }
> +      return 1;
>    }
> +
>    return 0;
>  }
>  
> 

Thread
bzr commit into mysql-5.1 branch (Dao-Gang.Qu:3095) Bug#45677Dao-Gang.Qu2 Sep
  • Re: bzr commit into mysql-5.1 branch (Dao-Gang.Qu:3095) Bug#45677He Zhenxing2 Sep