Bug #79867 unnecessary using temporary for update

Description:
when the binlog_row_image is FULL, we set read_set and write_set all before update. This leads to setting used_key_is_modified true, which lead to mysql_update use temporary at last.
Actually there is no need to use temporary in most cases.
The root cause is that we set read_set and write_set too early.

How to repeat:
Need to put the follow case in suite binlog.

--source include/have_log_bin.inc
--source include/have_binlog_format_row.inc

show variables like '%binlog_row_image%';

set sql_log_bin=0;
create table t1(id int primary key, a int) engine = innodb;

insert into t1 select 1,1;
insert into t1 select 2,1;
insert into t1 select 3,2;
insert into t1 select 4,2;

set binlog_row_image= FULL;
set sql_log_bin=1;
explain update t1 set a=a+1 where id < 3;
update t1 set a=a+1 where id < 3;

set binlog_row_image= MINIMAL;
explain update t1 set a=a+1 where id < 3;
update t1 set a=a+1 where id < 3;

set sql_log_bin=0;
flush logs;

let $MYSQLD_DATADIR=`select @@datadir`;
--exec $MYSQL_BINLOG --verbose $MYSQLD_DATADIR/master-bin.000001

drop table t1;

Suggested fix:
We should set the read_set and write_set just before write binlog.

diff --git a/sql/binlog.cc b/sql/binlog.cc
index e93fec8..c8c247b 100644
--- a/sql/binlog.cc
+++ b/sql/binlog.cc
@@ -8462,6 +8462,14 @@ int THD::binlog_write_row(TABLE* table, bool is_trans,
 { 
   DBUG_ASSERT(is_current_stmt_binlog_format_row() && mysql_bin_log.is_open());
 
+  MY_BITMAP *old_read_set= table->read_set;
+  MY_BITMAP *old_write_set= table->write_set;
+
+  if (table->in_use->variables.binlog_row_image == BINLOG_ROW_IMAGE_FULL)
+  {
+    table->column_bitmaps_set_no_signal(&table->all_set,
+                                        &table->all_set);
+  }
   /*
     Pack records into format for transfer. We are allocating more
     memory than needed, but that doesn't matter.
@@ -8472,6 +8480,7 @@ int THD::binlog_write_row(TABLE* table, bool is_trans,
 
   uchar *row_data= memory.slot(0);
 
+
   size_t const len= pack_row(table, table->write_set, row_data, record);
 
   Rows_log_event* const ev=
@@ -8479,6 +8488,8 @@ int THD::binlog_write_row(TABLE* table, bool is_trans,
                                       static_cast<Write_rows_log_event*>(0),
                                       extra_row_info);
 
+  table->column_bitmaps_set_no_signal(old_read_set,
+                                      old_write_set);
   if (unlikely(ev == 0))
     return HA_ERR_OUT_OF_MEM;
 
@@ -8623,6 +8634,7 @@ void THD::binlog_prepare_row_images(TABLE *table)
       (thd->variables.binlog_row_image < BINLOG_ROW_IMAGE_FULL) &&
       !ha_check_storage_engine_flag(table->s->db_type(), HTON_NO_BINLOG_ROW_OPT))
   {
+    MY_BITMAP *tmp_write_set= table->write_set;
     /**
       Just to be sure that tmp_set is currently not in use as
       the read_set already.
@@ -8658,7 +8670,13 @@ void THD::binlog_prepare_row_images(TABLE *table)
 
     /* set the temporary read_set */
     table->column_bitmaps_set_no_signal(&table->tmp_set,
-                                        table->write_set);
+                                        tmp_write_set);
+  }
+
+  if (thd->variables.binlog_row_image == BINLOG_ROW_IMAGE_FULL)
+  {
+    table->column_bitmaps_set_no_signal(&table->all_set,
+                                        &table->all_set);
   }
 
   DBUG_PRINT_BITSET("debug", "table->read_set (after preparing): %s", table->read_set);
diff --git a/sql/table.cc b/sql/table.cc
index 7867a71..5bb2fff 100644
--- a/sql/table.cc
+++ b/sql/table.cc
@@ -2315,7 +2315,7 @@ partititon_err:
   /* Allocate bitmaps */
 
   bitmap_size= share->column_bitmap_size;
-  if (!(bitmaps= (uchar*) alloc_root(&outparam->mem_root, bitmap_size*3)))
+  if (!(bitmaps= (uchar*) alloc_root(&outparam->mem_root, bitmap_size*4)))
     goto err;
   bitmap_init(&outparam->def_read_set,
               (my_bitmap_map*) bitmaps, share->fields, FALSE);
@@ -2323,6 +2323,9 @@ partititon_err:
               (my_bitmap_map*) (bitmaps+bitmap_size), share->fields, FALSE);
   bitmap_init(&outparam->tmp_set,
               (my_bitmap_map*) (bitmaps+bitmap_size*2), share->fields, FALSE);
+  bitmap_init(&outparam->all_set,
+              (my_bitmap_map*) (bitmaps+bitmap_size*3), share->fields, FALSE);
+  bitmap_set_all(&outparam->all_set);
   outparam->default_column_bitmaps();
 
   /* The table struct is now initialized;  Open the table */
@@ -5437,11 +5440,13 @@ void TABLE::mark_columns_per_binlog_row_image()
 
     switch (thd->variables.binlog_row_image)
     {
+        /*
       case BINLOG_ROW_IMAGE_FULL:
         if (s->primary_key < MAX_KEY)
           bitmap_set_all(read_set);
         bitmap_set_all(write_set);
         break;
+        */
       case BINLOG_ROW_IMAGE_NOBLOB:
         /* for every field that is not set, mark it unless it is a blob */
         for (Field **ptr=field ; *ptr ; ptr++)
@@ -5464,6 +5469,7 @@ void TABLE::mark_columns_per_binlog_row_image()
             bitmap_set_bit(write_set, field->field_index);
         }
         break;
+      case BINLOG_ROW_IMAGE_FULL:
       case BINLOG_ROW_IMAGE_MINIMAL:
         /* mark the primary key if available in the read_set */
         if (s->primary_key < MAX_KEY)
diff --git a/sql/table.h b/sql/table.h
index fd8d898..df83b70 100644
--- a/sql/table.h
+++ b/sql/table.h
@@ -1056,7 +1056,7 @@ public:
   const char   *alias;                   /* alias or table name */
   uchar                *null_flags;
   my_bitmap_map        *bitmap_init_value;
-  MY_BITMAP     def_read_set, def_write_set, tmp_set; /* containers */
+  MY_BITMAP     def_read_set, def_write_set, tmp_set, all_set; /* containers */
   MY_BITMAP     *read_set, *write_set;          /* Active column sets */
   /*
    The ID of the query that opened and is using this table. Has different

Hello zhang yingqiang,

Thank you for the report and contribution.

Thanks,
Umesh

// 5.6.28

worker[1] Using MTR_BUILD_THREAD 300, with reserved ports 13000..13009
binlog.rpl_79867 'mix'                   [ skipped ]  Doesn't support --binlog-format='mixed'
show variables like '%binlog_row_image%';
Variable_name   Value
binlog_row_image        FULL
set sql_log_bin=0;
create table t1(id int primary key, a int) engine = innodb;
insert into t1 select 1,1;
insert into t1 select 2,1;
insert into t1 select 3,2;
insert into t1 select 4,2;
set binlog_row_image= FULL;
set sql_log_bin=1;
explain update t1 set a=a+1 where id < 3;
id      select_type     table   type    possible_keys   key     key_len ref     rows    Extra
1       SIMPLE  t1      range   PRIMARY PRIMARY 4       const   2       Using where; Using temporary
update t1 set a=a+1 where id < 3;
set binlog_row_image= MINIMAL;
explain update t1 set a=a+1 where id < 3;
id      select_type     table   type    possible_keys   key     key_len ref     rows    Extra
1       SIMPLE  t1      range   PRIMARY PRIMARY 4       const   2       Using where
update t1 set a=a+1 where id < 3;
set sql_log_bin=0;
flush logs;
/*!50530 SET @@SESSION.PSEUDO_SLAVE_MODE=1*/;
/*!40019 SET @@session.max_insert_delayed_threads=0*/;
/*!50003 SET @OLD_COMPLETION_TYPE=@@COMPLETION_TYPE,COMPLETION_TYPE=0*/;
DELIMITER /*!*/;
# at 4
#160107 15:12:37 server id 1  end_log_pos 120 CRC32 0x99631b4f  Start: binlog v 4, server v 5.6.28-enterprise-commercial-advanced-log created 160107 15:12:37 at startup
ROLLBACK/*!*/;
BINLOG '
NVaOVg8BAAAAdAAAAHgAAAAAAAQANS42LjI4LWVudGVycHJpc2UtY29tbWVyY2lhbC1hZHZhbmNl
ZC1sb2cAAAAAAAAAAAA1Vo5WEzgNAAgAEgAEBAQEEgAAXAAEGggAAAAICAgCAAAACgoKGRkAAU8b
Y5k=
'/*!*/;
# at 120
#160107 15:12:38 server id 1  end_log_pos 192 CRC32 0xa96bc13b  Query   thread_id=2     exec_time=0     error_code=0
SET TIMESTAMP=1452168758/*!*/;
SET @@session.pseudo_thread_id=2/*!*/;
SET @@session.foreign_key_checks=1, @@session.sql_auto_is_null=0, @@session.unique_checks=1, @@session.autocommit=1/*!*/;
SET @@session.sql_mode=1073741824/*!*/;
SET @@session.auto_increment_increment=1, @@session.auto_increment_offset=1/*!*/;
/*!\C latin1 *//*!*/;
SET @@session.character_set_client=8,@@session.collation_connection=8,@@session.collation_server=8/*!*/;
SET @@session.lc_time_names=0/*!*/;
SET @@session.collation_database=DEFAULT/*!*/;
BEGIN
/*!*/;
# at 192
#160107 15:12:38 server id 1  end_log_pos 238 CRC32 0x69d472a4  Table_map: `test`.`t1` mapped to number 81
# at 238
#160107 15:12:38 server id 1  end_log_pos 310 CRC32 0xe49cfbc5  Update_rows: table id 81 flags: STMT_END_F

BINLOG '
NlaOVhMBAAAALgAAAO4AAAAAAFEAAAAAAAEABHRlc3QAAnQxAAIDAwACpHLUaQ==
NlaOVh8BAAAASAAAADYBAAAAAFEAAAAAAAEAAgAC///8AQAAAAEAAAD8AQAAAAIAAAD8AgAAAAEA
AAD8AgAAAAIAAADF+5zk
'/*!*/;
### UPDATE `test`.`t1`
### WHERE
###   @1=1
###   @2=1
### SET
###   @1=1
###   @2=2
### UPDATE `test`.`t1`
### WHERE
###   @1=2
###   @2=1
### SET
###   @1=2
###   @2=2
# at 310
#160107 15:12:38 server id 1  end_log_pos 341 CRC32 0x17b600a0  Xid = 37
COMMIT/*!*/;
# at 341
#160107 15:12:38 server id 1  end_log_pos 413 CRC32 0x69d65b92  Query   thread_id=2     exec_time=0     error_code=0
SET TIMESTAMP=1452168758/*!*/;
BEGIN
/*!*/;
# at 413
#160107 15:12:38 server id 1  end_log_pos 459 CRC32 0x8ae82807  Table_map: `test`.`t1` mapped to number 81
# at 459
#160107 15:12:38 server id 1  end_log_pos 515 CRC32 0x3bd7eb3c  Update_rows: table id 81 flags: STMT_END_F

BINLOG '
NlaOVhMBAAAALgAAAMsBAAAAAFEAAAAAAAEABHRlc3QAAnQxAAIDAwACByjoig==
NlaOVh8BAAAAOAAAAAMCAAAAAFEAAAAAAAEAAgACAQL+AQAAAP4DAAAA/gIAAAD+AwAAADzr1zs=
'/*!*/;
### UPDATE `test`.`t1`
### WHERE
###   @1=1
### SET
###   @2=3
### UPDATE `test`.`t1`
### WHERE
###   @1=2
### SET
###   @2=3
# at 515
#160107 15:12:38 server id 1  end_log_pos 546 CRC32 0xbedb603a  Xid = 40
COMMIT/*!*/;
# at 546
#160107 15:12:38 server id 1  end_log_pos 594 CRC32 0x68c6788b  Rotate to master-bin.000002  pos: 4
DELIMITER ;
# End of log file
ROLLBACK /* added by mysqlbinlog */;
/*!50003 SET COMPLETION_TYPE=@OLD_COMPLETION_TYPE*/;
/*!50530 SET @@SESSION.PSEUDO_SLAVE_MODE=0*/;
drop table t1;
binlog.rpl_79867 'row'                   [ pass ]     18

Patch for Bug#79867 fix.

Attachment: Bug79867_mysql-5.6.diff (text/x-patch), 10.50 KiB.

Please find the above patch that fixes the issue.
Give it a try at your end and please let us know if you see any issues with it.

Posted by developer:
 
Thank you for your feedback, this has been fixed in upcoming versions and the following was added to the 5.6.33 and 5.7.15 change logs:
With binlog_row_image=FULL, when updating single tables temporary tables were unnecessarily being used. The fix ensures single table update follows the same pattern as multi-table update. Thanks to Zhang Yingqiang for the contribution that this fix was based on.

Posted by developer:
 
Sorry, made a mistake and actually the patch was not based on Zhang's contribution. Therefore the changelog entry was modified to:
With binlog_row_image=FULL, when updating single tables temporary tables were unnecessarily being used. The fix ensures single table update follows the same pattern as multi-table update.

Bug#80424 EXPLAIN output depends on binlog_format setting

was marked as a duplicate.