From: Sergey Petrunia
Date: March 9 2009 9:13pm
Subject: Re: bzr commit into mysql-6.0-opt branch (igor:2727) Bug#42955
List-Archive: http://lists.mysql.com/commits/68705
Message-Id: <20090309211301.GE15114@pslp2.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7BIT

Hello Igor,

> #At file:///home/igor/dev-bzr/mysql-6.0-opt-bug42955/
> 
>  2727 Igor Babaev	2009-03-04
>       Fixed bug #42955.
>       If a join buffer is employed to join a table through BNL or
>       BKA algorithm for a query with a ORDER BY / GROUP BY clause
>       then the result set has always to be sorted unless the clause
>       can be optimized away. 
...
> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2009-02-16 21:18:45 +0000
> +++ b/sql/sql_select.cc	2009-03-05 05:38:19 +0000
> @@ -1909,7 +1909,16 @@ JOIN::optimize()
>  	      (group_list && order) ||
>  	      test(select_options & OPTION_BUFFER_RESULT)));
>  
> -  uint no_jbuf_after= make_join_orderinfo(this);
> +  /*
> +    Here we'd rather make a call of the make_join_orderinfo function
> +    to return the proper value for no_jbuf_after in the case when
> +    the hint FORCE INDEX FOR ORDER BY/GROUP BY is used for the table
> +    whose columns are required to be returned in a sorted order.
> +    Yet the current implementation of FORCE INDEX hints does not
> +    allow us to do it in a clean manner.
> +  */   
Are the incorrect handling of hints and the fact that there is deadcode in 
make_join_orderinfo() (as we discussed on skype) two different problems or 
one problem? I assumed that they are two different problems, but the comment
implies they are somehow connected?

Also, the "used for the table whose columns are required to be returned in a
sorted order." passage is very hard to read.

> +  uint no_jbuf_after= 1 ? tables :  make_join_orderinfo(this);
This looks very confusing, especially for one who's skimming through the code.
Please make the fact that we never call make_join_orderinfo() explicit:
- comment out the call here
- wrap it within #if 0  (this will also avoid the "unused static function"
  warning).

>    ulonglong select_opts_for_readinfo= 
>      (select_options & (SELECT_DESCRIBE | SELECT_NO_JOIN_CACHE)) |
>      (select_lex->ftfunc_list->elements ?  SELECT_NO_JOIN_CACHE : 0);
> @@ -9322,22 +9331,11 @@ static void push_index_cond(JOIN_TAB *ta
>  
>  static uint make_join_orderinfo(JOIN *join)
>  {
> -  uint i;
> +  JOIN_TAB *tab;
>    if (join->need_tmp)
>      return join->tables;
> -
> -  for (i=join->const_tables ; i < join->tables ; i++)
> -  {
> -    JOIN_TAB *tab=join->join_tab+i;
> -    TABLE *table=tab->table;
> -    if ((table == join->sort_by_table && 
> -         (!join->order || join->skip_sort_order)) ||
> -        (join->sort_by_table == (TABLE *) 1 && i != join->const_tables))
> -    {
> -      break;
> -    }
> -  }
> -  return i-1;
> +  tab= join->get_sort_by_join_tab();
> +  return tab ? tab-join->join_tab : join->tables;
>  }
>  
>  /*
> @@ -10245,6 +10243,29 @@ make_join_readinfo(JOIN *join, ulonglong
>      }
>    }
>    join->join_tab[join->tables-1].next_select=0; /* Set by do_select */
> +  for (i=join->const_tables ; i < join->tables ; i++)
> +  {
> +    JOIN_TAB *tab=join->join_tab+i;
> +    /* 
> +      If a join buffer is is used to join a table the ordering by an index
                          ^^^^^ typo?
> +      for the first non-constant table cannot be employed anymore.
> +    */
Do you think it could make sense to put the above comment right before the
loop?
> +    if (tab->use_join_cache)
> +    {
> +      JOIN_TAB *sort_by_tab= join->get_sort_by_join_tab();
> +      if (sort_by_tab && !join->need_tmp)
> +      {
> +        join->need_tmp= 1;
> +        join->simple_order= join->simple_group= 0;
> +        if (sort_by_tab->type == JT_NEXT)
> +        {
> +          sort_by_tab->type= JT_ALL;
> +          sort_by_tab->read_first_record= join_init_read_record;
> +        }
> +      }
> +      break;
> +    }
> +  } 
>    DBUG_RETURN(FALSE);
>  }
>  
> === modified file 'sql/sql_select.h'
> --- a/sql/sql_select.h	2009-02-16 21:18:45 +0000
> +++ b/sql/sql_select.h	2009-03-05 05:38:19 +0000
> @@ -1726,6 +1726,15 @@ public:
>      return (unit == &thd->lex->unit && (unit->fake_select_lex == 0 ||
>                                          select_lex == unit->fake_select_lex));
>    }
> +  /* 
> +    Return the table for which an index scan can be used to satisfy 
> +    the sort order needed by the ORDER BY/GROUP BY clause 
> +  */
> +  JOIN_TAB *get_sort_by_join_tab()
> +  {
> +    return (need_tmp || !sort_by_table || skip_sort_order) ?
> +              0 : join_tab+const_tables;
Please s/0/NULL/ as it's a pointer.
> +  }
>  private:
>    bool make_simple_join(JOIN *join, TABLE *tmp_table);
>  };


Ok to push after the above is addressed.
BR
 Sergey
-- 
Sergey Petrunia, Lead Software Engineer
MySQL AB, www.mysql.com
Office: N/A
Blog: http://s.petrunia.net/blog