Hi Olav,
thank you for commenting.
On 06.02.11 23.34, Olav Sandstaa wrote:
> Hi Roy,
>
> The patch looks good. Thanks for adding great comments both for the commit and
> in the source files.
>
> The number of explain changes was large, thanks for adding explanation for most
> of these. I have not examined in detail why the plans changes but agree that
> most of them are improvements compared to the previous plan. Is this also the
> case for the new plans that contains "Range checked for each record (index map:
> 0x2)"?
>
> I have some minor comments to the code, see inline. You can feel free to decide
> whether to ignore these or not.
>
> OK to push.
>
> Olav
>
>
> On 02/04/2011 12:29 PM, Roy Lyseng wrote:
>>
>> === modified file 'sql/sql_select.cc'
>> --- a/sql/sql_select.cc 2011-02-02 13:48:49 +0000
>> +++ b/sql/sql_select.cc 2011-02-04 11:28:14 +0000
>> @@ -9505,12 +9505,16 @@ static bool pushdown_on_conditions(JOIN*
>> */
>> Item *on_expr= *first_inner_tab->on_expr_ref;
>>
>> - table_map used_tables= (join->const_table_map |
>> - OUTER_REF_TABLE_BIT | RAND_TABLE_BIT);
>> + table_map used_tables= (table_map)0;
>
> Here you have included an explicit cast to (table_map). In the corresponding
> code in make_join_select() you do not have this cast. The cast is not necessary
> but it maybe improves readability? (if so it should also be added in
> make_join_select).
I do not know a reason to have this, so I'm deleting it.
>
>> +
>> for (JOIN_TAB *join_tab= join->join_tab+join->const_tables;
>> join_tab<= last_tab ; join_tab++)
>> {
>> table_map current_map= join_tab->table->map;
>> + if (join_tab == join->join_tab+join->const_tables)
>> + current_map|= join->const_table_map | OUTER_REF_TABLE_BIT;
>> + if (join_tab == last_tab)
>> + current_map|= RAND_TABLE_BIT;
>
> I liked the comment you added for the similar code in make_join_select(). Would
> it be a good idea to add the same comment here?
I guess I can add a reference to make_join_select(), Step #2.
>
>> used_tables|= current_map;
>> Item *tmp_cond= make_cond_for_table(on_expr, used_tables, current_map,
> 0);
>> if (!tmp_cond)
>> @@ -9567,7 +9571,6 @@ static bool make_join_select(JOIN *join,
>> DBUG_ENTER("make_join_select");
>> {
>> add_not_null_conds(join);
>> - table_map used_tables;
>> /*
>> Step #1: Extract constant condition
>> - Extract and check the constant part of the WHERE
>> @@ -9634,22 +9637,34 @@ static bool make_join_select(JOIN *join,
>> /*
>> Step #2: Extract WHERE/ON parts
>> */
>> + table_map used_tables= 0;
>> table_map save_used_tables= 0;
>> - used_tables= join->const_table_map | OUTER_REF_TABLE_BIT |
> RAND_TABLE_BIT;
>> - JOIN_TAB *tab;
>> - table_map current_map;
>> for (uint i=join->const_tables ; i< join->tables ; i++)
>> {
>> - tab= join->join_tab+i;
>> + JOIN_TAB *tab= join->join_tab+i;
>> /*
>> first_inner is the X in queries like:
>> SELECT * FROM t1 LEFT OUTER JOIN (t2 JOIN t3) ON X
>> */
>> JOIN_TAB *first_inner_tab= tab->first_inner;
>> - current_map= tab->table->map;
>> bool use_quick_range=0;
>> Item *tmp;
>>
>> + /*
>> + Calculate used table information added at this stage.
>> + The current table is always added. Const tables are assumed to be
>> + available together with the first table in the join order.
>> + All outer references are available, so these may be evaluated together
>> + with the first table.
>> + Random expressions must be added to the last table's condition.
>> + It solves problem with queries like SELECT * FROM t1 WHERE rand()> 0.5
>
> Tab is used in front of the two last lines of this comment.
Outch. I'd wish my editor would highlight TAB characters...
>
>> + */
>> + table_map current_map= tab->table->map;
>> + if (i == join->const_tables)
>> + current_map|= OUTER_REF_TABLE_BIT | join->const_table_map;
>
> I think the line above would be better to read and follow the above comment more
> closely if you reversed the order of OUTER_REF_TABLE_BIT | join->const_table_map.
> (in pushdown_on_condition() you already have it in the opposite order).
Done.
>
>> + if (i == join->tables-1)
>> + current_map|= RAND_TABLE_BIT;
>> +
>> /*
>> Tables that are within SJ-Materialization nests cannot have their
>> conditions referring to preceding non-const tables.
>> @@ -9660,17 +9675,10 @@ static bool make_join_select(JOIN *join,
>> !(used_tables& tab->emb_sj_nest->sj_inner_tables))
>> {
>> save_used_tables= used_tables;
>> - used_tables= join->const_table_map | OUTER_REF_TABLE_BIT |
>> - RAND_TABLE_BIT;
>> + used_tables= OUTER_REF_TABLE_BIT | join->const_table_map;
>
> Same comment as above: I liked the original ordering better.
Done.
>> }
>>
>> - /*
>> - Following force including random expression in last table condition.
>> - It solve problem with select like SELECT * FROM t1 WHERE rand()> 0.5
>> - */
>> - if (i == join->tables-1)
>> - current_map|= OUTER_REF_TABLE_BIT | RAND_TABLE_BIT;
>> - used_tables|=current_map;
>> + used_tables|= current_map;
>>
>> if (tab->type == JT_REF&& tab->quick&&
>> (uint) tab->ref.key == tab->quick->index&&
>> @@ -19144,10 +19152,25 @@ static bool replace_subcondition(JOIN *j
>> specified in 'tables' bitmap are available.
>> If 'used_table' is 0, extract conditions for all tables in 'tables'.
>>
>> - The function assumes that
>> - - Constant parts of the condition has already been checked.
>> - - Condition that could be checked for tables in 'tables' has already
>> - been checked.
>> + This function can be used to extract conditions relevant for a table
>> + in a join order. It will ensure that all conditions are attached to
>> + the first table in the join order where all necessary fields are
>> + available, and it will also ensure that a given condition is attached
>> + to only one table.
>> +
>> + To accomplish this, first initialize "tables" to the empty
>> + set. Then loop over all tables in the join order, set "used_table" to
>> + the bit representing the current table, accumulate "used_table" into the
>> + "tables" set, and call this function. To ensure correct handling of
>> + const expressions and outer references, add the const table map and
>> + OUTER_REF_TABLE_BIT to "used_table" for the first table. To ensure
>> + that random expressions are evaluated for the final table, add
>> + RAND to "used_table" for the final table.
>> +
>> + The function assumes that constant, inexpensive parts of the condition
>> + have already been checked. Constant, expensive parts will be attached
>> + to the first table in the join order, provided that the above call
>> + sequence is followed.
>
> Great comment!
>
>>
>> The function takes into account that some parts of the condition are
>> guaranteed to be true by employed 'ref' access methods (the code that
>> @@ -19176,20 +19199,13 @@ make_cond_for_table_from_pred(Item *root
>> Ignore this condition if
>> 1. We are extracting conditions for a specific table, and
>> 2. that table is not referenced by the condition, and
>> - 3. exclude constant conditions not checked at optimization time if
>> - the table we are pushing conditions to is the first one.
>> - As a result, such conditions are not considered as already checked
>> - and will be checked at execution time, attached to the first table.
>> + 3. this is a constant condition not checked at optimization time and
>> + this is not the first table we are extracting conditions for.
>> + (Assuming that used_table == tables for the first table.)
>> */
>> if (used_table&& //
> 1
>> !(cond->used_tables()& used_table)&&
> // 2
>> - /*
>> - psergey: TODO: "used_table& 1" doesn't make sense in nearly any
>> - context. Look at setup_table_map(), table bits reflect the order
>> - the tables were encountered by the parser. Check what we should
>> - replace this condition with.
>> - */
>> - !((used_table& 1)&& cond->is_expensive()))
> // 3
>> + !(used_table == tables&& cond->is_expensive()))
> // 3
>> return NULL;
>>
>> if (cond->type() == Item::COND_ITEM)
>>
>>
>>
>>
>
Regards,
Roy
