Hi Vicențiu, The patch is ok to push.
On Sun, Mar 01, 2015 at 12:10:44AM +0200, Vicențiu Ciorbaru wrote: > 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: <[email protected]> > Date: 28 February 2015 at 23:58 > Subject: 45b6edb: MDEV-6838: Using too big key for internal temp tables > To: [email protected] > > > 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) -- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
