List:Commits« Previous MessageNext Message »
From:Roy Lyseng Date:August 9 2010 2:04pm
Subject:Re: bzr commit into mysql-next-mr-opt-backporting branch (guilhem:3208)
Bug#54437
View as plain text  
Hi Guilhem,

fix is approved, but please have a look at the minor comments below.


On 28.06.10 22.43, Guilhem Bichot wrote:
> #At file:///home/mysql_src/bzrrepos_new/opt-back-52636/ based on
> revid:guilhem@stripped
>
>   3208 Guilhem Bichot	2010-06-28
>        Fix for Bug#54437 "Extra rows with LEFT JOIN + semijoin (firstmatch and
> duplicates weedout)":
>        convergence between evaluate_join_record() and
> evaluate_null_complemented_record().
>       @ mysql-test/include/subquery_sj.inc
>          Without the code fix, main.subquery_sj_dupsweed,
> main.subquery_sj_firstmatch
>          and main.subquery_sj_loosescan would return 4 1's instead of 2.
>          There was no problem at jcl6/7 because the recent fix for BUG 54235 had
>          already aligned JOIN_CACHE::join_null_complements() with
>          JOIN_CACHE::join_matching_records().

It does not seem that any of the tests invoke the FirstMatch strategy. It would 
be good to have coverage for that situation (but not required).

>       @ sql/sql_select.cc
>          During execution, Firstmatch and Duplicates weedout are handled in
>          evaluate_join_record() (function which applies to matching records), not
>          in evaluate_null_complemented_record() (function which applies to
>          non-matching records). Thus, if the semijoin inner expression is a LEFT
>          JOIN yielding NULLs (see subquery_sj.inc: t3 columns are filled with
>          NULLs), evaluate_null_complemented_record() for t3 goes to the final
>          end_send(), without any firstmatch or duplicates weedout treatment, so
>          duplicate rows are returned.
>          The easy and safe fix would be to put in
>          evaluate_null_complemented_record() a copy of the code of
>          evaluate_join_record() which handles firstmatch and duplicates
>          weedout. In an attempt to reduce code size and unify execution flows,
>          the chosen fix is rather to make evaluate_null_complemented_record()
>          call evaluate_join_record(). The first function is responsible for
>          creating NULL values for all inner tables at the right of LEFT JOIN, and
>          for testing pushed down conditions, from then on this record is sent to
>          the second function for more complete evaluation of it (firstmatch
>          etc). This should minimize future distortion between the two functions.
>          Note that the loop over first_unmatched, in
>          evaluate_null_complemented_record(), which this patch shortens to its
>          first iteration, is already present in evaluate_join_record().
>
>      modified:
>        mysql-test/include/subquery_sj.inc
>        mysql-test/r/subquery_sj_all.result
>        mysql-test/r/subquery_sj_all_jcl6.result
>        mysql-test/r/subquery_sj_all_jcl7.result
>        mysql-test/r/subquery_sj_dupsweed.result
>        mysql-test/r/subquery_sj_dupsweed_jcl6.result
>        mysql-test/r/subquery_sj_dupsweed_jcl7.result
>        mysql-test/r/subquery_sj_firstmatch.result
>        mysql-test/r/subquery_sj_firstmatch_jcl6.result
>        mysql-test/r/subquery_sj_firstmatch_jcl7.result
>        mysql-test/r/subquery_sj_loosescan.result
>        mysql-test/r/subquery_sj_loosescan_jcl6.result
>        mysql-test/r/subquery_sj_loosescan_jcl7.result
>        mysql-test/r/subquery_sj_mat.result
>        mysql-test/r/subquery_sj_mat_jcl6.result
>        mysql-test/r/subquery_sj_mat_jcl7.result
>        mysql-test/r/subquery_sj_mat_nosj.result
>        mysql-test/r/subquery_sj_none.result
>        mysql-test/r/subquery_sj_none_jcl6.result
>        mysql-test/r/subquery_sj_none_jcl7.result
>        sql/sql_select.cc

Clipped lots of test output...

> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2010-06-17 09:31:07 +0000
> +++ b/sql/sql_select.cc	2010-06-28 20:43:32 +0000
> @@ -16992,7 +16992,15 @@ evaluate_join_record(JOIN *join, JOIN_TA
>         for (JOIN_TAB *tab= first_unmatched; tab<= join_tab; tab++)
>         {
>           if (tab->table->reginfo.not_exists_optimize)
> +        {
> +          /*
> +            We have a match in 'tab' but wanted none: no need to explore this

Suggest rewrite; "We have a match in 'tab' but wanted none: " ->
"We have a match in 'tab' that causes this condition to evaluate to FALSE: "
> +            record and other records of 'tab', so we return "no records". But
> +            as we set 'found' above, evaluate_join_record() at the upper level
> +            will not yield a NULL-complemented record.
> +          */
>             DBUG_RETURN(NESTED_LOOP_NO_MORE_ROWS);
> +        }
>           /* Check all predicates that has just been activated. */
>           /*
>             Actually all predicates non-guarded by first_unmatched->found
> @@ -17165,38 +17173,23 @@ evaluate_null_complemented_join_record(J

Suggestion: Take the first comment in the code body and create a doxygen 
parameter comment out of it.

>     }
>     join_tab--;

Suggestion: The above line would be more explicit with
    join_tab= last_inner_tab;

>     /*
> -    The row complemented by nulls might be the first row
> -    of embedding outer joins.
> -    If so, perform the same actions as in the code
> -    for the first regular outer join row above.
> +    From the point of view of the rest of execution, this record matches
> +    (it has been built and satisfies conditions, no need to do more evaluation
> +    on it). See similar code in evaluate_join_record().
>     */
> -  for ( ; ; )
> -  {
> -    JOIN_TAB *first_unmatched= join_tab->first_unmatched;
> -    if ((first_unmatched= first_unmatched->first_upper)&&
> -        first_unmatched->last_inner != join_tab)
> -      first_unmatched= 0;
> -    join_tab->first_unmatched= first_unmatched;
> -    if (!first_unmatched)
> -      break;
> -    first_unmatched->found= 1;
> -    for (JOIN_TAB *tab= first_unmatched; tab<= join_tab; tab++)
> -    {
> -      if (tab->select_cond&&  !tab->select_cond->val_int())
> -      {
> -        join->return_tab= tab;
> -        DBUG_RETURN(NESTED_LOOP_OK);
> -      }
> -    }
> -  }
> +  JOIN_TAB *first_unmatched= join_tab->first_unmatched;
> +  if ((first_unmatched= first_unmatched->first_upper)&&
> +      first_unmatched->last_inner != join_tab)
 > +    first_unmatched= 0;

Suggestion: Rewrite the above four lines like this:
   JOIN_TAB *first_unmatched= join_tab->first_unmatched->first_upper;
   if (first_unmatched && first_unmatched->last_inner != join_tab)
     first_unmatched= NULL;

If possible, please document what this code block does...

> +  join_tab->first_unmatched= first_unmatched;
>     /*
>       The row complemented by nulls satisfies all conditions
>       attached to inner tables.
> -    Send the row complemented by nulls to be joined with the
> -    remaining tables.
> +    Finish evaluation of record and send it to be joined with
> +    remaining tables:

Please add something like:
Note that evaluate_join_record will re-evaluate the condition attached to the 
last inner table of the current outer join. This is not deemed to have a 
significant performance impact.
>     */
> -  enum_nested_loop_state nls= (*join_tab->next_select)(join, join_tab+1, 0);
> -  DBUG_RETURN(nls);
> +  const enum_nested_loop_state rc= evaluate_join_record(join, join_tab, 0);
> +  DBUG_RETURN(rc);
>   }

Thanks,
Roy
Thread
bzr commit into mysql-next-mr-opt-backporting branch (guilhem:3208) Bug#54437Guilhem Bichot28 Jun
Re: bzr commit into mysql-next-mr-opt-backporting branch(guilhem:3208) Bug#54437Jørgen Løland30 Jun
  • Re: bzr commit into mysql-next-mr-opt-backporting branch(guilhem:3208) Bug#54437Guilhem Bichot30 Jun
  • Re: bzr commit into mysql-next-mr-opt-backporting branch(guilhem:3208) Bug#54437Guilhem Bichot1 Jul
    • Re: bzr commit into mysql-next-mr-opt-backporting branch(guilhem:3208) Bug#54437Guilhem Bichot7 Jul
      • Re: bzr commit into mysql-next-mr-opt-backporting branch (guilhem:3208)Bug#54437Guilhem Bichot12 Aug
  • Re: bzr commit into mysql-next-mr-opt-backporting branch (guilhem:3208)Bug#54437Jorgen Loland9 Aug
Re: bzr commit into mysql-next-mr-opt-backporting branch (guilhem:3208)Bug#54437Roy Lyseng9 Aug
  • Re: bzr commit into mysql-next-mr-opt-backporting branch (guilhem:3208)Bug#54437Guilhem Bichot11 Aug
    • Re: bzr commit into mysql-next-mr-opt-backporting branch (guilhem:3208)Bug#54437Roy Lyseng11 Aug
      • Re: bzr commit into mysql-next-mr-opt-backporting branch (guilhem:3208)Bug#54437Guilhem Bichot11 Aug
        • Re: bzr commit into mysql-next-mr-opt-backporting branch (guilhem:3208)Bug#54437Guilhem Bichot12 Aug