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
