From: Roy Lyseng
Date: October 29 2010 2:57pm
Subject: Re: bzr commit into mysql-next-mr-bugfixing branch (roy.lyseng:3270)
 Bug#52068
List-Archive: http://lists.mysql.com/commits/122316
Message-Id: <4CCAE0C5.7090709@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit

Thank you for the review, Øystein!

On 28.10.10 13.02, Øystein Grøvlen wrote:
> Roy,
>
> I looked a bit further into why it seems we need to set function pointers etc.
> for each scan. It appears that init_read_record resets these variables. That
> made me look into what part of init_read_record is actually needed for each
> scan, and it seems the only thing is the call to ha_rnd_init(). Hence, an
> alternative is to keep the current initialization as is, and instead add a call
> to ha_rnd_init() in the scan block. I have tested this and it seems to work.
> What do you think?

Yes, I think that this looks like a better solution. Please see the next commit.

Roy
>
> Test result changes looks OK. See inline for comments to the existing solution.
>
> --
> Øystein
>
> On 10/26/10 04:15 PM, Roy Lyseng wrote:
>> #At file:///home/rl136806/mysql/repo/mysql-work5/ based on
>> revid:roy.lyseng@stripped
>>
>> 3270 Roy Lyseng 2010-10-26
>> Bug#52068: Optimizer generates invalid semijoin materialization plan
>>
>> When MaterializeScan semijoin strategy was used and there were one
>> or more outer dependent tables before the semijoin tables, the scan
>> over the materialized table was not properly reset for each row of
>> the prefix outer tables.
>>
>> Example: suppose we have a join order:
>>
>> ot1 SJ-Mat-Scan(it2 it3) ot4
>>
>> Notice that this is called a MaterializeScan, even though there is an
>> outer table ahead of the materialized tables. Usually a MaterializeScan
>> has the outer tables after the materialized table, but this is
>> a special (but legal) case with outer dependent tables both before and
>> after the materialized table.
>>
>> For each qualifying row from ot1, a new scan over the materialized
>> table must be set up. The code failed to do that, so all scans after
>> the first one returned zero rows from the materialized table.
>>
>> mysql-test/include/subquery_sj.inc
>> Bug#52068: Optimizer generates invalid semijoin materialization plan
>> Added new test.
>>
>> mysql-test/r/subquery_sj_all.result
>> mysql-test/r/subquery_sj_all_jcl6.result
>> mysql-test/r/subquery_sj_all_jcl7.result
>> mysql-test/r/subquery_sj_dupsweed.result
>> mysql-test/r/subquery_sj_dupsweed_jcl6.result
>> mysql-test/r/subquery_sj_dupsweed_jcl7.result
>> mysql-test/r/subquery_sj_firstmatch.result
>> mysql-test/r/subquery_sj_firstmatch_jcl6.result
>> mysql-test/r/subquery_sj_firstmatch_jcl7.result
>> mysql-test/r/subquery_sj_loosescan.result
>> mysql-test/r/subquery_sj_loosescan_jcl6.result
>> mysql-test/r/subquery_sj_loosescan_jcl7.result
>> mysql-test/r/subquery_sj_mat.result
>> mysql-test/r/subquery_sj_mat_jcl6.result
>> mysql-test/r/subquery_sj_mat_jcl7.result
>> mysql-test/r/subquery_sj_mat_nosj.result
>> mysql-test/r/subquery_sj_none.result
>> mysql-test/r/subquery_sj_none_jcl6.result
>> mysql-test/r/subquery_sj_none_jcl7.result
>> Bug#52068: Optimizer generates invalid semijoin materialization plan
>> Earlier wrong semijoin materialization test results are corrected.
>>
>> sql/sql_select.cc
>> Bug#52068: Optimizer generates invalid semijoin materialization plan
>>
>> In sub_select_sjm(), moved code that initializes the scan over the
>> materialized table so that it is now performed for each scan of
>> table, instead of only for the first scan.
>>
>
> ...
>
>> === modified file 'sql/sql_select.cc'
>> --- a/sql/sql_select.cc 2010-10-26 10:43:50 +0000
>> +++ b/sql/sql_select.cc 2010-10-26 14:15:32 +0000
>> @@ -16954,28 +16954,27 @@ sub_select_sjm(JOIN *join, JOIN_TAB *joi
>> Ok, materialization finished. Initialize the access to the temptable
>> */
>> sjm->materialized= TRUE;
>> +
>> join_tab->read_record.read_record= join_no_more_records;
>> - if (sjm->is_scan)
>> - {
>> - /* Initialize full scan */
>> - JOIN_TAB *last_tab= join_tab + (sjm->table_count - 1);
>> - init_read_record(&last_tab->read_record, join->thd,
>> - sjm->table, NULL, TRUE, TRUE, FALSE);
>> -
>> - DBUG_ASSERT(last_tab->read_record.read_record == rr_sequential);
>
> Based on that init_read_record resets read_record.read_record maybe this assert
> should also be moved. If rr_sequential is not chosen, it probably does not make
> sense to attempt a sequential scan.
>
>> - last_tab->read_first_record= join_read_record_no_init;
>> - last_tab->read_record.copy_field= sjm->copy_field;
>> - last_tab->read_record.copy_field_end= sjm->copy_field +
>> - sjm->table_cols.elements;
>> - last_tab->read_record.read_record= rr_sequential_and_unpack;
>> - }
>> }
>>
>> if (sjm->is_scan)
>> {
>> - /* Do full scan of the materialized table */
>> + /*
>> + For each prefix partial record (if any), initialize and perform
>> + a full scan of the materialized table.
>> + */
>> JOIN_TAB *last_tab= join_tab + (sjm->table_count - 1);
>>
>> + init_read_record(&last_tab->read_record, join->thd,
>> + sjm->table, NULL, TRUE, TRUE, FALSE);
>> +
>> + last_tab->read_first_record= join_read_record_no_init;
>> + last_tab->read_record.read_record= rr_sequential_and_unpack;
>> + last_tab->read_record.copy_field= sjm->copy_field;
>> + last_tab->read_record.copy_field_end= sjm->copy_field +
>> + sjm->table_cols.elements;
>
> Minor nitpick: If it were up to me, I would put the whole RHS of the last
> assignment on a separate line.
>
>> +
>> Item *save_cond= last_tab->select_cond;
>> last_tab->set_select_cond(sjm->join_cond, __LINE__);
>> rc= sub_select(join, last_tab, end_of_records);
>>
>>
>>
>>
>>
>