Bug #29136 erred multi-delete on trans table does not rollback the statement

similar to bug_27716, but it was stressed on in the synopsis on that there is another side of the artifact affecting behaviour in transaction. Fixed with deploying multi_delete::send_error() - otherwise never called - and refining its logic to perform binlogging job if needed. The changeset includes the following side effects: - added tests to check bug_23333's scenarios on the mixture of tables for multi_update; - fixes bug@30763 with two-liner patch and a test coinciding to one added for bug_23333.
18 years ago · ed0ab76e28
10 changed files with 274 additions and 29 deletions
--- a/mysql-test/r/innodb.result
+++ b/mysql-test/r/innodb.result
@ -1119,6 +1119,19 @@ show master status /* there must be no UPDATE query event */;
 File	Position	Binlog_Do_DB	Binlog_Ignore_DB
 master-bin.000001	98		
 drop table t1, t2;
+drop table if exists t1, t2;
+CREATE TABLE t1 (a int, PRIMARY KEY (a));
+CREATE TABLE t2 (a int, PRIMARY KEY (a)) ENGINE=InnoDB;
+create trigger trg_del_t2 after  delete on t2 for each row
+insert into t1 values (1);
+insert into t1 values (1);
+insert into t2 values (1),(2);
+delete t2 from t2;
+ERROR 23000: Duplicate entry '1' for key 1
+select count(*) from t2 /* must be 2 as restored after rollback caused by the error */;
+count(*)
+2
+drop table t1, t2;
 create table t1 (a int, b int) engine=innodb;
 insert into t1 values(20,null);
 select t2.b, ifnull(t2.b,"this is null") from t1 as t2 left join t1 as t3 on
@ -1792,10 +1805,10 @@ Variable_name	Value
 Innodb_page_size	16384
 show status like "Innodb_rows_deleted";
 Variable_name	Value
-Innodb_rows_deleted	72
+Innodb_rows_deleted	73
 show status like "Innodb_rows_inserted";
 Variable_name	Value
-Innodb_rows_inserted	29732
+Innodb_rows_inserted	29734
 show status like "Innodb_rows_updated";
 Variable_name	Value
 Innodb_rows_updated	29532
--- a/mysql-test/r/mix_innodb_myisam_binlog.result
+++ b/mysql-test/r/mix_innodb_myisam_binlog.result
@ -351,7 +351,7 @@ drop function if exists bug27417;
 drop table if exists t1,t2;
 CREATE TABLE t1 (a int NOT NULL auto_increment primary key) ENGINE=MyISAM;
 CREATE TABLE t2 (a int NOT NULL auto_increment, PRIMARY KEY (a));
-create function bug27417(n int) 
+create function bug27417(n int)
 RETURNS int(11)
 begin
 insert into t1 values (null);
@ -393,7 +393,9 @@ count(*)
 drop table t1,t2;
 CREATE TABLE t1 (a int  NOT NULL auto_increment primary key) ENGINE=MyISAM;
 CREATE TABLE t2 (a int, PRIMARY KEY (a)) ENGINE=InnoDB;
-CREATE TABLE t3 (a int, PRIMARY KEY (a), b int unique);
+CREATE TABLE t3 (a int, PRIMARY KEY (a), b int unique) ENGINE=MyISAM;
+CREATE TABLE t4 (a int, PRIMARY KEY (a), b int unique) ENGINE=Innodb;
+CREATE TABLE t5 (a int, PRIMARY KEY (a)) ENGINE=InnoDB;
 insert into t2 values (1);
 reset master;
 insert into t2 values (bug27417(1));
@ -427,6 +429,31 @@ master-bin.000001	190
 select count(*) from t1 /* must be 2 */;
 count(*)
 2
+delete from t3;
+delete from t4;
+insert into t3 values (1,1);
+insert into t4 values (1,1),(2,2);
+reset master;
+UPDATE t4,t3 SET t4.a=t3.a + bug27417(1) /* top level non-ta table */;
+ERROR 23000: Duplicate entry '2' for key 1
+show master status /* the offset must denote there is the query */;
+File	Position	Binlog_Do_DB	Binlog_Ignore_DB
+master-bin.000001	301		
+select count(*) from t1 /* must be 4 */;
+count(*)
+4
+delete from t1;
+delete from t3;
+delete from t4;
+insert into t3 values (1,1),(2,2);
+insert into t4 values (1,1),(2,2);
+reset master;
+UPDATE t3,t4 SET t3.a=t4.a + bug27417(1);
+ERROR 23000: Duplicate entry '2' for key 1
+select count(*) from t1 /* must be 1 */;
+count(*)
+1
+drop table t4;
 delete from t1;
 delete from t2;
 delete from t3;
@ -443,6 +470,23 @@ master-bin.000001	246
 select count(*) from t1 /* must be 1 */;
 count(*)
 1
+drop trigger trg_del;
+delete from t1;
+delete from t2;
+delete from t5;
+create trigger trg_del_t2 after  delete on t2 for each row
+insert into t1 values (1);
+insert into t2 values (2),(3);
+insert into t5 values (1),(2);
+reset master;
+delete t2.* from t2,t5 where t2.a=t5.a + 1;
+ERROR 23000: Duplicate entry '1' for key 1
+show master status /* the offset must denote there is the query */;
+File	Position	Binlog_Do_DB	Binlog_Ignore_DB
+master-bin.000001	274		
+select count(*) from t1 /* must be 1 */;
+count(*)
+1
 delete from t1;
 create table t4 (a int default 0, b int primary key) engine=innodb;
 insert into t4 values (0, 17);
@ -458,7 +502,7 @@ count(*)
 show master status /* the offset must denote there is the query */;
 File	Position	Binlog_Do_DB	Binlog_Ignore_DB
 master-bin.000001	376		
-drop trigger trg_del;
-drop table t1,t2,t3,t4;
+drop trigger trg_del_t2;
+drop table t1,t2,t3,t4,t5;
 drop function bug27417;
 end of tests
--- a/mysql-test/r/multi_update.result
+++ b/mysql-test/r/multi_update.result
@ -545,7 +545,7 @@ a	b
 4	4
 show master status /* there must be the UPDATE query event */;
 File	Position	Binlog_Do_DB	Binlog_Ignore_DB
-master-bin.000001	189		
+master-bin.000001	260		
 delete from t1;
 delete from t2;
 insert into t1 values (1,2),(3,4),(4,4);
@ -555,6 +555,26 @@ UPDATE t2,t1  SET t2.a=t2.b where t2.a=t1.a;
 ERROR 23000: Duplicate entry '4' for key 1
 show master status /* there must be the UPDATE query event */;
 File	Position	Binlog_Do_DB	Binlog_Ignore_DB
-master-bin.000001	204		
+master-bin.000001	275		
 drop table t1, t2;
+drop table if exists t1, t2, t3;
+CREATE TABLE t1 (a int, PRIMARY KEY (a));
+CREATE TABLE t2 (a int, PRIMARY KEY (a));
+CREATE TABLE t3 (a int, PRIMARY KEY (a)) ENGINE=MyISAM;
+create trigger trg_del_t3 before  delete on t3 for each row insert into t1 values (1);
+insert into t2 values (1),(2);
+insert into t3 values (1),(2);
+reset master;
+delete t3.* from t2,t3 where t2.a=t3.a;
+ERROR 23000: Duplicate entry '1' for key 1
+select count(*) from t1 /* must be 1 */;
+count(*)
+1
+select count(*) from t3 /* must be 1 */;
+count(*)
+1
+show binlog events from 98;
+Log_name	Pos	Event_type	Server_id	End_log_pos	Info
+master-bin.000001	98	Query	1	#	use `test`; delete t3.* from t2,t3 where t2.a=t3.a
+drop table t1, t2, t3;
 end of tests
--- a/mysql-test/t/innodb.test
+++ b/mysql-test/t/innodb.test
@ -792,6 +792,38 @@ show master status /* there must be no UPDATE query event */;
 # cleanup bug#27716
 drop table t1, t2;

+#
+# Bug #29136  	erred multi-delete on trans table does not rollback 
+#
+
+# prepare
+--disable_warnings
+drop table if exists t1, t2;
+--enable_warnings
+CREATE TABLE t1 (a int, PRIMARY KEY (a));
+CREATE TABLE t2 (a int, PRIMARY KEY (a)) ENGINE=InnoDB;
+create trigger trg_del_t2 after  delete on t2 for each row
+       insert into t1 values (1);
+insert into t1 values (1);
+insert into t2 values (1),(2);
+
+
+# exec cases A, B - see multi_update.test
+
+# A. send_error() w/o send_eof() branch
+
+--error ER_DUP_ENTRY
+delete t2 from t2;
+
+# check
+
+select count(*) from t2 /* must be 2 as restored after rollback caused by the error */;
+
+# cleanup bug#29136
+
+drop table t1, t2;
+
+
 #
 # Testing of IFNULL
 #
--- a/mysql-test/t/mix_innodb_myisam_binlog.test
+++ b/mysql-test/t/mix_innodb_myisam_binlog.test
@ -347,7 +347,7 @@ CREATE TABLE t1 (a int NOT NULL auto_increment primary key) ENGINE=MyISAM;
 CREATE TABLE t2 (a int NOT NULL auto_increment, PRIMARY KEY (a));

 delimiter |;
-create function bug27417(n int) 
+create function bug27417(n int)
 RETURNS int(11)
 begin
  insert into t1 values (null);
@ -385,13 +385,17 @@ drop table t1,t2;

 #
 # Bug#23333 using the patch (and the test) for bug#27471
+#
 # throughout the bug tests 
 # t1 - non-trans side effects gatherer;
 # t2 - transactional table;
 #
+
 CREATE TABLE t1 (a int  NOT NULL auto_increment primary key) ENGINE=MyISAM;
 CREATE TABLE t2 (a int, PRIMARY KEY (a)) ENGINE=InnoDB;
-CREATE TABLE t3 (a int, PRIMARY KEY (a), b int unique);
+CREATE TABLE t3 (a int, PRIMARY KEY (a), b int unique) ENGINE=MyISAM;
+CREATE TABLE t4 (a int, PRIMARY KEY (a), b int unique) ENGINE=Innodb;
+CREATE TABLE t5 (a int, PRIMARY KEY (a)) ENGINE=InnoDB;


 #
@ -434,7 +438,7 @@ CREATE TABLE t3 (a int, PRIMARY KEY (a), b int unique);
 select count(*) from t1 /* must be 2 */;

 #
-# UPDATE (multi-update see bug#27716)
+# UPDATE inc multi-update
 #

 # prepare
@ -450,9 +454,48 @@ CREATE TABLE t3 (a int, PRIMARY KEY (a), b int unique);
 show master status /* the offset must denote there is the query */;
 select count(*) from t1 /* must be 2 */;

+## multi_update::send_eof() branch
+
+# prepare
+ delete from t3;
+ delete from t4;
+ insert into t3 values (1,1);
+ insert into t4 values (1,1),(2,2);
+
+ reset master;
+
+# execute
+ --error ER_DUP_ENTRY
+ UPDATE t4,t3 SET t4.a=t3.a + bug27417(1) /* top level non-ta table */;
+
+# check
+ show master status /* the offset must denote there is the query */;
+ select count(*) from t1 /* must be 4 */;
+
+## send_error() branch of multi_update
+
+# prepare
+ delete from t1;
+ delete from t3;
+ delete from t4;
+ insert into t3 values (1,1),(2,2);
+ insert into t4 values (1,1),(2,2);
+
+ reset master;
+
+# execute
+ --error ER_DUP_ENTRY
+ UPDATE t3,t4 SET t3.a=t4.a + bug27417(1);
+
+# check
+ select count(*) from t1 /* must be 1 */;
+
+# cleanup
+ drop table t4;
+

 #
-# DELETE (for multi-delete see Bug #29136)
+# DELETE incl multi-delete
 #

 # prepare
@ -472,6 +515,27 @@ CREATE TABLE t3 (a int, PRIMARY KEY (a), b int unique);
 show master status /* the offset must denote there is the query */;
 select count(*) from t1 /* must be 1 */;

+# cleanup
+ drop trigger trg_del;
+
+# prepare
+ delete from t1;
+ delete from t2;
+ delete from t5;
+ create trigger trg_del_t2 after  delete on t2 for each row
+   insert into t1 values (1);
+ insert into t2 values (2),(3);
+ insert into t5 values (1),(2);
+ reset master;
+
+# execute
+ --error ER_DUP_ENTRY
+ delete t2.* from t2,t5 where t2.a=t5.a + 1;
+
+# check
+ show master status /* the offset must denote there is the query */;
+ select count(*) from t1 /* must be 1 */;
+

 #
 # LOAD DATA
@ -496,8 +560,8 @@ CREATE TABLE t3 (a int, PRIMARY KEY (a), b int unique);
 #


-drop trigger trg_del;
-drop table t1,t2,t3,t4;
+drop trigger trg_del_t2;
+drop table t1,t2,t3,t4,t5;
 drop function bug27417;


--- a/mysql-test/t/multi_update.test
+++ b/mysql-test/t/multi_update.test
@ -574,4 +574,38 @@ show master status /* there must be the UPDATE query event */;
 # cleanup bug#27716
 drop table t1, t2;

+#
+# Bug #29136  	erred multi-delete on trans table does not rollback 
+#
+
+# prepare
+--disable_warnings
+drop table if exists t1, t2, t3;
+--enable_warnings
+CREATE TABLE t1 (a int, PRIMARY KEY (a));
+CREATE TABLE t2 (a int, PRIMARY KEY (a));
+CREATE TABLE t3 (a int, PRIMARY KEY (a)) ENGINE=MyISAM;
+create trigger trg_del_t3 before  delete on t3 for each row insert into t1 values (1);
+
+insert into t2 values (1),(2);
+insert into t3 values (1),(2);
+reset master;
+
+# exec cases B, A - see innodb.test
+
+# B. send_eof() and send_error() afterward
+
+--error ER_DUP_ENTRY
+delete t3.* from t2,t3 where t2.a=t3.a;
+
+# check
+select count(*) from t1 /* must be 1 */;
+select count(*) from t3 /* must be 1 */;
+# the query must be in binlog (no surprise though)
+source include/show_binlog_events.inc;
+
+# cleanup bug#29136
+drop table t1, t2, t3;
+
+
 --echo end of tests
--- a/sql/sql_class.h
+++ b/sql/sql_class.h
@ -2356,6 +2356,11 @@ class multi_delete :public select_result_interceptor
  /* True if at least one table we delete from is not transactional */
  bool normal_tables;
  bool delete_while_scanning;
+  /*
+     error handling (rollback and binlogging) can happen in send_eof()
+     so that afterward send_error() needs to find out that.
+  */
+  bool error_handled;

 public:
  multi_delete(TABLE_LIST *dt, uint num_of_tables);
@ -2391,6 +2396,11 @@ class multi_update :public select_result_interceptor
  /* True if the update operation has made a change in a transactional table */
  bool transactional_tables;
  bool ignore;
+  /* 
+     error handling (rollback and binlogging) can happen in send_eof()
+     so that afterward send_error() needs to find out that.
+  */
+  bool error_handled;

 public:
  multi_update(TABLE_LIST *ut, TABLE_LIST *leaves_list,
--- a/sql/sql_delete.cc
+++ b/sql/sql_delete.cc
@ -504,7 +504,7 @@ bool mysql_multi_delete_prepare(THD *thd)
 multi_delete::multi_delete(TABLE_LIST *dt, uint num_of_tables_arg)
  : delete_tables(dt), deleted(0), found(0),
    num_of_tables(num_of_tables_arg), error(0),
-    do_delete(0), transactional_tables(0), normal_tables(0)
+    do_delete(0), transactional_tables(0), normal_tables(0), error_handled(0)
 {
  tempfiles= (Unique **) sql_calloc(sizeof(Unique *) * num_of_tables);
 }
@ -685,12 +685,14 @@ void multi_delete::send_error(uint errcode,const char *err)
  /* First send error what ever it is ... */
  my_message(errcode, err, MYF(0));

-  /* If nothing deleted return */
-  if (!deleted)
+  /* the error was handled or nothing deleted and no side effects return */
+  if (error_handled ||
+      !thd->transaction.stmt.modified_non_trans_table && !deleted)
    DBUG_VOID_RETURN;

  /* Something already deleted so we have to invalidate cache */
-  query_cache_invalidate3(thd, delete_tables, 1);
+  if (deleted)
+    query_cache_invalidate3(thd, delete_tables, 1);

  /*
    If rows from the first table only has been deleted and it is
@ -710,12 +712,29 @@ void multi_delete::send_error(uint errcode,const char *err)
    */
    error= 1;
    send_eof();
+    DBUG_ASSERT(error_handled);
+    DBUG_VOID_RETURN;
+  }
+  
+  if (thd->transaction.stmt.modified_non_trans_table)
+  {
+    /* 
+       there is only side effects; to binlog with the error
+    */
+    if (mysql_bin_log.is_open())
+    {
+      Query_log_event qinfo(thd, thd->query, thd->query_length,
+                            transactional_tables, FALSE);
+      mysql_bin_log.write(&qinfo);
+    }
+    thd->transaction.all.modified_non_trans_table= true;
  }
  DBUG_ASSERT(!normal_tables || !deleted || thd->transaction.stmt.modified_non_trans_table);
  DBUG_VOID_RETURN;
 }


+
 /*
  Do delete from other tables.
  Returns values:
@ -832,6 +851,8 @@ bool multi_delete::send_eof()
    if (thd->transaction.stmt.modified_non_trans_table)
      thd->transaction.all.modified_non_trans_table= TRUE;
  }
+  if (local_error != 0)
+    error_handled= TRUE; // to force early leave from ::send_error()

  /* Commit or rollback the current SQL statement */
  if (transactional_tables)
--- a/sql/sql_parse.cc
+++ b/sql/sql_parse.cc
@ -3701,6 +3701,13 @@ end_with_restore_list:
 			SELECT_NO_JOIN_CACHE | SELECT_NO_UNLOCK |
                        OPTION_SETUP_TABLES_DONE,
 			del_result, unit, select_lex);
+      res|= thd->net.report_error;
+      if (unlikely(res))
+      {
+        /* If we had a another error reported earlier then this will be ignored */
+        del_result->send_error(ER_UNKNOWN_ERROR, "Execution of the query failed");
+        del_result->abort();
+      }
      delete del_result;
    }
    else
--- a/sql/sql_update.cc
+++ b/sql/sql_update.cc
@ -994,8 +994,8 @@ multi_update::multi_update(TABLE_LIST *table_list,
  :all_tables(table_list), leaves(leaves_list), update_tables(0),
   tmp_tables(0), updated(0), found(0), fields(field_list),
   values(value_list), table_count(0), copy_field(0),
-   handle_duplicates(handle_duplicates_arg), do_update(1), trans_safe(0),
-   transactional_tables(1), ignore(ignore_arg)
+   handle_duplicates(handle_duplicates_arg), do_update(1), trans_safe(1),
+   transactional_tables(1), ignore(ignore_arg), error_handled(0)
 {}


@ -1202,7 +1202,6 @@ multi_update::initialize_tables(JOIN *join)
  if ((thd->options & OPTION_SAFE_UPDATES) && error_if_full_join(join))
    DBUG_RETURN(1);
  main_table=join->join_tab->table;
-  trans_safe= transactional_tables= main_table->file->has_transactions();
  table_to_update= 0;

  /* Any update has at least one pair (field, value) */
@ -1484,12 +1483,14 @@ void multi_update::send_error(uint errcode,const char *err)
  /* First send error what ever it is ... */
  my_error(errcode, MYF(0), err);

-  /* If nothing updated return */
-  if (updated == 0) /* the counter might be reset in send_eof */
-    return;         /* and then the query has been binlogged */
+  /* the error was handled or nothing deleted and no side effects return */
+  if (error_handled ||
+      !thd->transaction.stmt.modified_non_trans_table && !updated)
+    return;

  /* Something already updated so we have to invalidate cache */
-  query_cache_invalidate3(thd, update_tables, 1);
+  if (updated)
+    query_cache_invalidate3(thd, update_tables, 1);
  /*
    If all tables that has been updated are trans safe then just do rollback.
    If not attempt to do remaining updates.
@ -1525,8 +1526,7 @@ void multi_update::send_error(uint errcode,const char *err)
                            transactional_tables, FALSE);
      mysql_bin_log.write(&qinfo);
    }
-    if (!trans_safe)
-      thd->transaction.all.modified_non_trans_table= TRUE;
+    thd->transaction.all.modified_non_trans_table= TRUE;
  }
  DBUG_ASSERT(trans_safe || !updated || thd->transaction.stmt.modified_non_trans_table);
  
@ -1739,8 +1739,6 @@ bool multi_update::send_eof()
    {
      if (local_error == 0)
        thd->clear_error();
-      else
-        updated= 0; /* if there's an error binlog it here not in ::send_error */
      Query_log_event qinfo(thd, thd->query, thd->query_length,
 			    transactional_tables, FALSE);
      if (mysql_bin_log.write(&qinfo) && trans_safe)
@ -1749,6 +1747,8 @@ bool multi_update::send_eof()
    if (thd->transaction.stmt.modified_non_trans_table)
      thd->transaction.all.modified_non_trans_table= TRUE;
  }
+  if (local_error != 0)
+    error_handled= TRUE; // to force early leave from ::send_error()

  if (transactional_tables)
  {