List:Commits« Previous MessageNext Message »
From:Jorgen Loland Date:August 20 2010 9:19am
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (roy.lyseng:3189)
Bug#31480
View as plain text  
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
Thread
Re: bzr commit into mysql-next-mr-bugfixing branch (roy.lyseng:3189)Bug#31480Jorgen Loland20 Aug
  • Re: bzr commit into mysql-next-mr-bugfixing branch (roy.lyseng:3189)Bug#31480Roy Lyseng14 Oct