List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:September 7 2010 3:10pm
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (oystein.grovlen:3232)
Bug#48213
View as plain text  
Hello Oystein,

oystein.grovlen@stripped a écrit, Le 07.09.2010 13:15:
> #At file:///home/oysteing/mysql-new/mysql-next-mr-opt-backporting-2/ based on
> revid:epotemkin@stripped
> 
>  3232 oystein.grovlen@stripped	2010-09-07
>       Bug#48213 Materialized subselect crashes if using GEOMETRY type

> === modified file 'sql/item.cc'
> --- a/sql/item.cc	2010-08-14 09:38:42 +0000
> +++ b/sql/item.cc	2010-09-07 11:15:07 +0000
> @@ -1081,6 +1081,32 @@ int Item::save_in_field_no_warnings(Fiel
>  }
>  
>  
> +/**
> +   Check if an item either is a blob field, or will be represented as a BLOB
> +   field if a field is created based on this item.
> +   
> +   Note: Original non-BLOB items that are longer than

'Note' could be @note

> +   CONVERT_IF_BIGGER_TO_BLOB will be converted to BLOBs when a field
> +   is created for it.
> +
> +   @retval TRUE  If a field based on this item will be a BLOB field,
> +   @retval FALSE Otherwise.
> +*/
> +bool Item::is_blob_field() const
> +{
> +  DBUG_ASSERT(fixed);
> +
> +  enum_field_types type= field_type();
> +  if (type == MYSQL_TYPE_BLOB || type == MYSQL_TYPE_GEOMETRY)
> +    return TRUE;
> +
> +  if (max_length > CONVERT_IF_BIGGER_TO_BLOB)
> +    return TRUE;
> +
> +  return FALSE;

or
return (type == MYSQL_TYPE_BLOB || type == MYSQL_TYPE_GEOMETRY ||
         max_length > CONVERT_IF_BIGGER_TO_BLOB);
at your option.

> === modified file 'sql/item.h'
> --- a/sql/item.h	2010-08-19 12:54:22 +0000
> +++ b/sql/item.h	2010-09-07 11:15:07 +0000
> @@ -1276,6 +1276,15 @@ public:
>      Return TRUE if the item points to a column of an outer-joined table.
>    */
>    virtual bool is_outer_field() const { DBUG_ASSERT(fixed); return FALSE; }
> +
> +  /**
> +     Check if an item either is a blob field, or will be represented as a BLOB
> +     field if a field is created based on this item.
> +
> +     @retval TRUE  If a field based on this item will be a BLOB field,
> +     @retval FALSE Otherwise.
> +  */
> +  bool is_blob_field() const;

The latest coding style says:
"Comments for pure virtual functions, documentation for API usage should 
be placed in front of (member, or non-member) function declarations. 
Description of implementation details, algorithms, anything that does 
not impact usage, should be put in front of implementation. Please try 
to not duplicate information. Make a reference to the declaration from 
the implementation if necessary. If the implementation and usage are too 
interleaved, put a reference from the interface to the implementation, 
and keep the entire comment in a single place. "

Here we have duplication; I suggest to remove the .h comment, move the 
.cc comment to the .h. It's ok to have the @note in the .h comment, as 
this impacts the expectable behaviour of the function.

> === modified file 'sql/item_subselect.cc'
> --- a/sql/item_subselect.cc	2010-08-04 10:34:01 +0000
> +++ b/sql/item_subselect.cc	2010-09-07 11:15:07 +0000
> @@ -1847,52 +1847,42 @@ bool Item_in_subselect::fix_fields(THD *
>      execution.
>  
>    @returns
> -    @retval TRUE  memory allocation error occurred
> +    @retval TRUE  memory allocation error occurred, or was not able to create
> +                  temporary table
>      @retval FALSE an execution method was chosen successfully
>  */
>  
>  bool Item_in_subselect::setup_engine()
>  {
> -  subselect_hash_sj_engine *new_engine= NULL;
> -  bool res= FALSE;
> -
> +  subselect_hash_sj_engine *hash_engine= NULL;

=NULL is not needed.

>    DBUG_ENTER("Item_in_subselect::setup_engine");
>  
>    if (engine->engine_type() == subselect_engine::SINGLE_SELECT_ENGINE)
>    {
>      /* Create/initialize objects in permanent memory. */
> -    subselect_single_select_engine *old_engine;
> -    Query_arena *arena= thd->stmt_arena, backup;
> -
> -    old_engine= (subselect_single_select_engine*) engine;
> -
> +    Query_arena *arena= thd->stmt_arena;
> +    Query_arena backup;
>      if (arena->is_conventional())
>        arena= 0;
>      else
>        thd->set_n_backup_active_arena(arena, &backup);
>  
> -    if (!(new_engine= new subselect_hash_sj_engine(thd, this,
> -                                                   old_engine)) ||
> -        new_engine->init_permanent(unit->get_unit_column_types()))
> +    subselect_single_select_engine *old_engine= 
> +      static_cast<subselect_single_select_engine*>(engine);
> +    if (!(hash_engine= new subselect_hash_sj_engine(thd, this,
> +                                                    old_engine)) ||
> +        hash_engine->init_permanent(unit->get_unit_column_types()))
>      {
> -      Item_subselect::trans_res trans_res;
>        /*
> -        If for some reason we cannot use materialization for this IN predicate,
> -        delete all materialization-related objects, and apply the IN=>EXISTS
> -        transformation.
> +        For some reason we cannot use materialization for this IN predicate.
> +        Delete all materialization-related objects, and return error.
>        */
> -      delete new_engine;
> -      new_engine= NULL;
> -      exec_method= EXEC_UNSPECIFIED;
> -      if (left_expr->cols() == 1)
> -        trans_res= single_value_in_to_exists_transformer(old_engine->join,
> -                                                         &eq_creator);
> -      else
> -        trans_res= row_value_in_to_exists_transformer(old_engine->join);
> -      res= (trans_res != Item_subselect::RES_OK);
> +      delete hash_engine;
> +      if (arena)
> +        thd->restore_active_arena(arena, &backup);
> +      DBUG_RETURN(TRUE);
>      }
> -    if (new_engine)
> -      engine= new_engine;
> +    engine= hash_engine;
>  
>      if (arena)
>        thd->restore_active_arena(arena, &backup);
> @@ -1900,26 +1890,21 @@ bool Item_in_subselect::setup_engine()
>    else
>    {
>      DBUG_ASSERT(engine->engine_type() == subselect_engine::HASH_SJ_ENGINE);
> -    new_engine= (subselect_hash_sj_engine*) engine;
> +    hash_engine= (subselect_hash_sj_engine*) engine;

you may want a static/reinterpret cast here, as you already did in 
previous lines.

>    }
>  
> -  /* Initilizations done in runtime memory, repeated for each execution. */
> -  if (new_engine)
> -  {
> -    /*
> -      Reset the LIMIT 1 set in Item_exists_subselect::fix_length_and_dec.
> -      TODO:
> -      Currently we set the subquery LIMIT to infinity, and this is correct
> -      because we forbid at parse time LIMIT inside IN subqueries (see
> -      Item_in_subselect::test_limit). However, once we allow this, here
> -      we should set the correct limit if given in the query.
> -    */
> -    unit->global_parameters->select_limit= NULL;
> -    if ((res= new_engine->init_runtime()))
> -      DBUG_RETURN(res);
> -  }
> +  /*
> +    Reset the LIMIT 1 set in Item_exists_subselect::fix_length_and_dec.
> +    TODO:
> +    Currently we set the subquery LIMIT to infinity, and this is correct
> +    because we forbid at parse time LIMIT inside IN subqueries (see
> +    Item_in_subselect::test_limit). However, once we allow this, here
> +    we should set the correct limit if given in the query.
> +  */
> +  unit->global_parameters->select_limit= NULL;
>  
> -  DBUG_RETURN(res);
> +  /* Initilizations done in runtime memory, repeated for each execution. */

let's seize this opportunity to fix the old typo Initilizations

> +  DBUG_RETURN(hash_engine->init_runtime());

While this is common in our code, the usual problem with
func1()
{
   ...
   DBUG_RETURN(func2());
}
is that if func2() has DBUG printouts, the dbug trace looks like
 >func1
...
<func1
 >func2
<func2
which does not reflect the fact that func2 was called by func1.
That is why some write
func1()
{
   ...
   const int res= func2();
   DBUG_RETURN(res);
} . This is at your option.

Nice polished patch. Ok to push.
Thread
bzr commit into mysql-next-mr-bugfixing branch (oystein.grovlen:3232)Bug#48213oystein.grovlen7 Sep
  • Re: bzr commit into mysql-next-mr-bugfixing branch (oystein.grovlen:3232)Bug#48213Guilhem Bichot7 Sep