Hi Sergei and Sergey!
I've implemented the fix for MDEV-6838. I'd like a review from both of you
regarding the patch. There are a couple of changes made. The main issues
are outlined in the commit message.
I've changed the way create_key_part_by_field gets called in order to
decouple the KEY_PART_INFO creation process from any KEYINFO modifications
and also documented what the method does. My other concern is that
create_key_part_by_field does not seem to make any sense within the table
class as it's basically a constructor for KEY_PART_INFO and makes no use of
table fields. I suggest we move the code as either a constructor for
KEY_PART_INFO, or an init() function. I think that would be better left as
a separate patch, but the method itself is very easy to factor out in case
you agree with the change.
I'm looking forward for your input.
Regards,
Vicențiu
---------- Forwarded message ----------
From: <vicentiu(a)mariadb.org>
Date: 28 February 2015 at 23:58
Subject: 45b6edb: MDEV-6838: Using too big key for internal temp tables
To: commits(a)mariadb.org
revision-id: 45b6edb158f8101d641f550179ee15df363f686f
parent(s): fa87fc733d7855e0e5f9b959ca0bddf772ca57e5
committer: Vicențiu Ciorbaru
branch nick: server
timestamp: 2015-02-28 23:58:05 +0200
message:
MDEV-6838: Using too big key for internal temp tables
This bug manifests due to wrong computation and evaluation of
keyinfo->key_length. The issues were:
* Using table->file->max_key_length() as an absolute value that must not be
reached for a key, while it represents the maximum number of bytes
possible for a table key.
* Incorrectly computing the keyinfo->key_length size during
KEY_PART_INFO creation. The metadata information regarding the key
such the field length (for strings) was added twice.
---
mysql-test/r/table_keyinfo-6838.result | 12 ++++++++++++
mysql-test/t/table_keyinfo-6838.test | 18 ++++++++++++++++++
sql/sql_select.cc | 7 ++++---
sql/structs.h | 1 +
sql/table.cc | 34
++++++++++++++++++++++++++--------
sql/table.h | 2 +-
6 files changed, 62 insertions(+), 12 deletions(-)
diff --git a/mysql-test/r/table_keyinfo-6838.result
b/mysql-test/r/table_keyinfo-6838.result
new file mode 100644
index 0000000..55b0350
--- /dev/null
+++ b/mysql-test/r/table_keyinfo-6838.result
@@ -0,0 +1,12 @@
+CREATE TABLE t1 (i INT, state VARCHAR(997)) ENGINE=MyISAM;
+INSERT INTO t1 VALUES (2,'Louisiana'),(9,'Maine');
+CREATE TABLE t2 (state VARCHAR(997), j INT) ENGINE=MyISAM;
+INSERT INTO t2 VALUES ('Louisiana',9),('Alaska',5);
+INSERT INTO t2 SELECT t2.* FROM t2 JOIN t2 AS t3 JOIN t2 AS t4 JOIN t2 AS
t5 JOIN t2 AS t6;
+SET @@max_heap_table_size= 16384;
+set @@optimizer_switch='derived_merge=OFF';
+set @@optimizer_switch='extended_keys=ON';
+SELECT * FROM t1 AS t1_1 LEFT JOIN ( t1 AS t1_2 INNER JOIN (SELECT * FROM
t2) v2 ON t1_2.i = j ) ON t1_1.state = v2.state LIMIT 1;
+i state i state state j
+2 Louisiana 9 Maine Louisiana 9
+DROP TABLE t1, t2;
diff --git a/mysql-test/t/table_keyinfo-6838.test
b/mysql-test/t/table_keyinfo-6838.test
new file mode 100644
index 0000000..5f4c9f4
--- /dev/null
+++ b/mysql-test/t/table_keyinfo-6838.test
@@ -0,0 +1,18 @@
+# Test case for MDEV-6838.
+# Due to incorrect key_length computation, this usecase failed with the
error
+# Using too big key for internal temp table.
+
+CREATE TABLE t1 (i INT, state VARCHAR(997)) ENGINE=MyISAM;
+INSERT INTO t1 VALUES (2,'Louisiana'),(9,'Maine');
+
+CREATE TABLE t2 (state VARCHAR(997), j INT) ENGINE=MyISAM;
+INSERT INTO t2 VALUES ('Louisiana',9),('Alaska',5);
+INSERT INTO t2 SELECT t2.* FROM t2 JOIN t2 AS t3 JOIN t2 AS t4 JOIN t2 AS
t5 JOIN t2 AS t6;
+
+SET @@max_heap_table_size= 16384;
+set @@optimizer_switch='derived_merge=OFF';
+set @@optimizer_switch='extended_keys=ON';
+
+SELECT * FROM t1 AS t1_1 LEFT JOIN ( t1 AS t1_2 INNER JOIN (SELECT * FROM
t2) v2 ON t1_2.i = j ) ON t1_1.state = v2.state LIMIT 1;
+
+DROP TABLE t1, t2;
diff --git a/sql/sql_select.cc b/sql/sql_select.cc
index 387fdb3..dabe46e 100644
--- a/sql/sql_select.cc
+++ b/sql/sql_select.cc
@@ -7957,7 +7957,8 @@ static bool create_hj_key_for_table(JOIN *join,
JOIN_TAB *join_tab,
{
Field *field= table->field[keyuse->keypart];
uint fieldnr= keyuse->keypart+1;
- table->create_key_part_by_field(keyinfo, key_part_info, field,
fieldnr);
+ table->create_key_part_by_field(key_part_info, field, fieldnr);
+ keyinfo->key_length += key_part_info->store_length;
key_part_info++;
}
}
@@ -15921,7 +15922,7 @@ bool create_internal_tmp_table(TABLE *table, KEY
*keyinfo,
goto err;
bzero(seg, sizeof(*seg) * keyinfo->key_parts);
- if (keyinfo->key_length >= table->file->max_key_length() ||
+ if (keyinfo->key_length > table->file->max_key_length() ||
keyinfo->key_parts > table->file->max_key_parts() ||
share->uniques)
{
@@ -16107,7 +16108,7 @@ bool create_internal_tmp_table(TABLE *table, KEY
*keyinfo,
goto err;
bzero(seg, sizeof(*seg) * keyinfo->key_parts);
- if (keyinfo->key_length >= table->file->max_key_length() ||
+ if (keyinfo->key_length > table->file->max_key_length() ||
keyinfo->key_parts > table->file->max_key_parts() ||
share->uniques)
{
diff --git a/sql/structs.h b/sql/structs.h
index ae71819..9840cec 100644
--- a/sql/structs.h
+++ b/sql/structs.h
@@ -76,6 +76,7 @@ typedef struct st_key_part_info { /* Info about a key
part */
*/
uint16 store_length;
uint16 key_type;
+ /* Fieldnr begins counting from 1 */
uint16 fieldnr; /* Fieldnum in UNIREG */
uint16 key_part_flag; /* 0 or HA_REVERSE_SORT */
uint8 type;
diff --git a/sql/table.cc b/sql/table.cc
index cfa0b6c..7df2ae6 100644
--- a/sql/table.cc
+++ b/sql/table.cc
@@ -5968,18 +5968,32 @@ bool TABLE::alloc_keys(uint key_count)
}
-void TABLE::create_key_part_by_field(KEY *keyinfo,
- KEY_PART_INFO *key_part_info,
+/**
+ @brief
+ Populate a KEY_PART_INFO structure with the data related to a field
entry.
+
+ @param key_part_info The structure to fill.
+ @param field The field entry that represents the key part.
+ @param fleldnr The number of the field, count starting from 1.
+
+ TODO: This method does not make use of any table specific fields. It
+ could be refactored to act as a constructor for KEY_PART_INFO instead.
+*/
+
+void TABLE::create_key_part_by_field(KEY_PART_INFO *key_part_info,
Field *field, uint fieldnr)
-{
+{
key_part_info->null_bit= field->null_bit;
key_part_info->null_offset= (uint) (field->null_ptr -
(uchar*) record[0]);
key_part_info->field= field;
key_part_info->fieldnr= fieldnr;
key_part_info->offset= field->offset(record[0]);
- key_part_info->length= (uint16) field->pack_length();
- keyinfo->key_length+= key_part_info->length;
+ /*
+ field->key_length() accounts for the raw length of the field,
excluding
+ any metadata such as length of field or the NULL flag.
+ */
+ key_part_info->length= (uint16) field->key_length();
key_part_info->key_part_flag= 0;
/* TODO:
The below method of computing the key format length of the
@@ -5991,17 +6005,20 @@ void TABLE::create_key_part_by_field(KEY *keyinfo,
*/
key_part_info->store_length= key_part_info->length;
+ /*
+ The total store length of the key part is the raw length of the field
+
+ any metadata information, such as its length for strings and/or the
null
+ flag.
+ */
if (field->real_maybe_null())
{
key_part_info->store_length+= HA_KEY_NULL_LENGTH;
- keyinfo->key_length+= HA_KEY_NULL_LENGTH;
}
if (field->type() == MYSQL_TYPE_BLOB ||
field->type() == MYSQL_TYPE_GEOMETRY ||
field->real_type() == MYSQL_TYPE_VARCHAR)
{
key_part_info->store_length+= HA_KEY_BLOB_LENGTH;
- keyinfo->key_length+= HA_KEY_BLOB_LENGTH; // ???
key_part_info->key_part_flag|=
field->type() == MYSQL_TYPE_BLOB ? HA_BLOB_PART: HA_VAR_LENGTH_PART;
}
@@ -6124,7 +6141,8 @@ bool TABLE::add_tmp_key(uint key, uint key_parts,
if (key_start)
(*reg_field)->key_start.set_bit(key);
(*reg_field)->part_of_key.set_bit(key);
- create_key_part_by_field(keyinfo, key_part_info, *reg_field,
fld_idx+1);
+ create_key_part_by_field(key_part_info, *reg_field, fld_idx+1);
+ keyinfo->key_length += key_part_info->store_length;
(*reg_field)->flags|= PART_KEY_FLAG;
key_start= FALSE;
key_part_info++;
diff --git a/sql/table.h b/sql/table.h
index 7a1e380..8324454 100644
--- a/sql/table.h
+++ b/sql/table.h
@@ -1269,7 +1269,7 @@ struct TABLE
bool add_tmp_key(uint key, uint key_parts,
uint (*next_field_no) (uchar *), uchar *arg,
bool unique);
- void create_key_part_by_field(KEY *keyinfo, KEY_PART_INFO *key_part_info,
+ void create_key_part_by_field(KEY_PART_INFO *key_part_info,
Field *field, uint fieldnr);
void use_index(int key_to_save);
void set_table_map(table_map map_arg, uint tablenr_arg)