[Maria-developers] feedback on proposed fix to MySQL bug 57430
Hello all, Continuing with patch contributions (as encouraged at the storage engine summit last Friday), here is a very simple patch we have to fix MySQL bug 57430 (http://bugs.mysql.com/bug.php?id=57430). The problem is described in the bug report we sent to MySQL. We think this simple fix addresses the problem that we ran into, but we do not understand the code well enough to know if this fix is sufficient. Feedback is welcome. Thanks -Zardosht
Hi!
"Zardosht" == Zardosht Kasheff <zardosht@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
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 <monty@askmonty.org> wrote:
Hi!
"Zardosht" == Zardosht Kasheff <zardosht@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
Hi!
"Zardosht" == Zardosht Kasheff <zardosht@gmail.com> writes:
Zardosht> The issue that I see with this proposal is that if quick_records is Zardosht> much much greater than best_records, then we may not want to set Zardosht> best_key to nr even though this nr is a covering index. Agree, but this is what is happening with covering indexes anyway if they where found before a normal index. This comes from the code a bit higher up: if ((is_best_covering && !is_covering) || (is_covering && ref_key_quick_rows < select_limit)) continue; The first part of this test means that as soon as we have found a covering index, all other index are disregarded. My change fixes things so that all clustered indexes are treated equal, in spite of their position among the keys. The right way to fix this is to calculate the cost of the index and remove the testing of number of key parts and covering keys. I will look at this as soon as all my other 5.3 tasks are done. (ETA: 1-2 weeks) Regards, Monty
Hi, Will this fix also solve the bug http://bugs.mysql.com/bug.php?id=36817 (and reopened in maria as https://bugs.launchpad.net/maria/+bug/639949 ) ? Thanks and regards, Jocelyn Fournier Le 19/05/2011 16:46, Michael Widenius a écrit :
Hi!
"Zardosht" == Zardosht Kasheff<zardosht@gmail.com> writes: Zardosht> The issue that I see with this proposal is that if quick_records is Zardosht> much much greater than best_records, then we may not want to set Zardosht> best_key to nr even though this nr is a covering index.
Agree, but this is what is happening with covering indexes anyway if they where found before a normal index.
This comes from the code a bit higher up:
if ((is_best_covering&& !is_covering) || (is_covering&& ref_key_quick_rows< select_limit)) continue;
The first part of this test means that as soon as we have found a covering index, all other index are disregarded. My change fixes things so that all clustered indexes are treated equal, in spite of their position among the keys.
The right way to fix this is to calculate the cost of the index and remove the testing of number of key parts and covering keys.
I will look at this as soon as all my other 5.3 tasks are done. (ETA: 1-2 weeks)
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
participants (3)
-
Jocelyn Fournier -
Michael Widenius -
Zardosht Kasheff