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.
