From: Guilhem Bichot
Date: June 24 2011 1:00pm
Subject: Re: bzr commit into mysql-trunk branch (guilhem.bichot:3385) Bug#12586926
 Bug#12619510 Bug#12619868
List-Archive: http://lists.mysql.com/commits/139824
Message-Id: <4E048A79.6010807@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit

Hello,

Roy Lyseng a écrit, Le 22.06.2011 14:46:
> Hi Guilhem,
> 
> thank you for a clever bugfix, it certainly makes the logic of the 
> optimizer simpler and clearer.

Thanks!

> I have a few typographical comments, and a suggestion for a followup 
> patch below, otherwise the bugfix is approved.

ok, please see below

> On 17.06.11 14.46, Guilhem Bichot wrote:
>> #At file:///home/mysql_src/bzrrepos_new/mysql-next-mr-opt-backporting/ 
>> based on revid:jorgen.loland@stripped
>>
>>   3385 Guilhem Bichot    2011-06-17
>>        Fix for
>>        BUG#12586926 "EXTRA ROW WITH JOIN + GROUP BY + ORDER BY WITH 
>> JCL>=5 AND MRR ENABLED"
>>        BUG#12619510 "JCL: MORE ROWS AND DIFFERENT OUTPUT WITH JCL>=5"
>>        BUG#12619868 "JCL: MORE ROWS OF OUTPUT WHEN JCL>=5":
> 
> I'd appreciate it if the commit log lines were max. 70 characters wide. 
> Some tools have a nasty habit of wrapping lines wider than that.

I try to watch the length of lines (you already asked for it in the 
past) but sometimes I forget; gcommit does not warn about this.

>>          the function takes at attempt at disabling the choice of 
>> index ordering for GROUP BY
> 
> "an attempt"

fixed

>>          3) The recently added assertion in setup_join_buffering() is 
>> removed;
>>          this assertion probably intended to say that if we disable 
>> join buffering in this
>>          function we should already have known it before, when we 
>> computed costs; but
>>          - it could never fire before, because no_jbuf_after was 
>> always bigger than tableno
>>          - now it can fire, due to "FORCE INDEX FOR (GROUP|ORDER) BY" 
>> (which now
>>          properly disables join buffering); in that case it's ok that 
>> the cost
>>          calculation was not aware: when the user uses FORCE INDEX 
>> she/he intentionally
>>          ignores cost calculations.
> 
> I do not agree to this. There is good reason to have join buffering 
> considered as early as possible.
> 
> If we have a join of tables t1, t2 and t3 and force to use an index for 
> t1 (which effectlively makes the table first in the join order), 
> efficient calculation of the join plan for t2 and t3 still matters. By 
> applying this information at the earlier stage inside the greedy 
> optimizer, would make the cost estimates more accurate and hence there 
> is a better chance of getting an optimal plan.

ok, now I understand.

> It think it would be possible to do this as follows, in a separate 
> followup patch:
> 
> 1. When a table has a FORCE INDEX FOR GROUP BY/ORDER BY on it, make it 
> first in the join plan by setting all other tables to be dependent on 
> this table (unless the table itself has dependencies to it).
> 
> 2. When considering the first table in best_access_path(), set a flag in 
> the JOIN object that tells whether to consider join buffering for 
> subsequent tables or not.
> 
> 3. Take this flag into consideration when determining whether to use 
> join buffering for all tables.

I think I could, in a follow-up patch, do 2) and 3) and restore your 
assertion. As for 1), it scares me to death - I fear it changes plans, 
or I mess up with table dependencies.
Let me know if that compromise would be ok, and I'll put it on my todo.

>> +CREATE TABLE t2 (
>> +pk int(11) NOT NULL AUTO_INCREMENT,
>> +col_varchar_key varchar(1) NOT NULL,
>> +PRIMARY KEY (pk)
>> +);
> 
> Perhaps delete AUTO_INCREMENT.

done

>> +CREATE TABLE t1 (
>> + col_varchar_key varchar(1))
>> + ENGINE=MyISAM DEFAULT CHARSET=latin1;
>> +CREATE TABLE t2 (
>> + pk int(11) NOT NULL AUTO_INCREMENT,
>> + col_int_nokey int(11) NOT NULL,
>> + col_int_key int(11) NOT NULL,
>> + PRIMARY KEY (pk),
>> + KEY col_int_key (col_int_key)
>> +) ENGINE=MyISAM AUTO_INCREMENT=21 DEFAULT CHARSET=latin1;
> 
> I think that you can delete AUTO_INCREMENT and DEFAULT CHARSET above.

done, and deleted ENGINE=MyISAM (it's superfluous).