List:Commits« Previous MessageNext Message »
From:Øystein Grøvlen Date:September 2 2010 7:35am
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (oystein.grovlen:3232)
Bug#48213
View as plain text  
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
Thread
bzr commit into mysql-next-mr-bugfixing branch (oystein.grovlen:3232)Bug#48213oystein.grovlen29 Aug
  • Re: bzr commit into mysql-next-mr-bugfixing branch (oystein.grovlen:3232)Bug#48213Guilhem Bichot31 Aug
    • Re: bzr commit into mysql-next-mr-bugfixing branch (oystein.grovlen:3232)Bug#48213Øystein Grøvlen1 Sep
      • Re: bzr commit into mysql-next-mr-bugfixing branch (oystein.grovlen:3232)Bug#48213Guilhem Bichot1 Sep
Re: bzr commit into mysql-next-mr-bugfixing branch (oystein.grovlen:3232)Bug#48213Roy Lyseng1 Sep
  • Re: bzr commit into mysql-next-mr-bugfixing branch (oystein.grovlen:3232)Bug#48213Øystein Grøvlen2 Sep