The issue that I see with this proposal is that if quick_records is much much greater than best_records, then we may not want to set best_key to nr even though this nr is a covering index.
On Thu, May 19, 2011 at 6:04 AM, Michael Widenius <mo...@askmonty.org> wrote: > > Hi! > >>>>>> "Zardosht" == Zardosht Kasheff <zardo...@gmail.com> writes: > > Zardosht> Hello all, > Zardosht> Continuing with patch contributions (as encouraged at the storage > Zardosht> engine summit last Friday), here is a very simple patch we have to > fix > Zardosht> MySQL bug 57430 (http://bugs.mysql.com/bug.php?id=57430). > > Zardosht> The problem is described in the bug report we sent to MySQL. We > think > Zardosht> this simple fix addresses the problem that we ran into, but we do > not > Zardosht> understand the code well enough to know if this fix is sufficient. > > Zardosht> Feedback is welcome. > >> +++ sql/sql_select.cc (revision 25793) >> @@ -13964,7 +13964,8 @@ >> if (best_key < 0 || >> (select_limit <= min(quick_records,best_records) ? >> keyinfo->key_parts < best_key_parts : >> - quick_records < best_records)) >> + quick_records < best_records) || >> + (quick_records == best_records && !is_best_covering && >> is_covering)) >> { >> best_key= nr; >> best_key_parts= keyinfo->key_parts; > > The above suggestion is not good as it depends on > quick_records == best_records which is not likely to be the same for > two indexes for any larger tables. > > In the same loop code we have the test: > if ((is_best_covering && !is_covering) || ...) > continue; > > This is done without any testing of cost and to make things symetrical > we could just remove the 'quick_records == best_records' from the > above patch. > > A better solution is to try to calculate the cost of using covered and > not covered keys. This will allow us to use covering keys also when we > will hit a bit more rows that way. > > However, this is not an easy task (thought and talked about this for > 2-4 hours) so I will leave the cost calculation to after 5.3. > > The change I did was simple: > >> + quick_records < best_records) || >> + (!is_best_covering && is_covering)) > > This makes covering indexes work identical independent of their > position in the key array. > > After 5.3 I will change the filesort code to calculate the true cost > for each index (instead of as now do the decision based of number of > key parts, which doesn't make any sense). > > I expect my changes to be pushed today into 5.3 > > Regards, > Monty > _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
