maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Nikita!

On Jul 27, Nikita Malyavin wrote:
> revision-id: ce289840131 (mariadb-10.3.30-31-gce289840131)
> parent(s): b50ea900636
> author: Nikita Malyavin
> committer: Nikita Malyavin
> timestamp: 2021-07-22 22:38:31 +0300
> message:
> 
> MDEV-24511 null field is created with CREATE..SELECT
> 
> The regression has been initially appeared by MDEV-12588 patch (441349aa).
> 
> The reason CREATE... SELECT NULL does not create null field is that
> it calls handle_select(), where the field is created directly from Item,
> by calling type_handler->type_handler_for_tmp_table(), instead of plain
> type_handler.
> 
> In UNION case, we'll already have Item_field's containing UNION'ed tmp
> table fields to the moment of select_create::prepare() call.
> 
> There, item's type_handler is used directly in
> st_select_lex_unit::join_union_item_types(), instead of
> type_handler_for_tmp_table() or type_handler_for_union().
> 
> Using those two involve too many changes in CREATE...SELECT...UNION
> behavior, which may be reasonably undesired to be pushed in 10.3
> 
> However, patching Field_null::type_handler() directly seems to be accurate
> enough.
> 
> diff --git a/mysql-test/main/union.result b/mysql-test/main/union.result
> index a892f6c9e40..b19427948a6 100644
> --- a/mysql-test/main/union.result
> +++ b/mysql-test/main/union.result
> @@ -1609,7 +1609,7 @@ NULL	binary(0)	YES		NULL
>  CREATE TABLE t5 SELECT NULL UNION SELECT NULL;
>  DESC t5;
>  Field	Type	Null	Key	Default	Extra
> -NULL	null	YES		NULL	
> +NULL	binary(0)	YES		NULL	
>  CREATE TABLE t6 
>  SELECT * FROM (SELECT * FROM (SELECT NULL)a) b UNION SELECT a FROM t1;
>  DESC t6;
> diff --git a/sql/field.h b/sql/field.h
> index a3385736c0a..2af5346bea9 100644
> --- a/sql/field.h
> +++ b/sql/field.h
> @@ -2526,7 +2526,7 @@ class Field_null :public Field_str {
>      :Field_str(ptr_arg, len_arg, null, 1,
>  	       unireg_check_arg, field_name_arg, collation)
>      {}
> -  const Type_handler *type_handler() const { return &type_handler_null; }
> +  const Type_handler *type_handler() const { return &type_handler_string; }
>    Information_schema_character_attributes
>      information_schema_character_attributes() const
>    {

This looks suspiciously risky to me. The problem was specifically in
UNION, but you change the very basic idea of what type the Field_null
is.

I cannot clearly see what else might this possibly affect, field's type
is used everywhere.

It would be safer and more logical to fix specifically field creation
for UNION.

I've discussed it with Bar, who implemented this whole Type_handler
hierarchy, he suggested to use Type_handler::union_element_finalize()
for that. I've attached the patch that does that.

union_element_finalize() is for 10.4, so I also backported it to 10.3,
it's very straightforward though.

What do you think?

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx

commit eb4ff69c0fd
Author: Sergei Golubchik <serg@xxxxxxxxxxx>
Date:   Tue Jul 27 23:45:30 2021 +0200

    MDEV-24511 null field is created with CREATE..SELECT
    
    When creating fields for UNION results, Field_null is not allowed.
    Should create binary(0) instead.

diff --git a/mysql-test/main/union.result b/mysql-test/main/union.result
index a892f6c9e40..b19427948a6 100644
--- a/mysql-test/main/union.result
+++ b/mysql-test/main/union.result
@@ -1609,7 +1609,7 @@ NULL	binary(0)	YES		NULL
 CREATE TABLE t5 SELECT NULL UNION SELECT NULL;
 DESC t5;
 Field	Type	Null	Key	Default	Extra
-NULL	null	YES		NULL	
+NULL	binary(0)	YES		NULL	
 CREATE TABLE t6 
 SELECT * FROM (SELECT * FROM (SELECT NULL)a) b UNION SELECT a FROM t1;
 DESC t6;
diff --git a/sql/sql_type.cc b/sql/sql_type.cc
index 720fea7ebb4..128a4e68533 100644
--- a/sql/sql_type.cc
+++ b/sql/sql_type.cc
@@ -5933,7 +5933,7 @@ void Type_handler_geometry::Item_param_set_param_func(Item_param *param,
 
 /***************************************************************************/
 
-bool Type_handler_string_result::union_element_finalize(const Item * item) const
+bool Type_handler_string_result::union_element_finalize(Item_type_holder *item) const
 {
   if (item->collation.derivation == DERIVATION_NONE)
   {
@@ -5943,6 +5943,12 @@ bool Type_handler_string_result::union_element_finalize(const Item * item) const
   return false;
 }
 
+bool Type_handler_null::union_element_finalize(Item_type_holder *item) const
+{
+  item->set_handler(&type_handler_string);
+  return false;
+}
+
 /***************************************************************************/
 
 bool Type_handler::Vers_history_point_resolve_unit(THD *thd,
diff --git a/sql/sql_type.h b/sql/sql_type.h
index 383ce800f7b..08b599af7ab 100644
--- a/sql/sql_type.h
+++ b/sql/sql_type.h
@@ -62,6 +62,7 @@ class Item_func_minus;
 class Item_func_mul;
 class Item_func_div;
 class Item_func_mod;
+class Item_type_holder;
 class cmp_item;
 class in_vector;
 class Type_handler_hybrid_field_type;
@@ -1191,7 +1192,7 @@ class Type_handler
     Performs the final data type validation for a UNION element,
     after the regular "aggregation for result" was done.
   */
-  virtual bool union_element_finalize(const Item * item) const
+  virtual bool union_element_finalize(Item_type_holder *item) const
   {
     return false;
   }
@@ -2244,7 +2245,7 @@ class Type_handler_string_result: public Type_handler
   void sortlength(THD *thd,
                   const Type_std_attributes *item,
                   SORT_FIELD_ATTR *attr) const;
-  bool union_element_finalize(const Item * item) const;
+  bool union_element_finalize(Item_type_holder *item) const;
   bool Column_definition_prepare_stage1(THD *thd,
                                         MEM_ROOT *mem_root,
                                         Column_definition *c,
@@ -3118,6 +3119,7 @@ class Type_handler_null: public Type_handler_general_purpose_string
   bool Item_send(Item *item, Protocol *protocol, st_value *buf) const;
   Field *make_conversion_table_field(TABLE *, uint metadata,
                                      const Field *target) const;
+  bool union_element_finalize(Item_type_holder *item) const;
   bool Column_definition_fix_attributes(Column_definition *c) const;
   bool Column_definition_prepare_stage1(THD *thd,
                                         MEM_ROOT *mem_root,
diff --git a/sql/sql_union.cc b/sql/sql_union.cc
index c89e59a06f8..48d8c16db68 100644
--- a/sql/sql_union.cc
+++ b/sql/sql_union.cc
@@ -1154,7 +1154,8 @@ bool st_select_lex_unit::prepare(TABLE_LIST *derived_arg,
         Test if the aggregated data type is OK for a UNION element.
         E.g. in case of string data, DERIVATION_NONE is not allowed.
       */
-      if (type->type_handler()->union_element_finalize(type))
+      if (type->type() == Item::TYPE_HOLDER && type->type_handler()->
+            union_element_finalize(static_cast<Item_type_holder*>(type)))
         goto err;
     }