Re: [Maria-developers] d1ec0231553: MDEV-26732 Assertion `0' failed in Item::val_native
Hi, Alexander! On Oct 25, Alexander Barkov wrote:
revision-id: d1ec0231553 (mariadb-10.5.4-550-gd1ec0231553) parent(s): 8b115503563 author: Alexander Barkov committer: Alexander Barkov timestamp: 2021-10-07 20:58:18 +0400 message:
MDEV-26732 Assertion `0' failed in Item::val_native
Also fixes MDEV-24619 Wrong result or Assertion `0' in Item::val_native / Type_handler_inet6::Item_val_native_with_conversion
would be good to have some explanation here, what was wrong, what was the fix.
diff --git a/plugin/type_inet/mysql-test/type_inet/type_inet6.result b/plugin/type_inet/mysql-test/type_inet/type_inet6.result index da949481337..ac16f5c06ce 100644 --- a/plugin/type_inet/mysql-test/type_inet/type_inet6.result +++ b/plugin/type_inet/mysql-test/type_inet/type_inet6.result @@ -2159,3 +2159,38 @@ IFNULL(c, '::1') ::1 DROP TABLE t2; DROP TABLE t1; +# +# MDEV-26732 Assertion `0' failed in Item::val_native +# +SELECT CAST(CONCAT('::', REPEAT('',RAND())) AS INET6) AS f, var_pop('x') FROM dual HAVING f > '';
do you need RAND() here for a test case? can you replace it with a literal? do you need var_pop() here?
+f var_pop('x') +Warnings: +Warning 1292 Truncated incorrect DOUBLE value: 'x' +Warning 1292 Incorrect inet6 value: '' +SELECT CAST(CONCAT('::', REPEAT('',RAND())) AS INET6) AS f, var_pop(1) FROM dual HAVING f >= '::'; +f var_pop(1) +:: 0.0000 +CREATE TABLE t1(id INET6 NOT NULL PRIMARY KEY, dsc INET6); +INSERT INTO t1 VALUES ('::1', '1::1'),('::3', '1::3'),('::4', NULL); +CREATE TABLE t2 SELECT COALESCE(t1.dsc), COUNT(*) FROM t1 GROUP BY t1.id; +SELECT * FROM t2 ORDER BY 1,2; +COALESCE(t1.dsc) COUNT(*) +NULL 1 +1::1 1 +1::3 1 +DROP TABLE t1, t2; +# +# MDEV-24619 Wrong result or Assertion `0' in Item::val_native / Type_handler_inet6::Item_val_native_with_conversion +# +CREATE TABLE t1 (a INET6); +INSERT INTO t1 VALUES ('::'),('::'); +SELECT IF(1, '::', a) AS f FROM t1 GROUP BY 'foo' HAVING f != ''; +f +Warnings: +Warning 1292 Incorrect inet6 value: '' +SELECT IF(1, '::', a) AS f FROM t1 GROUP BY 'foo' HAVING f != '::'; +f +SELECT IF(1, '::', a) AS f FROM t1 GROUP BY 'foo' HAVING f != '::1'; +f +:: +DROP TABLE t1;
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hello Sergei, On 10/25/21 7:28 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Oct 25, Alexander Barkov wrote:
revision-id: d1ec0231553 (mariadb-10.5.4-550-gd1ec0231553) parent(s): 8b115503563 author: Alexander Barkov committer: Alexander Barkov timestamp: 2021-10-07 20:58:18 +0400 message:
MDEV-26732 Assertion `0' failed in Item::val_native
Also fixes MDEV-24619 Wrong result or Assertion `0' in Item::val_native / Type_handler_inet6::Item_val_native_with_conversion
would be good to have some explanation here, what was wrong, what was the fix.
Right, I suggest this comment: MDEV-26732 Assertion `0' failed in Item::val_native Also fixes MDEV-24619 Wrong result or Assertion `0' in Item::val_native / Type_handler_inet6::Item_val_native_with_conversion Type_handler::create_item_copy() created a generic Item_copy_string, which does not implement val_native() - it has a dummy implementation with DBUG_ASSERT(0), which made the server crash. Fix: - Adding a new class Item_copy_inet6, which implements val_native(). - Fixing Type_handler::create_item_copy() to make Item_copy_inet6 instead of Item_copy_string.
diff --git a/plugin/type_inet/mysql-test/type_inet/type_inet6.result b/plugin/type_inet/mysql-test/type_inet/type_inet6.result index da949481337..ac16f5c06ce 100644 --- a/plugin/type_inet/mysql-test/type_inet/type_inet6.result +++ b/plugin/type_inet/mysql-test/type_inet/type_inet6.result @@ -2159,3 +2159,38 @@ IFNULL(c, '::1') ::1 DROP TABLE t2; DROP TABLE t1; +# +# MDEV-26732 Assertion `0' failed in Item::val_native +# +SELECT CAST(CONCAT('::', REPEAT('',RAND())) AS INET6) AS f, var_pop('x') FROM dual HAVING f > '';
do you need RAND() here for a test case? can you replace it with a literal?
do you need var_pop() here?
I need both: - RAND(), to avoid treating the expressions as a constant - var_pop(), or any other aggregate functions. With count() instead of var_pop() it also crashes. If I remove any of these two, it does not crash - the execution goes through different paths.
+f var_pop('x') +Warnings: +Warning 1292 Truncated incorrect DOUBLE value: 'x' +Warning 1292 Incorrect inet6 value: '' +SELECT CAST(CONCAT('::', REPEAT('',RAND())) AS INET6) AS f, var_pop(1) FROM dual HAVING f >= '::'; +f var_pop(1) +:: 0.0000 +CREATE TABLE t1(id INET6 NOT NULL PRIMARY KEY, dsc INET6); +INSERT INTO t1 VALUES ('::1', '1::1'),('::3', '1::3'),('::4', NULL); +CREATE TABLE t2 SELECT COALESCE(t1.dsc), COUNT(*) FROM t1 GROUP BY t1.id; +SELECT * FROM t2 ORDER BY 1,2; +COALESCE(t1.dsc) COUNT(*) +NULL 1 +1::1 1 +1::3 1 +DROP TABLE t1, t2; +# +# MDEV-24619 Wrong result or Assertion `0' in Item::val_native / Type_handler_inet6::Item_val_native_with_conversion +# +CREATE TABLE t1 (a INET6); +INSERT INTO t1 VALUES ('::'),('::'); +SELECT IF(1, '::', a) AS f FROM t1 GROUP BY 'foo' HAVING f != ''; +f +Warnings: +Warning 1292 Incorrect inet6 value: '' +SELECT IF(1, '::', a) AS f FROM t1 GROUP BY 'foo' HAVING f != '::'; +f +SELECT IF(1, '::', a) AS f FROM t1 GROUP BY 'foo' HAVING f != '::1'; +f +:: +DROP TABLE t1;
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Alexander! On Oct 25, Alexander Barkov wrote:
MDEV-26732 Assertion `0' failed in Item::val_native
Also fixes MDEV-24619 Wrong result or Assertion `0' in Item::val_native / Type_handler_inet6::Item_val_native_with_conversion
would be good to have some explanation here, what was wrong, what was the fix.
Right, I suggest this comment:
MDEV-26732 Assertion `0' failed in Item::val_native
Also fixes MDEV-24619 Wrong result or Assertion `0' in Item::val_native / Type_handler_inet6::Item_val_native_with_conversion
Type_handler::create_item_copy() created a generic Item_copy_string, which does not implement val_native() - it has a dummy implementation with DBUG_ASSERT(0), which made the server crash.
Why does it call val_native() if Item_copy_string doesn't implement it? If the item was a string in the first place, that is, if Item_copy_string would've been used correctly where it should've been used, then val_native() wouldn't be called? What would it use, val_str()? So why does it use val_native() here, because that Item_copy_string uses Type_handler_inet6?
Fix:
- Adding a new class Item_copy_inet6, which implements val_native(). - Fixing Type_handler::create_item_copy() to make Item_copy_inet6 instead of Item_copy_string.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hello Sergei, On 10/25/21 11:03 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Oct 25, Alexander Barkov wrote:
MDEV-26732 Assertion `0' failed in Item::val_native
Also fixes MDEV-24619 Wrong result or Assertion `0' in Item::val_native / Type_handler_inet6::Item_val_native_with_conversion
would be good to have some explanation here, what was wrong, what was the fix.
Right, I suggest this comment:
MDEV-26732 Assertion `0' failed in Item::val_native
Also fixes MDEV-24619 Wrong result or Assertion `0' in Item::val_native / Type_handler_inet6::Item_val_native_with_conversion
Type_handler::create_item_copy() created a generic Item_copy_string, which does not implement val_native() - it has a dummy implementation with DBUG_ASSERT(0), which made the server crash.
Why does it call val_native() if Item_copy_string doesn't implement it?
Because I forgot to implement Item_copy_inet6 - I missed the reported scenario in my original test coverage for INET6. The default implementation in Item::val_native(), which is called for Item_copy_string in this scenario, intentionally consists only of DBUG_ASSERT. If it had some working implementation, returning the expected value, it would hide a performance problem: Conversion from the string INET6 representation to the binary INET6 representation would happen per row. This is not desirable. So the DBUG_ASSERT is a trap to find such places not implementing fast code paths for the native representation.
If the item was a string in the first place, that is, if Item_copy_string would've been used correctly where it should've been used, then val_native() wouldn't be called? What would it use, val_str()?
Yes, val_str().
So why does it use val_native() here, because that Item_copy_string uses Type_handler_inet6?
Correct.
Fix:
- Adding a new class Item_copy_inet6, which implements val_native(). - Fixing Type_handler::create_item_copy() to make Item_copy_inet6 instead of Item_copy_string.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Alexander! On Oct 26, Alexander Barkov wrote:
Fix:
- Adding a new class Item_copy_inet6, which implements val_native(). - Fixing Type_handler::create_item_copy() to make Item_copy_inet6 instead of Item_copy_string.
Could there be a universal solution that uses memcpy and doesn't need Item_copy variant for every new type? Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei, On 10/26/21 11:54 AM, Sergei Golubchik wrote:
Hi, Alexander!
On Oct 26, Alexander Barkov wrote:
Fix:
- Adding a new class Item_copy_inet6, which implements val_native(). - Fixing Type_handler::create_item_copy() to make Item_copy_inet6 instead of Item_copy_string.
Could there be a universal solution that uses memcpy and doesn't need Item_copy variant for every new type?
I'm not sure it's easy to simplify the code at this point quickly. val_str() needs the underlying storage class to call to_string(). The currently proposed patch implements it in 10.5 as: + String *val_str(String *to) override + { + if (null_value) + return NULL; + Inet6_null tmp(m_value.ptr(), m_value.length()); + return tmp.is_null() || tmp.to_string(to) ? NULL : to; + } and I already have a working 10.7 version, where the new class goes inside FixedBinTypeBundle: + class Item_copy_fbt: public Item_copy + { + NativeBuffer<Fbt::binary_length()+1> m_value; + ... + String *val_str(String *to) override + { + if (null_value) + return NULL; + Fbt_null tmp(m_value.ptr(), m_value.length()); + return tmp.is_null() || tmp.to_string(to) ? NULL : to; + } + .. so there won't be a separate *source* variant for every new type. I propose to go with this solution for now and at some point revise how to share more code.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (2)
-
Alexander Barkov -
Sergei Golubchik