maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

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: <vicentiu@xxxxxxxxxxx>
> Date: 28 February 2015 at 23:58
> Subject: 45b6edb: MDEV-6838: Using too big key for internal temp tables
> To: commits@xxxxxxxxxxx
> 
> 
> 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