Hi, Oleksandr!
On Oct 09, Oleksandr Byelkin wrote:
> revision-id: 62c35fe93e1 (mariadb-10.2.31-487-g62c35fe93e1)
> parent(s): f584d567dbb
> author: Oleksandr Byelkin <sa...@mariadb.com>
> committer: Oleksandr Byelkin <sa...@mariadb.com>
> timestamp: 2020-10-07 13:43:39 +0200
> message:
>
> MDEV-15703 Crash in EXECUTE IMMEDIATE 'CREATE OR REPLACE TABLE t1 (a INT
> DEFAULT ?)' USING DEFAULT
>
> part 2:
>
> - check that expressions are evaluable for
> making empty row and assigning PS variable
>
> - correctly handling writing to a temporary tabe during multi-update
> by setting associated field
>
> - Item::raise_error_not_evaluable() bakported from 10.4 and made vitrual
>
> - Item::is_evaluable_expression() bakported from 10.4
>
> - Item::check_is_evaluable_expression_or_error() bakported from 10.4
I still don't quite understand all details of your patch. But here're
more questions and comments below:
> diff --git a/sql/field.cc b/sql/field.cc
> index bdaaecc2026..8ce412a207d 100644
> --- a/sql/field.cc
> +++ b/sql/field.cc
> @@ -11183,6 +11205,16 @@ bool Field::save_in_field_default_value(bool
> view_error_processing)
> {
> THD *thd= table->in_use;
>
> + if ( // table is not opened properly
> + table == NULL || table->pos_in_table_list == NULL ||
> + table->pos_in_table_list->select_lex == NULL ||
> + // we are in subquery/derived/CTE
> +
> !table->pos_in_table_list->top_table()->select_lex->is_top_level_select())
> + {
> + DBUG_ASSERT(0); // shoud not happened
> + my_error(ER_INVALID_DEFAULT_PARAM, MYF(0));
> + return false;
> + }
I'd still think it'd be better to do a series of asserts.
a complex condition to check for something that cannot ever happen?
> if (flags & NO_DEFAULT_VALUE_FLAG &&
> real_type() != MYSQL_TYPE_ENUM)
> {
> @@ -11221,13 +11253,30 @@ bool Field::save_in_field_default_value(bool
> view_error_processing)
>
> bool Field::save_in_field_ignore_value(bool view_error_processing)
> {
> - enum_sql_command com= table->in_use->lex->sql_command;
> - // All insert-like commands
> - if (com == SQLCOM_INSERT || com == SQLCOM_REPLACE ||
> - com == SQLCOM_INSERT_SELECT || com == SQLCOM_REPLACE_SELECT ||
> - com == SQLCOM_LOAD)
> - return save_in_field_default_value(view_error_processing);
> - return 0; // ignore
> + if ( // table is not opened properly
> + table == NULL || table->pos_in_table_list == NULL ||
> + table->pos_in_table_list->select_lex == NULL ||
> + // we are in subquery/derived/CTE
> +
> !table->pos_in_table_list->top_table()->select_lex->is_top_level_select())
> + {
> + DBUG_ASSERT(0); // shoud not happened
> + my_error(ER_INVALID_DEFAULT_PARAM, MYF(0));
> + return false;
> + }
ditto
> + switch (find_ignore_reaction(table->in_use))
> + {
> + case IGNORE_MEANS_DEFAULT:
> + return save_in_field_default_value(view_error_processing);
> + case IGNORE_MEANS_FIELD_VALUE:
> + return 0; // ignore
> + default:
> + ; // fall through to error
> + }
> +
> + // unexpected command
> + DBUG_ASSERT(0); // shoud not happened
> + my_error(ER_INVALID_DEFAULT_PARAM, MYF(0));
> + return false;
> }
>
>
> diff --git a/sql/item.cc b/sql/item.cc
> index 994d45a9dc3..401ee5d6be2 100644
> --- a/sql/item.cc
> +++ b/sql/item.cc
> @@ -84,6 +85,17 @@ void item_init(void)
> }
>
>
> +void Item::raise_error_not_evaluable()
> +{
> + String tmp;
> + this->print(&tmp, 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());
> + DBUG_ASSERT(0); // cannot be happen before 10.5
> +}
> +
> +
better remove it, this dead code won't simplify merges. In 10.3
it'll be added with a merge normally.
having check_is_evaluable_expression_or_error() makes sense though,
it is used and copying it from 10.3 will help to merge indeed
or, may be, just backport the patch from 10.3 to 10.2?
> void Item::push_note_converted_to_negative_complement(THD *thd)
> {
> push_warning(thd, Sql_condition::WARN_LEVEL_NOTE, ER_UNKNOWN_ERROR,
> @@ -8968,18 +9084,26 @@ bool Item_default_value::fix_fields(THD *thd, Item
> **items)
> def_field->move_field_offset((my_ptrdiff_t)
> (def_field->table->s->default_values -
> def_field->table->record[0]));
> - set_field(def_field);
> - return FALSE;
> + return def_field;
> +}
>
> -error:
> - context->process_error(thd);
> - return TRUE;
> +bool Item_default_value::associate_with_target_field(THD *thd,
> + Item_field *field)
> +{
> + associated= true;
> + arg= field;
iirc, `arg` used to tell the difference between DEFAULT and DEFAULT(x).
how are you doing it now?
> + return tie_field(thd);
> }
>
> void Item_default_value::cleanup()
> {
> delete cached_field; // Free cached blob data
> cached_field= 0;
> + if (associated)
> + {
> + arg= NULL;
> + associated= false;
> + }
> Item_field::cleanup();
> }
>
> @@ -9102,8 +9226,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);
> + 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)
how is IGNORE with arg!=0 differs from IGNORE with arg==0?
when happens what?
> + {
> + switch (find_ignore_reaction(field->table->in_use))
> + {
> + case IGNORE_MEANS_DEFAULT:
> + DBUG_ASSERT(0); // impossible now, but fully working code if needed
> + /*
> + 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/sp_head.cc b/sql/sp_head.cc
> index 6a650183fb8..41a27a3aaaf 100644
> --- a/sql/sp_head.cc
> +++ b/sql/sp_head.cc
> @@ -418,6 +418,9 @@ sp_eval_expr(THD *thd, Field *result_field, Item
> **expr_item_ptr)
> if (!(expr_item= sp_prepare_func_item(thd, expr_item_ptr)))
> goto error;
>
> + if (expr_item->check_is_evaluable_expression_or_error())
> + goto error;
for CALL(?) ?
> +
> /*
> Set THD flags to emit warnings/errors in case of overflow/type errors
> during saving the item into the field.
> diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> index cc77b58cb3e..b52d6f96a84 100644
> --- a/sql/sql_base.cc
> +++ b/sql/sql_base.cc
> @@ -8345,6 +8346,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;
Is it a good idea to do this check constantly for every user and for every
row in selects, insert/update, etc?
Could it be done inside IGNORE/DEFAULT? For example, by checking
if field->table->tmp_table == INTERNAL_TMP_TABLE.
> +
> if (use_value)
> value->save_val(field);
> else
> diff --git a/sql/unireg.cc b/sql/unireg.cc
> index 083960523c1..91f9073c7ef 100644
> --- a/sql/unireg.cc
> +++ b/sql/unireg.cc
> @@ -1002,6 +1002,12 @@ static bool make_empty_rec(THD *thd, uchar *buff, uint
> table_options,
>
> int res= !expr->fixed && // may be already fixed if ALTER TABLE
> expr->fix_fields(thd, &expr);
> + if (!res && expr->check_is_evaluable_expression_or_error())
is this for CREATE TABLE case?
> + {
> + 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
