Jacob Mathew (Inactive) added a comment - 2017-02-14 01:49 I have completed my changes to fix the bug. The changes are ready to be reviewed.

Jacob Mathew (Inactive) added a comment - 2017-02-14 18:06 - edited Hi Sergei, Please review my changes to fix CREATE TABLE bug MDEV-10355 . Thanks, Jacob commit cd53525d7c97a7c2b69873fbbd28982844e3cae3 Author: Jacob Mathew <jacob.mathew@mariadb.com> Date: Fri Feb 10 13:41:34 2017 -0800   Changes to fix CREATE TABLE bug MDEV_10355.   diff --git a/sql/item.cc b/sql/item.cc index 369b2df2675..dd2c4fd75a5 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -9478,7 +9478,7 @@ Item *Item_cache_int::convert_to_basic_const_item(THD *thd) Item_cache_temporal::Item_cache_temporal(THD *thd, enum_field_types field_type_arg): - Item_cache_int(thd, field_type_arg) + Item_cache_int(thd, field_type_arg), cached_temporal_item_name(NULL) { if (mysql_type_to_time_type(Item_cache_temporal::field_type()) == MYSQL_TIMESTAMP_ERROR) @@ -9614,11 +9614,29 @@ int Item_cache_temporal::save_in_field(Field *field, bool no_conversions) void Item_cache_temporal::store_packed(longlong val_arg, Item *example_arg) { - /* An explicit values is given, save it. */ + /* An explicit value is given, save it. */ store(example_arg); value_cached= true; value= val_arg; null_value= false; + + switch (example_arg->type()) + { + case FUNC_ITEM: + { + // Instead of "cache", use the actual function name, + // which is more meaningful to the end user + const char *func_name = ( ( Item_func* ) example_arg )->func_name(); + char *ptr = ( char* ) current_thd->alloc( strlen( func_name ) + 3 ); + + if ( ptr ) + { + strxmov( ptr, func_name, "()", NullS ); + } + + cached_temporal_item_name = ptr; + } + } } diff --git a/sql/item.h b/sql/item.h index bb2f05b4c6d..72b48b5e421 100644 --- a/sql/item.h +++ b/sql/item.h @@ -5436,6 +5436,7 @@ class Item_cache: public Item_basic_constant, static Item_cache* get_cache(THD *thd, const Item *item); static Item_cache* get_cache(THD *thd, const Item* item, const Item_result type); + virtual const char* cached_item_name() const { return "cache"; } virtual void keep_array() {} virtual void print(String *str, enum_query_type query_type); bool eq_def(const Field *field) @@ -5448,7 +5449,7 @@ class Item_cache: public Item_basic_constant, } bool check_vcol_func_processor(void *arg) { - return mark_unsupported_function("cache", arg, VCOL_IMPOSSIBLE); + return mark_unsupported_function(cached_item_name(), arg, VCOL_IMPOSSIBLE); } /** Check if saved item has a non-NULL value. @@ -5529,6 +5530,7 @@ class Item_cache_int: public Item_cache class Item_cache_temporal: public Item_cache_int { + const char *cached_temporal_item_name; public: Item_cache_temporal(THD *thd, enum_field_types field_type_arg); String* val_str(String *str); @@ -5551,6 +5553,8 @@ class Item_cache_temporal: public Item_cache_int Item *convert_to_basic_const_item(THD *thd); Item *get_copy(THD *thd, MEM_ROOT *mem_root) { return get_item_copy<Item_cache_temporal>(thd, mem_root, this); } + const char *cached_item_name() const + { return cached_temporal_item_name ? cached_temporal_item_name : "cache"; } }; diff --git a/sql/table.cc b/sql/table.cc index 4242b870d55..10f5388e380 100644 --- a/sql/table.cc +++ b/sql/table.cc @@ -48,7 +48,7 @@ #define MYSQL57_GCOL_HEADER_SIZE 4 static Virtual_column_info * unpack_vcol_info_from_frm(THD *, MEM_ROOT *, - TABLE *, String *, Virtual_column_info **, bool *); + TABLE *, String *, Virtual_column_info **, uint, bool *); static bool check_vcol_forward_refs(Field *, Virtual_column_info *); /* INFORMATION_SCHEMA name */ @@ -1065,24 +1065,24 @@ bool parse_vcol_defs(THD *thd, MEM_ROOT *mem_root, TABLE *table, case VCOL_GENERATED_VIRTUAL: case VCOL_GENERATED_STORED: vcol= unpack_vcol_info_from_frm(thd, mem_root, table, &expr_str, - &((*field_ptr)->vcol_info), error_reported); + &((*field_ptr)->vcol_info), type, error_reported); *(vfield_ptr++)= *field_ptr; break; case VCOL_DEFAULT: vcol= unpack_vcol_info_from_frm(thd, mem_root, table, &expr_str, &((*field_ptr)->default_value), - error_reported); + type, error_reported); *(dfield_ptr++)= *field_ptr; break; case VCOL_CHECK_FIELD: vcol= unpack_vcol_info_from_frm(thd, mem_root, table, &expr_str, &((*field_ptr)->check_constraint), - error_reported); + type, error_reported); *check_constraint_ptr++= (*field_ptr)->check_constraint; break; case VCOL_CHECK_TABLE: vcol= unpack_vcol_info_from_frm(thd, mem_root, table, &expr_str, - check_constraint_ptr, error_reported); + check_constraint_ptr, type, error_reported); check_constraint_ptr++; break; } @@ -1103,7 +1103,7 @@ bool parse_vcol_defs(THD *thd, MEM_ROOT *mem_root, TABLE *table, expr_str.append(')'); vcol= unpack_vcol_info_from_frm(thd, mem_root, table, &expr_str, &((*field_ptr)->default_value), - error_reported); + VCOL_DEFAULT, error_reported); *(dfield_ptr++)= *field_ptr; if (!field->default_value->expr) goto end; @@ -2774,7 +2774,7 @@ bool fix_session_vcol_expr_for_read(THD *thd, Field *field, */ static bool fix_and_check_vcol_expr(THD *thd, TABLE *table, - Virtual_column_info *vcol) + Virtual_column_info *vcol, uint vcol_type) { Item* func_expr= vcol->expr; DBUG_ENTER("fix_and_check_vcol_expr"); @@ -2809,9 +2809,14 @@ static bool fix_and_check_vcol_expr(THD *thd, TABLE *table, int error= func_expr->walk(&Item::check_vcol_func_processor, 0, &res); if (error || (res.errors & VCOL_IMPOSSIBLE)) - { // this can only happen if the frm was corrupted + { + // This can legitimately occur if the column is virtual or has a + // default value that is cached. When the statement is originally + // parsed, the parser needs to allow cached values in order to + // handle CREATE with a SELECT, so the parser would not have + // encountered this error. my_error(ER_VIRTUAL_COLUMN_FUNCTION_IS_NOT_ALLOWED, MYF(0), res.name, - "???", "?????"); + vcol_type_name((enum_vcol_info_type) vcol_type), vcol->name.str); DBUG_RETURN(1); } vcol->flags= res.errors; @@ -2864,7 +2869,7 @@ static bool fix_and_check_vcol_expr(THD *thd, TABLE *table, static Virtual_column_info * unpack_vcol_info_from_frm(THD *thd, MEM_ROOT *mem_root, TABLE *table, String *expr_str, Virtual_column_info **vcol_ptr, - bool *error_reported) + uint vcol_type, bool *error_reported) { Create_field vcol_storage; // placeholder for vcol_info Parser_state parser_state; @@ -2892,7 +2897,7 @@ unpack_vcol_info_from_frm(THD *thd, MEM_ROOT *mem_root, TABLE *table, 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 (!fix_and_check_vcol_expr(thd, table, vcol_storage.vcol_info, vcol_type)) { *vcol_ptr= vcol_info= vcol_storage.vcol_info; // Expression ok DBUG_ASSERT(vcol_info->expr);

Jacob Mathew (Inactive) added a comment - 2017-03-29 02:12 In this commit, I have corrected the original problem and some others that I discovered afterwards: https://github.com/MariaDB/server/commit/b4a223e338152c8764c980aeae59fb927eea1b0b

Jacob Mathew (Inactive) added a comment - 2017-03-29 02:15 Hi Sergei, I have squashed all of my changes to fix this bug MDEV-10355 into a single commit b4a223e. Please review. Thanks, Jacob

Jacob Mathew (Inactive) added a comment - 2017-04-19 04:57 I have merged the final version of the fix, committed in https://github.com/MariaDB/server/commit/0b52b28b91eac1018d865f2f918b83c416565f2f , into the 10.2 branch.