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