maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Aleksey,

Thanks. I'll send a separate review email, there will be only replies
here:

On Apr 05, Aleksey Midenkov wrote:
> Hi, Sergei!
> 
> > > @@ -5709,8 +5708,6 @@ bool Item_field::fix_fields(THD *thd, Item **reference)
> > >    }
> > >  #endif
> > >    fixed= 1;
> > > -  if (field->vcol_info)
> > > -    fix_session_vcol_expr_for_read(thd, field, field->vcol_info);
> >
> > Did you revert the optimization from 7450cb7f69db?
> > It was "re-fix vcols on demand, not always for every SELECT"
> >
> > and as far as I can see now you again always re-fix everything,
> > is that so?
> 
> It was your suggestion in the previous review to refix always
> (therefore everything). The original fix worked with
> fix_session_vcol_expr_for_read(). But the current fix is simpler and
> cleaner. Was there a real performance problem or was it a theoretical
> issue?

I checked previous emails and wasn't able to find where I suggested to
refix always.

Anyway, it was indeed a theoretical issue and I have no proofs to claim
that that "optimization" made a difference. So, okay, feel free to refix
always.

> > >    if (thd->variables.sql_mode & MODE_ONLY_FULL_GROUP_BY &&
> > >        !outer_fixed &&
> > >        select &&
> > > diff --git a/sql/table.h b/sql/table.h
> > > index 38b63d285c6..ab1d96538c0 100644
> > > --- a/sql/table.h
> > > +++ b/sql/table.h
> > > @@ -1374,8 +1374,13 @@ struct TABLE
> > >    */
> > >    bool auto_increment_field_not_null;
> > >    bool insert_or_update;             /* Can be used by the handler */
> > > +  /*
> > > +     NOTE: alias_name_used is only a hint! It works only in need_correct_ident()
> > > +     condition. On other cases it is FALSE even if table_name is alias!
> >
> > As I asked earlier, do you have any test case that proves this claim?
> 
> I replied that MDEV-25672 bug's own test case proves that, didn't I?
> 
> (rr) p alias_name_used
> $4 = false
> (rr) p table_name
> $5 = 0x7fdc28024790 "x"
> 
> #0  Item_field::set_field (this=0x7fdc28012e60,
> field_par=0x7fdc28024440) at /home/midenok/src/mariadb/10
> .3/src/sql/item.cc:3289
> #1  0x0000000000b7bc28 in Item_field::fix_fields (this=0x7fdc28012e60,
> thd=0x7fdc28000d28, reference=0x7fdc28034ad8) at /home/midenok/src/mariadb/10.3/src/sql/item.cc:6332
> ...
> #15 0x00000000007c65f6 in mysql_parse (thd=0x7fdc28000d28,
> rawbuf=0x7fdc28010520 "UPDATE `test_table` as
> `x` SET order_date = NULL", length=48, parser_state=0x7fdc39d09548,
> is_com_multi=false, is_next_command=false) at /home/midenok/src/mariadb/10.3/src/sql/sql_parse.cc:7870

indeed, thanks. Could you add it to the comment? Like "e.g. in update t1 as x set a = 1"

> > > +  */
> > >    bool alias_name_used;              /* true if table_name is alias */
> > >    bool get_fields_in_item_tree;      /* Signal to fix_field */
> > > +  List<Virtual_column_info> vcol_cleanup_list;
> > >    CHARSET_INFO *save_character_set_client= thd->variables.character_set_client;
> > >    CHARSET_INFO *save_collation= thd->variables.collation_connection;
> > >    Query_arena  *backup_stmt_arena_ptr= thd->stmt_arena;
> > > @@ -2831,36 +2841,162 @@ static bool fix_vcol_expr(THD *thd, Virtual_column_info *vcol)
...
> > > +bool Vcol_expr_context::init()
> > > +{
> > > +  /*
> > > +      As this is vcol expression we must narrow down name resolution to
> > > +      single table.
> > > +  */
> > > +  if (init_lex_with_single_table(thd, table, &lex))
> >
> > Do you have a test that fails otherwise?
> 
> That fails at least 3 tests:
> 
> main.default vcol.vcol_syntax gcol.gcol_bugfixes
> 
> CURRENT_TEST: main.default
> mysqltest: At line 1302: query 'INSERT INTO t1 VALUES( DEFAULT )'
> failed: 2013: Lost connection to MySQL server during query

doesn't crash for me
(after building your branch and commenting out lex swapping)

> CURRENT_TEST: vcol.vcol_syntax
> mysqltest: At line 118: query 'select * from t1' failed: 2013: Lost
> connection to MySQL server during query

doesn't crash for me. I got a warning about bad double value 'x'
likely because count_cuted_fields being initialized differently in
a new lex or something.

> CURRENT_TEST: gcol.gcol_bugfixes
> mysqltest: At line 579: query 'INSERT INTO t1 (suppliersenttoday)
> VALUES (0)' failed: 2013: Lost connection to MySQL server during query

this one crashes. on the next line, 580, though.
because you set CONTEXT_ANALYSIS_ONLY_VCOL_EXPR, so
Type_std_attributes::agg_item_set_converter does not wrap items
in Item_func_conv_charset. Which is likely incorrect, because items
without wrapping cannot be properly evaluated, and it looks like they
has to be evaluated later, so it's not "context analysys only".

Crash on wrapped items is https://jira.mariadb.org/browse/MDEV-25638
that Sanja is looking at right now.

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