List:Commits« Previous MessageNext Message »
From:Roy Lyseng Date:September 1 2010 8:01am
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (oystein.grovlen:3232)
Bug#48213
View as plain text  
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".
 >
 >       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.

 > === 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.

 > +  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 :)


 >
 > === 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.

Thanks,
Roy
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