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