List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:August 11 2010 1:58pm
Subject:Re: bzr commit into mysql-next-mr-opt-backporting branch (guilhem:3208)
Bug#54437
View as plain text  
Hello,

thanks for the review, please find below some small questions.

Roy Lyseng a écrit, Le 09.08.2010 16:04:
> 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).

good idea; will try and let you know.

>> === 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: "

What condition would "this condition" mean?
not_exists_optimize means that the optimizer has determined, long ago, 
that there is a condition so that if there is a matching row in "tab", 
the row of tab-1 should not be returned. Example:
   select * from t1 left join t2 on <condition>
   where <blah> and t2.a is null;
(assuming t2.a is declared as NOT NULL). In this example, if a matching 
row is found in t2, t2.a is surely not null, so this matching row is 
rejected, and as there is a matching row there will be no 
null-complemented rows, so this row of t1 cannot be in the result set.
So, I find that at the place where I write the comment, the user cannot 
easily see what condition I would be talking about.
What do you think?

>> +            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.

I could not find the comment in question: which "first comment" did you 
mean? could you please paste it here?

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

done

>>     /*
>> -    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;

done, like this:
   JOIN_TAB *first_unmatched= join_tab->first_unmatched->first_upper;
   if (first_unmatched != NULL &&
       first_unmatched->last_inner != join_tab)
     first_unmatched= NULL;
   join_tab->first_unmatched= first_unmatched;

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

Jorgen also asked for documentation of this same block. In my patch it 
has documentation:
   /*
     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().
   */
and in evaluate_join_record() it has comment:
       /*
         Check whether join_tab is not the last inner table
         for another embedding outer join.
       */
So somehow, the first comment suggest to look at the second comment.
Do you mean I should copy-paste the comment "check whether etc" into 
evaluate_null_etc() ?

>> +  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.

done

>>     */
>> -  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);
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