List:Commits« Previous MessageNext Message »
From:Jorgen Loland Date:January 10 2011 8:50am
Subject:Re: bzr commit into mysql-5.1 branch (ole.john.aske:3534) Bug#59308
View as plain text  
Hi Ole John,

The patch looks good, and I only have a few minor comments. See inline.

A note of caution: I think merging this to 5.5/trunk will be non-trivial since 
the test_if_skip_sort_order() has changed a bit to work correctly with ICP. I 
can take a quick look at the 5.5/trunk versions of this fix as well before you 
push them.

 >  3534 Ole John Aske	2011-01-06
 >       Updated Fix for bug#59308: Incorrect result for SELECT DISTINCT 
<col>... ORDER BY <col> DESC.
 >
 >       Also fix bug#59110: Memory leak of QUICK_SELECT_I allocated memory.
 >
 >       Root cause of these bugs are that test_if_skip_sort_order() decided to
 >       revert the 'skip_sort_order' descision (and use filesort) after the
 >       query plan has been updated to reflect a 'skip' of the sort order.
 >
 >       This might happen in 'check_reverse_order:' if we have a
 >       select->quick which could not be made descending by appending
 >       a QUICK_SELECT_DESC. ().
 >
 >       The original 'save_quick' was then restored after the QEP has been 
modified, which caused:
 >
 >        - An incorrect 'precomputed_group_by= TRUE' may have been set, and not 
reverted, as
 >          part of the already modifified QEP (Bug#59398)

jl: typo: 59308

 >        - A 'select->quick' might have been created which we fail to delete 
(bug#59110).
 >
 >       This fix is a refactorication of test_if_skip_sort_order() where all logic
 >       related to modification of QEP (controlled by argument 'bool 
no_changes'), is
 >       moved to the end of test_if_skip_sort_order(), and done after *all* 
'test_if_skip'
 >       checks has been performed - including the 'check_reverse_order:' checks.
 >
 >       The refactorication above contains now intentional changes to the logic 
which has been
 >       moved to the end of the function.
 >
 >       Furthermore, a smaller part of the fix address the handling of the 
select->quick objects
 >       which may already exists when we call 'test_if_skip_sort_order()' 
(save_quick) - and
 >       new select->quick's created during test_if_skip_sort_order():
 >
 >         - Before new select->quick may be created by calling 
::test_quick_select(), we
 >           set 'select->quick= 0' to avoid that ::test_quick_select() prematurely
 >           delete the save_quick's. (After this call we may have both a 
'save_quick'
 >           and 'select->quick')
 >
 >         - All returns from ::test_if_skip_sort_order() where we may have both a
 >           'save_quick' and a 'select->quick' has been changed to goto's to the
 >           exit points 'skiped_sort_order:' or 'need_filesort:' where we
 >           decide which of the QUICK_SELECT's to keep, and delete the other.
 >
 >     modified:
 >       mysql-test/r/order_by.result
 >       mysql-test/t/order_by.test
 >       sql/sql_select.cc
(...)
 > === modified file 'sql/sql_select.cc'
 > --- a/sql/sql_select.cc	2010-12-28 23:47:05 +0000
 > +++ b/sql/sql_select.cc	2011-01-06 08:01:54 +0000
 > @@ -13755,37 +13707,130 @@ 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
 > +        }
 > +      }
 > +    }
 > +  }
 > +
 > +  /*
 > +    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)
 > +    {
 > +      bool quick_created=
 > +        (select && select->quick &&
select->quick!=save_quick);
 > +
 > +      /*
 > +         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)
 > +      {
 > +        if (select)
 > +          select->quick= 0;          // Throw any existing quick select

jl: It wasn't obvious to me at first that here, select->quick must be
the same quick as save_quick, and that select->quick is therefore
either set back to save_quick or deleted by "delete save_quick" later.
I think you should add a comment that this is taken care of.

 > +
 > +        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)
 > +      {
 > +        QUICK_SELECT_DESC *tmp;
 >          /* ORDER BY range_key DESC */
 > -	tmp= new QUICK_SELECT_DESC((QUICK_RANGE_SELECT*)(select->quick),
 > +        tmp= new QUICK_SELECT_DESC((QUICK_RANGE_SELECT*)(select->quick),
 >                                      used_key_parts);
 > -	if (!tmp || tmp->error)
 > +        if (tmp && select->quick==save_quick)

jl: coding style: space before and after ==

 > +          save_quick= 0;    // ::QUICK_SELECT_DESC consumed it
 > +
 > +        if (!tmp || tmp->error)
 >  	{
 > -	  delete tmp;
 > -          select->quick= save_quick;
 > +          delete tmp;
 >            tab->limit= 0;
 > -	  DBUG_RETURN(0);		// Reverse sort not supported
 > +          goto need_filesort;		// Reverse sort not supported -> filesort
 >  	}
 > -	select->quick=tmp;
 > +        select->quick=tmp;

jl: coding style: space after =

 >        }
 > -    }
 > -    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
 > +      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

jl: since you're editing this line anyway, you might as well use space for
indenatation

 >
 > -	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;
 > +	  Use a traversal function that starts by reading the last row
 > +	  with key part (A) and then traverse the index backwards.

jl: since you're editing this line anyway, you might as well use space for
indenatation

 > +        */
 > +        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.

jl: please break line at <=80 characters

 > +    Delete the one that we wan't use.
 > +  */
 > +
 > +//skiped_sort_order:

jl: please remove line ^

 > +  // 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.1 branch (ole.john.aske:3534) Bug#59308Ole John Aske6 Jan
  • Re: bzr commit into mysql-5.1 branch (ole.john.aske:3534) Bug#59308Jorgen Loland10 Jan