List:Commits« Previous MessageNext Message »
From:Jorgen Loland Date:February 4 2011 12:38pm
Subject:Re: bzr commit into mysql-trunk branch (ole.john.aske:3592) Bug#59308
View as plain text  
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
>                                    <col>... ORDER BY<col>  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<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 '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<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/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<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 '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
Thread
bzr commit into mysql-trunk branch (ole.john.aske:3592) Bug#59308Ole John Aske2 Feb
  • Re: bzr commit into mysql-trunk branch (ole.john.aske:3592) Bug#59308Jorgen Loland4 Feb