maria-developers team mailing list archive

[Aleksey Midenkov <midenok@xxxxxxxxx>]

Sergei,

On Wed, Apr 6, 2022 at 11:03 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
>
> 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.

Sorry, you suggested to cleanup in the end of statement and I came to
fix always in the implementation. That of course different things.

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

Added.

>
> > > > +  */
> > > >    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.

Agree, I have the same picture now. Previous faults were on work in
progress. So it faults anyway and we keep
init_lex_with_single_table(), right?

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



-- 
@midenok