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