Re: bk commit into 6.0 tree (davi:1.2610) BUG#15192

[Konstantin Osipov]

* Davi Arnaut <davi@stripped> [08/03/28 12:50]:
> @@ -4685,6 +4685,7 @@ void Item_func_get_user_var::fix_length_
>      max_length= MAX_BLOB_WIDTH;
>    }
>  
> +  /* Might have been a logic error which is fatal. */
>    if (error)
>      thd->fatal_error();

I think this call is for an error in get_variable(). Please
remove this, instead inspect all allocations in the preceding code
and add FATALERROR flag when necessary.

A "logic" error can indeed happen here, but I see no reason why it
should be fatal.

E.g. var::check() may return an error:

    my_error(ER_WRONG_VALUE_FOR_VAR, MYF(0),
                 name, var->value->str_value.ptr());

But why not allow continue handlers catch it?

Indeed, current continue handlers *will* catch it, since they are
invoked at the moment when the error is pushed into the
diagnostics area. But later on we will have is_fatal_error flag
set.

>  void Item_user_var_as_out_param::set_null_value(CHARSET_INFO* cs)
>  {
> -  if (::update_hash(entry, TRUE, 0, 0, STRING_RESULT, cs,
> -                    DERIVATION_IMPLICIT, 0 /* unsigned_arg */))
> -    current_thd->fatal_error();			// Probably end of memory
> +  ::update_hash(entry, TRUE, 0, 0, STRING_RESULT, cs,
> +                DERIVATION_IMPLICIT, 0 /* unsigned_arg */);
>  }

Good, update_hash will set both error and fatal error if
necessary. Please add a comment clarifying that.

> diff -Nrup a/sql/item_subselect.cc b/sql/item_subselect.cc
> --- a/sql/item_subselect.cc	2008-03-12 05:25:22 -03:00
> +++ b/sql/item_subselect.cc	2008-03-27 23:18:15 -03:00
> @@ -2005,8 +2005,8 @@ subselect_union_engine::subselect_union_
>    :subselect_engine(item_arg, result_arg)
>  {
>    unit= u;
> -  if (!result_arg)				//out of memory
> -    current_thd->fatal_error();
> +  if (!result_arg)
> +    my_error(ER_OUT_OF_RESOURCES, MYF(ME_FATALERROR));
>    unit->item= item_arg;
>  }

It's better to remove this completely:

See sql_alloc_error_handler, which is assigned to
mem_root->error_handler in all mem roots used in sql/.
It already pushes this error.

So please remove this code and inspect all sql/ to ensure that
it uses init_sql_alloc to initialize memory roots, not the simple
init_alloc_root.

> @@ -2045,7 +2045,7 @@ int subselect_single_select_engine::prep
>  		 select_lex->options | SELECT_NO_UNLOCK, result);
>    if (!join || !result)
>    {
> -    thd->fatal_error();				//out of memory
> +    my_error(ER_OUT_OF_RESOURCES, MYF(ME_FATALERROR));
>      return 1;
>    }

Same here. The fatal error should have been set already, and an
error should have been pushed.
Please a comment and/or assert, and remove this code.

> @@ -3943,6 +3943,7 @@ sp_add_to_query_tables(THD *thd, LEX *le
>  
>    if (!(table= (TABLE_LIST *)thd->calloc(sizeof(TABLE_LIST))))
>    {
> +    /* Out of memory error was set. */
>      thd->fatal_error();

This code must be redundant (see my rationale above), please
remove.

> -      db= new_db ? my_strndup(new_db, new_db_len, MYF(MY_WME)) : NULL;
> +      if (new_db)
> +        db= my_strndup(new_db, new_db_len, MYF(MY_WME | ME_FATALERROR));
> +      else
> +        db= NULL;
>      }

OK.


>      db_length= db ? new_db_len : 0;
>      return new_db && !db;
> diff -Nrup a/sql/sql_insert.cc b/sql/sql_insert.cc
> --- a/sql/sql_insert.cc	2008-03-27 15:58:39 -03:00
> +++ b/sql/sql_insert.cc	2008-03-27 23:18:15 -03:00
> @@ -1872,19 +1872,18 @@ bool delayed_get_table(THD *thd, TABLE_L
>      {
>        if (!(di= new Delayed_insert()))
>        {
> -        thd->fatal_error();
> +        my_error(ER_OUT_OF_RESOURCES, MYF(ME_FATALERROR));

Delayed_insert is inherited from ilink, which operator new calls
my_malloc with ME_FAE (fatal if any error). In other words, it
even might exit(1) if out of memory. I think you should not push
an error here, but instead change ilink constructor to call
my_error with ME_FATALERROR.


Thank you for working on this!

-- 
Konstantin