Bug#58912 InnoDB unnecessarily avoids update-in-place on column prefix indexes

row_upd_changes_ord_field_binary(): Do not return TRUE if the update
vector changes a column that is covered by a prefix index, but does
not change the column prefix. Add the row_ext_t parameter for
determining whether the prefixes of externally stored columns match.

dfield_datas_are_binary_equal(): Add the parameter len, for comparing
column prefixes when len > 0.

innodb.test: Add a test case where the patch of Bug #55284 failed
without this fix.

rb:537 approved by Jimmy Yang

mysql-test/suite/innodb_plugin/r/innodb.result

@@ -1,5 +1,8 @@
 drop table if exists t1,t2,t3,t4;
 drop database if exists mysqltest;
+CREATE TABLE bug58912 (a BLOB, b TEXT, PRIMARY KEY(a(1))) ENGINE=InnoDB;
+INSERT INTO bug58912 VALUES(REPEAT('a',8000),REPEAT('b',8000));
+UPDATE bug58912 SET a=REPEAT('a',7999);
 create table t1 (id int unsigned not null auto_increment, code tinyint unsigned not null, name char(20) not null, primary key (id), key (code), unique (name)) engine=innodb;
 insert into t1 (code, name) values (1, 'Tim'), (1, 'Monty'), (2, 'David'), (2, 'Erik'), (3, 'Sasha'), (3, 'Jeremy'), (4, 'Matt');
 select id, code, name from t1 order by id;
@@ -1676,10 +1679,10 @@ variable_value - @innodb_rows_deleted_orig
 71
 SELECT variable_value - @innodb_rows_inserted_orig FROM information_schema.global_status WHERE LOWER(variable_name) = 'innodb_rows_inserted';
 variable_value - @innodb_rows_inserted_orig
-1066
+1067
 SELECT variable_value - @innodb_rows_updated_orig FROM information_schema.global_status WHERE LOWER(variable_name) = 'innodb_rows_updated';
 variable_value - @innodb_rows_updated_orig
-865
+866
 SELECT variable_value - @innodb_row_lock_waits_orig FROM information_schema.global_status WHERE LOWER(variable_name) = 'innodb_row_lock_waits';
 variable_value - @innodb_row_lock_waits_orig
 0
@@ -3239,3 +3242,4 @@ Variable_name	Value
 Handler_update	1
 Variable_name	Value
 Handler_delete	1
+DROP TABLE bug58912;

mysql-test/suite/innodb_plugin/t/innodb.test

@@ -43,6 +43,15 @@ drop table if exists t1,t2,t3,t4;
 drop database if exists mysqltest;
 --enable_warnings
 
+# Bug#58912 InnoDB unnecessarily avoids update-in-place on column prefixes
+CREATE TABLE bug58912 (a BLOB, b TEXT, PRIMARY KEY(a(1))) ENGINE=InnoDB;
+INSERT INTO bug58912 VALUES(REPEAT('a',8000),REPEAT('b',8000));
+UPDATE bug58912 SET a=REPEAT('a',7999);
+# The above statements used to trigger a failure during purge when
+# Bug#55284 was fixed while Bug#58912 was not. Defer the DROP TABLE,
+# so that purge gets a chance to run (and a double free of the
+# off-page column can be detected, if one is to occur.)
+
 #
 # Small basic test with ignore
 #
@@ -2541,6 +2550,9 @@ SET GLOBAL innodb_thread_concurrency = @innodb_thread_concurrency_orig;
 
 -- enable_query_log
 
+# Clean up after the Bug#55284/Bug#58912 test case.
+DROP TABLE bug58912;
+
 #######################################################################
 #                                                                     #
 # Please, DO NOT TOUCH this file as well as the innodb.result file.   #

storage/innodb_plugin/ChangeLog

@@ -1,8 +1,18 @@
+2010-12-21	The InnoDB Team
+
+	* include/data0data.h, include/data0data.ic, include/row0upd.h,
+	btr/btr0cur.c, row/row0purge.c, row/row0umod.c, row/row0upd.c,
+	innodb.result, innodb.test:
+	Fix Bug#58912 InnoDB unnecessarily avoids update-in-place
+	on column prefix indexes
+
 2010-12-09	The InnoDB Team
+
 	* buf/buf0lru.c:
 	Fix Bug#57600 output of I/O sum[%lu] can go negative
 
 2010-11-11	The InnoDB Team
+
 	* thr/thr0loc.c, trx/trx0i_s.c:
 	Fix Bug#57802 Empty ASSERTION parameter passed to the HASH_SEARCH macro
 

storage/innodb_plugin/btr/btr0cur.c

@@ -1756,7 +1756,8 @@ btr_cur_update_in_place(
 		NOT call it if index is secondary */
 
 		if (!dict_index_is_clust(index)
-		    || row_upd_changes_ord_field_binary(NULL, index, update)) {
+		    || row_upd_changes_ord_field_binary(NULL, NULL,
+							index, update)) {
 
 			/* Remove possible hash index pointer to this record */
 			btr_search_update_hash_on_delete(cursor);

storage/innodb_plugin/include/data0data.h

@@ -154,14 +154,19 @@ dfield_dup(
 	dfield_t*	field,	/*!< in/out: data field */
 	mem_heap_t*	heap);	/*!< in: memory heap where allocated */
 /*********************************************************************//**
-Tests if data length and content is equal for two dfields.
-@return	TRUE if equal */
+Tests if two data fields are equal.
+If len==0, tests the data length and content for equality.
+If len>0, tests the first len bytes of the content for equality.
+@return	TRUE if both fields are NULL or if they are equal */
 UNIV_INLINE
 ibool
 dfield_datas_are_binary_equal(
 /*==========================*/
 	const dfield_t*	field1,	/*!< in: field */
-	const dfield_t*	field2);/*!< in: field */
+	const dfield_t*	field2,	/*!< in: field */
+	ulint		len)	/*!< in: maximum prefix to compare,
+				or 0 to compare the whole field length */
+	__attribute__((nonnull, warn_unused_result));
 /*********************************************************************//**
 Tests if dfield data length and content is equal to the given.
 @return	TRUE if equal */

storage/innodb_plugin/include/data0data.ic

@@ -229,20 +229,30 @@ dfield_dup(
 }
 
 /*********************************************************************//**
-Tests if data length and content is equal for two dfields.
-@return	TRUE if equal */
+Tests if two data fields are equal.
+If len==0, tests the data length and content for equality.
+If len>0, tests the first len bytes of the content for equality.
+@return	TRUE if both fields are NULL or if they are equal */
 UNIV_INLINE
 ibool
 dfield_datas_are_binary_equal(
 /*==========================*/
 	const dfield_t*	field1,	/*!< in: field */
-	const dfield_t*	field2)	/*!< in: field */
+	const dfield_t*	field2,	/*!< in: field */
+	ulint		len)	/*!< in: maximum prefix to compare,
+				or 0 to compare the whole field length */
 {
-	ulint	len;
+	ulint	len2 = len;
 
+	if (field1->len == UNIV_SQL_NULL || len == 0 || field1->len < len) {
 		len = field1->len;
+	}
 
-	return(len == field2->len
+	if (field2->len == UNIV_SQL_NULL || len2 == 0 || field2->len < len2) {
+		len2 = field2->len;
+	}
+
+	return(len == len2
 	       && (len == UNIV_SQL_NULL
 		   || !memcmp(field1->data, field2->data, len)));
 }

storage/innodb_plugin/include/row0upd.h

@@ -286,10 +286,13 @@ row_upd_changes_ord_field_binary(
 				row and the data values in update are not
 				known when this function is called, e.g., at
 				compile time */
+	const row_ext_t*ext,	/*!< NULL, or prefixes of the externally
+				stored columns in the old row */
 	dict_index_t*	index,	/*!< in: index of the record */
-	const upd_t*	update);/*!< in: update vector for the row; NOTE: the
+	const upd_t*	update)	/*!< in: update vector for the row; NOTE: the
 				field numbers in this MUST be clustered index
 				positions! */
+	__attribute__((nonnull(3,4), warn_unused_result));
 /***********************************************************//**
 Checks if an update vector changes an ordering field of an index record.
 This function is fast if the update vector is short or the number of ordering

storage/innodb_plugin/row/row0purge.c

@@ -413,7 +413,7 @@ row_purge_upd_exist_or_extern(
 	while (node->index != NULL) {
 		index = node->index;
 
-		if (row_upd_changes_ord_field_binary(NULL, node->index,
+		if (row_upd_changes_ord_field_binary(NULL, NULL, node->index,
 						     node->update)) {
 			/* Build the older version of the index entry */
 			entry = row_build_index_entry(node->row, NULL,

storage/innodb_plugin/row/row0umod.c

@@ -668,8 +668,8 @@ row_undo_mod_upd_exist_sec(
 	while (node->index != NULL) {
 		index = node->index;
 
-		if (row_upd_changes_ord_field_binary(node->row, node->index,
-						     node->update)) {
+		if (row_upd_changes_ord_field_binary(
+			    node->row, node->ext, node->index, node->update)) {
 
 			/* Build the newest version of the index entry */
 			entry = row_build_index_entry(node->row, node->ext,

storage/innodb_plugin/row/row0upd.c

@@ -1198,20 +1198,21 @@ row_upd_changes_ord_field_binary(
 				row and the data values in update are not
 				known when this function is called, e.g., at
 				compile time */
+	const row_ext_t*ext,	/*!< NULL, or prefixes of the externally
+				stored columns in the old row */
 	dict_index_t*	index,	/*!< in: index of the record */
 	const upd_t*	update)	/*!< in: update vector for the row; NOTE: the
 				field numbers in this MUST be clustered index
 				positions! */
 {
 	ulint			n_unique;
-	ulint		n_upd_fields;
-	ulint		i, j;
-	dict_index_t*	clust_index;
+	ulint			i;
+	const dict_index_t*	clust_index;
 
-	ut_ad(update && index);
+	ut_ad(update);
+	ut_ad(index);
 
 	n_unique = dict_index_get_n_unique(index);
-	n_upd_fields = upd_get_n_fields(update);
 
 	clust_index = dict_table_get_first_index(index->table);
 
@@ -1219,33 +1220,72 @@ row_upd_changes_ord_field_binary(
 
 		const dict_field_t*	ind_field;
 		const dict_col_t*	col;
-		ulint			col_pos;
 		ulint			col_no;
+		const upd_field_t*	upd_field;
+		const dfield_t*		dfield;
+		dfield_t		dfield_ext;
+		ulint			dfield_len;
+		const byte*		buf;
 
 		ind_field = dict_index_get_nth_field(index, i);
 		col = dict_field_get_col(ind_field);
-		col_pos = dict_col_get_clust_pos(col, clust_index);
 		col_no = dict_col_get_no(col);
 
-		for (j = 0; j < n_upd_fields; j++) {
+		upd_field = upd_get_field_by_field_no(
+			update, dict_col_get_clust_pos(col, clust_index));
 
-			const upd_field_t*	upd_field
-				= upd_get_nth_field(update, j);
-
-			/* Note that if the index field is a column prefix
-			then it may be that row does not contain an externally
-			stored part of the column value, and we cannot compare
-			the datas */
-
-			if (col_pos == upd_field->field_no
-			    && (row == NULL
-				|| ind_field->prefix_len > 0
-				|| !dfield_datas_are_binary_equal(
-					dtuple_get_nth_field(row, col_no),
-					&(upd_field->new_val)))) {
+		if (upd_field == NULL) {
+			continue;
+		}
 
+		if (row == NULL) {
+			ut_ad(ext == NULL);
 			return(TRUE);
 		}
+
+		dfield = dtuple_get_nth_field(row, col_no);
+
+		/* This treatment of column prefix indexes is loosely
+		based on row_build_index_entry(). */
+
+		if (UNIV_LIKELY(ind_field->prefix_len == 0)
+		    || dfield_is_null(dfield)) {
+			/* do nothing special */
+		} else if (UNIV_LIKELY_NULL(ext)) {
+			/* See if the column is stored externally. */
+			buf = row_ext_lookup(ext, col_no, &dfield_len);
+
+			ut_ad(col->ord_part);
+
+			if (UNIV_LIKELY_NULL(buf)) {
+				if (UNIV_UNLIKELY(buf == field_ref_zero)) {
+					/* This should never happen, but
+					we try to fail safe here. */
+					ut_ad(0);
+					return(TRUE);
+				}
+
+				goto copy_dfield;
+			}
+		} else if (dfield_is_ext(dfield)) {
+			dfield_len = dfield_get_len(dfield);
+			ut_a(dfield_len > BTR_EXTERN_FIELD_REF_SIZE);
+			dfield_len -= BTR_EXTERN_FIELD_REF_SIZE;
+			ut_a(dict_index_is_clust(index)
+			     || ind_field->prefix_len <= dfield_len);
+			buf = dfield_get_data(dfield);
+copy_dfield:
+			ut_a(dfield_len > 0);
+			dfield_copy(&dfield_ext, dfield);
+			dfield_set_data(&dfield_ext, buf, dfield_len);
+			dfield = &dfield_ext;
+		}
+
+		if (!dfield_datas_are_binary_equal(
+			    dfield, &upd_field->new_val,
+			    ind_field->prefix_len)) {
+
+			return(TRUE);
 		}
 	}
 
@@ -1329,7 +1369,7 @@ row_upd_changes_first_fields_binary(
 			if (col_pos == upd_field->field_no
 			    && !dfield_datas_are_binary_equal(
 				    dtuple_get_nth_field(entry, i),
-				    &(upd_field->new_val))) {
+				    &upd_field->new_val, 0)) {
 
 				return(TRUE);
 			}
@@ -1568,8 +1608,8 @@ row_upd_sec_step(
 	ut_ad(!dict_index_is_clust(node->index));
 
 	if (node->state == UPD_NODE_UPDATE_ALL_SEC
-	    || row_upd_changes_ord_field_binary(node->row, node->index,
-						node->update)) {
+	    || row_upd_changes_ord_field_binary(node->row, node->ext,
+						node->index, node->update)) {
 		return(row_upd_sec_index_entry(node, thr));
 	}
 
@@ -1973,7 +2013,8 @@ exit_func:
 
 	row_upd_store_row(node);
 
-	if (row_upd_changes_ord_field_binary(node->row, index, node->update)) {
+	if (row_upd_changes_ord_field_binary(node->row, node->ext, index,
+					     node->update)) {
 
 		/* Update causes an ordering field (ordering fields within
 		the B-tree) of the clustered index record to change: perform