On Mon, Jul 26, 2010 at 5:47 PM, Guilhem Bichot
<guilhem.bichot@stripped>wrote:
> Hello Tor,
>
> Tor Didriksen a écrit, Le 22.07.2010 09:06:
>
> #At file:///export/home/didrik/repo/next-mr-opt-backporting-valgrind/
>> based on revid:roy.lyseng@stripped
>>
>> 3217 Tor Didriksen 2010-07-22
>> 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
>> Terminate search in optimize_wo_join_buffering() if we 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'
>> --- a/sql/sql_select.cc 2010-07-16 12:21:31 +0000
>> +++ b/sql/sql_select.cc 2010-07-22 07:06:33 +0000
>> @@ -13299,8 +13299,6 @@ static bool check_interleaving_with_nj(J
>> table
>> reopt_rec_count OUT New output record count
>> reopt_cost OUT New join prefix cost
>> - sj_inner_fanout OUT Fanout in the [first_tab; last_tab] range
>> that
>> - is produced by semi-join-inner tables.
>> DESCRIPTION
>> Given a join prefix [0; ... first_tab], change the access to the
>> tables
>> @@ -13317,23 +13315,19 @@ static bool check_interleaving_with_nj(J
>> void optimize_wo_join_buffering(JOIN *join, uint first_tab, uint
>> last_tab, table_map last_remaining_tables,
>> bool first_alt, uint no_jbuf_before,
>> - double *reopt_rec_count, double
>> *reopt_cost,
>> - double *sj_inner_fanout)
>> + double *reopt_rec_count, double
>> *reopt_cost)
>> {
>> - double cost, outer_fanout, inner_fanout= 1.0;
>> + double cost=0.0, outer_fanout=1.0, inner_fanout= 1.0;
>> table_map reopt_remaining_tables= last_remaining_tables;
>> uint i;
>> + DBUG_ENTER("optimize_wo_join_buffering");
>> +
>> if (first_tab > join->const_tables)
>> {
>> cost= join->positions[first_tab - 1].prefix_cost.total_cost();
>> outer_fanout= join->positions[first_tab - 1].prefix_record_count;
>> }
>> - else
>> - {
>> - cost= 0.0;
>> - outer_fanout= 1;
>> - }
>>
>
> moving this "else" block to unconditional initializations is more a
> matter of taste than a bugfix. I liked the original code better: it says
> that if the table is the first in join order, the cost so far is 0, and
> the fanout is 1, that's logical. Whereas the new code sets to 0 and 1
> and corrects that if the table is not first.
> But that is a detail, you can keep your change if you like it.
rolled back
>
>
> for (i= first_tab; i <= last_tab; i++)
>> reopt_remaining_tables |= join->positions[i].table->table->map;
>> @@ -13346,16 +13340,28 @@ void optimize_wo_join_buffering(JOIN *jo
>> if ((i == first_tab && first_alt) ||
>> join->positions[i].use_join_buffer)
>> {
>> /* Find the best access method that would not use join buffering */
>> - best_access_path(join, rs, reopt_remaining_tables, i, -
>> test(i < no_jbuf_before), inner_fanout*outer_fanout,
>> - &pos, &loose_scan_pos);
>> + best_access_path(join,
>> + rs,
>> + reopt_remaining_tables,
>> + i, + test(i <
>> no_jbuf_before),
>>
>
> This isn't your code, but this test() is superfluous, we could just use
> "i < no_jbuf_before", it is up to you.
>
agree
>
> + inner_fanout*outer_fanout,
>>
>
> This isn't your code, but the coding style asks for spaces around *.
>
done
>
> + &pos,
>> + &loose_scan_pos);
>>
>
> That makes 8 lines out of 3, and I found the 3 were sufficiently
> readable, so... do you really want this layout change?
>
>
reverted
>
> }
>> else pos= join->positions[i];
>> - if ((i == first_tab && first_alt))
>> + if (i == first_tab && first_alt)
>> pos= loose_scan_pos;
>> + if (pos.read_time == DBL_MAX)
>>
>
> I suggest adding a comment describing this condition (it must be about
> something being impossible, though I don't picture myself what an
> "impossible pos" can be).
done
>
>
> + {
>> + *reopt_rec_count = DBL_MAX;
>>
>
> no space before =
OK
>
>
> + *reopt_cost= DBL_MAX;
>> + DBUG_VOID_RETURN;
>> + }
>> +
>> reopt_remaining_tables &= ~rs->table->map;
>>
>
> Should this arithmetic above be moved to before the if(), to mark that
> the table has been handled? Or maybe we can rather say that it has not
> been handled?
>
I do not understand this comment.
reopt_remaining_tables is a local variable.
>
> cost += pos.read_time;
>>
>
> @@ -13525,11 +13533,10 @@ void advance_sj_state(JOIN *join, table_
>> Got a complete FirstMatch range.
>> Calculate correct costs and fanout
>> */
>> - double reopt_cost, reopt_rec_count, sj_inner_fanout;
>> + double reopt_cost, reopt_rec_count;
>> optimize_wo_join_buffering(join, pos->first_firstmatch_table, idx,
>> remaining_tables, FALSE, idx,
>> - &reopt_rec_count, &reopt_cost, -
>> &sj_inner_fanout);
>> + &reopt_rec_count, &reopt_cost);
>> /*
>> We don't yet know what are the other strategies, so pick the
>> FirstMatch.
>> @@ -13589,7 +13596,7 @@ void advance_sj_state(JOIN *join, table_
>> first=join->positions + pos->first_loosescan_table; uint
>> n_tables= my_count_bits(first->table->emb_sj_nest->sj_inner_tables);
>> /* Got a complete LooseScan range. Calculate its cost */
>> - double reopt_cost, reopt_rec_count, sj_inner_fanout;
>> + double reopt_cost, reopt_rec_count;
>> /*
>> The same problem as with FirstMatch - we need to save POSITIONs
>> somewhere but reserving space for all cases would require too
>> @@ -13600,18 +13607,23 @@ void advance_sj_state(JOIN *join, table_
>> TRUE, //first_alt
>> pos->first_loosescan_table + n_tables,
>> &reopt_rec_count, -
>> &reopt_cost, &sj_inner_fanout);
>> - /*
>> - 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;
>> + &reopt_cost);
>> + 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"));
>> }
>> }
>>
>
> The function comment of optimize_wo_join_buffering() does not suggest
> that this function can "fail", I mean, can return a plan with DBL_MAX
> which apparently means "not usable". I think the comment should say that
> this is possible. And, is such "failure" possible only if first_alt is
> true? If yes, I would state it in the comment. If no, i.e. it can happen
> with first_alt==false, then the code for SJ_OPT_FIRSTMATCH:
if best_access_path returns DBL_MAX it does not make sense to continue
cost+= DBL_MAX becomes infinite
>
> optimize_wo_join_buffering(join, pos->first_firstmatch_table, idx,
> remaining_tables, FALSE, idx,
> &reopt_rec_count, &reopt_cost,
> &sj_inner_fanout);
> <cut comment>
> pos->sj_strategy= SJ_OPT_FIRST_MATCH;
>
> *current_read_time= reopt_cost;
> *current_read_time= reopt_cost;
> *current_record_count= reopt_rec_count;
> handled_by_fm_or_ls= pos->firstmatch_need_tables;
> is incorrect (needs to test for reopt_cost==DBL_MAX).
Added test for this case as well.
-- didrik
>
>
>
> --
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:
> http://lists.mysql.com/commits?unsub=1
>
>
