Re: bzr commit into mysql-next-mr-opt-backporting branch (guilhem:3208) Bug#54437

[Roy Lyseng]

On 11.08.10 15.58, Guilhem Bichot wrote:
> 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?

I was not completely sure about what to write, so I defaulted to "this 
condition". AFAIU, it is the condition attached to the current join_tab (which 
defaults to TRUE if pointer is NULL), but I am not sure on how to express that 
so it will be understood.
>
>>> + 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?

I meant this:
   /*
     The table join_tab is the first inner table of a outer join operation
     and no matches has been found for the current outer row.
   */


The first part of this should go as the parameter documentation. The second part 
is just repeating the function comment.

>
>>> }
>>> 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() ?

I think that "check whether..." can be derived from the code. But what does the 
assignment to first_unmatched really mean? This goes beyond your fix, so do not 
take it as a requirement.
>
>>> + 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);


Thanks,
Roy