From: Guilhem Bichot Date: July 26 2010 3:47pm Subject: Re: bzr commit into mysql-next-mr-opt-backporting branch (tor.didriksen:3217) Bug#55042 List-Archive: http://lists.mysql.com/commits/114382 Message-Id: <4C4DAE19.2000707@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8bit 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. > 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. > + inner_fanout*outer_fanout, This isn't your code, but the coding style asks for spaces around *. > + &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? > } > 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). > + { > + *reopt_rec_count = DBL_MAX; no space before = > + *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? > 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: optimize_wo_join_buffering(join, pos->first_firstmatch_table, idx, remaining_tables, FALSE, idx, &reopt_rec_count, &reopt_cost, &sj_inner_fanout); 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).