#At file:///home/marko/innobase/dev/mysql2a/5.1-innodb/ based on revid:vasil.dimov@strippedfmkgzcz2725
3669 Marko Mäkelä 2010-12-21
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
modified:
mysql-test/suite/innodb_plugin/r/innodb.result
mysql-test/suite/innodb_plugin/t/innodb.test
storage/innodb_plugin/ChangeLog
storage/innodb_plugin/btr/btr0cur.c
storage/innodb_plugin/include/data0data.h
storage/innodb_plugin/include/data0data.ic
storage/innodb_plugin/include/row0upd.h
storage/innodb_plugin/row/row0purge.c
storage/innodb_plugin/row/row0umod.c
storage/innodb_plugin/row/row0upd.c
=== modified file 'mysql-test/suite/innodb_plugin/r/innodb.result'
--- a/mysql-test/suite/innodb_plugin/r/innodb.result revid:vasil.dimov@strippedom-20101214093819-wb2offmkgzcz2725
+++ b/mysql-test/suite/innodb_plugin/r/innodb.result revid:marko.makela@oracle.com-20101221093919-mcmmgd4zpse9567d
@@ -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_or
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;
=== modified file 'mysql-test/suite/innodb_plugin/t/innodb.test'
--- a/mysql-test/suite/innodb_plugin/t/innodb.test revid:vasil.dimov@strippedoffmkgzcz2725
+++ b/mysql-test/suite/innodb_plugin/t/innodb.test revid:marko.makela@strippedgd4zpse9567d
@@ -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 = @
-- 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. #
=== modified file 'storage/innodb_plugin/ChangeLog'
--- a/storage/innodb_plugin/ChangeLog revid:vasil.dimov@strippedwb2offmkgzcz2725
+++ b/storage/innodb_plugin/ChangeLog revid:marko.makela@stripped7d
@@ -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
=== modified file 'storage/innodb_plugin/btr/btr0cur.c'
--- a/storage/innodb_plugin/btr/btr0cur.c revid:vasil.dimov@stripped3819-wb2offmkgzcz2725
+++ b/storage/innodb_plugin/btr/btr0cur.c revid:marko.makela@strippedd4zpse9567d
@@ -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);
=== modified file 'storage/innodb_plugin/include/data0data.h'
--- a/storage/innodb_plugin/include/data0data.h revid:vasil.dimov@stripped
+++ b/storage/innodb_plugin/include/data0data.h revid:marko.makela@stripped
@@ -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 */
=== modified file 'storage/innodb_plugin/include/data0data.ic'
--- a/storage/innodb_plugin/include/data0data.ic revid:vasil.dimov@stripped3819-wb2offmkgzcz2725
+++ b/storage/innodb_plugin/include/data0data.ic revid:marko.makela@stripped9-mcmmgd4zpse9567d
@@ -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;
- len = field1->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)));
}
=== modified file 'storage/innodb_plugin/include/row0upd.h'
--- a/storage/innodb_plugin/include/row0upd.h revid:vasil.dimov@strippedoffmkgzcz2725
+++ b/storage/innodb_plugin/include/row0upd.h revid:marko.makela@strippedse9567d
@@ -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
=== modified file 'storage/innodb_plugin/row/row0purge.c'
--- a/storage/innodb_plugin/row/row0purge.c revid:vasil.dimov@strippedoffmkgzcz2725
+++ b/storage/innodb_plugin/row/row0purge.c revid:marko.makela@stripped9567d
@@ -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,
=== modified file 'storage/innodb_plugin/row/row0umod.c'
--- a/storage/innodb_plugin/row/row0umod.c revid:vasil.dimov@stripped819-wb2offmkgzcz2725
+++ b/storage/innodb_plugin/row/row0umod.c revid:marko.makela@strippedd4zpse9567d
@@ -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,
=== modified file 'storage/innodb_plugin/row/row0upd.c'
--- a/storage/innodb_plugin/row/row0upd.c revid:vasil.dimov@strippedoffmkgzcz2725
+++ b/storage/innodb_plugin/row/row0upd.c revid:marko.makela@stripped67d
@@ -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 n_unique;
+ 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);
+ if (upd_field == NULL) {
+ continue;
+ }
- /* 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 (row == NULL) {
+ ut_ad(ext == NULL);
+ return(TRUE);
+ }
- 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)))) {
+ dfield = dtuple_get_nth_field(row, col_no);
- return(TRUE);
+ /* 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
Attachment: [text/bzr-bundle] bzr/marko.makela@oracle.com-20101221093919-mcmmgd4zpse9567d.bundle
| Thread |
|---|
| • bzr commit into mysql-5.1-innodb branch (marko.makela:3669) Bug#58912 | marko.makela | 21 Dec |
