Re: bzr commit into mysql-5.1-bugteam branch (davi:2994) Bug#45261

[Sergei Golubchik]

Hi, Davi!

On Jul 04, Davi Arnaut wrote:
> # At a local mysql-5.1-bugteam repository of davi
> 
>  2994 Davi Arnaut	2009-07-04
>       Bug#45261: Crash, stored procedure + decimal
>       
>       The problem was that creating a DECIMAL column from decimal
>       value could lead to a assertion as decimal values can have
>       a higher precision than those attached to a table. The assert
>       could be triggered by creating a table from a decimal with
>       a large (> 30) scale.
>       
>       The solution is to ensure that truncation procedure is executed
>       when deducing a DECIMAL column from a decimal value of higher
>       precision. If the integer part is equal to or bigger than the
>       maximum precision for the DECIMAL type (65), the integer part

Didn't we agree that DECIMAL_MAX_PRECISION should be 73, not 65 ?
(81-8, not 81-8*2)

>       is truncated to fit and the fractional becomes zero. Otherwise,
>       the fractional part is truncated to fit into the space left
>       after the integer part is copied.
>       
>       This patch borrows code and ideas from Martin Hansson's patch.

> === modified file 'sql/sql_select.cc'
> *** a/sql/sql_select.cc	2009-06-26 19:57:42 +0000
> --- b/sql/sql_select.cc	2009-07-04 04:05:37 +0000
> ***************
> *** 9368,9407 ****
>       break;
>     case DECIMAL_RESULT:
>     {
> -     uint8 dec= item->decimals;
> -     uint8 intg= ((Item_decimal *) item)->decimal_precision() - dec;
> -     uint32 len= item->max_length;
> - 
>       /*
> !       Trying to put too many digits overall in a DECIMAL(prec,dec)
> !       will always throw a warning. We must limit dec to
> !       DECIMAL_MAX_SCALE however to prevent an assert() later.
> !     */
>   
> !     if (dec > 0)
> !     {
> !       signed int overflow;
>   
> !       dec= min(dec, DECIMAL_MAX_SCALE);
>   
> !       /*
> !         If the value still overflows the field with the corrected dec,
> !         we'll throw out decimals rather than integers. This is still
> !         bad and of course throws a truncation warning.
> !         +1: for decimal point
> !       */
>   
> !       overflow= my_decimal_precision_to_length(intg + dec, dec,
> !                                                item->unsigned_flag) - len;
>   
> !       if (overflow > 0)
> !         dec= max(0, dec - overflow);            // too long, discard fract
> !       else
> !         len -= item->decimals - dec;            // corrected value fits
>       }
>   
> !     new_field= new Field_new_decimal(len, maybe_null, item->name,
> !                                      dec, item->unsigned_flag);
>       break;
>     }
>     case ROW_RESULT:
> --- 9368,9446 ----
>       break;
>     case DECIMAL_RESULT:
>     {
>       /*
> !       The MySQL DECIMAL data type has a characteristic that needs to be
> !       taken into account when deducing the type from a Item_decimal.
>   
> !       But first, let's briefly recap what is the new MySQL DECIMAL type:
>   
> !       The declaration syntax for a decimal is DECIMAL(M,D), where:
>   
> !       * M is the maximum number of digits (the precision).
> !         It has a range of 1 to 65.
> !       * D is the number of digits to the right of the decimal separator (the
> scale).
> !         It has a range of 0 to 30 and must be no larger than M.
>   
> !       D and M are used to determine the storage requirements for the integer
> !       and fractional parts of each value. The integer part is to the left of
> !       the decimal separator and to the right is the fractional part. Hence:
>   
> !       M is the number of digits for the integer and fractional part.
> !       D is the number of digits for the fractional part.
> ! 
> !       Consequently, M - D is the number of digits for the integer part. For
> !       example, a DECIMAL(20,10) column has ten digits on either side of the
> !       decimal separator.
> ! 
> !       The characteristic that needs to be taken into account is that the
> !       backing type for Item_decimal is a my_decimal that has a higher precision
> !       (DECIMAL_MAX_POSSIBLE_PRECISION, see my_decimal.h) than DECIMAL.
> ! 
> !       Drawing a comparison between my_decimal and DECIMAL:
> ! 
> !       * M has a range of 1 to 81.
> !       * D has a range of 0 to 72.
> ! 
> !       The difference in range is that the fractional part must always be on
> !       on a group boundary, leaving one group for the integer part. Since each
> !       group is 9 (DIG_PER_DEC1) digits and there are 9 (DECIMAL_BUFF_LENGTH)
> !       groups, the fractional part is limited to 72 digits.

No, having no groups for tne integer part is ok, the decimal point still
be at the group boundary. So, D can be up to 81:

mysql> select
> .123456789012345678901234567890123456789012345678901234567890123456789012345678901;
+-------------------------------------------------------------------------------------+
| .123456789012345678901234567890123456789012345678901234567890123456789012345678901  |
+-------------------------------------------------------------------------------------+
| 0.123456789012345678901234567890123456789012345678901234567890123456789012345678901 | 
+-------------------------------------------------------------------------------------+
1 row in set (0.00 sec)

> !       Although the backing type for a DECIMAL is also my_decimal, every time
> !       a my_decimal is stored in a DECIMAL field, the precision and scale are
> !       explicitly capped at 65 (DECIMAL_MAX_PRECISION) and 30 (DECIMAL_MAX_SCALE)
> !       digits, following my_decimal truncation procedure (FIX_INTG_FRAC_ERROR).
> !     */
> ! 
> !     uint8 scale= item->decimals;
> !     uint8 precision= ((Item_decimal *) item)->decimal_precision();

why uint8 ?

> !     /*
> !       Employ a procedure along the lines of the my_decimal truncation process:
> !       - If the integer part is equal to or bigger than the maximum precision:
> !         Truncate integer part to fit and the fractional becomes zero.
> !       - Otherwise:
> !         Truncate fractional part to fit.
> !     */
> ! 
> !     uint intg= precision - scale;
> ! 
> !     if (intg >= DECIMAL_MAX_PRECISION)
> !     {
> !       intg= DECIMAL_MAX_PRECISION;
> !       scale= 0;
>       }
> +     else
> +     {
> +       uint room= min(DECIMAL_MAX_PRECISION - intg, DECIMAL_MAX_SCALE);
> +       if (scale > room)
> +         scale= room;
> +     }

where's a truncation warning/note generated ?

>   
> !     uint len= my_decimal_precision_to_length(intg + scale, scale,
> !                                              item->unsigned_flag);
> !     new_field= new Field_new_decimal(len, maybe_null, item->name, scale,
> !                                      item->unsigned_flag);
>       break;
>     }
>     case ROW_RESULT:

Regards / Mit vielen Grüßen,
Sergei

-- 
   __  ___     ___ ____  __
  /  |/  /_ __/ __/ __ \/ /   Sergei Golubchik <serg@stripped>
 / /|_/ / // /\ \/ /_/ / /__  Principal Software Engineer/Server Architect
/_/  /_/\_, /___/\___\_\___/  Sun Microsystems GmbH, HRB München 161028
       <___/                  Sonnenallee 1, 85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schroeder, Wolfgang Engels, Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin Häring