Hi, Sachin! On Jan 29, Sachin Setiya wrote:

commit 5e2f5aa78a5f53b000a2b9c6989c9ed7076e2527 Author: Sachin Setiya <sachin.setiya@maridb.com>

Note the typo in your email ^^^^^

Date: Mon Jan 29 17:09:32 2018 +0530

MDEV-14785 SYSTEM_INVISIBLE behaviour not consistent

TODO Commit message

diff --git a/sql/item.h b/sql/item.h index d178d9f..0072dfd 100644 --- a/sql/item.h +++ b/sql/item.h @@ -2907,11 +2907,17 @@ class Item_field :public Item_ident bool check_vcol_func_processor(void *arg) { context= 0; - if (field && (field->unireg_check == Field::NEXT_NUMBER)) + if (field) { - // Auto increment fields are unsupported - return mark_unsupported_function(field_name.str, arg, VCOL_FIELD_REF | VCOL_AUTO_INC); + if (field->unireg_check == Field::NEXT_NUMBER) + // Auto increment fields are unsupported + return mark_unsupported_function(field_name.str, arg, + VCOL_FIELD_REF | VCOL_AUTO_INC); + if (field->invisible > INVISIBLE_USER) + return mark_unsupported_function(field_name.str, arg, + VCOL_FIELD_REF | VCOL_SYSTEM_INVISIBLE);

Why would you do that, instead of hiding system invisible fields in CREATE TABLE? From the user point of view they aren't fields, they don't exist in the table, and the error should be "no such field", not "Function or expression 'on SYSTEM_INVISIBLE column' cannot be used"

} + return mark_unsupported_function(field_name.str, arg, VCOL_FIELD_REF); } bool set_fields_as_dependent_processor(void *arg) diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 17ff7eb..89ba2ed 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -8128,6 +8128,11 @@ fill_record(THD *thd, TABLE *table_arg, List<Item> &fields, List<Item> &values, value=v++; DBUG_ASSERT(value); Field *rfield= field->field; + if (rfield->invisible == INVISIBLE_SYSTEM) + { + my_error(ER_BAD_FIELD_ERROR, MYF(0), rfield->field_name.str, thd->where);

The error is correct. But do you need to do it in fill_record()? I'd thought you can do it when the statement is prepared, once, not in fill_record() which is called for every row, many times per statement.

+ goto err; + } TABLE* table= rfield->table; if (table->next_number_field && rfield->field_index == table->next_number_field->field_index) diff --git a/sql/sql_select.cc b/sql/sql_select.cc index d2fd2ce..e4971d7 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -16639,6 +16639,7 @@ Field *create_tmp_field_from_field(THD *thd, Field *org_field, new_field->next_equal_field= NULL; new_field->option_list= NULL; new_field->option_struct= NULL; + new_field->invisible= org_field->invisible;

What is it for? It'd mean that temporary tables can have invisible fields. What bugs does this change fix?

} return new_field; } diff --git a/sql/sql_update.cc b/sql/sql_update.cc index 7b4938f..7bdedc7 100644 --- a/sql/sql_update.cc +++ b/sql/sql_update.cc @@ -1497,6 +1497,9 @@ int mysql_multi_update_prepare(THD *thd) TABLE_LIST *table_list= lex->query_tables; TABLE_LIST *tl; List<Item> *fields= &lex->select_lex.item_list; + List_iterator<Item> it(*fields); + Item *item; + Item_field *field; table_map tables_for_update; bool update_view= 0; /* @@ -1564,7 +1567,21 @@ int mysql_multi_update_prepare(THD *thd) { DBUG_RETURN(TRUE); } - + //Check if we got any INVISIBLE_SYSTEM field + while ((item= it++)) + { + if (Item::FIELD_ITEM == item->type()) + { + field= (Item_field *)item; + if (field->field->invisible == INVISIBLE_SYSTEM) + { + my_error(ER_BAD_FIELD_ERROR,MYF(0), + field->field->field_name.str, + field->field->table->s->table_name.str); + DBUG_RETURN(TRUE); + } + } + }

Dunno. What if I do UPDATE ... SET normal_field = row_start you'll have an invisible field, but it's not updated, so should be ok, right? May be something like if (field->field->invisible == INVISIBLE_SYSTEM && bitmap_is_set(field->table->write_set, field->field_index)) { ... my_error } And why here? I'd think this belongs into check_fields() function.

thd->table_map_for_update= tables_for_update= get_table_map(fields);

Regards, Sergei Chief Architect MariaDB and security@mariadb.org