List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:April 8 2011 12:38pm
Subject:Re: bzr commit into mysql-trunk branch (roy.lyseng:3358) Bug#11822517
View as plain text  
Hello,

Roy Lyseng a écrit, Le 06.04.2011 13:57:
> #At file:///home/rl136806/mysql/repo/mysql-work5/ based on
> revid:tor.didriksen@stripped
> 
>  3358 Roy Lyseng	2011-04-06
>       Bug#11822517: Refactor call interface of choose_plan()
>       
>       Part 1 - create class Optimize_table_order.
>       
>       This patch creates a class that is used to optimize the join order of
>       tables in a query. The main purpose of the refactoring is to simplify
>       function interfaces and to encapsulate information needed to perform
>       optimization in a dedicated class.

> @@ -1953,8 +2022,7 @@ JOIN::optimize()
>  
>    /* Calculate how to do the join */
>    THD_STAGE_INFO(thd, stage_statistics);
> -  if (make_join_statistics(this, select_lex->leaf_tables, conds, &keyuse) ||
> -      thd->is_fatal_error)

why don't we test is_fatal_error anymore?

> +  if (make_join_statistics(this, select_lex->leaf_tables, conds, &keyuse))
>    {
>      DBUG_PRINT("error",("Error: make_join_statistics() failed"));
>      DBUG_RETURN(1);

> @@ -4636,25 +4705,33 @@ make_join_statistics(JOIN *join, TABLE_L
>    JOIN_TAB *stat_vector[MAX_TABLES+1];
>    DBUG_ENTER("make_join_statistics");
>  
> +  /*
> +    Conceptually, this initialization belongs immediately before the call
> +    to table_order.choose_table_order(), but due to the use of the error label,
> +    it must be placed before all 'goto error' statements.
> +  */
> +  Optimize_table_order table_order(thd, join, NULL);

This early construction feels dangerous, in case "join" is not fully 
initialized at this point.
I think you can move the line above, down to where it's used i.e. at
   if (table_order.choose_table_order() || thd->killed)
     DBUG_RETURN(true);

in this form:

   if (Optimize_table_order table_order(thd, join, 
NULL).choose_table_order() || thd->killed)
     DBUG_RETURN(true);

and I suspect there won't be any compilation error.
If there is one, I suggest wrapping this if() into a scope with {}.

Normally, the object won't be in scope at label "error" so the compiler 
should not say that it's not a problem to jump over it.

  >    if (!(join->positions=
> -        new (join->thd->mem_root) POSITION[table_count+1]))
> -    DBUG_RETURN(TRUE);
> +        new (thd->mem_root) POSITION[table_count+1]))
> +    DBUG_RETURN(true);
>  
>    if (!(join->best_positions=
> -        new (join->thd->mem_root) POSITION[table_count+1]))
> -    DBUG_RETURN(TRUE);
> +        new (thd->mem_root) POSITION[table_count+1]))
> +    DBUG_RETURN(true);
>  
>    join->best_ref=stat_vector;
>  
>    stat_end=stat+table_count;
> -  found_const_table_map= all_table_map=0;
> +  join->found_const_table_map=0;
> +  join->all_table_map= 0;

at your option: both settings above on one line:
join->found_const_table_map= join->all_table_map= 0;

> -static bool optimize_semijoin_nests(JOIN *join, table_map all_table_map)
> +static bool optimize_semijoin_nests(JOIN *join)
>  {
>    DBUG_ENTER("optimize_semijoin_nests");
>    List_iterator<TABLE_LIST> sj_list_it(join->select_lex->sj_nests);
> @@ -5186,9 +5250,9 @@ static bool optimize_semijoin_nests(JOIN
>        */
>        if (semijoin_types_allow_materialization(sj_nest))
>        {
> -        join->emb_sjm_nest= sj_nest;
> -        if (choose_plan(join, all_table_map & ~join->const_table_map))
> -          DBUG_RETURN(TRUE); /* purecov: inspected */
> +        Optimize_table_order table_order(join->thd, join, sj_nest);
> +        if (table_order.choose_table_order())
> +          DBUG_RETURN(true);

can combine both in one:
        if (Optimize_table_order(join->thd, join, 
sj_nest).choose_table_order())
           DBUG_RETURN(true);
The advantage is that this makes a short-lived object, with 
join->emb_sjm_nest back to NULL more quickly.

Or maybe it's a feature to let join->emb_sjm_nest be NULL until the end 
of the iteration, as in the original code? Some code depends on it 
maybe? I let you decide.


>          /*
>            The best plan to run the subquery is now in join->best_positions,
>            save it.

> -static bool
> -choose_plan(JOIN *join, table_map join_tables)
> +bool Optimize_table_order::choose_table_order()
>  {
> -  uint search_depth= join->thd->variables.optimizer_search_depth;
> -  uint prune_level=  join->thd->variables.optimizer_prune_level;
> -  bool straight_join= test(join->select_options & SELECT_STRAIGHT_JOIN);
> -  DBUG_ENTER("choose_plan");
> +  const bool straight_join= test(join->select_options &
> SELECT_STRAIGHT_JOIN);
> +  table_map join_tables;

can move the two variables one above down to where they are about to be 
initialized. So that we don't even compute straight_join if we only have 
constant tables.

> +  DBUG_ENTER("Optimize_table_order::choose_table_order");
> +
> +  /* Are there any tables to optimize? */
> +  if (join->const_tables == join->tables)
> +  {
> +    memcpy(join->best_positions, join->positions,
> +	   sizeof(POSITION) * join->const_tables);
> +    join->best_read= 1.0;
> +    DBUG_RETURN(false);
> +  }

> @@ -7593,14 +7675,11 @@ choose_plan(JOIN *join, table_map join_t
>  
>    if (straight_join)
>    {
> -    optimize_straight_join(join, join_tables);
> +    optimize_straight_join(join_tables);
>    }
>    else
>    {
> -    if (search_depth == 0)
> -      /* Automatically determine a reasonable value for 'search_depth' */
> -      search_depth= determine_search_depth(join);
> -    if (greedy_search(join, join_tables, search_depth, prune_level))
> +    if (greedy_search(join_tables))
>        DBUG_RETURN(TRUE);

while you're at it, could you please change TRUE to true?

> @@ -7980,48 +8056,43 @@ semijoin_order_allows_materialization(co
>      In the future, 'greedy_search' might be extended to support other
>      implementations of 'best_extension', e.g. some simpler quadratic procedure.
>  
> -  @param join             pointer to the structure providing all context info
> -                          for the query
> +  @par
> +    @c search_depth from Optimize_table_order controls the exhaustiveness
> +    of the search, and @c prune_levelcontrols the pruning heuristics that

space after prune_level

> +    should be applied during search.

> === modified file 'sql/sql_select.h'
> --- a/sql/sql_select.h	2011-03-29 07:30:44 +0000
> +++ b/sql/sql_select.h	2011-04-06 11:56:55 +0000
> @@ -1675,11 +1675,10 @@ public:
>    bool     first_record,full_join, no_field_update;
>    bool	   group;          /**< If query contains GROUP BY clause */
>    bool	   do_send_rows;
> -  table_map const_table_map,found_const_table_map;
> -  /*
> -     Bitmap of all inner tables from outer joins
> -  */
> -  table_map outer_join;
> +  table_map all_table_map;   /**< Set of tables contained in query */
> +  table_map const_table_map; /**< Set of tables found to be const */

at your option: this one-line comment can use ///< syntax.

> +  table_map found_const_table_map; /**< Tables that are const and non-empty */
> +  table_map outer_join;      /**< Bitmap of all inner tables from outer joins */
>    /* Number of records produced after join + group operation */
>    ha_rows  send_records;
>    ha_rows found_records,examined_rows,row_limit;
> @@ -1700,20 +1699,15 @@ public:
>  
>  /******* Join optimization state members start *******/
>    /*
> -    pointer - we're doing optimization for a semi-join materialization nest.
> -    NULL    - otherwise
> +    if non-NULL, we are optimizing a materialized semi join nest.

s/if/If

> +    if NULL, we are optimizing a complete join plan.

s/if/If

Ok to push. It's a very clean patch which improves the situation.
Thread
bzr commit into mysql-trunk branch (roy.lyseng:3358) Bug#11822517Roy Lyseng6 Apr
  • Re: bzr commit into mysql-trunk branch (roy.lyseng:3358) Bug#11822517Øystein Grøvlen8 Apr
    • Re: bzr commit into mysql-trunk branch (roy.lyseng:3358) Bug#11822517Roy Lyseng8 Apr
    • Re: bzr commit into mysql-trunk branch (roy.lyseng:3358) Bug#11822517Roy Lyseng11 Apr
      • Re: bzr commit into mysql-trunk branch (roy.lyseng:3358) Bug#11822517Øystein Grøvlen11 Apr
        • Re: bzr commit into mysql-trunk branch (roy.lyseng:3358) Bug#11822517Roy Lyseng11 Apr
          • Re: bzr commit into mysql-trunk branch (roy.lyseng:3358) Bug#11822517Øystein Grøvlen11 Apr
  • Re: bzr commit into mysql-trunk branch (roy.lyseng:3358) Bug#11822517Guilhem Bichot8 Apr
    • Re: bzr commit into mysql-trunk branch (roy.lyseng:3358) Bug#11822517Roy Lyseng11 Apr