From: Guilhem Bichot
Date: August 12 2010 1:11pm
Subject: Re: bzr commit into mysql-next-mr-opt-backporting branch (roy.lyseng:3228)
 Bug#50489
List-Archive: http://lists.mysql.com/commits/115583
Message-Id: <4C63F2E7.2020501@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit

Roy Lyseng a écrit, Le 12.08.2010 14:15:
> #At file:///home/rl136806/mysql/repo/mysql-work5/ based on revid:guilhem.bichot@stripped
> 
>  3228 Roy Lyseng	2010-08-12
>       Bug#50489: another segfault in fix_semijoin_strategies...

> @@ -3250,6 +3248,12 @@ JOIN::destroy()
>    if (exec_tmp_table2)
>      free_tmp_table(thd, exec_tmp_table2);
>    destroy_sj_tmp_tables(this);
> +  List_iterator<TABLE_LIST> sj_list_it(select_lex->sj_nests);
> +  TABLE_LIST *sj_nest;
> +  while ((sj_nest= sj_list_it++))
> +  {

{ and } could optionally be removed.

> +    sj_nest->sj_mat_info= NULL;
> +  }

seeing the destroy_sj_tmp_tables() above, I think you chose a good place.

> @@ -4890,14 +4894,16 @@ 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;
> -  /*
> -    The statement may have been executed with 'semijoin=on' earlier.
> -    We need to verify that 'semijoin=on' still holds.
> -   */
> -  if (join->thd->optimizer_switch_flag(OPTIMIZER_SWITCH_SEMIJOIN) &&
> -      join->thd->optimizer_switch_flag(OPTIMIZER_SWITCH_MATERIALIZATION))
> +
> +  while ((sj_nest= sj_list_it++))
>    {
> -    while ((sj_nest= sj_list_it++))
> +    /* As a precaution, reset pointers that were used in prior execution */
> +    sj_nest->sj_mat_info= NULL;
> +    sj_nest->nested_join->sjm.positions= NULL;

ok, or just DBUG_ASSERT that they are already NULL as they should be.
My humble opinion is that if we keep the reset at start like above, 
resetting at end of the prior execution is superfluous, it just wastes 
CPU time and lines of code. I would have preffered only resets at end of 
the prior execution.
But other people have different opinions about such "defensive 
programming", and I wasted your time enough on this micro-topic, so do 
as you prefer :-)

> @@ -8413,6 +8433,13 @@ static void fix_semijoin_strategies_for_
>        pos->sj_strategy= SJ_OPT_NONE;
>      remaining_tables |= s->table->map;
>    }
> +
> +  List_iterator<TABLE_LIST> sj_list_it(join->select_lex->sj_nests);
> +  TABLE_LIST *sj_nest;
> +  while ((sj_nest= sj_list_it++))
> +    sj_nest->nested_join->sjm.positions= NULL;

optionally there could be a comment stating that this sjm or positions 
are not needed anymore.

Ok to push.