List:Commits« Previous MessageNext Message »
From:Øystein Grøvlen Date:May 26 2011 10:45am
Subject:Re: bzr commit into mysql-trunk branch (epotemkin:3004) Bug#11825482
View as plain text  
Hi Evgeny,

Code changes looks good (see inline for minor comments), but the test 
and result files does not quite match.  Also, it seems you need to use 
INNODB tables in order to reproduce the bug.

On 25/05/2011 13:05, Evgeny Potemkin wrote:
> #At file:///work/bzrroot/11825482-bug/ based on
> revid:epotemkin@stripped
>
>   3004 Evgeny Potemkin	2011-05-25
>        Bug#11825482: Broken key length calculation for btree index.
>        Materializable views/derived tables employs HEAP engine for result small
>        enough to fit into memory. For doing btree index lookups heap engine
>        converts key provided by mysql server into internal format. When a null value
>        were given for a variable length field converter wasn't taking into account
>        length part thus creating a wrong internal key. This led to a wrong result.
>        Also fixed a 'failed assertion' bug that manifests itself while explaining
>        complex queries.
>
>        Now hp_rb_pack_key function takes int account length part of key when
> skipping
>        null values for a variable length fields.
>        Item_sum_num::fix_fields now checks 'fixed' flag prior to calling fix_fields
>        function.
>       @ mysql-test/r/heap.result
>          Added a test case for the bug#11825482.
>       @ mysql-test/t/heap.test
>          Added a test case for the bug#11825482.
>          Minor improvement for Vim's syntax highligh.
>       @ sql/item_sum.cc
>          Bug#11825482: Broken key length calculation for btree index.
>          Item_sum_num::fix_fields now checks 'fixed' flag prior to calling
> fix_fields
>          function.
>       @ storage/heap/hp_hash.c
>          Bug#11825482: Broken key length calculation for btree index.
>          Now hp_rb_pack_key function takes int account length part of key when
> skipping
>          null values for a variable length fields.
>
...


> === modified file 'sql/item_sum.cc'
> --- a/sql/item_sum.cc	2011-02-02 09:21:41 +0000
> +++ b/sql/item_sum.cc	2011-05-25 11:05:49 +0000
> @@ -1108,7 +1108,8 @@ Item_sum_num::fix_fields(THD *thd, Item
>     maybe_null=0;
>     for (uint i=0 ; i<  arg_count ; i++)
>     {
> -    if (args[i]->fix_fields(thd, args + i) || args[i]->check_cols(1))
> +    if ((!args[i]->fixed&&  args[i]->fix_fields(thd, args + i)) ||
> +        args[i]->check_cols(1))
>         return TRUE;

I wonder whether this would be easier to read if split into two 
if-statements now.

>       set_if_bigger(decimals, args[i]->decimals);
>       maybe_null |= args[i]->maybe_null;
>
> === modified file 'storage/heap/hp_hash.c'
> --- a/storage/heap/hp_hash.c	2010-07-27 10:16:49 +0000
> +++ b/storage/heap/hp_hash.c	2011-05-25 11:05:49 +0000
> @@ -806,8 +806,17 @@ uint hp_rb_pack_key(HP_KEYDEF *keydef, u
>       if (seg->null_bit)
>       {
>         if (!(*key++= (char) 1 - *old++))

Not part of your fix, but if you understand what the above is 
doing/testing, it would be nice if you could add a comment.  It looks 
kind of cryptic to me.

--
Øystein

> +      {
> +        /*
> +          Skip length part of a variable length field.
> +          Length of key-part used with heap_rkey() always 2.
> +          See also hp_hashnr().
> +        */
> +        if (seg->flag&  (HA_VAR_LENGTH_PART | HA_BLOB_PART))
> +          old+= 2;
>           continue;
>         }
> +    }
>       if (seg->flag&  HA_SWAP_KEY)
>       {
>         uint length= seg->length;
>
>
>
>
>


-- 
Øystein Grøvlen, Principal Software Engineer
MySQL Group, Oracle
Trondheim, Norway
Thread
bzr commit into mysql-trunk branch (epotemkin:3004) Bug#11825482Evgeny Potemkin25 May
  • Re: bzr commit into mysql-trunk branch (epotemkin:3004) Bug#11825482Øystein Grøvlen26 May
  • Re: bzr commit into mysql-trunk branch (epotemkin:3004) Bug#11825482Roy Lyseng26 May