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
