From: Jorgen Loland
Date: February 4 2011 12:38pm
Subject: Re: bzr commit into mysql-trunk branch (ole.john.aske:3592) Bug#59308
List-Archive: http://lists.mysql.com/commits/130405
Message-Id: <4D4BF353.8070709@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit
Hi Ole John
Patch approved. See comment about 5.5 and 5.1 patch versions in other email.
On 02/02/2011 03:51 PM, Ole John Aske wrote:
> #At file:///net/fimafeng09/export/home/tmp/oleja/mysql/mysql-trunk/ based on revid:marc.alff@stripped
>
> 3592 Ole John Aske 2011-02-02
> Updated Fix for bug#59308: Incorrect result for SELECT DISTINCT
> ... ORDER BY DESC.
>
> Rebased to mysql-trunk 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/include/order_by.inc
> mysql-test/r/order_by_icp_mrr.result
> mysql-test/r/order_by_none.result
> sql/sql_select.cc
> === modified file 'mysql-test/include/order_by.inc'
> --- a/mysql-test/include/order_by.inc 2010-12-17 09:41:21 +0000
> +++ b/mysql-test/include/order_by.inc 2011-02-02 14:51:11 +0000
> @@ -1686,8 +1686,25 @@ GROUP BY t1.a
> ORDER by c
> LIMIT 2;
>
> -DROP TABLE t1, t2;
> + 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... ORDER BY 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 'mysql-test/r/order_by_icp_mrr.result'
> --- a/mysql-test/r/order_by_icp_mrr.result 2010-12-17 09:41:21 +0000
> +++ b/mysql-test/r/order_by_icp_mrr.result 2011-02-02 14:51:11 +0000
> @@ -2523,6 +2523,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... ORDER BY 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/r/order_by_none.result'
> --- a/mysql-test/r/order_by_none.result 2010-12-17 09:41:21 +0000
> +++ b/mysql-test/r/order_by_none.result 2011-02-02 14:51:11 +0000
> @@ -2522,6 +2522,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... ORDER BY 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 'sql/sql_select.cc'
> --- a/sql/sql_select.cc 2011-02-01 14:24:57 +0000
> +++ b/sql/sql_select.cc 2011-02-02 14:51:11 +0000
> @@ -19878,11 +19878,12 @@ 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;
> QUICK_SELECT_I *save_quick= 0;
> + int best_key= -1;
> Item *orig_select_cond= 0;
> bool orig_select_cond_saved= false;
> bool changed_key= false;
> @@ -20001,6 +20002,7 @@ test_if_skip_sort_order(JOIN_TAB *tab,OR
> key_map new_ref_key_map; // 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) ?
> @@ -20024,7 +20026,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;
>
> @@ -20048,77 +20049,16 @@ test_if_skip_sort_order(JOIN_TAB *tab,OR
>
> if (best_key>= 0)
> {
> - bool quick_created= FALSE;
> if (table->quick_keys.is_set(best_key)&& best_key != ref_key)
> {
> key_map map; // 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,
> - TRUE, FALSE)> 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);
> - if (tab->pre_idx_push_select_cond)
> - {
> - tab->set_cond(tab->pre_idx_push_select_cond, __LINE__);
> - /*
> - orig_select_cond is a part of pre_idx_push_select_cond,
> - no need to restore it.
> - */
> - orig_select_cond= 0;
> - orig_select_cond_saved= false;
> - }
> - 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=QS_RANGE;
> - 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,
> + TRUE, FALSE);
> }
> order_direction= best_key_direction;
> /*
> @@ -20136,6 +20076,8 @@ test_if_skip_sort_order(JOIN_TAB *tab,OR
> }
>
> check_reverse_order:
> + DBUG_ASSERT(order_direction != 0);
> +
> if (order_direction == -1) // If ORDER BY ... DESC
> {
> if (select&& select->quick)
> @@ -20144,9 +20086,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 ||
> @@ -20154,36 +20097,128 @@ check_reverse_order:
> quick_type == QUICK_SELECT_I::QS_TYPE_GROUP_MIN_MAX)
> {
> tab->limit= 0;
> - select->quick= save_quick;
> goto use_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;
> - goto use_filesort; // 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);
> + if (tab->pre_idx_push_select_cond)
> + {
> + tab->set_cond(tab->pre_idx_push_select_cond, __LINE__);
> + /*
> + orig_select_cond is a part of pre_idx_push_select_cond,
> + no need to restore it.
> + */
> + orig_select_cond= 0;
> + orig_select_cond_saved= false;
> + }
> + 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=QS_RANGE;
> + 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 use_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->need_sorted_output();
> +
> + } // 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->need_sorted_output();
> /*
> Restore condition only if we didn't chose index different to what we used
> for ICP.
> @@ -20193,6 +20228,12 @@ check_reverse_order:
> DBUG_RETURN(1);
>
> use_filesort:
> + // Restore original save_quick
> + if (select&& select->quick != save_quick)
> + {
> + delete select->quick;
> + select->quick= save_quick;
> + }
> if (orig_select_cond_saved)
> tab->set_cond(orig_select_cond, __LINE__);
> DBUG_RETURN(0);
>
>
>
>
>
--
Jørgen Løland | Senior Software Engineer | +47 73842138
Oracle MySQL
Trondheim, Norway