Re: [Maria-developers] Please review MDEV-9653 Assertion `length || !scale' failed in uint my_decimal_length_to_precision(uint, uint, bool)
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 secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] Please review MDEV-9653 Assertion `length || !scale' failed in uint my_decimal_length_to_precision(uint, uint, bool)
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.100 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.100, which is wrong. > > I'll come up with a new patch soon. > > Thanks. > > >> >> Regards, >> Sergei >> Chief Architect MariaDB >> and secur...@mariadb.org >> diff --git a/mysql-test/r/func_hybrid_type.result b/mysql-test/r/func_hybrid_type.result index 95a8a82..eeaa79e 100644 --- a/mysql-test/r/func_hybrid_type.result +++ b/mysql-test/r/func_hybrid_type.result @@ -2175,10 +2175,10 @@ def case_a_b 12 19 19 Y 128 0 63 def case_b_a 12 19 19 Y 128 0 63 def coalesce_a_b 12 19 19 Y 128 0 63 def coalesce_b_a 12 19 19 Y 128 0 63 -def if___a_b 12 10 19 Y 128 0 63 -def if___b_a 12 10 19 Y 128 0 63 -def ifnull___a_b 12 10 19 Y 128 0 63 -def ifnull___b_a 12 10 19 Y 128 0 63 +def if___a_b 12 19 19 Y 128 0 63 +def if___b_a 12 19 19 Y 128 0 63 +def ifnull___a_b 12 19 19 Y 128 0 63 +def ifnull___b_a 12 19 19 Y 128 0 63 def leasta_b 12 10 19 Y 128 0 63 def leastb_a 12 10 19 Y 128 0 63 def greatest_a_b 12 10 19 Y 128 0 63 @@ -3396,5 +3396,36 @@ c1 DROP TABLE t2; DROP TABLE t1; # +# MDEV-9653 Assertion `length || !scale' failed in uint my_decimal_length_to_precision(uint, uint, bool) +# +SELECT CASE 0 WHEN 1 THEN (CASE 2 WHEN 3 THEN NULL END) WHEN 4 THEN 5 END; +CASE 0 WHEN 1 THEN (CASE 2 WHEN 3 THEN NULL END) WHEN 4 THEN 5 END +NULL +SELECT CASE 0 WHEN 1 THEN (COALESCE(NULL)) WHEN 4 THEN 5 END; +CASE 0 WHEN 1 THEN (COALESCE(NULL)) WHEN 4 THEN 5 END +NULL +SELECT CASE WHEN TRUE THEN COALESCE(NULL) ELSE 4 END; +CASE WHEN TRUE THEN COALESCE(NULL) ELSE 4 END +NULL +SELECT COALESCE(COALESCE(NULL), 1.1) AS c0, IF(0, COALESCE(NULL), 1.1) AS c1; +Catalog Database Table Table_alias Column Column_alias Type Length Max length Is_null Flags Decimals Charsetnr +def c0 246 4 3 Y 32896 1 63 +def c1 246 4 3 Y 32896 1 63 +c0 c1 +1.1 1.1 +# +# MDEV-9752 Wrong data type for COALEASCE(?,1) in prepared statements +# +PREPARE stmt FROM "CREATE TABLE t1 AS SELECT CONCAT(COALESCE(?,1)) AS a, CONCAT(CASE WHEN TRUE THEN ? ELSE 1 END) AS b"; +SET @a=1; +EXECUTE stmt USING @a,@a; +SHOW CREATE TABLE t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `a` varchar(21) DEFAULT NULL, + `b` varchar(21) DEFAULT NULL +) ENGINE=MyISAM DEFAULT CHARSET=latin1 +DROP TABLE t1; +# # End of 10.1 tests # diff --git a/mysql-test/t/func_hybrid_type.test
Re: [Maria-developers] Please review MDEV-9653 Assertion `length || !scale' failed in uint my_decimal_length_to_precision(uint, uint, bool)
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.100 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 secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] Please review MDEV-9653 Assertion `length || !scale' failed in uint my_decimal_length_to_precision(uint, uint, bool)
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 secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] Please review MDEV-9653 Assertion `length || !scale' failed in uint my_decimal_length_to_precision(uint, uint, bool)
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 secur...@mariadb.org
>
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] Please review MDEV-9653 Assertion `length || !scale' failed in uint my_decimal_length_to_precision(uint, uint, bool)
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.100, which is wrong. I'll come up with a new patch soon. Thanks. > > Regards, > Sergei > Chief Architect MariaDB > and secur...@mariadb.org > ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
[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!
diff --git a/mysql-test/r/func_hybrid_type.result b/mysql-test/r/func_hybrid_type.result
index 95a8a82..29f9f46 100644
--- a/mysql-test/r/func_hybrid_type.result
+++ b/mysql-test/r/func_hybrid_type.result
@@ -3396,5 +3396,17 @@ c1
DROP TABLE t2;
DROP TABLE t1;
#
+# MDEV-9653 Assertion `length || !scale' failed in uint my_decimal_length_to_precision(uint, uint, bool)
+#
+SELECT CASE 0 WHEN 1 THEN (CASE 2 WHEN 3 THEN NULL END) WHEN 4 THEN 5 END;
+CASE 0 WHEN 1 THEN (CASE 2 WHEN 3 THEN NULL END) WHEN 4 THEN 5 END
+NULL
+SELECT CASE 0 WHEN 1 THEN (COALESCE(NULL)) WHEN 4 THEN 5 END;
+CASE 0 WHEN 1 THEN (COALESCE(NULL)) WHEN 4 THEN 5 END
+NULL
+SELECT CASE WHEN TRUE THEN COALESCE(NULL) ELSE 4 END;
+CASE WHEN TRUE THEN COALESCE(NULL) ELSE 4 END
+NULL
+#
# End of 10.1 tests
#
diff --git a/mysql-test/t/func_hybrid_type.test b/mysql-test/t/func_hybrid_type.test
index 047e5f7..cc5d067 100644
--- a/mysql-test/t/func_hybrid_type.test
+++ b/mysql-test/t/func_hybrid_type.test
@@ -434,5 +434,13 @@ DROP TABLE t1;
--echo #
+--echo # MDEV-9653 Assertion `length || !scale' failed in uint my_decimal_length_to_precision(uint, uint, bool)
+--echo #
+SELECT CASE 0 WHEN 1 THEN (CASE 2 WHEN 3 THEN NULL END) WHEN 4 THEN 5 END;
+SELECT CASE 0 WHEN 1 THEN (COALESCE(NULL)) WHEN 4 THEN 5 END;
+SELECT CASE WHEN TRUE THEN COALESCE(NULL) ELSE 4 END;
+
+
+--echo #
--echo # End of 10.1 tests
--echo #
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).
+my_decimal_length_to_precision() does not expect this kind of input
+and would crash on DBUG_ASSERT. See MDEV-9653.
+So let's skip length calculation for explicit NULLs and
+for expressions derived from explicit NULLs.
+ */
+ if (arg->field_type() != MYSQL_TYPE_NULL)
+ {
+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);
+ }
unsigned_flag= unsigned_flag && arg->unsigned_flag;
}
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
