[Maria-developers] Please review MDEV-9653 Assertion `length || !scale' failed in uint my_decimal_length_to_precision(uint, uint, bool)
Hello Sergei, Please review a patch for: MDEV-9653 Assertion `length || !scale' failed in uint my_decimal_length_to_precision(uint, uint, bool) Thanks!
Hi, Alexander! On Mar 16, Alexander Barkov wrote:
Hello Sergei,
Please review a patch for:
MDEV-9653 Assertion `length || !scale' failed in uint my_decimal_length_to_precision(uint, uint, bool)
Thanks!
diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc index 2783a05..73a47ac 100644 --- a/sql/item_cmpfunc.cc +++ b/sql/item_cmpfunc.cc @@ -3086,10 +3086,28 @@ void Item_func_case::agg_str_lengths(Item* arg)
void Item_func_case::agg_num_lengths(Item *arg) { - uint len= my_decimal_length_to_precision(arg->max_length, arg->decimals, - arg->unsigned_flag) - arg->decimals; - set_if_bigger(max_length, len); - set_if_bigger(decimals, arg->decimals); + /* + In a query like this: + SELECT CASE WHEN TRUE + THEN COALESCE(CAST(NULL AS UNSIGNED)) + ELSE 40 + END; + "arg" corresponding to Item_func_coalesce has: + arg->max_length==0 + arg->decimals==31 (NOT_FIXED_DEC).
why is it NOT_FIXED_DEC? it seems to me that it has to be 0 here. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi Sergei, On 03/17/2016 01:16 AM, Sergei Golubchik wrote:
Hi, Alexander!
On Mar 16, Alexander Barkov wrote:
Hello Sergei,
Please review a patch for:
MDEV-9653 Assertion `length || !scale' failed in uint my_decimal_length_to_precision(uint, uint, bool)
Thanks!
diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc index 2783a05..73a47ac 100644 --- a/sql/item_cmpfunc.cc +++ b/sql/item_cmpfunc.cc @@ -3086,10 +3086,28 @@ void Item_func_case::agg_str_lengths(Item* arg)
void Item_func_case::agg_num_lengths(Item *arg) { - uint len= my_decimal_length_to_precision(arg->max_length, arg->decimals, - arg->unsigned_flag) - arg->decimals; - set_if_bigger(max_length, len); - set_if_bigger(decimals, arg->decimals); + /* + In a query like this: + SELECT CASE WHEN TRUE + THEN COALESCE(CAST(NULL AS UNSIGNED)) + ELSE 40 + END; + "arg" corresponding to Item_func_coalesce has: + arg->max_length==0 + arg->decimals==31 (NOT_FIXED_DEC).
why is it NOT_FIXED_DEC? it seems to me that it has to be 0 here.
Perhaps my example in the comments made you think that it happens in a very special case with CAST AS UNSIGNED. It also happens in simpler cases: SELECT CASE WHEN TRUE THEN COALESCE(NULL) ELSE 40 END; I guess I should fix the example to this ^^^. Explicit NULL has STRING_RESULT as cmp_type() and result_type(). So it behaves like strings. It's all around the code. For COALESCE, IF, IFULL, CASE with explicit NULL input decimals is set to NOT_FIXED_DEC in here: bool Item_func::count_string_result_length(enum_field_types field_type_arg, Item **items, uint nitems) { if (agg_arg_charsets_for_string_result(collation, items, nitems, 1)) return true; if (is_temporal_type(field_type_arg)) count_datetime_length(items, nitems); else { decimals= NOT_FIXED_DEC; count_only_length(items, nitems); } return false; } But the problem is that it happens not only here. It's also done that way in all string functions: For example: MariaDB [test]> SELECT LEFT(null,1); Field 1: `LEFT(null,1)` Type: VAR_STRING Collation: binary (63) Length: 0 Max_length: 0 Decimals: 31 -- NOT_FIXED_DEC Flags: BINARY MariaDB [test]> SELECT CONCAT(null); Field 1: `CONCAT(null)` Type: VAR_STRING Collation: binary (63) Length: 0 Max_length: 0 Decimals: 31 -- NOT_FIXED_DEC Flags: BINARY It has always been that way, for years. I guess other non-function Item types do the same for an explicit NULL input. Fixing all Items to have decimals==0 in case of explicit NULL input looks too dangerous in 10.1. (btw, in 10.2 it should be easier). So in 10,1 I decided to fix Item_func_case::agg_num_lengths.
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Alexander! On Mar 17, Alexander Barkov wrote:
why is it NOT_FIXED_DEC? it seems to me that it has to be 0 here.
Perhaps my example in the comments made you think that it happens in a very special case with CAST AS UNSIGNED. It also happens in simpler cases:
SELECT CASE WHEN TRUE THEN COALESCE(NULL) ELSE 40 END;
I guess I should fix the example to this ^^^.
Yes, please. More questions: 1. Perhaps my_decimal_length_to_precision() then? It's used not only in Item_func_case::agg_num_lengths. Quick grepping finds more potentially dangerous places. For example Item_decimal_typecast::print. 2. Why my_decimal_length_to_precision is only used in Item_func_case? How do other functions aggregate types? Perhaps CASE needs to be changed to work like other similar functions do? Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi Sergei, On 03/17/2016 01:14 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Mar 17, Alexander Barkov wrote:
why is it NOT_FIXED_DEC? it seems to me that it has to be 0 here.
Perhaps my example in the comments made you think that it happens in a very special case with CAST AS UNSIGNED. It also happens in simpler cases:
SELECT CASE WHEN TRUE THEN COALESCE(NULL) ELSE 40 END;
I guess I should fix the example to this ^^^.
Yes, please.
More questions:
1. Perhaps my_decimal_length_to_precision() then? It's used not only in Item_func_case::agg_num_lengths. Quick grepping finds more potentially dangerous places. For example Item_decimal_typecast::print.
I could not find places where my_decimal_length_to_precision() can be called with (0,NOT_FIXED_DEC). Item_decimal_typecast::print() is safe. Item_decimal_typecast::max_length and Item_decimal_typecast::decimals are set to valid values in constructor. The latter cannot be NOT_FIXED_DEC.
2. Why my_decimal_length_to_precision is only used in Item_func_case? How do other functions aggregate types? Perhaps CASE needs to be changed to work like other similar functions do?
There is some code duplication in hybrid type functions: - Item_func_case::agg_num_lengths() - Item_func::count_decimal_length(), which is called from Item_func_coalesce::fix_length_and_dec() - Item_func_case_abbreviation2::fix_length_and_dec() I earlier created a task for this for 10.2: https://jira.mariadb.org/browse/MDEV-9406 I found new bugs when replying to this letter: SELECT COALESCE(COALESCE(NULL), 1.1); SELECT IF(0, COALESCE(NULL), 1.1); return 1.1000000000000000000000000000000, which is wrong. I'll come up with a new patch soon. Thanks.
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi Sergei, It appeared to be very easy to fix all hybrid functions to use the same attribute aggregation code, and therefore get rid of duplicate implementations for Item_func_case_abbreviation2 and Item_func_case. This patch fixes: - The original problem reported in MDEV-9653 - An additional problem, see new comments about COALESCE and IF returning 1.1000000000000000000000000000000 instead of just 1.1 - A new problem that I found: MDEV-9752 Wrong data type for COALEASCE(?,1) in prepared statements Note, the patch does not cover LEAST/GREATEST. They have another version of their own aggregation code. I don't want to touch them in this fix. Let's do them separately. Note, I also removed Item_func_case::agg_str_lengths(). It was a dead code. Thanks. On 03/17/2016 03:06 PM, Alexander Barkov wrote:
Hi Sergei,
On 03/17/2016 01:14 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Mar 17, Alexander Barkov wrote:
why is it NOT_FIXED_DEC? it seems to me that it has to be 0 here.
Perhaps my example in the comments made you think that it happens in a very special case with CAST AS UNSIGNED. It also happens in simpler cases:
SELECT CASE WHEN TRUE THEN COALESCE(NULL) ELSE 40 END;
I guess I should fix the example to this ^^^.
Yes, please.
More questions:
1. Perhaps my_decimal_length_to_precision() then? It's used not only in Item_func_case::agg_num_lengths. Quick grepping finds more potentially dangerous places. For example Item_decimal_typecast::print.
I could not find places where my_decimal_length_to_precision() can be called with (0,NOT_FIXED_DEC).
Item_decimal_typecast::print() is safe. Item_decimal_typecast::max_length and Item_decimal_typecast::decimals are set to valid values in constructor. The latter cannot be NOT_FIXED_DEC.
2. Why my_decimal_length_to_precision is only used in Item_func_case? How do other functions aggregate types? Perhaps CASE needs to be changed to work like other similar functions do?
There is some code duplication in hybrid type functions:
- Item_func_case::agg_num_lengths()
- Item_func::count_decimal_length(), which is called from Item_func_coalesce::fix_length_and_dec()
- Item_func_case_abbreviation2::fix_length_and_dec()
I earlier created a task for this for 10.2: https://jira.mariadb.org/browse/MDEV-9406
I found new bugs when replying to this letter:
SELECT COALESCE(COALESCE(NULL), 1.1); SELECT IF(0, COALESCE(NULL), 1.1);
return 1.1000000000000000000000000000000, which is wrong.
I'll come up with a new patch soon.
Thanks.
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Alexander! Looks very good! Just one comment - instead of a special treatment for MYSQL_TYPE_NULL, I would try to set decimal=0 in Item_null. It already sets max_length=0, and I cannot think of any reason why we might want decimal to be 31 for it. On Mar 17, Alexander Barkov wrote:
Hi Sergei,
It appeared to be very easy to fix all hybrid functions to use the same attribute aggregation code, and therefore get rid of duplicate implementations for Item_func_case_abbreviation2 and Item_func_case.
This patch fixes: - The original problem reported in MDEV-9653
- An additional problem, see new comments about COALESCE and IF returning 1.1000000000000000000000000000000 instead of just 1.1
- A new problem that I found: MDEV-9752 Wrong data type for COALEASCE(?,1) in prepared statements
Note, the patch does not cover LEAST/GREATEST. They have another version of their own aggregation code. I don't want to touch them in this fix. Let's do them separately.
Note, I also removed Item_func_case::agg_str_lengths(). It was a dead code.
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
participants (2)
-
Alexander Barkov -
Sergei Golubchik