From: Roy Lyseng
Date: August 12 2010 10:56am
Subject: Re: bzr commit into mysql-next-mr-opt-backporting branch (roy.lyseng:3219)
 Bug#50489
List-Archive: http://lists.mysql.com/commits/115567
Message-Id: <4C63D372.9060000@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit

On 12.08.10 11.07, Guilhem Bichot wrote:
> Hello,
>
> Roy Lyseng a écrit, Le 12.08.2010 10:32:
>>> Minor comments below, nothing critical, ok to push anyway.

>>>> @@ -1362,7 +1360,7 @@ static bool sj_table_is_included(JOIN *j
>>>> It may be possible to consolidate the materialization strategies into one.
>>>> The choice between the strategies is made by the join optimizer (see
>>>> - advance_sj_state() and fix_semijoin_strategies_for_picked_join_order()).
>>>> + advance_sj_state() and fix_semijoin_strategies()).
>>>> This function sets up all fields/structures/etc needed for execution except
>>>> for setup/initialization of semi-join materialization which is done in
>>>> setup_sj_materialization() (todo: can't we move that to here also?)
>>>> @@ -4873,6 +4871,17 @@ static bool optimize_semijoin_nests(JOIN
>>>> DBUG_ENTER("optimize_semijoin_nests");
>>>> List_iterator<TABLE_LIST> sj_list_it(join->select_lex->sj_nests);
>>>> TABLE_LIST *sj_nest;
>>>> +
>>>> + /*
>>>> + Perform necessary cleanup in case semijoin nests have been used in prior
>>>> + executions.
>>>> + */
>>>
>>> ideally cleanup should be done by the prior executions when they end (that's
>>> supposed to be the "clean house" principle in MySQL code: don't leave it to next
>>> inhabitants to clean up after you). I don't know whether it's safely possible.
>>
>> I like that principle too... In this case, I just went for the simple
>> solution, as I wanted a quick fix to see if the solution worked :)
>>
>> Will investigate more...
>
> thanks!

I am adding reset of sjm.positions to fix_semijoin_strategies...(). 
sjm-positions is not used in the optimization process after this function.

The sj_mat_info pointer is reset in JOIN::destroy().

But I keep the reset code in optimize_semijoin_nests(), just in case the cleanup 
code is never called ;)
>

>>>> + SJ_MATERIALIZATION_INFO* sjm;
>>>> + if (!(sjm= new SJ_MATERIALIZATION_INFO))
>>>> + DBUG_RETURN(TRUE);
>>>
>>> I find "sjm" is confusing: here it's used for a SJ_MATERIALIZATION_INFO object,
>>> whereas in structure st_semijoin_mat "sjm" is a st_semijoin_mat; and one goal of
>>> the patch is to separate both structs.
>>
>> Yes, standalone names must have more expressive names...
>>
>> How about sjm_exec? (sjm_info is just as bad as sjm...)
>
> yes, sjm_exec is good. Maybe sjm_opt for the optimization-related counterpart?

There is no standalone variable for optimizer sjm struct ;)
>
>>>> else if (sjm_strategy == SJ_OPT_MATERIALIZE_LOOKUP)
>>>> {
>>>> COST_VECT prefix_cost; - SJ_MATERIALIZATION_INFO *mat_info=
>>>> emb_sj_nest->sj_mat_info;
>>>> - signed int first_tab= (int)idx - mat_info->tables;
>>>> + signed int first_tab=
>>>
>>> this is not your code, but I wonder whether 'signed' is useful here.
>>
>> I inserted DBUG_ASSERT(first_tab >= 0); and got verification that first_tab
>> may become -1.
>
> right; it should not be unsigned, and plain "int" would do.

signed removed.

Thanks,
Roy