Hi Ole John,
thank you for fixing this bug. The patch looks good, but it would be good to
align the 5.1/5.5/5.6 codebases a tiny bit:
5.1/5.5 uses goto label "need_filesort" while 5.6 uses "use_filesort". Since the
"use_filesort" label already existed in 5.6 prior to this bugfix, I suggest you
use that label name in your patches for 5.1 and 5.5 as well.
On 02/02/2011 03:50 PM, Ole John Aske wrote:
> #At file:///net/fimafeng09/export/home/tmp/oleja/mysql/mysql-5.5/ based on
> revid:ole.john.aske@stripped
>
> 3295 Ole John Aske 2011-02-02
> Updated Fix for bug#59308: Incorrect result for SELECT DISTINCT
> <col>... ORDER BY<col> DESC.
>
> Rebased to mysql-5.5 which has changed considerably in the areas
> affected by the original bugfix. (based on mysql-5.1)
>
> See http://lists.mysql.com/commits/130245 for original commit comments.
>
> modified:
> mysql-test/r/order_by.result
> mysql-test/t/order_by.test
> sql/sql_select.cc
> === modified file 'mysql-test/r/order_by.result'
> --- a/mysql-test/r/order_by.result 2010-09-13 12:46:55 +0000
> +++ b/mysql-test/r/order_by.result 2011-02-02 14:50:12 +0000
> @@ -1638,6 +1638,31 @@ id select_type table type possible_keys
> 1 SIMPLE t1 index NULL a 8 NULL 10 Using index; Using temporary; Using filesort
> 1 SIMPLE t2 eq_ref PRIMARY PRIMARY 4 test.t1.b 1 Using where
> DROP TABLE t1, t2;
> +#
> +# Bug #59110: Memory leak of QUICK_SELECT_I allocated memory
> +# and
> +# Bug #59308: Incorrect result for
> +SELECT DISTINCT<col>... ORDER BY<col> DESC
> +
> +# Use Valgrind to detect #59110!
> +#
> +CREATE TABLE t1 (a INT,KEY (a));
> +INSERT INTO t1 VALUES (1),(2),(3),(4),(5),(6),(7),(8),(9),(10);
> +EXPLAIN SELECT DISTINCT a,1 FROM t1 WHERE a<> 1 ORDER BY a DESC;
> +id select_type table type possible_keys key key_len ref rows Extra
> +1 SIMPLE t1 index a a 5 NULL 10 Using where; Using index; Using filesort
> +SELECT DISTINCT a,1 FROM t1 WHERE a<> 1 ORDER BY a DESC;
> +a 1
> +10 1
> +9 1
> +8 1
> +7 1
> +6 1
> +5 1
> +4 1
> +3 1
> +2 1
> +DROP TABLE t1;
> End of 5.1 tests
> #
> # Bug #38745: MySQL 5.1 optimizer uses filesort for ORDER BY
>
> === modified file 'mysql-test/t/order_by.test'
> --- a/mysql-test/t/order_by.test 2010-09-13 12:46:55 +0000
> +++ b/mysql-test/t/order_by.test 2011-02-02 14:50:12 +0000
> @@ -1492,6 +1492,23 @@ LIMIT 2;
> DROP TABLE t1, t2;
>
>
> +--echo #
> +--echo # Bug #59110: Memory leak of QUICK_SELECT_I allocated memory
> +--echo # and
> +--echo # Bug #59308: Incorrect result for
> +--echo SELECT DISTINCT<col>... ORDER BY<col> DESC
> +--echo
> +--echo # Use Valgrind to detect #59110!
> +--echo #
> +
> +CREATE TABLE t1 (a INT,KEY (a));
> +INSERT INTO t1 VALUES (1),(2),(3),(4),(5),(6),(7),(8),(9),(10);
> +
> +EXPLAIN SELECT DISTINCT a,1 FROM t1 WHERE a<> 1 ORDER BY a DESC;
> +SELECT DISTINCT a,1 FROM t1 WHERE a<> 1 ORDER BY a DESC;
> +
> +DROP TABLE t1;
> +
> --echo End of 5.1 tests
>
>
>
> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc 2011-02-01 14:19:34 +0000
> +++ b/sql/sql_select.cc 2011-02-02 14:50:12 +0000
> @@ -13619,12 +13619,14 @@ test_if_skip_sort_order(JOIN_TAB *tab,OR
> {
> int ref_key;
> uint ref_key_parts;
> - int order_direction;
> + int order_direction= 0;
> uint used_key_parts;
> TABLE *table=tab->table;
> SQL_SELECT *select=tab->select;
> key_map usable_keys;
> QUICK_SELECT_I *save_quick= 0;
> + int best_key= -1;
> +
> DBUG_ENTER("test_if_skip_sort_order");
> LINT_INIT(ref_key_parts);
>
> @@ -13728,13 +13730,14 @@ test_if_skip_sort_order(JOIN_TAB *tab,OR
> new_ref_key_map.clear_all(); // Force the creation of quick select
> new_ref_key_map.set_bit(new_ref_key); // only for new_ref_key.
>
> + select->quick= 0;
> if (select->test_quick_select(tab->join->thd, new_ref_key_map,
> 0,
> (tab->join->select_options&
> OPTION_FOUND_ROWS) ?
> HA_POS_ERROR :
>
> tab->join->unit->select_limit_cnt,0)<=
> 0)
> - DBUG_RETURN(0);
> + goto need_filesort;
> }
> ref_key= new_ref_key;
> }
> @@ -13749,7 +13752,6 @@ test_if_skip_sort_order(JOIN_TAB *tab,OR
> uint best_key_parts= 0;
> uint saved_best_key_parts= 0;
> int best_key_direction= 0;
> - int best_key= -1;
> JOIN *join= tab->join;
> ha_rows table_records= table->file->stats.records;
>
> @@ -13769,72 +13771,21 @@ test_if_skip_sort_order(JOIN_TAB *tab,OR
> tab->join->tables> tab->join->const_tables + 1)&&
> ((unsigned) best_key != table->s->primary_key ||
> !table->file->primary_key_is_clustered()))
> - DBUG_RETURN(0);
> + goto need_filesort;
>
> if (best_key>= 0)
> {
> - bool quick_created= FALSE;
> if (table->quick_keys.is_set(best_key)&& best_key != ref_key)
> {
> key_map map;
> map.clear_all(); // Force the creation of quick select
> map.set_bit(best_key); // only best_key.
> - quick_created=
> - select->test_quick_select(join->thd, map, 0,
> - join->select_options& OPTION_FOUND_ROWS
> ?
> - HA_POS_ERROR :
> - join->unit->select_limit_cnt,
> - 0)> 0;
> - }
> - if (!no_changes)
> - {
> - /*
> - If ref_key used index tree reading only ('Using index' in EXPLAIN),
> - and best_key doesn't, then revert the decision.
> - */
> - if (!table->covering_keys.is_set(best_key))
> - table->set_keyread(FALSE);
> - if (!quick_created)
> - {
> - tab->index= best_key;
> - tab->read_first_record= best_key_direction> 0 ?
> - join_read_first:join_read_last;
> - tab->type=JT_NEXT; // Read with index_first(), index_next()
> - if (select&& select->quick)
> - {
> - delete select->quick;
> - select->quick= 0;
> - }
> - if (table->covering_keys.is_set(best_key))
> - table->set_keyread(TRUE);
> - table->file->ha_index_or_rnd_end();
> - if (join->select_options& SELECT_DESCRIBE)
> - {
> - tab->ref.key= -1;
> - tab->ref.key_parts= 0;
> - if (select_limit< table_records)
> - tab->limit= select_limit;
> - }
> - }
> - else if (tab->type != JT_ALL)
> - {
> - /*
> - We're about to use a quick access to the table.
> - We need to change the access method so as the quick access
> - method is actually used.
> - */
> - DBUG_ASSERT(tab->select->quick);
> - tab->type=JT_ALL;
> - tab->use_quick=1;
> - tab->ref.key= -1;
> - tab->ref.key_parts=0; // Don't use ref key.
> - tab->read_first_record= join_init_read_record;
> - if (tab->is_using_loose_index_scan())
> - join->tmp_table_param.precomputed_group_by= TRUE;
> - /*
> - TODO: update the number of records in join->best_positions[tablenr]
> - */
> - }
> + select->quick= 0;
> + select->test_quick_select(join->thd, map, 0,
> + join->select_options& OPTION_FOUND_ROWS ?
> + HA_POS_ERROR :
> + join->unit->select_limit_cnt,
> + 0);
> }
> order_direction= best_key_direction;
> /*
> @@ -13847,10 +13798,12 @@ test_if_skip_sort_order(JOIN_TAB *tab,OR
> saved_best_key_parts : best_key_parts;
> }
> else
> - DBUG_RETURN(0);
> + goto need_filesort;
> }
>
> check_reverse_order:
> + DBUG_ASSERT(order_direction != 0);
> +
> if (order_direction == -1) // If ORDER BY ... DESC
> {
> if (select&& select->quick)
> @@ -13859,9 +13812,10 @@ check_reverse_order:
> Don't reverse the sort order, if it's already done.
> (In some cases test_if_order_by_key() can be called multiple times
> */
> - if (!select->quick->reverse_sorted())
> + if (select->quick->reverse_sorted())
> + goto skiped_sort_order;
> + else
> {
> - QUICK_SELECT_I *tmp;
> int quick_type= select->quick->get_type();
> if (quick_type == QUICK_SELECT_I::QS_TYPE_INDEX_MERGE ||
> quick_type == QUICK_SELECT_I::QS_TYPE_ROR_INTERSECT ||
> @@ -13869,37 +13823,128 @@ check_reverse_order:
> quick_type == QUICK_SELECT_I::QS_TYPE_GROUP_MIN_MAX)
> {
> tab->limit= 0;
> - select->quick= save_quick;
> - DBUG_RETURN(0); // Use filesort
> + goto need_filesort; // Use filesort
> }
> -
> - /* ORDER BY range_key DESC */
> - tmp= select->quick->make_reverse(used_key_parts);
> - if (!tmp)
> - {
> - select->quick= save_quick;
> - tab->limit= 0;
> - DBUG_RETURN(0); // Reverse sort not supported
> - }
> - select->set_quick(tmp);
> }
> }
> - else if (tab->type != JT_NEXT&& tab->type !=
> JT_REF_OR_NULL&&
> - tab->ref.key>= 0&& tab->ref.key_parts<=
> used_key_parts)
> + }
> +
> + /*
> + Update query plan with access pattern for doing
> + ordered access according to what we have decided
> + above.
> + */
> + if (!no_changes) // We are allowed to update QEP
> + {
> + if (best_key>= 0)
> {
> - /*
> - SELECT * FROM t1 WHERE a=1 ORDER BY a DESC,b DESC
> + bool quick_created=
> + (select&& select->quick&&
> select->quick!=save_quick);
>
> - Use a traversal function that starts by reading the last row
> - with key part (A) and then traverse the index backwards.
> + /*
> + If ref_key used index tree reading only ('Using index' in EXPLAIN),
> + and best_key doesn't, then revert the decision.
> */
> - tab->read_first_record= join_read_last_key;
> - tab->read_record.read_record= join_read_prev_same;
> + if (!table->covering_keys.is_set(best_key))
> + table->set_keyread(FALSE);
> + if (!quick_created)
> + {
> + if (select) // Throw any existing quick select
> + select->quick= 0; // Cleanup either reset to save_quick,
> + // or 'delete save_quick'
> + tab->index= best_key;
> + tab->read_first_record= order_direction> 0 ?
> + join_read_first:join_read_last;
> + tab->type=JT_NEXT; // Read with index_first(), index_next()
> +
> + if (table->covering_keys.is_set(best_key))
> + table->set_keyread(TRUE);
> + table->file->ha_index_or_rnd_end();
> + if (tab->join->select_options& SELECT_DESCRIBE)
> + {
> + tab->ref.key= -1;
> + tab->ref.key_parts= 0;
> + if (select_limit< table->file->stats.records)
> + tab->limit= select_limit;
> + }
> + }
> + else if (tab->type != JT_ALL)
> + {
> + /*
> + We're about to use a quick access to the table.
> + We need to change the access method so as the quick access
> + method is actually used.
> + */
> + DBUG_ASSERT(tab->select->quick);
> + tab->type=JT_ALL;
> + tab->use_quick=1;
> + tab->ref.key= -1;
> + tab->ref.key_parts=0; // Don't use ref key.
> + tab->read_first_record= join_init_read_record;
> + if (tab->is_using_loose_index_scan())
> + tab->join->tmp_table_param.precomputed_group_by= TRUE;
> + /*
> + TODO: update the number of records in join->best_positions[tablenr]
> + */
> + }
> + } // best_key>= 0
> +
> + if (order_direction == -1) // If ORDER BY ... DESC
> + {
> + if (select&& select->quick)
> + {
> + /* ORDER BY range_key DESC */
> + QUICK_SELECT_I *tmp= select->quick->make_reverse(used_key_parts);
> + if (!tmp)
> + {
> + tab->limit= 0;
> + goto need_filesort; // Reverse sort failed -> filesort
> + }
> + if (select->quick == save_quick)
> + save_quick= 0; // make_reverse() consumed it
> + select->set_quick(tmp);
> + }
> + else if (tab->type != JT_NEXT&& tab->type !=
> JT_REF_OR_NULL&&
> + tab->ref.key>= 0&& tab->ref.key_parts<=
> used_key_parts)
> + {
> + /*
> + SELECT * FROM t1 WHERE a=1 ORDER BY a DESC,b DESC
> +
> + Use a traversal function that starts by reading the last row
> + with key part (A) and then traverse the index backwards.
> + */
> + tab->read_first_record= join_read_last_key;
> + tab->read_record.read_record= join_read_prev_same;
> + }
> }
> + else if (select&& select->quick)
> + select->quick->sorted= 1;
> +
> + } // QEP has been modified
> +
> + /*
> + Cleanup:
> + We may have both a 'select->quick' and 'save_quick' (original)
> + at this point. Delete the one that we wan't use.
> + */
> +
> +skiped_sort_order:
> + // Keep current (ordered) select->quick
> + if (select&& save_quick != select->quick)
> + {
> + delete save_quick;
> + save_quick= NULL;
> }
> - else if (select&& select->quick)
> - select->quick->sorted= 1;
> DBUG_RETURN(1);
> +
> +need_filesort:
> + // Restore original save_quick
> + if (select&& select->quick != save_quick)
> + {
> + delete select->quick;
> + select->quick= save_quick;
> + }
> + DBUG_RETURN(0);
> }
>
>
>
>
>
>
>
--
Jørgen Løland | Senior Software Engineer | +47 73842138
Oracle MySQL
Trondheim, Norway
