Re: [Maria-developers] MDEV-26742 Assertion `field->type_handler() == this' failed...
Hi, Alexander! See comments below
diff --git a/plugin/type_inet/mysql-test/type_inet/type_inet6_innodb.test b/plugin/type_inet/mysql-test/type_inet/type_inet6_innodb.test index dd6049abbf3..3f3a6fdac2c 100644 --- a/plugin/type_inet/mysql-test/type_inet/type_inet6_innodb.test +++ b/plugin/type_inet/mysql-test/type_inet/type_inet6_innodb.test @@ -12,6 +12,30 @@ SET default_storage_engine=InnoDB; --source type_inet6_engines.inc
+--echo # +--echo # MDEV-26742 Assertion `field->type_handler() == this' failed in FixedBinTypeBundle<NATIVE_LEN, MAX_CHAR_LEN>::Type_handler_fbt::stored_field_cmp_to_item +--echo # + +CREATE TABLE t1 (pk inet6, c text) engine=myisam; +INSERT INTO t1 VALUES ('::',1); +CREATE TABLE t2 (d text, KEY (d)) engine=innodb ; +INSERT INTO t2 VALUES (2); +--error ER_TRUNCATED_WRONG_VALUE +UPDATE t2 JOIN t1 ON ( t1.pk > t2.d) SET t1.c = 1; +SET sql_mode=''; +UPDATE t2 JOIN t1 ON ( t1.pk > t2.d) SET t1.c = 1; +SET sql_mode=DEFAULT; +SELECT * FROM t1; +SELECT * FROM t2; +DROP TABLE t1, t2; + +CREATE TABLE t1 (pk inet6, c text) engine=myisam; +INSERT INTO t1 VALUES ('::',1); +CREATE TABLE t2 ( d text, KEY (d)) engine=innodb ;
I don't get it, why do you recreate tables with exactly the same definition and exactly the same content?
+INSERT INTO t2 VALUES (2); +SELECT * FROM t2 JOIN t1 ON ( t1.pk > t2.d); +DROP TABLE t1, t2; +
--echo # --echo # End of 10.5 tests diff --git a/plugin/type_inet/mysql-test/type_inet/type_inet6_myisam.test b/plugin/type_inet/mysql-test/type_inet/type_inet6_myisam.test index c5183f01cf0..1ccd654181c 100644 --- a/plugin/type_inet/mysql-test/type_inet/type_inet6_myisam.test +++ b/plugin/type_inet/mysql-test/type_inet/type_inet6_myisam.test @@ -10,6 +10,21 @@ SET default_storage_engine=MyISAM; --source type_inet6_engines.inc
+--echo # +--echo # MDEV-26742 Assertion `field->type_handler() == this' failed in FixedBinTypeBundle<NATIVE_LEN, MAX_CHAR_LEN>::Type_handler_fbt::stored_field_cmp_to_item +--echo # + +CREATE TABLE t1 (c varchar(64), key(c)) engine=myisam; +INSERT INTO t1 VALUES ('::1'),('::2'); +SELECT * FROM t1 WHERE c>CAST('::1' AS INET6); +DROP TABLE t1; + +CREATE TABLE t1 (c varchar(64), key(c)) engine=myisam; +INSERT INTO t1 VALUES ('::1'),('0::1');
also don't see much point in recreating
+SELECT * FROM t1 WHERE c=CAST('::1' AS INET6); +EXPLAIN SELECT * FROM t1 WHERE c=CAST('::1' AS INET6); +DROP TABLE t1; +
--echo # --echo # End of 10.5 tests diff --git a/plugin/type_uuid/mysql-test/type_uuid/type_uuid_innodb.test b/plugin/type_uuid/mysql-test/type_uuid/type_uuid_innodb.test index 062e25d366b..27e3214af15 100644 --- a/plugin/type_uuid/mysql-test/type_uuid/type_uuid_innodb.test +++ b/plugin/type_uuid/mysql-test/type_uuid/type_uuid_innodb.test @@ -12,5 +12,27 @@ SET default_storage_engine=InnoDB; --source type_uuid_engines.inc
--echo # ---echo # End of 10.5 tests +--echo # MDEV-26742 Assertion `field->type_handler() == this' failed in FixedBinTypeBundle<NATIVE_LEN, MAX_CHAR_LEN>::Type_handler_fbt::stored_field_cmp_to_item +--echo # + +CREATE TABLE t1 ( pk uuid, c text) engine=myisam; +INSERT INTO t1 VALUES ('00000000-0000-0000-0000-000000000000',1); +CREATE TABLE t2 ( d text, KEY (d)) engine=innodb ; +INSERT INTO t2 VALUES (2); +--error ER_TRUNCATED_WRONG_VALUE +UPDATE t2 JOIN t1 ON ( t1.pk > t2.d) SET t1.c = 1; +SET sql_mode=''; +UPDATE t2 JOIN t1 ON ( t1.pk > t2.d) SET t1.c = 1; +SET sql_mode=DEFAULT; +DROP TABLE t1, t2; + +CREATE TABLE t1 (pk uuid, c text) engine=myisam; +INSERT INTO t1 VALUES ('00000000-0000-0000-0000-000000000000',1); +CREATE TABLE t2 ( d text, KEY (d)) engine=innodb ; +INSERT INTO t2 VALUES (2);
same here
+SELECT * FROM t2 JOIN t1 ON ( t1.pk > t2.d); +DROP TABLE t1, t2; + +--echo # +--echo # End of 10.7 tests --echo # diff --git a/sql/field.cc b/sql/field.cc index 46a3a1deea3..fc3456e2ccb 100644 --- a/sql/field.cc +++ b/sql/field.cc @@ -7368,7 +7368,9 @@ bool Field_longstr::cmp_to_string_with_same_collation(const Item_bool_func *cond, const Item *item) const { + Type_handler_hybrid_field_type cmp(type_handler_for_comparison());
Can this be anything else but type_handler_long_blob ? if not, then I'd suggest Type_handler_hybrid_field_type cmp(type_handler_long_blob); DBUG_ASSERT(type_handler_for_comparison() == &type_handler_long_blob);
+ return !cmp.aggregate_for_comparison(item->type_handler_for_comparison()) && + cmp.type_handler() == type_handler_for_comparison() && - return item->cmp_type() == STRING_RESULT && charset() == cond->compare_collation();
This, basically, means, that cmp_type() is now meaningless, one cannot use it anymore? There's no INET6_RESULT and UUID_RESULT for comparison, so what's the point of cmp_type() now? One more question. What type do we have where you need to aggregate here and simple item->type_handler_for_comparison() != &type_handler_long_blob won't do?
}
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hello Sergei, Thanks for the review. An updated patch is attached. See comments below: On 10/11/21 8:44 PM, Sergei Golubchik wrote:
Hi, Alexander!
See comments below
diff --git a/plugin/type_inet/mysql-test/type_inet/type_inet6_innodb.test b/plugin/type_inet/mysql-test/type_inet/type_inet6_innodb.test index dd6049abbf3..3f3a6fdac2c 100644 --- a/plugin/type_inet/mysql-test/type_inet/type_inet6_innodb.test +++ b/plugin/type_inet/mysql-test/type_inet/type_inet6_innodb.test @@ -12,6 +12,30 @@ SET default_storage_engine=InnoDB; --source type_inet6_engines.inc
+--echo # +--echo # MDEV-26742 Assertion `field->type_handler() == this' failed in FixedBinTypeBundle<NATIVE_LEN, MAX_CHAR_LEN>::Type_handler_fbt::stored_field_cmp_to_item +--echo # + +CREATE TABLE t1 (pk inet6, c text) engine=myisam; +INSERT INTO t1 VALUES ('::',1); +CREATE TABLE t2 (d text, KEY (d)) engine=innodb ; +INSERT INTO t2 VALUES (2); +--error ER_TRUNCATED_WRONG_VALUE +UPDATE t2 JOIN t1 ON ( t1.pk > t2.d) SET t1.c = 1; +SET sql_mode=''; +UPDATE t2 JOIN t1 ON ( t1.pk > t2.d) SET t1.c = 1; +SET sql_mode=DEFAULT; +SELECT * FROM t1; +SELECT * FROM t2; +DROP TABLE t1, t2; + +CREATE TABLE t1 (pk inet6, c text) engine=myisam; +INSERT INTO t1 VALUES ('::',1); +CREATE TABLE t2 ( d text, KEY (d)) engine=innodb ;
I don't get it, why do you recreate tables with exactly the same definition and exactly the same content?
I joined these two tests inside a single CREATE..DROP.
+INSERT INTO t2 VALUES (2); +SELECT * FROM t2 JOIN t1 ON ( t1.pk > t2.d); +DROP TABLE t1, t2; +
--echo # --echo # End of 10.5 tests diff --git a/plugin/type_inet/mysql-test/type_inet/type_inet6_myisam.test b/plugin/type_inet/mysql-test/type_inet/type_inet6_myisam.test index c5183f01cf0..1ccd654181c 100644 --- a/plugin/type_inet/mysql-test/type_inet/type_inet6_myisam.test +++ b/plugin/type_inet/mysql-test/type_inet/type_inet6_myisam.test @@ -10,6 +10,21 @@ SET default_storage_engine=MyISAM; --source type_inet6_engines.inc
+--echo # +--echo # MDEV-26742 Assertion `field->type_handler() == this' failed in FixedBinTypeBundle<NATIVE_LEN, MAX_CHAR_LEN>::Type_handler_fbt::stored_field_cmp_to_item +--echo # + +CREATE TABLE t1 (c varchar(64), key(c)) engine=myisam; +INSERT INTO t1 VALUES ('::1'),('::2'); +SELECT * FROM t1 WHERE c>CAST('::1' AS INET6); +DROP TABLE t1; + +CREATE TABLE t1 (c varchar(64), key(c)) engine=myisam; +INSERT INTO t1 VALUES ('::1'),('0::1');
also don't see much point in recreating
Joined.
+SELECT * FROM t1 WHERE c=CAST('::1' AS INET6); +EXPLAIN SELECT * FROM t1 WHERE c=CAST('::1' AS INET6); +DROP TABLE t1; +
--echo # --echo # End of 10.5 tests diff --git a/plugin/type_uuid/mysql-test/type_uuid/type_uuid_innodb.test b/plugin/type_uuid/mysql-test/type_uuid/type_uuid_innodb.test index 062e25d366b..27e3214af15 100644 --- a/plugin/type_uuid/mysql-test/type_uuid/type_uuid_innodb.test +++ b/plugin/type_uuid/mysql-test/type_uuid/type_uuid_innodb.test @@ -12,5 +12,27 @@ SET default_storage_engine=InnoDB; --source type_uuid_engines.inc
--echo # ---echo # End of 10.5 tests +--echo # MDEV-26742 Assertion `field->type_handler() == this' failed in FixedBinTypeBundle<NATIVE_LEN, MAX_CHAR_LEN>::Type_handler_fbt::stored_field_cmp_to_item +--echo # + +CREATE TABLE t1 ( pk uuid, c text) engine=myisam; +INSERT INTO t1 VALUES ('00000000-0000-0000-0000-000000000000',1); +CREATE TABLE t2 ( d text, KEY (d)) engine=innodb ; +INSERT INTO t2 VALUES (2); +--error ER_TRUNCATED_WRONG_VALUE +UPDATE t2 JOIN t1 ON ( t1.pk > t2.d) SET t1.c = 1; +SET sql_mode=''; +UPDATE t2 JOIN t1 ON ( t1.pk > t2.d) SET t1.c = 1; +SET sql_mode=DEFAULT; +DROP TABLE t1, t2; + +CREATE TABLE t1 (pk uuid, c text) engine=myisam; +INSERT INTO t1 VALUES ('00000000-0000-0000-0000-000000000000',1); +CREATE TABLE t2 ( d text, KEY (d)) engine=innodb ; +INSERT INTO t2 VALUES (2);
same here
Joined.
+SELECT * FROM t2 JOIN t1 ON ( t1.pk > t2.d); +DROP TABLE t1, t2; + +--echo # +--echo # End of 10.7 tests --echo # diff --git a/sql/field.cc b/sql/field.cc index 46a3a1deea3..fc3456e2ccb 100644 --- a/sql/field.cc +++ b/sql/field.cc @@ -7368,7 +7368,9 @@ bool Field_longstr::cmp_to_string_with_same_collation(const Item_bool_func *cond, const Item *item) const { + Type_handler_hybrid_field_type cmp(type_handler_for_comparison());
Can this be anything else but type_handler_long_blob ? if not, then I'd suggest
Type_handler_hybrid_field_type cmp(type_handler_long_blob); DBUG_ASSERT(type_handler_for_comparison() == &type_handler_long_blob);
Generally we don't know. It's now for every single plugin to decide. One can make a plugin using Field_longstr as a base for its Field, but with a different comparison type handler. I wrote it in a generic way which should work for all cases.
+ return !cmp.aggregate_for_comparison(item->type_handler_for_comparison()) && + cmp.type_handler() == type_handler_for_comparison() && - return item->cmp_type() == STRING_RESULT && charset() == cond->compare_collation();
This, basically, means, that cmp_type() is now meaningless, one cannot use it anymore? There's no INET6_RESULT and UUID_RESULT for comparison, so what's the point of cmp_type() now?
cmp_type() is getting gradually replaced to operations on Type_handler pointers and to Type_handler virtual method calls. There are still some less urgent places where Item_result, cmp_type(), result_type() are used. They should be eventually rewritten to use Type_handler, e.g.: - Item_func_field::val_int() - Item_func_benchmark::val_int() - many DBUG_ASSERTs - these can be rewritten using dynamic_cast. I don't know yet if Item_result and cmp_type() will totally disappear at the end, or will be turned into a protected enum inside Type_collection_std. cmp_type() looks quite natural inside these methods: - Type_collection_std::aggregate_for_comparison() - Type_collection_std::aggregate_for_min_max() - Type_collection_std::aggregate_for_num_op()
One more question. What type do we have where you need to aggregate here and simple
item->type_handler_for_comparison() != &type_handler_long_blob
won't do?
We don't know. It's for a plugin to decide if its data type is stronger of weaker than CHAR/VARCHAR/TEXT. For INET6 and UUID we decided they are stronger. But eventually we may want to add some weaker data type.
}
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Alexander! On Oct 12, Alexander Barkov wrote:
diff --git a/sql/field.cc b/sql/field.cc index 46a3a1deea3..fc3456e2ccb 100644 --- a/sql/field.cc +++ b/sql/field.cc @@ -7368,7 +7368,9 @@ bool Field_longstr::cmp_to_string_with_same_collation(const Item_bool_func *cond, const Item *item) const { + Type_handler_hybrid_field_type cmp(type_handler_for_comparison());
Can this be anything else but type_handler_long_blob ? if not, then I'd suggest
Type_handler_hybrid_field_type cmp(type_handler_long_blob); DBUG_ASSERT(type_handler_for_comparison() == &type_handler_long_blob);
Generally we don't know. It's now for every single plugin to decide. One can make a plugin using Field_longstr as a base for its Field, but with a different comparison type handler.
I wrote it in a generic way which should work for all cases.
Yes, I know. But practically we won't have such plugins for quite a while, so I'd rather prefer a shorter execution path and a faster code. And an assert that will remind us to implement your gneralized approach when the assumption will be broken.
One more question. What type do we have where you need to aggregate here and simple
item->type_handler_for_comparison() != &type_handler_long_blob
won't do?
We don't know. It's for a plugin to decide if its data type is stronger of weaker than CHAR/VARCHAR/TEXT. For INET6 and UUID we decided they are stronger. But eventually we may want to add some weaker data type.
Same here. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
On 10/12/21 8:46 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Oct 12, Alexander Barkov wrote:
diff --git a/sql/field.cc b/sql/field.cc index 46a3a1deea3..fc3456e2ccb 100644 --- a/sql/field.cc +++ b/sql/field.cc @@ -7368,7 +7368,9 @@ bool Field_longstr::cmp_to_string_with_same_collation(const Item_bool_func *cond, const Item *item) const { + Type_handler_hybrid_field_type cmp(type_handler_for_comparison());
Can this be anything else but type_handler_long_blob ? if not, then I'd suggest
Type_handler_hybrid_field_type cmp(type_handler_long_blob); DBUG_ASSERT(type_handler_for_comparison() == &type_handler_long_blob);
Generally we don't know. It's now for every single plugin to decide. One can make a plugin using Field_longstr as a base for its Field, but with a different comparison type handler.
I wrote it in a generic way which should work for all cases.
Yes, I know. But practically we won't have such plugins for quite a while, so I'd rather prefer a shorter execution path and a faster code. And an assert that will remind us to implement your gneralized approach when the assumption will be broken.
This code is not heavily loaded and should not affect performance. I prefer the generic way.
One more question. What type do we have where you need to aggregate here and simple
item->type_handler_for_comparison() != &type_handler_long_blob
won't do?
We don't know. It's for a plugin to decide if its data type is stronger of weaker than CHAR/VARCHAR/TEXT. For INET6 and UUID we decided they are stronger. But eventually we may want to add some weaker data type.
Same here.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Alexander! On Oct 12, Alexander Barkov wrote:
Can this be anything else but type_handler_long_blob ? if not, then I'd suggest
Type_handler_hybrid_field_type cmp(type_handler_long_blob); DBUG_ASSERT(type_handler_for_comparison() == &type_handler_long_blob);
Generally we don't know. It's now for every single plugin to decide. One can make a plugin using Field_longstr as a base for its Field, but with a different comparison type handler.
I wrote it in a generic way which should work for all cases.
Yes, I know. But practically we won't have such plugins for quite a while, so I'd rather prefer a shorter execution path and a faster code. And an assert that will remind us to implement your gneralized approach when the assumption will be broken.
This code is not heavily loaded and should not affect performance. I prefer the generic way.
Feels like kind of a waste. We won't need this generalization for years. But yes, it's not a performance critical method, so I'm not going to argue. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (2)
-
Alexander Barkov -
Sergei Golubchik