List:Internals« Previous MessageNext Message »
From:Davi Arnaut Date:November 2 2009 12:17pm
Subject:Re: DECIMAL broken in information_schema by patch for Bug#45261
View as plain text  
Hi Kristian,

Sorry for the delay on responding, I was on vacation for most of Oct.

On 10/15/09 6:34 PM, Kristian Nielsen wrote:
> I found a problem introduced in MySQL 5.1.39 by the patch for Bug#45261.
>
> The bzr revision is this:
>
>      revid:davi.arnaut@stripped
>
> To see the problem, apply a patch like this:
>
> === modified file 'sql/sql_profile.cc'
> --- sql/sql_profile.cc	2009-07-16 12:43:17 +0000
> +++ sql/sql_profile.cc	2009-10-15 21:19:44 +0000
> @@ -34,7 +34,7 @@
>
>   #define TIME_FLOAT_DIGITS 9
>   /** two vals encoded: (dec*100)+len */
> -#define TIME_I_S_DECIMAL_SIZE (TIME_FLOAT_DIGITS*100)+(TIME_FLOAT_DIGITS-3)
> +#define TIME_I_S_DECIMAL_SIZE (20*100)+(TIME_FLOAT_DIGITS-3)
>
>   #define MAX_QUERY_LENGTH 300
>
>
> Then run the query SHOW CREATE TABLE information_schema.PROFILING:
>
>    `DURATION` decimal(9,6) NOT NULL DEFAULT '0.000000',
>
> In fact, no matter what precision is specified in sql_profile.cc for the
> table, the type is always decimal(9,6)!
>
> Specifically, the problem is with this part of the patch:
>
> === modified file 'sql/item.cc'
> --- sql/item.cc	2009-07-19 12:49:40 +0000
> +++ sql/item.cc	2009-08-24 19:47:08 +0000
> @@ -4902,9 +4911,7 @@ Field *Item::tmp_table_field_from_field_type
>     switch (field_type()) {
>     case MYSQL_TYPE_DECIMAL:
>     case MYSQL_TYPE_NEWDECIMAL:
> -    field= new Field_new_decimal((uchar*) 0, max_length, null_ptr, 0,
> -                                 Field::NONE, name, decimals, 0,
> -                                 unsigned_flag);
> +    field= Field_new_decimal::new_decimal_field(this);
>       break;
>     case MYSQL_TYPE_TINY:
>       field= new Field_tiny((uchar*) 0, max_length, null_ptr, 0, Field::NONE,
>
> This creates a Field from an Item, and is used when getting the Field
> describing a DECIMAL column of an information_schema table.
>
> Note that the old code uses item->max_length to set the precision, so the
> precision will be set from the _type_ (metadata) of the Item.

Kind of. max_length is not really supposed to be type metadata as can 
been seen in the constructor and other methods of Item_decimal. In spite 
of this, this thing is quite messy as max_length can be used to "stuff" 
things that are correlated but depend on other components of the item. 
For example, the integer part and decimal part are represented in the 
length, but if one of those is updated via another source (eg 
Item::decimals), its not possible to properly interpret max_length.

I think that the real problem lies in create_schema_table which violates 
any sense of interface and updates the item attributes by hand. This 
used to work as things were broken in conjunction.

> But the new code uses the new method Field_new_decimal::new_decimal_field()
> from the patch. This method sets the precision of the Field from the _value_
> of the item.

As is expected and somewhat documented.

> This causes the bug. In create_schema_table(), we call create_tmp_table() with
> a list of items. This call passes the items one by one to
> create_tmp_field_for_schema(), which just returns
> item->tmp_table_field_from_field_type(). With the above change, the field type
> becomes set from the value of the item not the type, causing the wrong
> behaviour.
>
> Any idea how to fix this regression?
>

For the time being, I plan to revert Bug#45261 as it also caused other 
regressions for things that are not exactly right either. Better to work 
on a extended patch for a later milestone.

Thanks and regards,

-- Davi Arnaut
Thread
DECIMAL broken in information_schema by patch for Bug#45261Kristian Nielsen15 Oct
  • Re: DECIMAL broken in information_schema by patch for Bug#45261Davi Arnaut2 Nov
    • Re: DECIMAL broken in information_schema by patch for Bug#45261Kristian Nielsen2 Nov