On 09/ 1/10 10:01 AM, Roy Lyseng wrote:
> Hi Øystein,
>
> thanks for fixing this.
>
> Some small comments in addition to Guilhem's comments.
>
> On 29.08.10 08.31, oystein.grovlen@stripped wrote:
> > #At file:///home/oysteing/mysql-new/mysql-next-mr-opt-backporting-2/
> based on revid:epotemkin@stripped
> >
> > 3232 oystein.grovlen@stripped 2010-08-29
> > Bug#48213 Materialized subselect crashes if using GEOMETRY type
> >
> > The problem occurred because during execution of MaterializationLookup
> > strategy for semi-join, a materialized table was created which
> > contained a GEOMETRY column, a specialized BLOB column. This caused
> > an segmentation fault because such tables will have extra columns, and
> > the semi-join code was not prepared for that.
>
> The terms used are "MaterializeLookup" and "MaterializeScan".
OK. I will make sure to use those terms.
> >
> > The solution is to disable MaterializationLookup strategy when
> > Blob/Geometry columns would need to be materialized. Blob columns
> > cannot be used for index look-up anyway, so it does not makes sense to
> > use MaterializationLookup.
> >
> > This fix also implies that it is detected earlier that subquery
> > materialization can not be used either. This means that code to
> > revert the decision to use subquery materialization is no longer
> > needed, and this has been removed.
> >
> > In addition to adding a new test case for this bug, some existing
> > tests for subqueries involving BLOB columns have been moved from
> > subquery_mat.inc to subquery_sj.inc. This way, these tests will also
> > be run for more cases involving semijoin materialization. Also, the
> > extended query plans for many of these tests had to be updated since
> > it is now reflected that in->exist optimization will be performed for
> > the involved queries.
> > @ mysql-test/include/subquery_mat.inc
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> > @ mysql-test/include/subquery_sj.inc
> > Added new test case for Bug#48213
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> > @ mysql-test/r/subquery_mat.result
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> > @ mysql-test/r/subquery_mat_all.result
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> > @ mysql-test/r/subquery_mat_none.result
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> > @ mysql-test/r/subquery_sj_all.result
> > Added new test case for Bug#48213
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> > @ mysql-test/r/subquery_sj_all_jcl6.result
> > Added new test case for Bug#48213
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> > @ mysql-test/r/subquery_sj_all_jcl7.result
> > Added new test case for Bug#48213
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> > @ mysql-test/r/subquery_sj_dupsweed.result
> > Added new test case for Bug#48213
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> > @ mysql-test/r/subquery_sj_dupsweed_jcl6.result
> > Added new test case for Bug#48213
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> > @ mysql-test/r/subquery_sj_dupsweed_jcl7.result
> > Added new test case for Bug#48213
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> > @ mysql-test/r/subquery_sj_firstmatch.result
> > Added new test case for Bug#48213
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> > @ mysql-test/r/subquery_sj_firstmatch_jcl6.result
> > Added new test case for Bug#48213
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> > @ mysql-test/r/subquery_sj_firstmatch_jcl7.result
> > Added new test case for Bug#48213
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> > @ mysql-test/r/subquery_sj_loosescan.result
> > Added new test case for Bug#48213
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> > @ mysql-test/r/subquery_sj_loosescan_jcl6.result
> > Added new test case for Bug#48213
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> > @ mysql-test/r/subquery_sj_loosescan_jcl7.result
> > Added new test case for Bug#48213
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> > @ mysql-test/r/subquery_sj_mat.result
> > Added new test case for Bug#48213
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> > @ mysql-test/r/subquery_sj_mat_jcl6.result
> > Added new test case for Bug#48213
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> > @ mysql-test/r/subquery_sj_mat_jcl7.result
> > Added new test case for Bug#48213
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> > @ mysql-test/r/subquery_sj_mat_nosj.result
> > Added new test case for Bug#48213
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> Some
> > extended query plans for the moved tests had to be updated since
> > it is now reflected that in->exist optimization will be performed for
> > the involved queries.
> > @ mysql-test/r/subquery_sj_none.result
> > Added new test case for Bug#48213
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> > @ mysql-test/r/subquery_sj_none_jcl6.result
> > Added new test case for Bug#48213
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> > @ mysql-test/r/subquery_sj_none_jcl7.result
> > Added new test case for Bug#48213
> > Moved tests for subqueries involving BLOB columns from
> subquery_mat.inc to
> > subquery_sj.inc to get better coverage for semijoin materialization.
> > @ sql/item.cc
> > Added method Item::allows_materialization_lookup() that checks if
> this item
> > allows MaterializationLookup strategy to be used for semijoin. That is,
> > if BLOB type is involved, MaterializationLookup cannot be used.
> > @ sql/item.h
> > Added method Item::allows_materialization_lookup() that checks if
> this item
> > allows MaterializationLookup strategy to be used for semijoin.
> > @ sql/item_subselect.cc
> > Removed code to revert the decision to use subquery materialization.
> > It will now be detected earlier that subquery materialization is not
> possible,
> > so this code is no longer needed.
> > @ sql/sql_select.cc
> > Added check for whether column types of subquery allow index to be
> created
> > on materialized table. If not, subquery materialization and
> > MaterializationLookup strategy for semijoin cannot be used.
> > @ sql/table.h
> > Added Semijoin_mat_optimize::lookup_allowed to record whether it is
> possible
> > to use MaterializationLookup strategy for this semijoin materialization.
> >
> > modified:
> > mysql-test/include/subquery_mat.inc
> > mysql-test/include/subquery_sj.inc
> > mysql-test/r/subquery_mat.result
> > mysql-test/r/subquery_mat_all.result
> > mysql-test/r/subquery_mat_none.result
> > mysql-test/r/subquery_sj_all.result
> > mysql-test/r/subquery_sj_all_jcl6.result
> > mysql-test/r/subquery_sj_all_jcl7.result
> > mysql-test/r/subquery_sj_dupsweed.result
> > mysql-test/r/subquery_sj_dupsweed_jcl6.result
> > mysql-test/r/subquery_sj_dupsweed_jcl7.result
> > mysql-test/r/subquery_sj_firstmatch.result
> > mysql-test/r/subquery_sj_firstmatch_jcl6.result
> > mysql-test/r/subquery_sj_firstmatch_jcl7.result
> > mysql-test/r/subquery_sj_loosescan.result
> > mysql-test/r/subquery_sj_loosescan_jcl6.result
> > mysql-test/r/subquery_sj_loosescan_jcl7.result
> > mysql-test/r/subquery_sj_mat.result
> > mysql-test/r/subquery_sj_mat_jcl6.result
> > mysql-test/r/subquery_sj_mat_jcl7.result
> > mysql-test/r/subquery_sj_mat_nosj.result
> > mysql-test/r/subquery_sj_none.result
> > mysql-test/r/subquery_sj_none_jcl6.result
> > mysql-test/r/subquery_sj_none_jcl7.result
> > sql/item.cc
> > sql/item.h
> > sql/item_subselect.cc
> > sql/sql_select.cc
> > sql/table.h
> >
> <Lots of test changes clipped>.
>
> Moving the tests should be a separate patch - it is very hard to follow
> what are caused by your changes and what are caused by moving tests.
OK. I can do the test move in a separate commit. There are only minor
changes to results if test cases are not moved. The main difference
comes from executing the tests with more combinations of
optimizer_switch settings.
>
> > === modified file 'sql/item.cc'
> > --- a/sql/item.cc 2010-08-14 09:38:42 +0000
> > +++ b/sql/item.cc 2010-08-29 06:31:42 +0000
> > @@ -1081,6 +1081,27 @@ int Item::save_in_field_no_warnings(Fiel
> > }
> >
> >
> > +/**
> > + @brief Check if item allows MaterializationLookup semijoin strategy
> > +
> > + @details MaterializationLookup strategy requires an index on the
> > + materialized subquery. Indexes are not supported for BLOB and
> > + GEOMETRY columns. Note also that original non-BLOB columns that are
> > + longer than CONVERT_IF_BIGGER_TO_BLOB will be converted to BLOB
> columns.
> > +*/
> > +bool Item::allows_materialization_lookup()
> > +
> > +{
>
> I think that you should add DBUG_ASSERT(fixed); here.
OK. I guess that is because field_type() is not valid before fix_fields
have been called.
>
> > + if (field_type() == MYSQL_TYPE_BLOB || field_type() ==
> MYSQL_TYPE_GEOMETRY)
> > + return FALSE;
> > +
> > + if (max_length > CONVERT_IF_BIGGER_TO_BLOB)
> > + return FALSE;
> > +
> > + return TRUE;
> > +}
> > +
> > +
> >
> /*****************************************************************************
>
> > Item_sp_variable methods
> >
> *****************************************************************************/
>
> >
> > === modified file 'sql/item.h'
> > --- a/sql/item.h 2010-08-19 12:54:22 +0000
> > +++ b/sql/item.h 2010-08-29 06:31:42 +0000
> > @@ -1276,6 +1276,14 @@ 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 item allows MaterializationLookup semijoin strategy
> > +
> > + @retval TRUE If subquery types allow MaterializationLookup.
> > + @retval FALSE Otherwise.
> > + */
> > + bool allows_materialization_lookup();
> > };
> >
> >
> >
> > === modified file 'sql/item_subselect.cc'
> > --- a/sql/item_subselect.cc 2010-08-04 10:34:01 +0000
> > +++ b/sql/item_subselect.cc 2010-08-29 06:31:42 +0000
> > @@ -1847,7 +1847,8 @@ 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
> > */
> >
> > @@ -1875,21 +1876,13 @@ bool Item_in_subselect::setup_engine()
> > old_engine)) ||
> > new_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);
> > + DBUG_RETURN(TRUE);
> > }
> > if (new_engine)
> > engine= new_engine;
>
> new_engine is never NULL here, hence the test can be removed.
>
> AFAIU, you can also eliminate the "if (new_engine)" test around the
> init_runtime() call, and you can eliminate the local variable "res" (do
> DBUG_RETURN(TRUE) and DBUG_RETURN(FALSE) instead of DBUG_RETURN(res).
>
> Nice little cleanup that eliminates two tests and a local variable :)
Good observation! I will make sure to do this.
>
>
> >
> > === modified file 'sql/sql_select.cc'
> > --- a/sql/sql_select.cc 2010-08-19 16:44:09 +0000
> > +++ b/sql/sql_select.cc 2010-08-29 06:31:42 +0000
> > @@ -984,7 +984,10 @@ bool subquery_types_allow_materializatio
> > if (!types_allow_materialization(predicate->left_expr->element_index(i),
> > inner))
> > DBUG_RETURN(FALSE);
> > + if (!inner->allows_materialization_lookup())
> > + DBUG_RETURN(FALSE);
> > }
> > +
> > DBUG_PRINT("info",("subquery_types_allow_materialization: ok,
> allowed"));
> > DBUG_RETURN(TRUE);
> > }
> > @@ -1049,6 +1052,7 @@ bool semijoin_types_allow_materializatio
> > List_iterator<Item> it2(sj_nest->nested_join->sj_inner_exprs);
> >
> > sj_nest->nested_join->sjm.scan_allowed= FALSE;
> > + sj_nest->nested_join->sjm.lookup_allowed= TRUE;
> >
> > bool all_are_fields= TRUE;
> > Item *outer, *inner;
> > @@ -1058,10 +1062,13 @@ bool semijoin_types_allow_materializatio
> > inner->real_item()->type() == Item::FIELD_ITEM);
> > if (!types_allow_materialization(outer, inner))
> > DBUG_RETURN(FALSE);
> > + sj_nest->nested_join->sjm.lookup_allowed&=
> > + inner->allows_materialization_lookup();
> > }
> > sj_nest->nested_join->sjm.scan_allowed= all_are_fields;
> > DBUG_PRINT("info",("semijoin_types_allow_materialization: ok,
> allowed"));
> > - DBUG_RETURN(TRUE);
> > + DBUG_RETURN(sj_nest->nested_join->sjm.scan_allowed ||
> > + sj_nest->nested_join->sjm.lookup_allowed);
> > }
> >
> >
> > @@ -7504,9 +7511,10 @@ semijoin_order_allows_materialization(co
> >
> > /*
> > Must use MaterializeScan strategy if there are outer correlated tables
> > - among the remaining tables, otherwise use MaterializeLookup.
> > + among the remaining tables, otherwise, if possible, use
> MaterializeLookup.
> > */
> > - if (remaining_tables & emb_sj_nest->nested_join->sj_depends_on)
> > + if (remaining_tables & emb_sj_nest->nested_join->sj_depends_on ||
> > + !emb_sj_nest->nested_join->sjm.lookup_allowed)
> > {
> > if (emb_sj_nest->nested_join->sjm.scan_allowed)
> > return SJ_OPT_MATERIALIZE_SCAN;
> >
> > === modified file 'sql/table.h'
> > --- a/sql/table.h 2010-08-23 12:05:47 +0000
> > +++ b/sql/table.h 2010-08-29 06:31:42 +0000
> > @@ -1976,6 +1976,8 @@ struct Semijoin_mat_optimize
> > {
> > /* Optimal join order calculated for inner tables of this semijoin
> op. */
> > struct st_position *positions;
> > + /* True if data types allow the MaterializeLookup semijoin strategy */
> > + bool lookup_allowed;
> > /* True if data types allow the MaterializeScan semijoin strategy */
> > bool scan_allowed;
> > /* Expected #rows in the materialized table */
>
> I am about to propose a refactoring of semijoin strategy that among
> other things will replace scan_allowed (and the new lookup_allowed) with
> an enum/bitmask field. It should be easy to integrate this after your
> patch, though.
OK. I am not sure I see why a bit-mask is an improvement, but that is
another discussion ...
--
Øystein
