From: Jorgen Loland
Date: January 10 2011 8:50am
Subject: Re: bzr commit into mysql-5.1 branch (ole.john.aske:3534) Bug#59308
List-Archive: http://lists.mysql.com/commits/128253
Message-Id: <4D2AC861.4040704@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit
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
... ORDER BY 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