Bug #27716 multi-update did partially and has not binlogged

Implementation of mysql_multi_update did not call multi_update::send_error method in some cases 
(see the test reported on bug page and test cases in changeset).

Fixed with deploying the method, ::send_error() is refined to get binlogging code which works whenever 
there is modified non-transactional table.
thd->no_trans_update.stmt flag is set in to TRUE to ease testing though being the beginning of relative 
bug#27417 fix (addresses a part of those issues).
Eliminating two minor issues (small bugs) in multi_update methods.
This patch for multi-update also addresses a part of the issues reported in bug#13270,bug#23333.

mysql-test/r/innodb.result

@@ -1086,6 +1086,39 @@ n	d
 1	30
 2	20
 drop table t1,t2;
+CREATE TABLE `t1` (
+`a` int(11) NOT NULL auto_increment,
+`b` int(11) default NULL,
+PRIMARY KEY  (`a`)
+) ENGINE=MyISAM DEFAULT CHARSET=latin1 ;
+CREATE TABLE `t2` (
+`a` int(11) NOT NULL auto_increment,
+`b` int(11) default NULL,
+PRIMARY KEY  (`a`)
+) ENGINE=INNODB DEFAULT CHARSET=latin1 ;
+insert into t1 values (1,1),(2,2);
+insert into t2 values (1,1),(4,4);
+reset master;
+UPDATE t2,t1 SET t2.a=t1.a+2;
+ERROR 23000: Duplicate entry '3' for key 1
+select * from t2 /* must be (3,1), (4,4) */;
+a	b
+1	1
+4	4
+show master status /* there must no UPDATE in binlog */;
+File	Position	Binlog_Do_DB	Binlog_Ignore_DB
+master-bin.000001	98		
+delete from t1;
+delete from t2;
+insert into t1 values (1,2),(3,4),(4,4);
+insert into t2 values (1,2),(3,4),(4,4);
+reset master;
+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 no UPDATE query event */;
+File	Position	Binlog_Do_DB	Binlog_Ignore_DB
+master-bin.000001	98		
+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
@@ -1642,14 +1675,14 @@ t2	CREATE TABLE `t2` (
 drop table t2, t1;
 show status like "binlog_cache_use";
 Variable_name	Value
-Binlog_cache_use	155
+Binlog_cache_use	158
 show status like "binlog_cache_disk_use";
 Variable_name	Value
 Binlog_cache_disk_use	0
 create table t1 (a int) engine=innodb;
 show status like "binlog_cache_use";
 Variable_name	Value
-Binlog_cache_use	156
+Binlog_cache_use	159
 show status like "binlog_cache_disk_use";
 Variable_name	Value
 Binlog_cache_disk_use	1
@@ -1658,7 +1691,7 @@ delete from t1;
 commit;
 show status like "binlog_cache_use";
 Variable_name	Value
-Binlog_cache_use	157
+Binlog_cache_use	160
 show status like "binlog_cache_disk_use";
 Variable_name	Value
 Binlog_cache_disk_use	1
@@ -1782,13 +1815,13 @@ Variable_name	Value
 Innodb_page_size	16384
 show status like "Innodb_rows_deleted";
 Variable_name	Value
-Innodb_rows_deleted	2070
+Innodb_rows_deleted	2072
 show status like "Innodb_rows_inserted";
 Variable_name	Value
-Innodb_rows_inserted	31727
+Innodb_rows_inserted	31732
 show status like "Innodb_rows_updated";
 Variable_name	Value
-Innodb_rows_updated	29530
+Innodb_rows_updated	29532
 show status like "Innodb_row_lock_waits";
 Variable_name	Value
 Innodb_row_lock_waits	0

mysql-test/r/multi_update.result

@@ -524,3 +524,37 @@ a
 30
 drop view v1;
 drop table t1, t2;
+CREATE TABLE `t1` (
+`a` int(11) NOT NULL auto_increment,
+`b` int(11) default NULL,
+PRIMARY KEY  (`a`)
+) ENGINE=MyISAM DEFAULT CHARSET=latin1 ;
+CREATE TABLE `t2` (
+`a` int(11) NOT NULL auto_increment,
+`b` int(11) default NULL,
+PRIMARY KEY  (`a`)
+) ENGINE=MyISAM DEFAULT CHARSET=latin1 ;
+insert into t1 values (1,1),(2,2);
+insert into t2 values (1,1),(4,4);
+reset master;
+UPDATE t2,t1 SET t2.a=t1.a+2;
+ERROR 23000: Duplicate entry '3' for key 1
+select * from t2 /* must be (3,1), (4,4) */;
+a	b
+3	1
+4	4
+show master status /* there must be the UPDATE query event */;
+File	Position	Binlog_Do_DB	Binlog_Ignore_DB
+master-bin.000001	189		
+delete from t1;
+delete from t2;
+insert into t1 values (1,2),(3,4),(4,4);
+insert into t2 values (1,2),(3,4),(4,4);
+reset master;
+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		
+drop table t1, t2;
+end of tests

mysql-test/t/innodb.test

@@ -753,6 +753,45 @@ select * from t1;
 select * from t2;
 drop table t1,t2;
 
+#
+# Bug#27716  	multi-update did partially and has not binlogged
+#
+
+CREATE TABLE `t1` (
+  `a` int(11) NOT NULL auto_increment,
+  `b` int(11) default NULL,
+  PRIMARY KEY  (`a`)
+) ENGINE=MyISAM DEFAULT CHARSET=latin1 ;
+
+CREATE TABLE `t2` (
+  `a` int(11) NOT NULL auto_increment,
+  `b` int(11) default NULL,
+  PRIMARY KEY  (`a`)
+) ENGINE=INNODB DEFAULT CHARSET=latin1 ;
+
+# A. testing multi_update::send_eof() execution branch
+insert into t1 values (1,1),(2,2);
+insert into t2 values (1,1),(4,4);
+reset master;
+--error ER_DUP_ENTRY
+UPDATE t2,t1 SET t2.a=t1.a+2;
+# check
+select * from t2 /* must be (3,1), (4,4) */;
+show master status /* there must no UPDATE in binlog */;
+
+# B. testing multi_update::send_error() execution branch
+delete from t1;
+delete from t2;
+insert into t1 values (1,2),(3,4),(4,4);
+insert into t2 values (1,2),(3,4),(4,4);
+reset master;
+--error ER_DUP_ENTRY
+UPDATE t2,t1  SET t2.a=t2.b where t2.a=t1.a;
+show master status /* there must be no UPDATE query event */;
+
+# cleanup bug#27716
+drop table t1, t2;
+
 #
 # Testing of IFNULL
 #

mysql-test/t/multi_update.test

@@ -534,3 +534,44 @@ select * from t1;
 select * from t2;
 drop view v1;
 drop table t1, t2;
+
+#
+# Bug#27716  	multi-update did partially and has not binlogged
+#
+
+CREATE TABLE `t1` (
+  `a` int(11) NOT NULL auto_increment,
+  `b` int(11) default NULL,
+  PRIMARY KEY  (`a`)
+) ENGINE=MyISAM DEFAULT CHARSET=latin1 ;
+
+CREATE TABLE `t2` (
+  `a` int(11) NOT NULL auto_increment,
+  `b` int(11) default NULL,
+  PRIMARY KEY  (`a`)
+) ENGINE=MyISAM DEFAULT CHARSET=latin1 ;
+
+# A. testing multi_update::send_eof() execution branch
+insert into t1 values (1,1),(2,2);
+insert into t2 values (1,1),(4,4);
+reset master;
+--error ER_DUP_ENTRY
+UPDATE t2,t1 SET t2.a=t1.a+2;
+# check
+select * from t2 /* must be (3,1), (4,4) */;
+show master status /* there must be the UPDATE query event */;
+
+# B. testing multi_update::send_error() execution branch
+delete from t1;
+delete from t2;
+insert into t1 values (1,2),(3,4),(4,4);
+insert into t2 values (1,2),(3,4),(4,4);
+reset master;
+--error ER_DUP_ENTRY
+UPDATE t2,t1  SET t2.a=t2.b where t2.a=t1.a;
+show master status /* there must be the UPDATE query event */;
+
+# cleanup bug#27716
+drop table t1, t2;
+
+--echo end of tests

sql/sql_update.cc

@@ -914,6 +914,7 @@ bool mysql_multi_update(THD *thd,
                         SELECT_LEX_UNIT *unit, SELECT_LEX *select_lex)
 {
   multi_update *result;
+  bool res;
   DBUG_ENTER("mysql_multi_update");
 
   if (!(result= new multi_update(table_list,
@@ -928,7 +929,7 @@ bool mysql_multi_update(THD *thd,
                                MODE_STRICT_ALL_TABLES));
 
   List<Item> total_list;
-  (void) mysql_select(thd, &select_lex->ref_pointer_array,
+  res= mysql_select(thd, &select_lex->ref_pointer_array,
                       table_list, select_lex->with_wild,
                       total_list,
                       conds, 0, (ORDER *) NULL, (ORDER *)NULL, (Item *) NULL,
@@ -936,6 +937,15 @@ bool mysql_multi_update(THD *thd,
                       options | SELECT_NO_JOIN_CACHE | SELECT_NO_UNLOCK |
                       OPTION_SETUP_TABLES_DONE,
                       result, unit, select_lex);
+  DBUG_PRINT("info",("res: %d  report_error: %d", res,
+                     thd->net.report_error));
+  res|= thd->net.report_error;
+  if (unlikely(res))
+  {
+    /* If we had a another error reported earlier then this will be ignored */
+    result->send_error(ER_UNKNOWN_ERROR, ER(ER_UNKNOWN_ERROR));
+    result->abort();
+  }
   delete result;
   thd->abort_on_warning= 0;
   DBUG_RETURN(FALSE);
@@ -1249,8 +1259,9 @@ multi_update::~multi_update()
   if (copy_field)
     delete [] copy_field;
   thd->count_cuted_fields= CHECK_FIELD_IGNORE;		// Restore this setting
-  if (!trans_safe)
+  if (!trans_safe)  // todo: remove since redundant
     thd->no_trans_update.all= TRUE;
+  DBUG_ASSERT(trans_safe || thd->no_trans_update.all);
 }
 
 
@@ -1336,8 +1347,15 @@ bool multi_update::send_data(List<Item> &not_used_values)
 	}
         else
         {
-          if (!table->file->has_transactions())
+          /* non-transactional or transactional table got modified   */
+          /* either multi_update class' flag is raised in its branch */
+          if (table->file->has_transactions())
+            transactional_tables= 1;
+          else
+          {
+            trans_safe= 0;
             thd->no_trans_update.stmt= TRUE;
+          }
           if (table->triggers &&
               table->triggers->process_triggers(thd, TRG_EVENT_UPDATE,
                                                 TRG_ACTION_AFTER, TRUE))
@@ -1379,8 +1397,8 @@ void multi_update::send_error(uint errcode,const char *err)
   my_error(errcode, MYF(0), err);
 
   /* If nothing updated return */
-  if (!updated)
-    return;
+  if (updated == 0) /* the counter might be reset in send_eof */
+    return;         /* and then the query has been binlogged */
 
   /* Something already updated so we have to invalidate cache */
   query_cache_invalidate3(thd, update_tables, 1);
@@ -1391,12 +1409,44 @@ void multi_update::send_error(uint errcode,const char *err)
   */
 
   if (trans_safe)
-    ha_rollback_stmt(thd);
-  else if (do_update && table_count > 1)
+  {
+    DBUG_ASSERT(transactional_tables);
+    (void) ha_autocommit_or_rollback(thd, 1);
+  }
+  else
+  { 
+    DBUG_ASSERT(thd->no_trans_update.stmt);
+    if (do_update && table_count > 1)
     {
       /* Add warning here */
+      /* 
+         todo/fixme: do_update() is never called with the arg 1.
+         should it change the signature to become argless?
+      */
       VOID(do_updates(0));
     }
+  }
+  if (thd->no_trans_update.stmt)
+  {
+    /*
+      The query has to binlog because there's a modified non-transactional table
+      either from the query's list or via a stored routine: bug#13270,23333
+    */
+    if (mysql_bin_log.is_open())
+    {
+      Query_log_event qinfo(thd, thd->query, thd->query_length,
+                            transactional_tables, FALSE);
+      mysql_bin_log.write(&qinfo);
+    }
+    if (!trans_safe)
+      thd->no_trans_update.all= TRUE;
+  }
+  DBUG_ASSERT(trans_safe || !updated || thd->no_trans_update.stmt);
+  
+  if (transactional_tables)
+  {
+    (void) ha_autocommit_or_rollback(thd, 1);
+  }
 }
 
 
@@ -1504,7 +1554,10 @@ int multi_update::do_updates(bool from_send_error)
       if (table->file->has_transactions())
         transactional_tables= 1;
       else
+      {
         trans_safe= 0;				// Can't do safe rollback
+        thd->no_trans_update.stmt= TRUE;
+      }
     }
     (void) table->file->ha_rnd_end();
     (void) tmp_table->file->ha_rnd_end();
@@ -1527,7 +1580,10 @@ err2:
     if (table->file->has_transactions())
       transactional_tables= 1;
     else
+    {
       trans_safe= 0;
+      thd->no_trans_update.stmt= TRUE;
+    }
   }
   DBUG_RETURN(1);
 }
@@ -1556,20 +1612,26 @@ bool multi_update::send_eof()
     Write the SQL statement to the binlog if we updated
     rows and we succeeded or if we updated some non
     transactional tables.
+    
+    The query has to binlog because there's a modified non-transactional table
+    either from the query's list or via a stored routine: bug#13270,23333
   */
 
-  if ((local_error == 0) || (updated && !trans_safe))
+  DBUG_ASSERT(trans_safe || !updated || thd->no_trans_update.stmt);
+  if (local_error == 0 || thd->no_trans_update.stmt)
   {
     if (mysql_bin_log.is_open())
     {
       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)
         local_error= 1;				// Rollback update
     }
-    if (!transactional_tables)
+    if (!trans_safe)
       thd->no_trans_update.all= TRUE;
   }
 
@@ -1581,7 +1643,7 @@ bool multi_update::send_eof()
 
   if (local_error > 0) // if the above log write did not fail ...
   {
-    /* Safety: If we haven't got an error before (should not happen) */
+    /* Safety: If we haven't got an error before (can happen in do_updates) */
     my_message(ER_UNKNOWN_ERROR, "An error occured in multi-table update",
 	       MYF(0));
     return TRUE;