From: Jorgen Loland Date: August 20 2010 9:19am Subject: Re: bzr commit into mysql-next-mr-bugfixing branch (roy.lyseng:3189) Bug#31480 List-Archive: http://lists.mysql.com/commits/116353 Message-Id: <4C6E48BD.8080604@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8bit Roy, Thank you for the patch and a very informative commit message. For the most part, the patch looks correct but I have a few questions/requirements for a new patch. See inline comments tagged with (jl). > === modified file 'sql/item.cc' > --- a/sql/item.cc 2010-05-14 10:58:39 +0000 > +++ b/sql/item.cc 2010-06-10 08:17:31 +0000 > @@ -2296,21 +2296,73 @@ table_map Item_field::used_tables() cons > } > > > -void Item_field::fix_after_pullout(st_select_lex *new_parent, Item **ref) > +table_map Item_field::resolved_used_tables() const > { > - if (new_parent == depended_from) > - depended_from= NULL; > - Name_resolution_context *ctx= new Name_resolution_context(); > - ctx->outer_context= NULL; // We don't build a complete name resolver > - ctx->table_list= NULL; // We rely on first_name_resolution_table instead > - ctx->select_lex= new_parent; > - ctx->first_name_resolution_table= context->first_name_resolution_table; > - ctx->last_name_resolution_table= context->last_name_resolution_table; > - ctx->error_processor= context->error_processor; > - ctx->error_processor_data= context->error_processor_data; > - ctx->resolve_in_select_list= context->resolve_in_select_list; > - ctx->security_ctx= context->security_ctx; > - this->context=ctx; > + if (field->table->const_table) > + return 0; // const item > + return field->table->map; > +} > + > +void Item_field::fix_after_pullout(st_select_lex *parent_select, > + st_select_lex *removed_select, > + Item **ref) > +{ (snip) > + if (depended_from) > + { > + /* > + Refresh used_tables information for subqueries between the definition > + scope and resolution scope of the field item reference. > + */ > + st_select_lex *child_select= context->select_lex; > + while (child_select->outer_select() != depended_from) > + { > + /* > + The subquery on this level is outer-correlated with respect to the field > + */ > + Item_subselect *subq_predicate= child_select->master_unit()->item; > + > + subq_predicate->used_tables_cache|= OUTER_REF_TABLE_BIT; > + > + child_select= child_select->outer_select(); > + } > + /* > + child_select is select_lex immediately inner to the depended_from level. > + Now, locate the subquery predicate that contains this select_lex and > + update used tables information. > + */ > + Item_subselect *subq_predicate= child_select->master_unit()->item; > + > + subq_predicate->used_tables_cache|= this->resolved_used_tables(); > + subq_predicate->const_item_cache&= this->const_item(); > + } (jl) The content of this 'while' looks incorrect to me. To my knowledge, OUTER_REF_TABLE_BIT should be set only if an item references a table in an outer select. Using your convention of selects and tables: --------- select a from t1 where a in (select c from t2 where d >= some(select e from t3 where b=e)); A - t1 (a, b) <----. \ (b has reference to this select) B - t2 (c, d) | \ | C - t3 (e) ----' --------- Here, only b in the innermost select should have OUTER_REF_TABLE_BIT. The above while loop changes this after sj pullout. It sets OUTER_REF_TABLE_BIT for all Item_subselect between some Item that has an outer reference and the removed select. I need one more nesting level to illustrate: Before pullout: A <---. \ | B | \ | C | \ | D --' *1) *1) OUTER_REF_TABLE_BIT is set for the item that refers to a table in A) After pullout of B: A <---. \ | C | *2) \ | D --' *1) *2) OUTER_REF_TABLE_BIT is set for the Item_subselect in C. This is incorrect I suggest you only set OUTER_REF_TABLE_BIT for the Item_field fix_after_pullout() is called on; something like: if (depended_from) { st_select_lex *child_select= context->select_lex; if (child_select->outer_select() != depended_from) { Item_subselect *subq_predicate= child_select->master_unit()->item; subq_predicate->used_tables_cache|= OUTER_REF_TABLE_BIT; } while (child_select->outer_select() != depended_from) child_select= child_select->outer_select(); /* child_select is select_lex immediately inner to the depended_from level. Now, locate the subquery predicate that contains this select_lex and update used tables information. */ Item_subselect *subq_predicate= child_select->master_unit()->item; subq_predicate->used_tables_cache|= this->resolved_used_tables(); subq_predicate->const_item_cache&= this->const_item(); } (jl) Q: Does const_item_cache change? > === modified file 'sql/item.h' > --- a/sql/item.h 2010-05-14 10:58:39 +0000 > +++ b/sql/item.h 2010-06-10 08:17:31 +0000 > @@ -583,12 +583,19 @@ public: (snip) > + /** > + Fix after tables have been moved from one select_lex level to the parent > + level. Basically re-calculate all attributes dependent on the tables. > > + @param parent_select select_lex that tables are moved to. > + @param removed_select select_lex that tables are moved away from, > + child of parent_select. > + @param ref updated with new ref whenever the function substitutes > + this item with another. > + */ > + virtual void fix_after_pullout(st_select_lex *parent_select, > + st_select_lex *removed_select, > + Item **ref) {}; (jl) Minor detail: Suggest you write something like "...to the parent level by semijoin conversion" > === modified file 'sql/item_cmpfunc.cc' > --- a/sql/item_cmpfunc.cc 2010-05-14 10:58:39 +0000 > +++ b/sql/item_cmpfunc.cc 2010-06-10 08:17:31 +0000 > @@ -1755,6 +1755,32 @@ bool Item_in_optimizer::fix_fields(THD * > } > > +void Item_in_optimizer::fix_after_pullout(st_select_lex *parent_select, > + st_select_lex *removed_select, > + Item **ref) > +{ > + used_tables_cache=0; > + not_null_tables_cache= 0; > + const_item_cache= 1; > + > + /* > + No need to call fix_after_pullout() on args[0] (ie left expression), > + as Item>in_subselect::fix_after_pullout() will do this. (jl) typo "Item>" + */ + //args[0]->fix_after_pullout(parent_select, removed_select, &args[0]); + + //used_tables_cache|= args[0]->used_tables(); + //not_null_tables_cache|= args[0]->not_null_tables(); + //const_item_cache&= args[0]->const_item(); (jl) Clean up this ^ > === modified file 'sql/item_subselect.cc' > --- a/sql/item_subselect.cc 2010-05-14 10:58:39 +0000 > +++ b/sql/item_subselect.cc 2010-06-10 08:17:31 +0000 > @@ -303,6 +303,67 @@ bool Item_subselect::exec() > } > + while ((item=li++)) > + item->fix_after_pullout(parent_select, removed_select, li.ref()); > + > + // Probably need to call fix_after_pullout for semijoin expressions as well. > + > + // No need to call fix_after_pullout for JOIN expressions, as these > + // cannot have outer references? > + > + // No need to resolve for ORDER and GROUP BY fields as these contain > + // references only? > + //for (ORDER *order= (ORDER*) sel->order_list.first; > + // order; > + // order= order->next) > + // order->fix_after_pullout(parent_select, removed_select, ref_arg); > + > + //for (ORDER *order= (ORDER*) sel->group_list.first; > + // order; > + // order= order->next) > + // order->fix_after_pullout(parent_select, removed_select, ref_arg); (jl) Clean up this ^ > @@ -3589,16 +3593,16 @@ bool convert_subq_to_sj(JOIN *parent_joi > /* Fix the created equality and AND */ > sj_nest->sj_on_expr->fix_fields(parent_join->thd, &sj_nest->sj_on_expr); > > + /* Unlink the child select_lex so it doesn't show up in EXPLAIN: */ > + subq_lex->master_unit()->exclude_level(); > /* > Walk through sj nest's WHERE and ON expressions and call > item->fix_table_changes() for all items. > */ > - sj_nest->sj_on_expr->fix_after_pullout(parent_lex, &sj_nest->sj_on_expr); > - fix_list_after_tbl_changes(parent_lex, &sj_nest->nested_join->join_list); > - > - > - /* Unlink the child select_lex so it doesn't show up in EXPLAIN: */ > - subq_lex->master_unit()->exclude_level(); > + sj_nest->sj_on_expr->fix_after_pullout(parent_lex, subq_lex, > + &sj_nest->sj_on_expr); > + fix_list_after_tbl_changes(parent_lex, subq_lex, > + &sj_nest->nested_join->join_list); (jl) Q: Was it necessary to move the "/*Unlink the child..." part before the "Walk through sj nest's..." part? -- Jørgen Løland | Senior Software Engineer | +47 73842138 Oracle MySQL Trondheim, Norway