maria-developers team mailing list archive

[Nikita Malyavin <nikitamalyavin@xxxxxxxxx>]

Hello, Sergei!

Looks fine for me. So if you both prefer it more, let's use this variant.

Regards,
Nikita

On Wed, Jul 28, 2021 at 12:08 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

> 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
>


--
Nikita Malyavin, Software Engineer

MariaDB Corporation