maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

Igor Babaev <igor@xxxxxxxxxxxx> 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).