Looks OK, but I have two suggestions that I think you should consider:
On 07/21/10 02:45 PM, Tor Didriksen wrote:
> #At file:///export/home/didrik/repo/next-mr-opt-backporting-valgrind/
based on revid:roy.lyseng@stripped
>
> 3217 Tor Didriksen 2010-07-21
> Bug #55042 valgrind reports error in best_access_path when
running subquery_mat_all
>
> The code was using loose_scan_pos, even though it was
explicitly set to invalid.
> @ sql/sql_select.cc
> Do not use loose_scan_pos in optimize_wo_join_buffering() if
it has a cost of DBL_MAX (as determined by best_access_path())
>
> Remove sj_inner_fanout output argument, it was unused.
>
> modified:
> sql/sql_select.cc
> === modified file 'sql/sql_select.cc'
...
> - if ((i == first_tab&& first_alt))
> + if (i == first_tab&& first_alt)
> + {
> + if (loose_scan_pos.read_time == DBL_MAX)
> + {
> + /* Loose scan not appliccable. */
> + *reopt_rec_count = DBL_MAX;
> + *reopt_cost= DBL_MAX;
> + DBUG_VOID_RETURN;
> + }
> pos= loose_scan_pos;
> + }
I suggest doing the test for DBL_MAX not only for loose_scan_pos, but
for all pos. That would catch any cases where pos.read_time is
DBL_MAX, in which case I think we also should avoid adding it to
cost. In other words:
if (i == first_tab && first_alt)
pos= loose_scan_pos;
if (pos.read_time == DBL_MAX)
{
*reopt_rec_count = DBL_MAX;
*reopt_cost= DBL_MAX;
DBUG_VOID_RETURN;
}
>
> reopt_remaining_tables&= ~rs->table->map;
> cost += pos.read_time;
...
> + if (reopt_cost == DBL_MAX)
> + {
> + DBUG_PRINT("info", ("Cannot use LooseScan"));
> + }
> + else
> + {
> + /*
> + We don't yet have any other strategies that could handle this
> + semi-join nest (the other options are Duplicate Elimination or
> + Materialization, which need at least the same set of tables in
> + the join prefix to be considered) so unconditionally pick the
> + LooseScan.
> + */
> + pos->sj_strategy= SJ_OPT_LOOSE_SCAN;
> + *current_read_time= reopt_cost;
> + *current_record_count= reopt_rec_count;
> + handled_by_fm_or_ls= first->table->emb_sj_nest->sj_inner_tables;
> + }
I am not sure the extra test for DBL_MAX is necessary since one of the
other strategies will surely be cheaper. But if you insists, I like
it better the other way around, but that is a mattter of taste:
if (reopt_cost < DBL_MAX)
{
/*
We don't yet have any other strategies that could handle this
semi-join nest (the other options are Duplicate Elimination or
Materialization, which need at least the same set of tables in
the join prefix to be considered) so unconditionally pick the
LooseScan.
*/
pos->sj_strategy= SJ_OPT_LOOSE_SCAN;
*current_read_time= reopt_cost;
*current_record_count= reopt_rec_count;
handled_by_fm_or_ls= first->table->emb_sj_nest->sj_inner_tables;
}
else
{
DBUG_PRINT("info", ("Cannot use LooseScan"));
}
--
Øystein
