Bug#43580: Issue with Innodb on multi-table update

                        
Certain multi-updates gave different results on InnoDB from
to MyISAM, due to on-the-fly updates being used on the former and
the update order matters.
Fixed by turning off on-the-fly updates when update order 
dependencies are present.


mysql-test/r/innodb_mysql.result:
  Bug#43580: Test result.
mysql-test/suite/rpl/r/rpl_slave_skip.result:
  Bug#43580: Changed test result. The InnoDB result is now what it would have been on MyISAM.
mysql-test/t/innodb_mysql.test:
  Bug#43580: Test case.
sql/sql_base.cc:
  Bug#43580: Added a word of caution about using tmp_set here.
sql/sql_update.cc:
  Bug#43580: Fix.
  Calls to TABLE::mark_columns_needed_for_update() are moved
  from mysql_multi_update_prepare() and right before the decison
  to do on-the-fly updates to the place where we do (or don't do)
  on-the-fly updates.

mysql-test/r/innodb_mysql.result

@@ -2021,4 +2021,31 @@ DROP TABLE t4;
 DROP TABLE t1;
 DROP TABLE t2;
 DROP TABLE t3;
+CREATE TABLE t1 (a INT, b INT, KEY (a)) ENGINE = INNODB;
+CREATE TABLE t2 (a INT KEY, b INT, KEY (b)) ENGINE = INNODB;
+CREATE TABLE t3 (a INT, b INT KEY, KEY (a)) ENGINE = INNODB;
+CREATE TABLE t4 (a INT KEY, b INT, KEY (b)) ENGINE = INNODB;
+INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3), (4, 4), (5, 5), (6, 6);
+INSERT INTO t2 VALUES (1, 1), (2, 2), (3, 3), (4, 4), (5, 5);
+INSERT INTO t3 VALUES (1, 101), (2, 102), (3, 103), (4, 104), (5, 105), (6, 106);
+INSERT INTO t4 VALUES (1, 1), (2, 2), (3, 3), (4, 4), (5, 5);
+UPDATE t1, t2 SET t1.a = t1.a + 100, t2.b = t1.a + 10 
+WHERE t1.a BETWEEN 2 AND 4 AND t2.a = t1.b;
+SELECT * FROM t2;
+a	b
+1	1
+2	12
+3	13
+4	14
+5	5
+UPDATE t3, t4 SET t3.a = t3.a + 100, t4.b = t3.a + 10 
+WHERE t3.a BETWEEN 2 AND 4 AND t4.a = t3.b - 100;
+SELECT * FROM t4;
+a	b
+1	1
+2	12
+3	13
+4	14
+5	5
+DROP TABLE t1, t2, t3, t4;
 End of 5.1 tests

mysql-test/suite/rpl/r/rpl_slave_skip.result

@@ -39,8 +39,8 @@ a	b
 SELECT * FROM t2;
 c	d
 1	2
-2	16
-3	54
+2	8
+3	18
 **** On Slave ****
 START SLAVE UNTIL MASTER_LOG_FILE='master-bin.000001', MASTER_LOG_POS=762;
 SHOW SLAVE STATUS;
@@ -50,7 +50,7 @@ Master_User	root
 Master_Port	MASTER_PORT
 Connect_Retry	1
 Master_Log_File	master-bin.000001
-Read_Master_Log_Pos	1133
+Read_Master_Log_Pos	1115
 Relay_Log_File	#
 Relay_Log_Pos	#
 Relay_Master_Log_File	master-bin.000001

mysql-test/t/innodb_mysql.test

@@ -332,4 +332,31 @@ DROP TABLE t1;
 DROP TABLE t2;
 DROP TABLE t3;
 
+#
+# Bug#43580: Issue with Innodb on multi-table update
+#
+CREATE TABLE t1 (a INT, b INT, KEY (a)) ENGINE = INNODB;
+CREATE TABLE t2 (a INT KEY, b INT, KEY (b)) ENGINE = INNODB;
+
+CREATE TABLE t3 (a INT, b INT KEY, KEY (a)) ENGINE = INNODB;
+CREATE TABLE t4 (a INT KEY, b INT, KEY (b)) ENGINE = INNODB;
+
+INSERT INTO t1 VALUES (1, 1), (2, 2), (3, 3), (4, 4), (5, 5), (6, 6);
+INSERT INTO t2 VALUES (1, 1), (2, 2), (3, 3), (4, 4), (5, 5);
+
+INSERT INTO t3 VALUES (1, 101), (2, 102), (3, 103), (4, 104), (5, 105), (6, 106);
+INSERT INTO t4 VALUES (1, 1), (2, 2), (3, 3), (4, 4), (5, 5);
+
+UPDATE t1, t2 SET t1.a = t1.a + 100, t2.b = t1.a + 10 
+WHERE t1.a BETWEEN 2 AND 4 AND t2.a = t1.b;
+--sorted_result
+SELECT * FROM t2;
+
+UPDATE t3, t4 SET t3.a = t3.a + 100, t4.b = t3.a + 10 
+WHERE t3.a BETWEEN 2 AND 4 AND t4.a = t3.b - 100;
+--sorted_result
+SELECT * FROM t4;
+
+DROP TABLE t1, t2, t3, t4;
+
 --echo End of 5.1 tests

sql/sql_base.cc

@@ -5585,6 +5585,13 @@ static void update_field_dependencies(THD *thd, Field *field, TABLE *table)
       other_bitmap=   table->read_set;
     }
 
+    /* 
+       The test-and-set mechanism in the bitmap is not reliable during
+       multi-UPDATE statements under MARK_COLUMNS_READ mode
+       (thd->mark_used_columns == MARK_COLUMNS_READ), as this bitmap contains
+       only those columns that are used in the SET clause. I.e they are being
+       set here. See multi_update::prepare()
+    */
     if (bitmap_fast_test_and_set(current_bitmap, field->field_index))
     {
       if (thd->mark_used_columns == MARK_COLUMNS_WRITE)

sql/sql_update.cc

@@ -1031,7 +1031,6 @@ int mysql_multi_update_prepare(THD *thd)
         DBUG_RETURN(TRUE);
       }
 
-      table->mark_columns_needed_for_update();
       DBUG_PRINT("info",("setting table `%s` for update", tl->alias));
       /*
         If table will be updated we should not downgrade lock for it and
@@ -1274,13 +1273,41 @@ int multi_update::prepare(List<Item> &not_used_values,
     DBUG_RETURN(1);
   }
 
+  /*
+    We gather the set of columns read during evaluation of SET expression in
+    TABLE::tmp_set by pointing TABLE::read_set to it and then restore it after
+    setup_fields().
+  */
+  for (table_ref= leaves; table_ref; table_ref= table_ref->next_leaf)
+  {
+    TABLE *table= table_ref->table;
+    if (tables_to_update & table->map)
+    {
+      DBUG_ASSERT(table->read_set == &table->def_read_set);
+      table->read_set= &table->tmp_set;
+      bitmap_clear_all(table->read_set);
+    }
+  }
+
   /*
     We have to check values after setup_tables to get covering_keys right in
     reference tables
   */
 
-  if (setup_fields(thd, 0, *values, MARK_COLUMNS_READ, 0, 0))
-    DBUG_RETURN(1);
+  int error= setup_fields(thd, 0, *values, MARK_COLUMNS_READ, 0, 0);
+
+  for (table_ref= leaves; table_ref; table_ref= table_ref->next_leaf)
+  {
+    TABLE *table= table_ref->table;
+    if (tables_to_update & table->map)
+    {
+      table->read_set= &table->def_read_set;
+      bitmap_union(table->read_set, &table->tmp_set);
+    }
+  }
+  
+  if (error)
+    DBUG_RETURN(1);    
 
   /*
     Save tables beeing updated in update_tables
@@ -1375,6 +1402,8 @@ int multi_update::prepare(List<Item> &not_used_values,
     a row in this table will never be read twice. This is true under
     the following conditions:
 
+    - No column is both written to and read in SET expressions.
+
     - We are doing a table scan and the data is in a separate file (MyISAM) or
       if we don't update a clustered key.
 
@@ -1389,6 +1418,9 @@ int multi_update::prepare(List<Item> &not_used_values,
   WARNING
     This code is a bit dependent of how make_join_readinfo() works.
 
+    The field table->tmp_set is used for keeping track of which fields are
+    read during evaluation of the SET expression. See multi_update::prepare.
+
   RETURN
     0		Not safe to update
     1		Safe to update
@@ -1409,6 +1441,8 @@ static bool safe_update_on_fly(THD *thd, JOIN_TAB *join_tab,
   case JT_REF_OR_NULL:
     return !is_key_used(table, join_tab->ref.key, table->write_set);
   case JT_ALL:
+    if (bitmap_is_overlapping(&table->tmp_set, table->write_set))
+      return FALSE;
     /* If range search on index */
     if (join_tab->quick)
       return !join_tab->quick->is_keys_used(table->write_set);
@@ -1464,17 +1498,18 @@ multi_update::initialize_tables(JOIN *join)
     ORDER     group;
     TMP_TABLE_PARAM *tmp_param;
 
-    table->mark_columns_needed_for_update();
     if (ignore)
       table->file->extra(HA_EXTRA_IGNORE_DUP_KEY);
     if (table == main_table)			// First table in join
     {
       if (safe_update_on_fly(thd, join->join_tab, table_ref, all_tables))
       {
-	table_to_update= main_table;		// Update table on the fly
+        table->mark_columns_needed_for_update();
+	table_to_update= table;			// Update table on the fly
 	continue;
       }
     }
+    table->mark_columns_needed_for_update();
     table->prepare_for_position();
 
     /*