Re: [Maria-developers] 93493a0e9b5: MDEV-24176 Server crashes after insert in the table with virtual column generated using date_format() and if()
Hi, Aleksey! Could you please explain in the commit comment what the bug was and what you are fixing? I wasn't able to understad that, so I had to debug those crashes myself. So, the reason is, objects allocated in the wrong arena, right? And it was done inconsistently, on different code paths the "re-fix" context was different. I agree that encapsulating all the context in one Vcol_expr_context is a good idea, it'll allow to have the same, correct, context everywhere. But always using expr_arena doesn't necessarily seem to be a correct choice. E.g. Arg_comparator creates Item_cache_temporal on every fix_field. If vcol expr is re-fixed all the time, expr_arena will grow continuously. Same for charset conversion, CASE, IN, etc. I suspect that if the vcol expr is always re-fixed, it has to use stmt arena. Why do you re-fix fields? I can hardly imagine that an Item_field can find itself in a different table. And if it's always in the same table, why to force it to find this table over and over? We've already discussed VCOL_TABLE_ALIAS, no need to go there again. On Jan 06, Aleksey Midenkov wrote:
revision-id: 93493a0e9b5 (mariadb-10.2.40-194-g93493a0e9b5) parent(s): a565e63c54c author: Aleksey Midenkov committer: Aleksey Midenkov timestamp: 2021-12-29 00:46:31 +0300 message:
MDEV-24176 Server crashes after insert in the table with virtual column generated using date_format() and if()
When table is reopened from cache vcol_info contains stale expression. We refresh expression via TABLE::vcol_fix_exprs() but first we must prepare a proper context (Vcol_expr_context) and do some preparations which meet some requirements:
1. fields must not be fixed, we unfix them with cleanup() via cleanup_processor.
2. Item_ident must have cached_table cleared. Again, this is stale data which was already freed. cached_table interferes with find_field_in_tables().
We cannot clear cached_table directly in Item_ident::cleanup() method. Lots of spaghetti logic depends on cached_table existence even after cleanup() done.
3. If Item_ident has table_name non-NULL we trigger expr update. That is done via Item_ident::check_vcol_func_processor() and VCOL_TABLE_ALIAS flag.
(4-7) The below conditions are prepared by Vcol_expr_context and used in both fix_session_expr_for_read() and TABLE::vcol_fix_exprs():
4. Any fix_expr() must be done on expr_arena as there may be new items created. It was a bug in fix_session_expr_for_read(). It was just not reproduced because there were no second refix. Now refix is done for more cases so it does reproduce. Tests affected: vcol.binlog
5. Also name resolution context must be narrowed to the single table with all crutches in place. Tests affected: vcol.update
6. sql_mode must be clean and not fail expr update.
sql_mode such as MODE_NO_BACKSLASH_ESCAPES, MODE_NO_ZERO_IN_DATE, etc must not affect vcol expression update. If the table was created successfully any further evaluation must not fail. Tests affected: main.func_like
7. Warnings are disabled during expr update. It is assumed that these warnings were printed before during initialization phase or later during execution phase. Tests affected: vcol.wrong_arena
Dictionary: refresh, update and refix are the synonyms for vcol_fix_exprs() or fix_session_expr_for_read() calls.
8. cleanup_excluding_fields_processor() removed. It was just a quick hack to exclude wrongly working Item_field from processing. Now it works due to correct execution environment (Vcol_expr_context). Related to MDEV-10355.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei! On Thu, Jan 6, 2022 at 7:13 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
Could you please explain in the commit comment what the bug was and what you are fixing?
I have added a new message to the last commit. This will be the message of the squashed patch.
I wasn't able to understad that, so I had to debug those crashes myself.
So, the reason is, objects allocated in the wrong arena, right? And it was done inconsistently, on different code paths the "re-fix" context was different.
Yes.
I agree that encapsulating all the context in one Vcol_expr_context is a good idea, it'll allow to have the same, correct, context everywhere.
But always using expr_arena doesn't necessarily seem to be a correct choice. E.g. Arg_comparator creates Item_cache_temporal on every fix_field. If vcol expr is re-fixed all the time, expr_arena will grow continuously. Same for charset conversion, CASE, IN, etc. I suspect that if the vcol expr is always re-fixed, it has to use stmt arena.
stmt_arena cannot be used as long as 'expr' item is created on expr_arena. 'expr' item is created at the vcol expression parse and is reused during the whole TABLE lifetime. If some of the containee items later were allocated on stmt_arena, 'expr' item will try to access them in a different statement when they are already released. That is what this bugfix tries to address. Yes, this fix introduces a small and in most cases imperceptible memory leak which can be easily overcame by FLUSH TABLES. The fix for this leak is non-trivial. 1. vcol expr is not always refixed, but conditionally controlled by vcols_need_refixing. And we must remove this control and truly refix always. 2. we must clone 'expr' item into statement mem_root at each statement. I have attached the leak fix PoC into this email. I do not know whether this is worth it.
Why do you re-fix fields? I can hardly imagine that an Item_field can find itself in a different table. And if it's always in the same table, why to force it to find this table over and over?
That was the alias fix. Refix of Item_field was required. I returned back cleanup_excluding_fields_processor() and removed VCOL_TABLE_ALIAS processing despite my opinion to apply that.
We've already discussed VCOL_TABLE_ALIAS, no need to go there again.
On Jan 06, Aleksey Midenkov wrote:
revision-id: 93493a0e9b5 (mariadb-10.2.40-194-g93493a0e9b5) parent(s): a565e63c54c author: Aleksey Midenkov committer: Aleksey Midenkov timestamp: 2021-12-29 00:46:31 +0300 message:
MDEV-24176 Server crashes after insert in the table with virtual column generated using date_format() and if()
When table is reopened from cache vcol_info contains stale expression. We refresh expression via TABLE::vcol_fix_exprs() but first we must prepare a proper context (Vcol_expr_context) and do some preparations which meet some requirements:
1. fields must not be fixed, we unfix them with cleanup() via cleanup_processor.
2. Item_ident must have cached_table cleared. Again, this is stale data which was already freed. cached_table interferes with find_field_in_tables().
We cannot clear cached_table directly in Item_ident::cleanup() method. Lots of spaghetti logic depends on cached_table existence even after cleanup() done.
3. If Item_ident has table_name non-NULL we trigger expr update. That is done via Item_ident::check_vcol_func_processor() and VCOL_TABLE_ALIAS flag.
(4-7) The below conditions are prepared by Vcol_expr_context and used in both fix_session_expr_for_read() and TABLE::vcol_fix_exprs():
4. Any fix_expr() must be done on expr_arena as there may be new items created. It was a bug in fix_session_expr_for_read(). It was just not reproduced because there were no second refix. Now refix is done for more cases so it does reproduce. Tests affected: vcol.binlog
5. Also name resolution context must be narrowed to the single table with all crutches in place. Tests affected: vcol.update
6. sql_mode must be clean and not fail expr update.
sql_mode such as MODE_NO_BACKSLASH_ESCAPES, MODE_NO_ZERO_IN_DATE, etc must not affect vcol expression update. If the table was created successfully any further evaluation must not fail. Tests affected: main.func_like
7. Warnings are disabled during expr update. It is assumed that these warnings were printed before during initialization phase or later during execution phase. Tests affected: vcol.wrong_arena
Dictionary: refresh, update and refix are the synonyms for vcol_fix_exprs() or fix_session_expr_for_read() calls.
8. cleanup_excluding_fields_processor() removed. It was just a quick hack to exclude wrongly working Item_field from processing. Now it works due to correct execution environment (Vcol_expr_context). Related to MDEV-10355.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- @midenok
Hi, Aleksey! On Jan 13, Aleksey Midenkov wrote:
But always using expr_arena doesn't necessarily seem to be a correct choice. E.g. Arg_comparator creates Item_cache_temporal on every fix_field. If vcol expr is re-fixed all the time, expr_arena will grow continuously. Same for charset conversion, CASE, IN, etc. I suspect that if the vcol expr is always re-fixed, it has to use stmt arena.
stmt_arena cannot be used as long as 'expr' item is created on expr_arena. 'expr' item is created at the vcol expression parse and is reused during the whole TABLE lifetime. If some of the containee items later were allocated on stmt_arena, 'expr' item will try to access them in a different statement when they are already released. That is what this bugfix tries to address.
Yes, this fix introduces a small and in most cases imperceptible memory leak which can be easily overcame by FLUSH TABLES. The fix for this leak is non-trivial.
1. vcol expr is not always refixed, but conditionally controlled by vcols_need_refixing. And we must remove this control and truly refix always.
2. we must clone 'expr' item into statement mem_root at each statement.
I have attached the leak fix PoC into this email. I do not know whether this is worth it.
vcols_need_refixing isn't run-time conditional, it depends on vcol flags, a specific vcol always needs refixing or never does. And a table has vcols_need_refixing if any of the vcols nees it. Thus, I think, a simpler fix would be to split the cleanup and fix_fields. Now both are done in fix_session_vcol_expr(). We can do cleanup at the end of every statement and fix_fields at the beginning of a statement. Just like for normal non-persistent items. For every vcol that needs it. This way they can safely use execution arena. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei! On Mon, Jan 17, 2022 at 7:56 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Jan 13, Aleksey Midenkov wrote:
But always using expr_arena doesn't necessarily seem to be a correct choice. E.g. Arg_comparator creates Item_cache_temporal on every fix_field. If vcol expr is re-fixed all the time, expr_arena will grow continuously. Same for charset conversion, CASE, IN, etc. I suspect that if the vcol expr is always re-fixed, it has to use stmt arena.
stmt_arena cannot be used as long as 'expr' item is created on expr_arena. 'expr' item is created at the vcol expression parse and is reused during the whole TABLE lifetime. If some of the containee items later were allocated on stmt_arena, 'expr' item will try to access them in a different statement when they are already released. That is what this bugfix tries to address.
Yes, this fix introduces a small and in most cases imperceptible memory leak which can be easily overcame by FLUSH TABLES. The fix for this leak is non-trivial.
1. vcol expr is not always refixed, but conditionally controlled by vcols_need_refixing. And we must remove this control and truly refix always.
2. we must clone 'expr' item into statement mem_root at each statement.
I have attached the leak fix PoC into this email. I do not know whether this is worth it.
vcols_need_refixing isn't run-time conditional, it depends on vcol flags, a specific vcol always needs refixing or never does. And a table has vcols_need_refixing if any of the vcols nees it.
Thus, I think, a simpler fix would be to split the cleanup and fix_fields. Now both are done in fix_session_vcol_expr().
We can do cleanup at the end of every statement and fix_fields at the beginning of a statement. Just like for normal non-persistent items. For every vcol that needs it. This way they can safely use execution arena.
Nice catch! Done. Btw, expr_arena was already used in update_virtual_fields(), update_default_fields(), set_default(). So the leak is already there and now we just didn't make it bigger. I marked leak places with TODO comments.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- @midenok
Hi, Aleksey, Here's a review of the combined `git diff 4daf9d7c3ee0 f67dcf6ba5f2`:
diff --git a/mysql-test/suite/vcol/r/vcol_syntax.result b/mysql-test/suite/vcol/r/vcol_syntax.result index 0063f38ea36..c5c8a33c0ec 100644 --- a/mysql-test/suite/vcol/r/vcol_syntax.result +++ b/mysql-test/suite/vcol/r/vcol_syntax.result @@ -94,6 +94,68 @@ create table t1 (a int, v_a int generated always as (a)); update t1 as x set a = 1; alter table t1 force; drop table t1; +create table t1 ( +id int not null auto_increment primary key, +order_date_time datetime not null, +order_date date generated always as (convert(order_date_time, date)), +language_id binary(16) null +); +update t1 as tx set order_date= null; +alter table t1 modify column language_id binary(16) not null; +drop table t1; # -# End of 10.2 tests +# MDEV-24176 Server crashes after insert in the table with virtual column generated using date_format() and if() # +create table t1 (d1 date not null, d2 date not null, +gd text as (concat(d1,if(d1 <> d2, date_format(d2, 'to %y-%m-%d '), ''))) ); +insert into t1(d1,d2) values +('2020-09-01','2020-09-01'),('2020-05-01','2020-09-01');
would be good to have a SELECT here, to verify the server not only not crashes, but actually inserts something
+drop table t1; +# MDEV-25772 (duplicate) and LOCK TABLES case +create table t1 (d1 datetime , v_d1 tinyint(1) as (d1 < curdate())); +insert into t1 (d1) values ('2021-09-11 08:38:23'), ('2021-09-01 08:38:23'); +lock tables t1 write; +select * from t1 where v_d1=1; +d1 v_d1 +2021-09-11 08:38:23 1 +2021-09-01 08:38:23 1 +select * from t1; +d1 v_d1 +2021-09-11 08:38:23 1 +2021-09-01 08:38:23 1 +unlock tables; +drop table t1; +# MDEV-26432 (duplicate) +create table t1 (v2 int, v1 int as ((user() like 'x'))) ; +select 1 from t1 where v1=1 ; +1 +select * from t1; +v2 v1 +drop table t1; +create table t1 (v2 int as ( user () like 'x')); +select 1 from t1 order by v2 ; +1 +alter table t1 add i int; +drop table t1; +# MDEV-26437 (duplicate) +create table v0 (v2 int not null, +v1 bigint as (case 'x' when current_user() then v2 end)); +select v2 as v3 from v0 where v1 like 'x' escape 'x'; +v3 +insert into v0 (v2) values (-128); +drop table v0; +create table t1 (vi int as (case 'x' when current_user() then 1 end)); +select 1 from t1 where vi=1; +1 +show create table t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `vi` int(11) GENERATED ALWAYS AS (case 'x' when current_user() then 1 end) VIRTUAL +) ENGINE=MyISAM DEFAULT CHARSET=latin1 +drop table t1; +create table t1 (vi int as (case 'x' when current_user() then 1 end)); +select 1 from t1 where vi=1; +1 +select 1 from t1 where vi=1; +1 +drop table t1; diff --git a/sql/field.cc b/sql/field.cc index 4e6bc6b8341..297edd9e75c 100644 --- a/sql/field.cc +++ b/sql/field.cc @@ -2453,6 +2453,7 @@ int Field::set_default() if (default_value) { Query_arena backup_arena; + /* TODO: this imposes memory leak until table flush */
could you add an example here? in what case, exactly, there can be a memory leak? you have more comments like that below, I'd suggest to add one example somewhere, and change other comments to say "see ... for an example"
table->in_use->set_n_backup_active_arena(table->expr_arena, &backup_arena); int rc= default_value->expr->save_in_field(this, 0); table->in_use->restore_active_arena(table->expr_arena, &backup_arena); diff --git a/sql/item.h b/sql/item.h index 2a904c1691a..e3028ae412c 100644 --- a/sql/item.h +++ b/sql/item.h @@ -2587,6 +2587,12 @@ class Item_ident :public Item_result_field const char *db_name; const char *table_name; const char *field_name; + /* + NOTE: came from TABLE::alias_name_used and this is only a hint! It works + only in need_correct_ident() condition. On other cases it is FALSE even if + table_name is alias! It cannot be TRUE in these cases, lots of spaghetti + logic depends on that. + */
this is a copy of the comment in table.h you've changed the comment in table.h, but missed this one.
bool alias_name_used; /* true if item was resolved against alias */ /* Cached value of index for this field in table->field array, used by prep. diff --git a/sql/sql_class.h b/sql/sql_class.h index 3f0fba8fc10..8b521f90108 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -1781,6 +1781,25 @@ class Drop_table_error_handler : public Internal_error_handler };
+struct Silence_warnings : public Internal_error_handler +{ +public: + virtual bool handle_condition(THD *, + uint, + const char* sqlstate, + Sql_condition::enum_warning_level *level, + const char* msg, + Sql_condition ** cond_hdl) + { + *cond_hdl= NULL; + if (*level == Sql_condition::WARN_LEVEL_WARN) + return TRUE; + + return FALSE; + } +};
Why do you disable warnings around fix_session_expr() ?
+ + /** Internal error handler to process an error from MDL_context::upgrade_lock() and mysql_lock_tables(). Used by implementations of HANDLER READ and diff --git a/sql/item.cc b/sql/item.cc index 3ff0219c3b3..438bf36060e 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -5507,7 +5506,7 @@ bool Item_field::fix_fields(THD *thd, Item **reference) if (context) { select= context->select_lex; - lex_s= context->select_lex->parent_lex; + lex_s= select ? select->parent_lex : NULL;
There's no this if() in 10.2 anymore, it was removed in d6ee351bbb66. How should this hunk be modified for d6ee351bbb66 ?
} else { @@ -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?
if (thd->variables.sql_mode & MODE_ONLY_FULL_GROUP_BY && !outer_fixed && select && diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 5173df260d5..09af805d60c 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -746,6 +746,8 @@ void close_thread_tables(THD *thd) /* Table might be in use by some outer statement. */ DBUG_PRINT("tcache", ("table: '%s' query_id: %lu", table->s->table_name.str, (ulong) table->query_id)); + if (thd->locked_tables_mode) + table->vcol_cleanup_expr(thd);
Why do you check for thd->locked_tables_mode ? It seems that you'll do vcol_cleanup_expr() unconditionally on any code path. Either here or later in close_thread_table(). So why not just do it always here?
if (thd->locked_tables_mode <= LTM_LOCK_TABLES || table->query_id == thd->query_id) { diff --git a/sql/temporary_tables.cc b/sql/temporary_tables.cc index 005a520ff64..f06dec3a7ff 100644 --- a/sql/temporary_tables.cc +++ b/sql/temporary_tables.cc @@ -1128,6 +1128,13 @@ TABLE *THD::open_temporary_table(TMP_TABLE_SHARE *share, table->pos_in_table_list= 0; table->query_id= query_id;
+ if (table->vcol_fix_expr(this)) + { + my_free(table);
Hmm, you sure? I'd expect that after successful open_table_from_share() you'd need much more than just my_free() to close the table.
+ DBUG_RETURN(NULL); + } + + /* Add table to the head of table list. */ share->all_tmp_tables.push_front(table);
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?
+ */ 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;
If you have a list of all items that need cleanup, may be can also use it to refix items? Meaning, you move it to TABLE_SHARE, remove need_refix, instead write - if (table->s->need_refix) + if (!table->s->vcol_refix_list.is_empty()) and, of course, don't pop elements on cleanup, but iterate the list in a non-destructive way.
private: bool m_needs_reopen; bool created; /* For tmp tables. TRUE <=> tmp table was actually created.*/ diff --git a/sql/table.cc b/sql/table.cc index d4f8170e0af..4bed87d7740 100644 --- a/sql/table.cc +++ b/sql/table.cc @@ -1005,6 +1004,22 @@ static void mysql57_calculate_null_position(TABLE_SHARE *share, bool parse_vcol_defs(THD *thd, MEM_ROOT *mem_root, TABLE *table, bool *error_reported, vcol_init_mode mode) { + struct check_vcol_forward_refs + { + static bool check(Field *field, Virtual_column_info *vcol) + { + return vcol && + vcol->expr->walk(&Item::check_field_expression_processor, 0, field); + } + static bool check(Field *field) + { + if (check(field, field->vcol_info) || + check(field, field->check_constraint) || + check(field, field->default_value)) + return true; + return false; + } + };
Hm, somehow I haven't seen this pattern before. nice.
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) @note this is done for all vcols for INSERT/UPDATE/DELETE, and only as needed for SELECTs. */ -bool fix_session_vcol_expr(THD *thd, Virtual_column_info *vcol) +bool Virtual_column_info::fix_session_expr(THD *thd, TABLE *table) { - DBUG_ENTER("fix_session_vcol_expr"); - if (!(vcol->flags & (VCOL_TIME_FUNC|VCOL_SESSION_FUNC))) - DBUG_RETURN(0); + if (!need_refix()) + return false;
- vcol->expr->walk(&Item::cleanup_excluding_fields_processor, 0, 0); - DBUG_ASSERT(!vcol->expr->fixed); - DBUG_RETURN(fix_vcol_expr(thd, vcol)); + DBUG_ASSERT(!expr->fixed); + if (expr->walk(&Item::change_context_processor, 0, thd->lex->current_context())) + return true;
Why do you need to change the context here?
+ table->vcol_cleanup_list.push_back(this, thd->mem_root); + if (fix_expr(thd)) + return true; + if (expr->walk(&Item::change_context_processor, 0, NULL)) + return true; + return false; +} + + +bool Virtual_column_info::cleanup_session_expr() +{ + DBUG_ASSERT(need_refix()); + if (expr->walk(&Item::cleanup_excluding_fields_processor, 0, 0)) + return true; + return false; }
-/** invoke fix_session_vcol_expr for a vcol
- @note this is called for generated column or a DEFAULT expression from - their corresponding fix_fields on SELECT. -*/ -bool fix_session_vcol_expr_for_read(THD *thd, Field *field, - Virtual_column_info *vcol) +class Vcol_expr_context { - DBUG_ENTER("fix_session_vcol_expr_for_read"); - TABLE_LIST *tl= field->table->pos_in_table_list; - if (!tl || tl->lock_type >= TL_WRITE_ALLOW_WRITE) - DBUG_RETURN(0); - Security_context *save_security_ctx= thd->security_ctx; - if (tl->security_ctx) + bool inited; + THD *thd; + TABLE *table; + LEX *old_lex; + LEX lex; + table_map old_map; + bool old_want_privilege; + Security_context *save_security_ctx; + sql_mode_t save_sql_mode; + Silence_warnings disable_warnings; + +public: + Vcol_expr_context(THD *_thd, TABLE *_table) : + inited(false), + thd(_thd), + table(_table), + old_lex(thd->lex), + old_map(table->map), + old_want_privilege(table->grant.want_privilege), + save_security_ctx(thd->security_ctx), + save_sql_mode(thd->variables.sql_mode) {} + bool init(); + + ~Vcol_expr_context(); +}; + + +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?
+ { + my_error(ER_OUT_OF_RESOURCES, MYF(0)); + table->map= old_map; + return true; + } + + thd->push_internal_handler(&disable_warnings); + table->grant.want_privilege= false;
1. why? 2. it's not bool, it's a set of privileges, don't use false here.
+ lex.sql_command= old_lex->sql_command; + thd->variables.sql_mode= 0; + + /* + NOTE: we refix also tmp tables used in ALTER TABLE,
Why? I don't see how a tmp table from ALTER can possibly be used in two different statements. There should be no need to re-fix it. Just fixing once, on open, should be enough.
+ they have (pos_in_table_list == NULL). + */ + TABLE_LIST const *tl= table->pos_in_table_list; + DBUG_ASSERT(tl || table->s->tmp_table); + + if (tl && tl->security_ctx) thd->security_ctx= tl->security_ctx; - bool res= fix_session_vcol_expr(thd, vcol); + + inited= true; + return false; +} + +Vcol_expr_context::~Vcol_expr_context() +{ + if (!inited) + return; + table->grant.want_privilege= old_want_privilege; + thd->pop_internal_handler(); + end_lex_with_single_table(thd, table, old_lex); + table->map= old_map; thd->security_ctx= save_security_ctx; - DBUG_RETURN(res); + thd->variables.sql_mode= save_sql_mode; +} + + +bool TABLE::vcol_fix_expr(THD *thd) +{ + DBUG_ASSERT(pos_in_table_list || s->tmp_table); + if ((pos_in_table_list && pos_in_table_list->placeholder()) || + !s->vcol_need_refix) + return false; + + if (!thd->stmt_arena->is_conventional() && + !vcol_cleanup_list.is_empty()) + { + /* NOTE: Under trigger we already have fixed expressions */ + return false; + } + DBUG_ASSERT(vcol_cleanup_list.is_empty()); + + Vcol_expr_context expr_ctx(thd, this); + if (expr_ctx.init()) + return true; + + for (Field **vf= vfield; vf && *vf; vf++) + if ((*vf)->vcol_info->fix_session_expr(thd, this)) + goto error; + + for (Field **df= default_field; df && *df; df++) + if ((*df)->default_value && + (*df)->default_value->fix_session_expr(thd, this)) + goto error; + + for (Virtual_column_info **cc= check_constraints; cc && *cc; cc++) + if ((*cc)->fix_session_expr(thd, this)) + goto error; + + return false; + +error: + DBUG_ASSERT(thd->get_stmt_da()->is_error()); + return true; +} + + +bool TABLE::vcol_cleanup_expr(THD *thd) +{ + if (vcol_cleanup_list.is_empty()) + return false; + + List_iterator<Virtual_column_info> it(vcol_cleanup_list); + bool result= false; + + while (Virtual_column_info *vcol= it++) + result|= vcol->cleanup_session_expr(); + + vcol_cleanup_list.empty(); + + DBUG_ASSERT(!result || thd->get_stmt_da()->is_error()); + return result; }
@@ -2939,14 +3072,17 @@ static bool fix_and_check_vcol_expr(THD *thd, TABLE *table, */ myf warn= table->s->frm_version < FRM_VER_EXPRESSSIONS ? ME_JUST_WARNING : 0; my_error(ER_VIRTUAL_COLUMN_FUNCTION_IS_NOT_ALLOWED, MYF(warn), - "AUTO_INCREMENT", vcol->get_vcol_type_name(), res.name); + "AUTO_INCREMENT", get_vcol_type_name(), res.name); if (!warn) DBUG_RETURN(1); } - vcol->flags= res.errors; + flags= res.errors;
- if (vcol->flags & VCOL_SESSION_FUNC) - table->s->vcols_need_refixing= true; + if (need_refix())
Old code only refixed VCOL_SESSION_FUNC, because those Items can change the medata on fix_fields. See the comment for 73a220aac384 Refixing VCOL_TIME_FUNC seems to be redundant
+ { + table->s->vcol_need_refix= true; + cleanup_session_expr();
why?
+ }
DBUG_RETURN(0); } @@ -3011,11 +3147,13 @@ unpack_vcol_info_from_frm(THD *thd, MEM_ROOT *mem_root, TABLE *table, if (error) goto end;
+ lex.sql_command= old_lex->sql_command;
why?
+ vcol_storage.vcol_info->set_vcol_type(vcol->get_vcol_type()); vcol_storage.vcol_info->stored_in_db= vcol->stored_in_db; vcol_storage.vcol_info->name= vcol->name; vcol_storage.vcol_info->utf8= vcol->utf8; - if (!fix_and_check_vcol_expr(thd, table, vcol_storage.vcol_info)) + if (!vcol_storage.vcol_info->fix_and_check_expr(thd, table)) { *vcol_ptr= vcol_info= vcol_storage.vcol_info; // Expression ok DBUG_ASSERT(vcol_info->expr);
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Sergei! On Thu, Mar 31, 2022 at 11:30 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey,
Here's a review of the combined `git diff 4daf9d7c3ee0 f67dcf6ba5f2`:
diff --git a/mysql-test/suite/vcol/r/vcol_syntax.result b/mysql-test/suite/vcol/r/vcol_syntax.result index 0063f38ea36..c5c8a33c0ec 100644 --- a/mysql-test/suite/vcol/r/vcol_syntax.result +++ b/mysql-test/suite/vcol/r/vcol_syntax.result @@ -94,6 +94,68 @@ create table t1 (a int, v_a int generated always as (a)); update t1 as x set a = 1; alter table t1 force; drop table t1; +create table t1 ( +id int not null auto_increment primary key, +order_date_time datetime not null, +order_date date generated always as (convert(order_date_time, date)), +language_id binary(16) null +); +update t1 as tx set order_date= null; +alter table t1 modify column language_id binary(16) not null; +drop table t1; # -# End of 10.2 tests +# MDEV-24176 Server crashes after insert in the table with virtual column generated using date_format() and if() # +create table t1 (d1 date not null, d2 date not null, +gd text as (concat(d1,if(d1 <> d2, date_format(d2, 'to %y-%m-%d '), ''))) ); +insert into t1(d1,d2) values +('2020-09-01','2020-09-01'),('2020-05-01','2020-09-01');
would be good to have a SELECT here, to verify the server not only not crashes, but actually inserts something
Added.
+drop table t1; +# MDEV-25772 (duplicate) and LOCK TABLES case +create table t1 (d1 datetime , v_d1 tinyint(1) as (d1 < curdate())); +insert into t1 (d1) values ('2021-09-11 08:38:23'), ('2021-09-01 08:38:23'); +lock tables t1 write; +select * from t1 where v_d1=1; +d1 v_d1 +2021-09-11 08:38:23 1 +2021-09-01 08:38:23 1 +select * from t1; +d1 v_d1 +2021-09-11 08:38:23 1 +2021-09-01 08:38:23 1 +unlock tables; +drop table t1; +# MDEV-26432 (duplicate) +create table t1 (v2 int, v1 int as ((user() like 'x'))) ; +select 1 from t1 where v1=1 ; +1 +select * from t1; +v2 v1 +drop table t1; +create table t1 (v2 int as ( user () like 'x')); +select 1 from t1 order by v2 ; +1 +alter table t1 add i int; +drop table t1; +# MDEV-26437 (duplicate) +create table v0 (v2 int not null, +v1 bigint as (case 'x' when current_user() then v2 end)); +select v2 as v3 from v0 where v1 like 'x' escape 'x'; +v3 +insert into v0 (v2) values (-128); +drop table v0; +create table t1 (vi int as (case 'x' when current_user() then 1 end)); +select 1 from t1 where vi=1; +1 +show create table t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `vi` int(11) GENERATED ALWAYS AS (case 'x' when current_user() then 1 end) VIRTUAL +) ENGINE=MyISAM DEFAULT CHARSET=latin1 +drop table t1; +create table t1 (vi int as (case 'x' when current_user() then 1 end)); +select 1 from t1 where vi=1; +1 +select 1 from t1 where vi=1; +1 +drop table t1; diff --git a/sql/field.cc b/sql/field.cc index 4e6bc6b8341..297edd9e75c 100644 --- a/sql/field.cc +++ b/sql/field.cc @@ -2453,6 +2453,7 @@ int Field::set_default() if (default_value) { Query_arena backup_arena; + /* TODO: this imposes memory leak until table flush */
could you add an example here? in what case, exactly, there can be a memory leak?
The change was introduced by db7edfed17ef Updated the comment to: /* TODO: this imposes memory leak until table flush when save_in_field() does expr_arena allocation. F.ex. case from main.default: CREATE TABLE t1 (a INT DEFAULT CONCAT('1 ')); INSERT INTO t1 VALUES (DEFAULT); */
you have more comments like that below, I'd suggest to add one example somewhere, and change other comments to say "see ... for an example"
Ok, but the other code does allocations for other cases. I'm not sure if it is needed to find all the test cases and why it is needed.
table->in_use->set_n_backup_active_arena(table->expr_arena, &backup_arena); int rc= default_value->expr->save_in_field(this, 0); table->in_use->restore_active_arena(table->expr_arena, &backup_arena); diff --git a/sql/item.h b/sql/item.h index 2a904c1691a..e3028ae412c 100644 --- a/sql/item.h +++ b/sql/item.h @@ -2587,6 +2587,12 @@ class Item_ident :public Item_result_field const char *db_name; const char *table_name; const char *field_name; + /* + NOTE: came from TABLE::alias_name_used and this is only a hint! It works + only in need_correct_ident() condition. On other cases it is FALSE even if + table_name is alias! It cannot be TRUE in these cases, lots of spaghetti + logic depends on that. + */
this is a copy of the comment in table.h you've changed the comment in table.h, but missed this one.
Fixed.
bool alias_name_used; /* true if item was resolved against alias */ /* Cached value of index for this field in table->field array, used by prep. diff --git a/sql/sql_class.h b/sql/sql_class.h index 3f0fba8fc10..8b521f90108 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -1781,6 +1781,25 @@ class Drop_table_error_handler : public Internal_error_handler };
+struct Silence_warnings : public Internal_error_handler +{ +public: + virtual bool handle_condition(THD *, + uint, + const char* sqlstate, + Sql_condition::enum_warning_level *level, + const char* msg, + Sql_condition ** cond_hdl) + { + *cond_hdl= NULL; + if (*level == Sql_condition::WARN_LEVEL_WARN) + return TRUE; + + return FALSE; + } +};
Why do you disable warnings around fix_session_expr() ?
It failed vcol.wrong_arena. Now it doesn't fail, so I removed that.
+ + /** Internal error handler to process an error from MDL_context::upgrade_lock() and mysql_lock_tables(). Used by implementations of HANDLER READ and diff --git a/sql/item.cc b/sql/item.cc index 3ff0219c3b3..438bf36060e 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -5507,7 +5506,7 @@ bool Item_field::fix_fields(THD *thd, Item **reference) if (context) { select= context->select_lex; - lex_s= context->select_lex->parent_lex; + lex_s= select ? select->parent_lex : NULL;
There's no this if() in 10.2 anymore, it was removed in d6ee351bbb66. How should this hunk be modified for d6ee351bbb66 ?
This was merged successfully as the original code chunk (no changes needed).
} else { @@ -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?
if (thd->variables.sql_mode & MODE_ONLY_FULL_GROUP_BY && !outer_fixed && select && diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 5173df260d5..09af805d60c 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -746,6 +746,8 @@ void close_thread_tables(THD *thd) /* Table might be in use by some outer statement. */ DBUG_PRINT("tcache", ("table: '%s' query_id: %lu", table->s->table_name.str, (ulong) table->query_id)); + if (thd->locked_tables_mode) + table->vcol_cleanup_expr(thd);
Why do you check for thd->locked_tables_mode ? It seems that you'll do vcol_cleanup_expr() unconditionally on any code path. Either here or later in close_thread_table().
So why not just do it always here?
close_thread_table() is called not only from close_thread_tables().
if (thd->locked_tables_mode <= LTM_LOCK_TABLES || table->query_id == thd->query_id) { diff --git a/sql/temporary_tables.cc b/sql/temporary_tables.cc index 005a520ff64..f06dec3a7ff 100644 --- a/sql/temporary_tables.cc +++ b/sql/temporary_tables.cc @@ -1128,6 +1128,13 @@ TABLE *THD::open_temporary_table(TMP_TABLE_SHARE *share, table->pos_in_table_list= 0; table->query_id= query_id;
+ if (table->vcol_fix_expr(this)) + { + my_free(table);
Hmm, you sure? I'd expect that after successful open_table_from_share() you'd need much more than just my_free() to close the table.
True. Should be replaced with drop_temporary_table() but I removed that completely (since we don't need the refix for temporary tables like you've noted).
+ DBUG_RETURN(NULL); + } + + /* Add table to the head of table list. */ share->all_tmp_tables.push_front(table);
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=0x7f dc28034ad8) 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=f alse) at /home/midenok/src/mariadb/10.3/src/sql/sql_parse.cc:7870
+ */ 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;
If you have a list of all items that need cleanup, may be can also use it to refix items? Meaning, you move it to TABLE_SHARE, remove need_refix, instead write
- if (table->s->need_refix) + if (!table->s->vcol_refix_list.is_empty())
and, of course, don't pop elements on cleanup, but iterate the list in a non-destructive way.
I made that in TABLE, now it looks cleaner, thanks. It cannot be in TABLE_SHARE as unpack_vcol_info_from_frm() is done for TABLE (and we don't want such refactorings in a bugfix).
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) @note this is done for all vcols for INSERT/UPDATE/DELETE, and only as needed for SELECTs. */ -bool fix_session_vcol_expr(THD *thd, Virtual_column_info *vcol) +bool Virtual_column_info::fix_session_expr(THD *thd, TABLE *table) { - DBUG_ENTER("fix_session_vcol_expr"); - if (!(vcol->flags & (VCOL_TIME_FUNC|VCOL_SESSION_FUNC))) - DBUG_RETURN(0); + if (!need_refix()) + return false;
- vcol->expr->walk(&Item::cleanup_excluding_fields_processor, 0, 0); - DBUG_ASSERT(!vcol->expr->fixed); - DBUG_RETURN(fix_vcol_expr(thd, vcol)); + DBUG_ASSERT(!expr->fixed); + if (expr->walk(&Item::change_context_processor, 0, thd->lex->current_context())) + return true;
Why do you need to change the context here?
That failed tests on the first revision of the bugfix. Now it seems to work fine AFAICS, so I removed that. Sorry for not cleaning the 2nd revision well.
+ table->vcol_cleanup_list.push_back(this, thd->mem_root); + if (fix_expr(thd)) + return true; + if (expr->walk(&Item::change_context_processor, 0, NULL)) + return true; + return false; +} + + +bool Virtual_column_info::cleanup_session_expr() +{ + DBUG_ASSERT(need_refix()); + if (expr->walk(&Item::cleanup_excluding_fields_processor, 0, 0)) + return true; + return false; }
-/** invoke fix_session_vcol_expr for a vcol
- @note this is called for generated column or a DEFAULT expression from - their corresponding fix_fields on SELECT. -*/ -bool fix_session_vcol_expr_for_read(THD *thd, Field *field, - Virtual_column_info *vcol) +class Vcol_expr_context { - DBUG_ENTER("fix_session_vcol_expr_for_read"); - TABLE_LIST *tl= field->table->pos_in_table_list; - if (!tl || tl->lock_type >= TL_WRITE_ALLOW_WRITE) - DBUG_RETURN(0); - Security_context *save_security_ctx= thd->security_ctx; - if (tl->security_ctx) + bool inited; + THD *thd; + TABLE *table; + LEX *old_lex; + LEX lex; + table_map old_map; + bool old_want_privilege; + Security_context *save_security_ctx; + sql_mode_t save_sql_mode; + Silence_warnings disable_warnings; + +public: + Vcol_expr_context(THD *_thd, TABLE *_table) : + inited(false), + thd(_thd), + table(_table), + old_lex(thd->lex), + old_map(table->map), + old_want_privilege(table->grant.want_privilege), + save_security_ctx(thd->security_ctx), + save_sql_mode(thd->variables.sql_mode) {} + bool init(); + + ~Vcol_expr_context(); +}; + + +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 CURRENT_TEST: vcol.vcol_syntax mysqltest: At line 118: query 'select * from t1' failed: 2013: Lost connection to MySQL server during query 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
+ { + my_error(ER_OUT_OF_RESOURCES, MYF(0)); + table->map= old_map; + return true; + } + + thd->push_internal_handler(&disable_warnings); + table->grant.want_privilege= false;
1. why?
Again I don't see failing tests. Removed that.
2. it's not bool, it's a set of privileges, don't use false here.
+ lex.sql_command= old_lex->sql_command; + thd->variables.sql_mode= 0; + + /* + NOTE: we refix also tmp tables used in ALTER TABLE,
Why? I don't see how a tmp table from ALTER can possibly be used in two different statements. There should be no need to re-fix it. Just fixing once, on open, should be enough.
Ok, I disabled refix for tmp tables. That seems to work fine.
+ they have (pos_in_table_list == NULL). + */ + TABLE_LIST const *tl= table->pos_in_table_list; + DBUG_ASSERT(tl || table->s->tmp_table); + + if (tl && tl->security_ctx) thd->security_ctx= tl->security_ctx; - bool res= fix_session_vcol_expr(thd, vcol); + + inited= true; + return false; +} + +Vcol_expr_context::~Vcol_expr_context() +{ + if (!inited) + return; + table->grant.want_privilege= old_want_privilege; + thd->pop_internal_handler(); + end_lex_with_single_table(thd, table, old_lex); + table->map= old_map; thd->security_ctx= save_security_ctx; - DBUG_RETURN(res); + thd->variables.sql_mode= save_sql_mode; +} + + +bool TABLE::vcol_fix_expr(THD *thd) +{ + DBUG_ASSERT(pos_in_table_list || s->tmp_table); + if ((pos_in_table_list && pos_in_table_list->placeholder()) || + !s->vcol_need_refix) + return false; + + if (!thd->stmt_arena->is_conventional() && + !vcol_cleanup_list.is_empty()) + { + /* NOTE: Under trigger we already have fixed expressions */ + return false; + } + DBUG_ASSERT(vcol_cleanup_list.is_empty()); + + Vcol_expr_context expr_ctx(thd, this); + if (expr_ctx.init()) + return true; + + for (Field **vf= vfield; vf && *vf; vf++) + if ((*vf)->vcol_info->fix_session_expr(thd, this)) + goto error; + + for (Field **df= default_field; df && *df; df++) + if ((*df)->default_value && + (*df)->default_value->fix_session_expr(thd, this)) + goto error; + + for (Virtual_column_info **cc= check_constraints; cc && *cc; cc++) + if ((*cc)->fix_session_expr(thd, this)) + goto error; + + return false; + +error: + DBUG_ASSERT(thd->get_stmt_da()->is_error()); + return true; +} + + +bool TABLE::vcol_cleanup_expr(THD *thd) +{ + if (vcol_cleanup_list.is_empty()) + return false; + + List_iterator<Virtual_column_info> it(vcol_cleanup_list); + bool result= false; + + while (Virtual_column_info *vcol= it++) + result|= vcol->cleanup_session_expr(); + + vcol_cleanup_list.empty(); + + DBUG_ASSERT(!result || thd->get_stmt_da()->is_error()); + return result; }
@@ -2939,14 +3072,17 @@ static bool fix_and_check_vcol_expr(THD *thd, TABLE *table, */ myf warn= table->s->frm_version < FRM_VER_EXPRESSSIONS ? ME_JUST_WARNING : 0; my_error(ER_VIRTUAL_COLUMN_FUNCTION_IS_NOT_ALLOWED, MYF(warn), - "AUTO_INCREMENT", vcol->get_vcol_type_name(), res.name); + "AUTO_INCREMENT", get_vcol_type_name(), res.name); if (!warn) DBUG_RETURN(1); } - vcol->flags= res.errors; + flags= res.errors;
- if (vcol->flags & VCOL_SESSION_FUNC) - table->s->vcols_need_refixing= true; + if (need_refix())
Old code only refixed VCOL_SESSION_FUNC, because those Items can change the medata on fix_fields. See the comment for 73a220aac384 Refixing VCOL_TIME_FUNC seems to be redundant
Ok, I removed VCOL_TIME_FUNC from the condition.
+ { + table->s->vcol_need_refix= true; + cleanup_session_expr();
why?
I did the refix on open_table() even for the first time, when the table was opened from share. Now I fixed that with from_share condition.
+ }
DBUG_RETURN(0); } @@ -3011,11 +3147,13 @@ unpack_vcol_info_from_frm(THD *thd, MEM_ROOT *mem_root, TABLE *table, if (error) goto end;
+ lex.sql_command= old_lex->sql_command;
why?
Looks like again this is an old remnant... Removed that.
+ vcol_storage.vcol_info->set_vcol_type(vcol->get_vcol_type()); vcol_storage.vcol_info->stored_in_db= vcol->stored_in_db; vcol_storage.vcol_info->name= vcol->name; vcol_storage.vcol_info->utf8= vcol->utf8; - if (!fix_and_check_vcol_expr(thd, table, vcol_storage.vcol_info)) + if (!vcol_storage.vcol_info->fix_and_check_expr(thd, table)) { *vcol_ptr= vcol_info= vcol_storage.vcol_info; // Expression ok DBUG_ASSERT(vcol_info->expr);
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- @midenok
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@mariadb.org
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
Hi, Aleksey, On Apr 12, Aleksey Midenkov wrote:
+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))
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?
No, quite the opposite. I think (see above) that CONTEXT_ANALYSIS_ONLY_VCOL_EXPR (it's set inside init_lex_with_single_table()) is wrong, what you're doing is not "context analysys only", you're preparing items for evaluation. Let's wait for Sanja to close his MDEV-25638, and then I'll check this test case again. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei, On Wed, Apr 13, 2022 at 8:18 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey,
On Apr 12, Aleksey Midenkov wrote:
+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))
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?
No, quite the opposite. I think (see above) that CONTEXT_ANALYSIS_ONLY_VCOL_EXPR (it's set inside init_lex_with_single_table()) is wrong, what you're doing is not "context analysys only", you're preparing items for evaluation.
That works identically in branch and vanilla 10.3: --source include/have_ucs2.inc create table t1 (c1 char(1) character set ucs2 collate ucs2_test_ci, v1 char(1) character set ucs2 collate ucs2_test_ci as (c1), v2 int as (c1 = 'b'), v3 int as (v1 = 'b')); insert into t1 (c1) values ('a'); select * from t1 where v1 = 'b'; drop table t1; Result: select * from t1 where v1 = 'b'; c1 v1 v2 v3 a a 0 0 .opt file: --character-sets-dir=$MYSQL_TEST_DIR/std_data/ldml/ Please, correct the test case if I missed something. ... As a side note here I tried to clear CONTEXT_ANALYSIS_ONLY_VCOL_EXPR: https://github.com/midenok/mariadb/issues/93#issuecomment-1098336828 Couple of tests failed.
Let's wait for Sanja to close his MDEV-25638, and then I'll check this test case again.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- @midenok
Hi, Aleksey, On Apr 13, Aleksey Midenkov wrote:
No, quite the opposite. I think (see above) that CONTEXT_ANALYSIS_ONLY_VCOL_EXPR (it's set inside init_lex_with_single_table()) is wrong, what you're doing is not "context analysys only", you're preparing items for evaluation.
That works identically in branch and vanilla 10.3:
--source include/have_ucs2.inc create table t1 (c1 char(1) character set ucs2 collate ucs2_test_ci, v1 char(1) character set ucs2 collate ucs2_test_ci as (c1), v2 int as (c1 = 'b'), v3 int as (v1 = 'b')); insert into t1 (c1) values ('a'); select * from t1 where v1 = 'b'; drop table t1;
Result: select * from t1 where v1 = 'b'; c1 v1 v2 v3 a a 0 0
.opt file: --character-sets-dir=$MYSQL_TEST_DIR/std_data/ldml/
Wow, this is a very good test. I wonder how you found it. Yes, it shows, exactly, that it's incorrect to set CONTEXT_ANALYSIS_ONLY_VCOL_EXPR and then evaluate those items. Items fixed under CONTEXT_ANALYSIS_ONLY_VCOL_EXPR are *not fit* to be evaluated. Take a look at the bb-10.3-serg-MDEV-24176 branch. Three commits there: two cherry-picks from 10.2 (they shouldn't be pushed into 10.3, instead your commit will eventually be rebased on top of them after they're merged into 10.3) and the "work-in-progress" commit that removes init_lex_with_single_table and CONTEXT_ANALYSIS_ONLY_VCOL_EXPR. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei! On Fri, Apr 15, 2022 at 6:00 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey,
On Apr 13, Aleksey Midenkov wrote:
No, quite the opposite. I think (see above) that CONTEXT_ANALYSIS_ONLY_VCOL_EXPR (it's set inside init_lex_with_single_table()) is wrong, what you're doing is not "context analysys only", you're preparing items for evaluation.
That works identically in branch and vanilla 10.3:
--source include/have_ucs2.inc create table t1 (c1 char(1) character set ucs2 collate ucs2_test_ci, v1 char(1) character set ucs2 collate ucs2_test_ci as (c1), v2 int as (c1 = 'b'), v3 int as (v1 = 'b')); insert into t1 (c1) values ('a'); select * from t1 where v1 = 'b'; drop table t1;
Result: select * from t1 where v1 = 'b'; c1 v1 v2 v3 a a 0 0
.opt file: --character-sets-dir=$MYSQL_TEST_DIR/std_data/ldml/
Wow, this is a very good test. I wonder how you found it.
I disabled execution of Type_std_attributes::agg_item_set_converter() and took the most appropriate failing test case.
Yes, it shows, exactly, that it's incorrect to set CONTEXT_ANALYSIS_ONLY_VCOL_EXPR and then evaluate those items. Items fixed under CONTEXT_ANALYSIS_ONLY_VCOL_EXPR are *not fit* to be evaluated.
Take a look at the bb-10.3-serg-MDEV-24176 branch. Three commits there: two cherry-picks from 10.2 (they shouldn't be pushed into 10.3, instead your commit will eventually be rebased on top of them after they're merged into 10.3) and the "work-in-progress" commit that removes init_lex_with_single_table and CONTEXT_ANALYSIS_ONLY_VCOL_EXPR.
I just tried to note this is a different bug (as it reproduces in master branch). And since it is lower priority maybe we push the blocker first?
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- @midenok
Hi, Aleksey,
Take a look at the bb-10.3-serg-MDEV-24176 branch. Three commits there: two cherry-picks from 10.2 (they shouldn't be pushed into 10.3, instead your commit will eventually be rebased on top of them after they're merged into 10.3) and the "work-in-progress" commit that removes init_lex_with_single_table and CONTEXT_ANALYSIS_ONLY_VCOL_EXPR.
I just tried to note this is a different bug (as it reproduces in master branch). And since it is lower priority maybe we push the blocker first?
Okay. But I have a fix for this different bug already, in bb-10.3-serg-MDEV-24176 So, go ahead and push, then I'll merge with 10.2 and push my fix too. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (2)
-
Aleksey Midenkov -
Sergei Golubchik