Hi, Oleksandr!
On Jul 29, Oleksandr Byelkin wrote:
> revision-id: bc73b455ba2 (mariadb-10.2.31-318-gbc73b455ba2)
> parent(s): 7311586ca25
> author: Oleksandr Byelkin <sa...@mariadb.com>
> committer: Oleksandr Byelkin <sa...@mariadb.com>
> timestamp: 2020-07-17 13:47:26 +0200
> message:
>
> MDEV-15703 Crash in EXECUTE IMMEDIATE 'CREATE OR REPLACE TABLE t1 (a INT
> DEFAULT ?)' USING DEFAULT
>
> part 2:
...
> diff --git a/sql/field.h b/sql/field.h
> index d5e2cb25788..2755f5f687c 100644
> --- a/sql/field.h
> +++ b/sql/field.h
> @@ -51,6 +51,16 @@ enum enum_check_fields
> CHECK_FIELD_ERROR_FOR_NULL
> };
>
> +
> +enum ignore_value_reaction
> +{
> + IGNORE_MEANS_ERROR,
> + IGNORE_MEANS_DEFAULT,
> + IGNORE_MEANS_FIELD_VALUE
I thought you'll have IGNORE_MEANS_IGNORE :)
> +};
> +
> +ignore_value_reaction find_ignore_reaction(THD *thd);
> +
> /*
> Common declarations for Field and Item
> */
> diff --git a/sql/item.h b/sql/item.h
> index 7338c8be47b..dabf237712b 100644
> --- a/sql/item.h
> +++ b/sql/item.h
> @@ -1404,6 +1424,16 @@ class Item: public Value_source,
> LOWEST_PRECEDENCE);
> }
> virtual void print(String *str, enum_query_type query_type);
> +
> + class Print: public String
> + {
> + public:
> + Print(Item *item, enum_query_type type)
> + {
> + item->print(this, type);
> + }
no, I don't think this should be part of the bugfix.
> + };
> +
> void print_item_w_name(String *str, enum_query_type query_type);
> void print_value(String *str);
>
> @@ -2077,6 +2107,15 @@ class Item: public Value_source,
> {
> marker &= ~EXTRACTION_MASK;
> }
> +
> + virtual bool associate_with_target_field(THD *thd __attribute__((unused)),
> + Item_field *field
> + __attribute__((unused)))
> + {
> + DBUG_ASSERT(fixed);
> + DBUG_ASSERT(fixed);
you want to be double-sure? :)
> + return false;
> + }
> };
>
>
> diff --git a/sql/item.cc b/sql/item.cc
> index 382eb3ac47a..d11017f3ca6 100644
> --- a/sql/item.cc
> +++ b/sql/item.cc
> @@ -84,6 +85,15 @@ void item_init(void)
> }
>
>
> +void Item::raise_error_not_evaluable()
> +{
> + Item::Print tmp(this, QT_ORDINARY);
> + // TODO-10.5: add an error message to errmsg-utf8.txt
> + my_printf_error(ER_UNKNOWN_ERROR,
> + "'%s' is not allowed in this context", MYF(0), tmp.ptr());
I don't see any tests for this error
> +}
> +
> +
> void Item::push_note_converted_to_negative_complement(THD *thd)
> {
> push_warning(thd, Sql_condition::WARN_LEVEL_NOTE, ER_UNKNOWN_ERROR,
> @@ -8945,6 +9022,14 @@ bool Item_default_value::fix_fields(THD *thd, Item
> **items)
> return FALSE;
> }
>
> + return tie_field(thd);
> +}
> +
> +
comment? what does tie_field do?
> +bool Item_default_value::tie_field(THD *thd)
> +{
> + Item *real_arg;
> + Item_field *field_arg;
> /*
> DEFAULT() do not need table field so should not ask handler to bring
> field value (mark column for read)
> @@ -9131,8 +9241,54 @@ void Item_ignore_value::print(String *str,
> enum_query_type query_type)
> str->append(STRING_WITH_LEN("ignore"));
> }
>
> +
> +bool Item_ignore_value::associate_with_target_field(THD *thd,
> + Item_field *field)
> +{
> + bitmap_set_bit(field->field->table->read_set, field->field->field_index);
may be, remove it from the write_set instead?
> + return Item_default_value::associate_with_target_field(thd, field);
> +}
> +
> +
> int Item_ignore_value::save_in_field(Field *field_arg, bool no_conversions)
> {
> + if (arg)
> + {
> + switch (find_ignore_reaction(field->table->in_use))
> + {
> + case IGNORE_MEANS_DEFAULT:
> + DBUG_ASSERT(0); // impossible now, but fully working code if needed
Impossible? One cannot use IGNORE in INSERT?
Not even in EXECUTE IMMEDIATE "INSERT ... " USING IGNORE?
> + /*
> + save default value
> + Note: (((Field*)(arg()->real_item()) is Item_field* (checked in
> + tie_field().
> + */
> + if ((((Item_field*)(arg->real_item()))->field->flags &
> + NO_DEFAULT_VALUE_FLAG))
> + {
> + my_error(ER_NO_DEFAULT_FOR_FIELD, MYF(0),
> + ((Item_field*)(arg->real_item()))->field->field_name);
> + return true;
> + }
> + calculate();
> + return Item_field::save_in_field(field_arg, no_conversions);
> + case IGNORE_MEANS_FIELD_VALUE:
> + // checked on tie_field
> + DBUG_ASSERT(arg->real_item()->type() == FIELD_ITEM);
> + /*
> + save original field (Item::save_in_field is not applicable because
> + result_field for the referenced field set to this temporary table).
> + */
> + arg->save_val(field_arg);
> + return false;
> + default:
> + ; // fall through to error
> + }
> + DBUG_ASSERT(0); //impossible
> + my_error(ER_INVALID_DEFAULT_PARAM, MYF(0));
> + return true;
> + }
> +
> return field_arg->save_in_field_ignore_value(context->error_processor ==
> &view_error_processor);
> }
> diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> index 436f753557e..4dbda30708a 100644
> --- a/sql/sql_base.cc
> +++ b/sql/sql_base.cc
> @@ -8340,6 +8341,9 @@ fill_record(THD *thd, TABLE *table, Field **ptr,
> List<Item> &values,
> field->field_name, table->s->table_name.str);
> }
>
> + if(evaluable && value->check_is_evaluable_expression_or_error())
> + goto err;
this looks like the wrong way of doing it.
whether an item is "evaluable" or not is metadata. It should be checked once
before the statement execution
> +
> if (use_value)
> value->save_val(field);
> else
> diff --git a/sql/sql_update.cc b/sql/sql_update.cc
> index 65a828147ae..26ee427e3e7 100644
> --- a/sql/sql_update.cc
> +++ b/sql/sql_update.cc
> @@ -1796,6 +1796,9 @@ int multi_update::prepare(List<Item> ¬_used_values,
> while ((item= (Item_field *) field_it++))
> {
> Item *value= value_it++;
> + if (value->associate_with_target_field(thd, item))
> + DBUG_RETURN(1);
what happens for single table updates?
> +
> uint offset= item->field->table->pos_in_table_list->shared;
> fields_for_table[offset]->push_back(item, thd->mem_root);
> values_for_table[offset]->push_back(value, thd->mem_root);
> diff --git a/sql/unireg.cc b/sql/unireg.cc
> index 083960523c1..a3bfc678e07 100644
> --- a/sql/unireg.cc
> +++ b/sql/unireg.cc
> @@ -1000,8 +1000,14 @@ static bool make_empty_rec(THD *thd, uchar *buff, uint
> table_options,
> {
> Item *expr= field->default_value->expr;
>
> - int res= !expr->fixed && // may be already fixed if ALTER TABLE
> - expr->fix_fields(thd, &expr);
> + int res= MY_TEST(!expr->fixed && // may be already fixed if ALTER TABLE
> + expr->fix_fields(thd, &expr));
eh? what's the point of this change?
> + if (!res && expr->check_is_evaluable_expression_or_error())
> + {
> + error= 1;
> + delete regfield; //To avoid memory leak
> + goto err;
> + }
> if (!res)
> res= expr->save_in_field(regfield, 1);
> if (!res && (field->flags & BLOB_FLAG))
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
