List:Commits« Previous MessageNext Message »
From:Martin Hansson Date:August 11 2010 1:55pm
Subject:Re: bzr commit into mysql-5.1-bugteam branch (Georgi.Kodinov:3479)
Bug#55580
View as plain text  
Hi Joro,
your patch is probably correct but I can't tell without a test case. Is 
there a reason for not having one? You should be able to at least 
simulate an error.

Georgi Kodinov skrev 2010-08-10 11.24:
> #At file:///home/kgeorge/mysql/work/B55580-5.1-bugteam/ based on
> revid:georgi.kodinov@stripped
>
>   3479 Georgi Kodinov	2010-08-10
>        Bug #55580 : segfault in read_view_sees_trx_id
>
>        The server was not checking for errors generated during
>        the execution of Item::val_xxx() methods when copying
>        data to the group, order, or distinct temp table's row.
>        Fixed by extending the copy_funcs() to return an error
>        code and by checking for that error code on the places
>        copy_funcs() is called.
>
>      modified:
>        sql/item_sum.cc
>        sql/sql_select.cc
>        sql/sql_select.h
> === modified file 'sql/item_sum.cc'
> --- a/sql/item_sum.cc	2010-06-10 20:45:22 +0000
> +++ b/sql/item_sum.cc	2010-08-10 09:24:35 +0000
> @@ -2556,7 +2556,8 @@ bool Item_sum_count_distinct::add()
>     if (always_null)
>       return 0;
>     copy_fields(tmp_table_param);
> -  copy_funcs(tmp_table_param->items_to_copy);
> +  if (copy_funcs(tmp_table_param->items_to_copy, table->in_use))
> +    return TRUE;
>
>     for (Field **field=table->field ; *field ; field++)
>       if ((*field)->is_real_null(0))
> @@ -3128,7 +3129,8 @@ bool Item_func_group_concat::add()
>     if (always_null)
>       return 0;
>     copy_fields(tmp_table_param);
> -  copy_funcs(tmp_table_param->items_to_copy);
> +  if (copy_funcs(tmp_table_param->items_to_copy, table->in_use))
> +    return TRUE;
>
>     for (uint i= 0; i<  arg_count_field; i++)
>     {
>
> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2010-07-19 18:34:28 +0000
> +++ b/sql/sql_select.cc	2010-08-10 09:24:35 +0000
> @@ -12485,7 +12485,9 @@ end_write(JOIN *join, JOIN_TAB *join_tab
>     if (!end_of_records)
>     {
>       copy_fields(&join->tmp_table_param);
> -    copy_funcs(join->tmp_table_param.items_to_copy);
> +    if (copy_funcs(join->tmp_table_param.items_to_copy, join->thd))
> +      DBUG_RETURN(NESTED_LOOP_ERROR);           /* purecov: inspected */
> +
>   #ifdef TO_BE_DELETED
>       if (!table->uniques)			// If not unique handling
>       {
> @@ -12591,7 +12593,8 @@ end_update(JOIN *join, JOIN_TAB *join_ta
>         memcpy(table->record[0]+key_part->offset, group->buff, 1);
>     }
>     init_tmptable_sum_functions(join->sum_funcs);
> -  copy_funcs(join->tmp_table_param.items_to_copy);
> +  if (copy_funcs(join->tmp_table_param.items_to_copy, join->thd))
> +    DBUG_RETURN(NESTED_LOOP_ERROR);           /* purecov: inspected */
>     if ((error=table->file->ha_write_row(table->record[0])))
>     {
>       if (create_myisam_from_heap(join->thd, table,&join->tmp_table_param,
> @@ -12626,7 +12629,8 @@ end_unique_update(JOIN *join, JOIN_TAB *
>
>     init_tmptable_sum_functions(join->sum_funcs);
>     copy_fields(&join->tmp_table_param);		// Groups are copied twice.
> -  copy_funcs(join->tmp_table_param.items_to_copy);
> +  if (copy_funcs(join->tmp_table_param.items_to_copy, join->thd))
> +    DBUG_RETURN(NESTED_LOOP_ERROR);           /* purecov: inspected */
>
>     if (!(error=table->file->ha_write_row(table->record[0])))
>       join->send_records++;			// New group
> @@ -12713,7 +12717,8 @@ end_write_group(JOIN *join, JOIN_TAB *jo
>       if (idx<  (int) join->send_group_parts)
>       {
>         copy_fields(&join->tmp_table_param);
> -      copy_funcs(join->tmp_table_param.items_to_copy);
> +      if (copy_funcs(join->tmp_table_param.items_to_copy, join->thd))
> +	DBUG_RETURN(NESTED_LOOP_ERROR);
>         if (init_sum_functions(join->sum_funcs, join->sum_funcs_end[idx+1]))
>   	DBUG_RETURN(NESTED_LOOP_ERROR);
>         if (join->procedure)
> @@ -15773,14 +15778,29 @@ update_sum_func(Item_sum **func_ptr)
>     return 0;
>   }
>
> -/** Copy result of functions to record in tmp_table. */
> +/**
> +  Copy result of functions to record in tmp_table.
>
> -void
> -copy_funcs(Item **func_ptr)
> +  Uses the thread pointer to check for errors in
> +  some of the val_xxx() methods called by the
> +  save_in_result_field() function.
> +  TODO: make the Item::val_xxx() return error code
> +
> +  @param func_ptr  list of the function Items to copy to the tmp table
>    
Should probably be called array, not list.
> +  @param thd       pointer to the current thread for error checking
> +  @retval
> +    0  if OK
> +  @retval
> +    1 on error
>    
The data type is bool,  so the result should be true or false, not 1 or 0.
> +*/
> +
> +bool
> +copy_funcs(Item **func_ptr, THD *thd)
>    
You should be able to take a const pointer to thd.
>   {
>     Item *func;
>     for (; (func = *func_ptr) ; func_ptr++)
>       func->save_in_result_field(1);
> +  return (thd->is_error());
>   }
>    
Don't you think it's better to exit as soon as you see the error happen?
>
>
>
> === modified file 'sql/sql_select.h'
> --- a/sql/sql_select.h	2010-02-26 13:16:46 +0000
> +++ b/sql/sql_select.h	2010-08-10 09:24:35 +0000
> @@ -601,7 +601,7 @@ bool setup_copy_fields(THD *thd, TMP_TAB
>   		       List<Item>  &new_list1, List<Item>  &new_list2,
>   		       uint elements, List<Item>  &fields);
>   void copy_fields(TMP_TABLE_PARAM *param);
> -void copy_funcs(Item **func_ptr);
> +bool copy_funcs(Item **func_ptr, THD *thd);
>   bool create_myisam_from_heap(THD *thd, TABLE *table, TMP_TABLE_PARAM *param,
>   			     int error, bool ignore_last_dupp_error);
>   uint find_shortest_key(TABLE *table, const key_map *usable_keys);
>
>    
>
>
>    
Best Regards

Martin Hansson


Thread
bzr commit into mysql-5.1-bugteam branch (Georgi.Kodinov:3479) Bug#55580Georgi Kodinov10 Aug
  • Re: bzr commit into mysql-5.1-bugteam branch (Georgi.Kodinov:3479)Bug#55580Martin Hansson11 Aug