Bug #23333 stored function + non-transac table + transac table = breaks stmt-based binlog

Binlogging of the statement with a side effect like a modified non-trans table did not happen.
The artifact involved all binloggable dml queries.

Fixed with changing the binlogging conditions all over the code to exploit thd->transaction.stmt.modified_non_trans_table
introduced by the patch for bug@27417.

Multi-delete case has own specific addressed by another bug@29136. Multi-update case has been addressed by bug#27716 and
patch and will need merging.

mysql-test/r/mix_innodb_myisam_binlog.result

@@ -365,7 +365,7 @@ insert into t2 values (bug27417(2));
 ERROR 23000: Duplicate entry '2' for key 1
 show master status;
 File	Position	Binlog_Do_DB	Binlog_Ignore_DB
-master-bin.000001	98		
+master-bin.000001	196		
 /* only (!) with fixes for #23333 will show there is the query */;
 select count(*) from t1 /* must be 3 */;
 count(*)
@@ -390,6 +390,75 @@ affected rows: 0
 select count(*) from t1 /* must be 7 */;
 count(*)
 7
-drop function bug27417;
 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);
+insert into t2 values (1);
+reset master;
+insert into t2 values (bug27417(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	267		
+select count(*) from t1 /* must be 1 */;
+count(*)
+1
+delete from t1;
+delete from t2;
+insert into t2 values (2);
+reset master;
+insert into t2 select bug27417(1) union select bug27417(2);
+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	290		
+select count(*) from t1 /* must be 2 */;
+count(*)
+2
+delete from t1;
+insert into t3 values (1,1),(2,3),(3,4);
+reset master;
+update t3 set b=b+bug27417(1);
+ERROR 23000: Duplicate entry '4' for key 2
+show master status /* the offset must denote there is the query */;
+File	Position	Binlog_Do_DB	Binlog_Ignore_DB
+master-bin.000001	190		
+select count(*) from t1 /* must be 2 */;
+count(*)
+2
+delete from t1;
+delete from t2;
+delete from t3;
+insert into t2 values (1);
+insert into t3 values (1,1);
+create trigger trg_del before delete on t2 for each row 
+insert into t3 values (bug27417(1), 2);
+reset master;
+delete from t2;
+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	246		
+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);
+reset master;
+load data infile '../std_data_ln/rpl_loaddata.dat' into table t4 (a, @b) set b= @b + bug27417(2);
+ERROR 23000: Duplicate entry '17' for key 1
+select * from t4;
+a	b
+0	17
+select count(*) from t1 /* must be 2 */;
+count(*)
+2
+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;
+drop function bug27417;
 end of tests

mysql-test/r/sp_trans_log.result

@@ -12,8 +12,9 @@ end|
 reset master|
 insert into t2 values (bug23333(),1)|
 ERROR 23000: Duplicate entry '1' for key 1
-show binlog events from 98 /* with fixes for #23333 will show there is the query */|
-Log_name	Pos	Event_type	Server_id	End_log_pos	Info
+show master status /* the offset must denote there is the query */|
+File	Position	Binlog_Do_DB	Binlog_Ignore_DB
+master-bin.000001	284		
 select count(*),@a from t1 /* must be 1,1 */|
 count(*)	@a
 1	1

mysql-test/t/mix_innodb_myisam_binlog.test

@@ -380,8 +380,126 @@ delete t2 from t2 where t2.a=bug27417(100) /* must not affect t2 */;
 --disable_info
 select count(*) from t1 /* must be 7 */;
 
-drop function bug27417;
+# function bug27417 remains for the following testing of bug#23333
 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);
+
+
+#
+# INSERT
+#
+
+# prepare
+  
+ insert into t2 values (1);
+ reset master;
+
+# execute
+
+ --error ER_DUP_ENTRY
+ insert into t2 values (bug27417(1)); 
+  
+# check
+
+ show master status /* the offset must denote there is the query */;
+ select count(*) from t1 /* must be 1 */;
+
+#
+# INSERT SELECT
+#
+
+# prepare
+ delete from t1;
+ delete from t2;
+ insert into t2 values (2);
+ reset master;
+
+# execute
+
+ --error ER_DUP_ENTRY
+ insert into t2 select bug27417(1) union select bug27417(2); 
+  
+# check
+
+ show master status /* the offset must denote there is the query */;
+ select count(*) from t1 /* must be 2 */;
+
+#
+# UPDATE (multi-update see bug#27716)
+#
+
+# prepare
+ delete from t1;
+ insert into t3 values (1,1),(2,3),(3,4);
+ reset master;
+
+# execute
+ --error ER_DUP_ENTRY
+ update t3 set b=b+bug27417(1);
+
+# check
+ show master status /* the offset must denote there is the query */;
+ select count(*) from t1 /* must be 2 */;
+
+
+#
+# DELETE (for multi-delete see Bug #29136)
+#
+
+# prepare
+ delete from t1;
+ delete from t2;
+ delete from t3;
+ insert into t2 values (1);
+ insert into t3 values (1,1);
+ create trigger trg_del before delete on t2 for each row 
+   insert into t3 values (bug27417(1), 2);
+ reset master;
+
+# execute
+ --error ER_DUP_ENTRY
+ delete from t2;
+# check
+ show master status /* the offset must denote there is the query */;
+ select count(*) from t1 /* must be 1 */;
+
+
+#
+# LOAD DATA
+#
+
+# prepare
+ delete from t1;
+ create table t4 (a int default 0, b int primary key) engine=innodb;
+ insert into t4 values (0, 17);
+ reset master;
+
+# execute
+ --error ER_DUP_ENTRY
+ load data infile '../std_data_ln/rpl_loaddata.dat' into table t4 (a, @b) set b= @b + bug27417(2);
+# check
+ select * from t4;
+ select count(*) from t1 /* must be 2 */;
+ show master status /* the offset must denote there is the query */;
+
+#
+# bug#23333 cleanup
+#
+
+
+drop trigger trg_del;
+drop table t1,t2,t3;
+drop function bug27417;
+
+
 --echo end of tests
 

mysql-test/t/sp_trans_log.test

@@ -26,8 +26,7 @@ end|
 reset master|
 --error ER_DUP_ENTRY
 insert into t2 values (bug23333(),1)| 
---replace_column 2 # 5 # 6 #
-show binlog events from 98 /* with fixes for #23333 will show there is the query */|
+show master status /* the offset must denote there is the query */|
 select count(*),@a from t1 /* must be 1,1 */|
 drop table t1, t2|
 

sql/sql_delete.cc

@@ -319,7 +319,7 @@ cleanup:
     thd->transaction.stmt.modified_non_trans_table= TRUE;
   
   /* See similar binlogging code in sql_update.cc, for comments */
-  if ((error < 0) || (deleted && !transactional_table))
+  if ((error < 0) || thd->transaction.stmt.modified_non_trans_table)
   {
     if (mysql_bin_log.is_open())
     {
@@ -817,7 +817,8 @@ bool multi_delete::send_eof()
   {
     query_cache_invalidate3(thd, delete_tables, 1);
   }
-  if ((local_error == 0) || (deleted && normal_tables))
+  DBUG_ASSERT(!normal_tables || !deleted || thd->transaction.stmt.modified_non_trans_table);
+  if ((local_error == 0) || thd->transaction.stmt.modified_non_trans_table)
   {
     if (mysql_bin_log.is_open())
     {
@@ -831,7 +832,6 @@ bool multi_delete::send_eof()
     if (thd->transaction.stmt.modified_non_trans_table)
       thd->transaction.all.modified_non_trans_table= TRUE;
   }
-  DBUG_ASSERT(!normal_tables || !deleted || thd->transaction.stmt.modified_non_trans_table);
 
   /* Commit or rollback the current SQL statement */
   if (transactional_tables)

sql/sql_insert.cc

@@ -866,8 +866,7 @@ bool mysql_insert(THD *thd,TABLE_LIST *table_list,
 
     transactional_table= table->file->has_transactions();
 
-    if ((changed= (info.copied || info.deleted || info.updated)) ||
-        was_insert_delayed)
+    if ((changed= (info.copied || info.deleted || info.updated)))
     {
       /*
         Invalidate the table in the query cache if something changed.
@@ -876,46 +875,47 @@ bool mysql_insert(THD *thd,TABLE_LIST *table_list,
       */
       if (changed)
         query_cache_invalidate3(thd, table_list, 1);
-      if (error <= 0 || !transactional_table)
+    }
+    if (changed && error <= 0 || thd->transaction.stmt.modified_non_trans_table
+        || was_insert_delayed)
+    {
+      if (mysql_bin_log.is_open())
       {
-        if (mysql_bin_log.is_open())
+        if (error <= 0)
         {
-          if (error <= 0)
-          {
-            /*
-              [Guilhem wrote] Temporary errors may have filled
-              thd->net.last_error/errno.  For example if there has
-              been a disk full error when writing the row, and it was
-              MyISAM, then thd->net.last_error/errno will be set to
-              "disk full"... and the my_pwrite() will wait until free
-              space appears, and so when it finishes then the
-              write_row() was entirely successful
-            */
-            /* todo: consider removing */
-            thd->clear_error();
-          }
-          /* bug#22725: 
-               
-          A query which per-row-loop can not be interrupted with
-          KILLED, like INSERT, and that does not invoke stored
-          routines can be binlogged with neglecting the KILLED error.
-          
-          If there was no error (error == zero) until after the end of
-          inserting loop the KILLED flag that appeared later can be
-          disregarded since previously possible invocation of stored
-          routines did not result in any error due to the KILLED.  In
-          such case the flag is ignored for constructing binlog event.
+          /*
+            [Guilhem wrote] Temporary errors may have filled
+            thd->net.last_error/errno.  For example if there has
+            been a disk full error when writing the row, and it was
+            MyISAM, then thd->net.last_error/errno will be set to
+            "disk full"... and the my_pwrite() will wait until free
+            space appears, and so when it finishes then the
+            write_row() was entirely successful
           */
-          Query_log_event qinfo(thd, thd->query, thd->query_length,
-                                transactional_table, FALSE,
-                                (error>0) ? thd->killed : THD::NOT_KILLED);
-          DBUG_ASSERT(thd->killed != THD::KILL_BAD_DATA || error > 0);
-          if (mysql_bin_log.write(&qinfo) && transactional_table)
-            error=1;
+          /* todo: consider removing */
+          thd->clear_error();
         }
-        if (thd->transaction.stmt.modified_non_trans_table)
-          thd->transaction.all.modified_non_trans_table= TRUE;
+        /* bug#22725: 
+           
+           A query which per-row-loop can not be interrupted with
+           KILLED, like INSERT, and that does not invoke stored
+           routines can be binlogged with neglecting the KILLED error.
+           
+           If there was no error (error == zero) until after the end of
+           inserting loop the KILLED flag that appeared later can be
+           disregarded since previously possible invocation of stored
+           routines did not result in any error due to the KILLED.  In
+           such case the flag is ignored for constructing binlog event.
+        */
+        Query_log_event qinfo(thd, thd->query, thd->query_length,
+                              transactional_table, FALSE,
+                              (error>0) ? thd->killed : THD::NOT_KILLED);
+        DBUG_ASSERT(thd->killed != THD::KILL_BAD_DATA || error > 0);
+        if (mysql_bin_log.write(&qinfo) && transactional_table)
+          error=1;
       }
+      if (thd->transaction.stmt.modified_non_trans_table)
+        thd->transaction.all.modified_non_trans_table= TRUE;
     }
     DBUG_ASSERT(transactional_table || !changed || 
                 thd->transaction.stmt.modified_non_trans_table);
@@ -3001,6 +3001,7 @@ void select_insert::abort()
     */
     DBUG_VOID_RETURN;
   }
+  changed= (info.copied || info.deleted || info.updated);
   transactional_table= table->file->has_transactions();
   if (!thd->prelocked_mode)
     table->file->end_bulk_insert();
@@ -3010,8 +3011,7 @@ void select_insert::abort()
     error while inserting into a MyISAM table) we must write to the binlog (and
     the error code will make the slave stop).
   */
-  if ((changed= info.copied || info.deleted || info.updated) &&
-      !transactional_table)
+  if (thd->transaction.stmt.modified_non_trans_table)
   {
     if (last_insert_id)
       thd->insert_id(last_insert_id);		// For binary log

sql/sql_load.cc

@@ -444,7 +444,7 @@ bool mysql_load(THD *thd,sql_exchange *ex,TABLE_LIST *table_list,
       /* If the file was not empty, wrote_create_file is true */
       if (lf_info.wrote_create_file)
       {
-	if ((info.copied || info.deleted) && !transactional_table)
+	if (thd->transaction.stmt.modified_non_trans_table)
 	  write_execute_load_query_log_event(thd, handle_duplicates,
 					     ignore, transactional_table);
 	else

sql/sql_update.cc

@@ -580,7 +580,7 @@ int mysql_update(THD *thd,
     Sometimes we want to binlog even if we updated no rows, in case user used
     it to be sure master and slave are in same state.
   */
-  if ((error < 0) || (updated && !transactional_table))
+  if ((error < 0) || thd->transaction.stmt.modified_non_trans_table)
   {
     if (mysql_bin_log.is_open())
     {