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
