Bug#14578060: INNODB: WARNING: USING A PARTIAL-FIELD KEY

Jorgen Loland · Jorgen Loland · commit ddf6c80ba102 · 2012-09-25T13:35:36.000+02:00
PREFIX IN SEARCH

When UPDATE creates a duplicate out of columns defined as unique,
an error is issued. The error message needs info about the column
values to make this error message, and if this info is not 
available in the offended index a table read is done through
ha_rnd_pos().

The were multiple problems in this code.
1) the error-creation code in mysql_update() called ha_rnd_pos()
   without calling ha_rnd_init() first. This was OK for InnoDB
   in MySQL 5.5 because InnoDB noticed it and called 
   change_active_index() internally. In 5.6 InnoDB no longer
   automatically switches index, resulting in this bug report.
2) When doing an UPDATE, InnoDB reads all columns of the row
   to update. For this reason, mysql_update() does not even 
   have to call ha_rnd_pos() to get the info. Furthermore,
   MyISAM always reads all rows anyway.

Due to 2), the simplest bug fix would have been to not read
additional columns from the table if the SE is InnoDB. However,
the problem would persist for other SEs. An alternative patch
would be to open another handler that calls 
ha_rnd_init();ha_rnd_pos();ha_rnd_end() and is then destroyed,
but it makes more sense to change the contract with the handler
slightly: If ha_update_row() returns duplicate key error, all
columns relevant for the error message must have been read. The
need for an extra ha_rnd_pos() is therefore eliminated.

MyISAM and InnoDB already obeys this new contract. NDB Cluster 
(Magnus) has agreed to fix this in their codebase.
diff --git a/sql/handler.h b/sql/handler.h
@@ -2990,6 +2990,14 @@ class handler :public Sql_alloc
     return HA_ERR_WRONG_COMMAND;
   }
 
+  /**
+    Update a single row.
+
+    Note: If HA_ERR_FOUND_DUPP_KEY is returned, the handler must read
+    all columns of the row so MySQL can create an error message. If
+    the columns required for the error message are not read, the error
+    message will contain garbage.
+  */
   virtual int update_row(const uchar *old_data __attribute__((unused)),
                          uchar *new_data __attribute__((unused)))
   {
@@ -3066,12 +3074,15 @@ class handler :public Sql_alloc
     that another call to bulk_update_row will occur OR a call to
     exec_bulk_update before the set of updates in this query is concluded.
 
+    Note: If HA_ERR_FOUND_DUPP_KEY is returned, the handler must read
+    all columns of the row so MySQL can create an error message. If
+    the columns required for the error message are not read, the error
+    message will contain garbage.
+
     @param    old_data       Old record
     @param    new_data       New record
     @param    dup_key_found  Number of duplicate keys found
 
-    @retval  0   Bulk delete used by handler
-    @retval  1   Bulk delete not used, normal operation used
   */
   virtual int bulk_update_row(const uchar *old_data, uchar *new_data,
                               uint *dup_key_found)
diff --git a/sql/sql_update.cc b/sql/sql_update.cc
@@ -190,75 +190,6 @@ static bool check_constant_expressions(List<Item> &values)
 }
 
 
-/**
-  Re-read record if more columns are needed for error message.
-
-  If we got a duplicate key error, we want to write an error
-  message containing the value of the duplicate key. If we do not have
-  all fields of the key value in record[0], we need to re-read the
-  record with a proper read_set.
-
-  @param[in] error   error number
-  @param[in] table   table
-*/
-
-static void prepare_record_for_error_message(int error, TABLE *table)
-{
-  Field **field_p;
-  Field *field;
-  uint keynr;
-  MY_BITMAP unique_map; /* Fields in offended unique. */
-  my_bitmap_map unique_map_buf[bitmap_buffer_size(MAX_FIELDS)];
-  DBUG_ENTER("prepare_record_for_error_message");
-
-  /*
-    Only duplicate key errors print the key value.
-    If storage engine does always read all columns, we have the value alraedy.
-  */
-  if ((error != HA_ERR_FOUND_DUPP_KEY) ||
-      !(table->file->ha_table_flags() & HA_PARTIAL_COLUMN_READ))
-    DBUG_VOID_RETURN;
-
-  /*
-    Get the number of the offended index.
-    We will see MAX_KEY if the engine cannot determine the affected index.
-  */
-  if ((keynr= table->file->get_dup_key(error)) >= MAX_KEY)
-    DBUG_VOID_RETURN;
-
-  /* Create unique_map with all fields used by that index. */
-  bitmap_init(&unique_map, unique_map_buf, table->s->fields, FALSE);
-  table->mark_columns_used_by_index_no_reset(keynr, &unique_map);
-
-  /* Subtract read_set and write_set. */
-  bitmap_subtract(&unique_map, table->read_set);
-  bitmap_subtract(&unique_map, table->write_set);
-
-  /*
-    If the unique index uses columns that are neither in read_set
-    nor in write_set, we must re-read the record.
-    Otherwise no need to do anything.
-  */
-  if (bitmap_is_clear_all(&unique_map))
-    DBUG_VOID_RETURN;
-
-  /* Get identifier of last read record into table->file->ref. */
-  table->file->position(table->record[0]);
-  /* Add all fields used by unique index to read_set. */
-  bitmap_union(table->read_set, &unique_map);
-  /* Tell the engine about the new set. */
-  table->file->column_bitmaps_signal();
-  /* Read record that is identified by table->file->ref. */
-  (void) table->file->ha_rnd_pos(table->record[1], table->file->ref);
-  /* Copy the newly read columns into the new record. */
-  for (field_p= table->field; (field= *field_p); field_p++)
-    if (bitmap_is_set(&unique_map, field->field_index))
-      field->copy_from_tmp(table->s->rec_buff_length);
-
-  DBUG_VOID_RETURN;
-}
-
-
 /*
   Process usual UPDATE
 
@@ -793,10 +724,6 @@ int mysql_update(THD *thd,
       check_constant_expressions(values))
     read_removal= table->check_read_removal(select->quick->index);
 
-  // For prepare_record_for_error_message():
-  if (table->file->ha_table_flags() & HA_PARTIAL_COLUMN_READ)
-    table->prepare_for_position();
-
   while (!(error=info.read_record(&info)) && !thd->killed)
   {
     thd->inc_examined_row_count(1);
@@ -895,7 +822,6 @@ int mysql_update(THD *thd,
           if (table->file->is_fatal_error(error, HA_CHECK_DUP_KEY))
             flags|= ME_FATALERROR; /* Other handler errors are fatal */
 
-          prepare_record_for_error_message(error, table);
 	  table->file->print_error(error,MYF(flags));
 	  error= 1;
 	  break;
@@ -932,7 +858,6 @@ int mysql_update(THD *thd,
               The handler should not report error of duplicate keys if they
               are ignored. This is a requirement on batching handlers.
             */
-            prepare_record_for_error_message(error, table);
             table->file->print_error(error,MYF(0));
             error= 1;
             break;
@@ -993,7 +918,6 @@ int mysql_update(THD *thd,
     */
   {
     /* purecov: begin inspected */
-    prepare_record_for_error_message(loc_error, table);
     table->file->print_error(loc_error,MYF(ME_FATALERROR));
     error= 1;
     /* purecov: end */
@@ -2135,7 +2059,6 @@ bool multi_update::send_data(List<Item> &not_used_values)
             if (table->file->is_fatal_error(error, HA_CHECK_DUP_KEY))
               flags|= ME_FATALERROR; /* Other handler errors are fatal */
 
-            prepare_record_for_error_message(error, table);
             table->file->print_error(error,MYF(flags));
             DBUG_RETURN(1);
           }
@@ -2416,7 +2339,6 @@ int multi_update::do_updates()
 
 err:
   {
-    prepare_record_for_error_message(local_error, table);
     table->file->print_error(local_error,MYF(ME_FATALERROR));
   }
 
diff --git a/sql/table.cc b/sql/table.cc
@@ -4935,13 +4935,16 @@ void TABLE::clear_column_bitmaps()
 }
 
 
-/*
+/**
   Tell handler we are going to call position() and rnd_pos() later.
   
-  NOTES:
   This is needed for handlers that uses the primary key to find the
   row. In this case we have to extend the read bitmap with the primary
   key fields.
+
+  @note: Calling this function does not initialize the table for
+  reading using rnd_pos(). rnd_init() still has to be called before
+  rnd_pos().
 */
 
 void TABLE::prepare_for_position()
diff --git a/storage/innobase/row/row0sel.cc b/storage/innobase/row/row0sel.cc
@@ -2533,6 +2533,7 @@ row_sel_convert_mysql_key_to_innobase(
 				dfield_set_len(dfield, len
 					       - (ulint) (key_ptr - key_end));
 			}
+                        ut_ad(0);
 		}
 
 		n_fields++;

Original file line number	Diff line number	Diff line change
`@@ -2533,6 +2533,7 @@ row_sel_convert_mysql_key_to_innobase(`
`2533`	`2533`	`dfield_set_len(dfield, len`
`2534`	`2534`	`- (ulint) (key_ptr - key_end));`
`2535`	`2535`	`}`
	`2536`	`+ ut_ad(0);`
`2536`	`2537`	`}`
`2537`	`2538`
`2538`	`2539`	`n_fields++;`