Igor Babaev <igor@askmonty.org> writes:
Here's my review.
Thanks a lot for your help! I checked through all your points (few detailed comments below), all your proposed solutions seem reasonable to me. In summary, we should as you suggest: - Rework patches for bugs 39022, 48483, and 49324 as you described (1,7,8). - Revert bug 45640 patch (5) and instead apply your proposed patch. - Revert patches for bugs 51242 and 52336, and instead apply your proposed fix (15,18). - Reject (eg. revert in 5.1-release) patch for bug 39653 (2) - Do nothing for bugs 40277, 45195, 45989, 49902, 50995, and 51494 (3,4,6,10,14,16), as necessary changes were already pushed to 5.1-release. - Do nothing for bugs 49829, 50335, 50591, 50843, and 52177 (9,11,12,13,17), as the patches for those are ok. You proposed on the call yesterday that you could prepare a patch with these changes for our merge tree: lp:~maria-captains/maria/5.1-release Please do so. I will try to get hold of Monty and discuss with him, but I think he will agree with making your proposed changes. If you need a full review for your changes, you will probably need to ask someone else than me, as I am unfamiliar with this part of the code. Thanks, - Kristian.
1. Bug #39022 (by Georgi Kodinov) ----------------------------------
SYNOPSIS Any call of SQL_SELECT::skip record ignore the fact that during evaluation of an expression (item) an error may occur.
CONCLUSION The patch cannot be applied as it is, requires some rework.
REASONS: The patch is incomplete: There are 5 calls of SQL_SELECT::skip_record in total: two - in sql_select.cc, and three others in filesort.cc, sql_delete.cc, sql_update.cc. Only the calls in sql_select.cc are handled in an error aware manner. The second call in sql_select is handled incorrectly: the error occurred at the evaluation of the select condition is caught only if the function returns false.
POSSIBLE SOLUTION: change the synopsis of SQL_SELECT::skip_record: int SQL_SELECT::skip_record() { int rc= test(cond && !cond->val_int()); if thd->is_error() rc=-1; return rc; }
(thd must be added to SQL_SELECT)
then after each call SQL_SELECT::retain_record add error handling for the cases when SQL_SELECT::skip_record returns -1: int rc= 0; if (!select || (rc= select->skip_record()) != 0) { if (rc < 0) { /* handle error returned by skip_record() */ ... } ... }
Agree with your proposed solution.
2. Bug #39653 (by Gleb Shchepa)
SYNOPSIS InnoDB covering primary index is used when using another covering index is more beneficial.
CONCLUSION The patch must be rejected.
REASONS: The patch is based on completely wrong idea that any covering secondary index is better than primary covering index for scanning in InnoDB.
Here an example demonstrating that it's not so: CREATE TABLE t1 ( a int, b int, c int, PRIMARY KEY (a, b), KEY idx (a,c) ); Both primary key and the secondary key here are covering for the query SELECT a FROM t1 WHERE a BETWEEN 1000 and 5000; Apparently scanning by the primary key will be faster here as it does not use random seeks.
The patch completely ignores non-InnoDB engines.
POSSIBLE SOLUTION: Cost based choice that takes into account that sequential access is C times faster then random access.
I am ok with rejecting the patch. It's hard to tell whether primary key or index will be better to use, in some cases secondary index may require no random seeks if it is not fragmented while primary key could be. In any case this seems a dangerous change in a stable release (user can force index of choice if he/she has more information about which will be best).
3. Bug #40277 (by Davi Arnaut) ------------------------------- see the notes from Monty's review
Ok, Monty already fixed this in 5.1-release. Agree.
4. 45195 (by Sergey Glukhov) ----------------------------- SYNOPSIS Reading uninitialized bytes from join cache buffer. (Valgrind's complain)
CONCLUSION The patch should be accepted as it is (Monty has some a comment on this patch though).
Agree, Monty already pushed his fixes to 5.1-release.
5. Bug 45640 (by Gleb Shchepa) ------------------------------- SYNOPSYS. Building Item_ref objects of a wrong type for outer references used in aliased expressions in a select with group by causes wrong results. Group by expressions containing references to aliases may cause wrong results.
CONCLUSION. The patch cannot be accepted as it is, requires a serious re-work.
REASON. Although the basic ideas behind the fix appear to be be valid their implementation is quite clumsy: -an unnecessary parameter is added to the function fix_inner_refs -the info about the syntax context of a field referenced is passed into Item_field::fix_fields in an unconventional and ugly manner -the group expression are traversed for each reference of the list of inner references
POSSIBLE SOLUTION See the patch at the very end of the post.
Agree with proposed solution, using the patch at end.
6. Bug #45989 (by Georgi Kodinov) ---------------------------------- This bug has been already fixed in MariaDB 5.1.44. Our fix is correct, the fix by Georgi is not quite correct (but not harmful).
Agree, yes the better fix is already pushed.
7. Bug #48483 (by Sergey Glukhov) ---------------------------------- (No public access)
SYNOPSIS Wrong calculation of table dependencies for join queries with outer join operation causes a crash.
CONCLUSION The patch can be accepted with one change that matters - if (!((prev_table->on_expr->used_tables() & ~RAND_TABLE_BIT) & - ~prev_used_tables)) + if (!((prev_table->on_expr->used_tables() & + ~(OUTER_REF_BIT | RAND_TABLE_BIT)) & + ~prev_used_tables))
I assume you mean OUTER_REF_TABLE_BIT here. Ok.
8. Bug #49324 (by Georgi Kodinov) ---------------------------------- SYNOPSIS With InnoDB a GROUP BY / ORDER BY can use an index extended by some number of major components of the primary. The value of rec_per_key for such extended indexes must be calculated in a special way.
CONCLUSION The patch could be accepted after changing the formula that calculates the value of rec_per_key for an extended index:
- rec_per_key= used_key_parts && - used_key_parts <= keyinfo->key_parts ? - keyinfo->rec_per_key[used_key_parts-1] : 1; + int used_index_parts= keyinfo->key_parts; + int used_pk_parts= 0; + set_if_bigger(used_pk_parts, + used_key_parts-used_index_parts); + rec_per_key= keyinfo->rec_per_key[used_key_parts-1]; + if (used_pk_parts) + { + KEY *pkinfo= tab->table->key_info+table->s->primary_key; + rec_per_key*= pkinfo->rec_per_key[used_pk_parts-1]; + rec_per_key/= pkinfo->rec_per_key[0]; + }
REASONS The formula in the patch does not take into account how many components of the of the primary key is used in the extended index.
Ok (I don't understand the calculation, but not knowing the code I'm willing to take your word for it).
9. Bug #49829 (by Staale Smedseng) ---------------------------------- SYNOPSYS Compiler problems (warnings) for a platform
CONCLUSION THe patch is ok.
Agree (it was kind of nice to see the explanation for these warnings, which I think I saw before but didn't know what meant).
10. Bug #49902 (by Sergey Vojtovich) ----------------------------------- See the comments/suggestions from Monty's review
Yes, this is pushed to 5.1-release (and I agree with Monty's comment).
11. Bug #50335 (by Alexey Kopytov) ----------------------------------- (No public access)
SYNOPSYS Failure of a wrong assertion
CONCLUSION The patch is ok.
Ok.
12. Bug #50591 (by Sergey Glukhov) ----------------------------------- SYNOPSIS Wrong result for a grouping query over a table with a BIT field
CONCLUSION The patch is ok.
Ok.
13. Bug #50843 (by Evgeny Potemkin) ------------------------------------ SYNOPSIS Performance degradation problem when join cache + filesort are used intead of a full index scan by a primary key.
CONCLUSION The patch looks ok.
Ok.
14. Bug #50995 (by Sergey Glukhov) ------------------------------------ SYNOPSIS A badly formed list for conditions causes wrong query results.
CONCLUSION The patch looks ok for me. See also Monty's recommendation from his review.
Yes. As far as I can see, Monty pushed his changes from his review to 5.1-release. He did however not update the comments for eliminate_item_equal() as per his suggestion (maybe he forgot): "Note that we should update the function comment for eliminate_item_equal() as this can't return 0. (If it would, then other things would break, just look at how this function is used)."
15. Bug #51242 (by Sergey Glukhov) ----------------------------------- SYNOPSYS The conjuncts that become false after substitution of the constant tables are ignored.
CONCLUSION The fix should be turned down (but not the test case).
REASON See the reasons for turning the fix for bug #52336 that is a correction for this patch.
POSSIBLE SOLUTION See the solution for bug #52336.
16. Bug #51494 (by Sergey Glukhov) ----------------------------------- SYNOPSIS Crash with explain of a query with outer join
CONCLUSION The patch must be turned down.
REASONS The patch triggers bug #53334 - a failure of a base join query for InnoDB. The bug is actually fixed by the patch for bug #52177 (see my comment for bug #53334).
Yes, it is already reverted in our tree.
17. Bug #52177 (by Sergey Glukhov) ----------------------------------- SYNOPSIS Crash with exaplain for a query with an outer join.
CONCLUSION The fix is correct and the patch should be applied. The patch also fixes the bug #51494.
Ok.
18. Bug #52336 (by Sergey Glukhov) ----------------------------------- SYNOPSYS A crash caused by an invalid fix for bug #51242.
CONCLUSION The patch rather should be turned down.
REASON. The patch does not fix the real cause of the problem: a wrong value is passed as a parameter in the call Item* sort_table_cond= make_cond_for_table(curr_join->tmp_having, used_tables, used_tables); The patch actually suggest a work-around that hides the bug. This work-around simultaneously adds a new feature. that catches impossible HAVINGs after constant table substitution. Yet impossible WHEREs appeared after this optimization remain uncaught. So the feature is introduced half-baked.
POSSIBLE SOLUTION - Item* sort_table_cond= make_cond_for_table(curr_join->tmp_having, - used_tables, - used_tables); + Item* sort_table_cond= make_cond_for_table(curr_join->tmp_having, + used_tables, + (table_map) 0);
Ok. This possible solution is not yet applied to our tree (we only discussed it so far).