List:Commits« Previous MessageNext Message »
From:Roy Lyseng Date:April 11 2011 7:06am
Subject:Re: bzr commit into mysql-trunk branch (roy.lyseng:3358) Bug#11822517
View as plain text  
Hi Guilhem,

thank you for the review comments.

On 08.04.11 14.38, Guilhem Bichot wrote:
> 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?

All error checking is now performed inside make_join_statistics(). I will 
document it in the commit notes.
>
>> + 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.

Yes, this seems to be a fine solution here.
>
>  > 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.

This seems workable, thanks.
>
>
>> /*
>> 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.

Ok.
>
>> + 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?

Done.
>
>> @@ -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

yep.
>
>> + 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.

Done.
>
>> + 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.

Thank you,
Roy
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