From: Jorgen Loland
Date: December 3 2010 10:08am
Subject: Re: bzr commit into mysql-trunk branch (tor.didriksen:3247) WL#1393
List-Archive: http://lists.mysql.com/commits/125917
Message-Id: <4CF8C1AE.90305@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit

Tor,

Thank you for addressing my comments (except one - see inline). PQ approved 
pending fix as agreed :-)

On 11/30/2010 12:54 PM, Tor Didriksen wrote:
> On 2010-11-29 11:03, Jorgen Loland wrote:
>> Tor,
>>
>> Thank you for implementing priority queue.
>>
>> I've written comments inline. For faster follow-up review it would be
>> great if you could commit the next changeset without uncommitting this
>> one.
>>
>> > === modified file 'mysql-test/r/order_by_none.result'
>> > --- mysql-test/r/order_by_none.result 2010-09-27 13:20:24 +0000
>> > +++ mysql-test/r/order_by_none.result 2010-11-25 09:01:19 +0000
>> > +SELECT SQL_CALC_FOUND_ROWS * FROM t1
>> > +ORDER BY f1, f0 LIMIT 0;
>> > +f0 f1 f2
>> > +SELECT FOUND_ROWS();
>> > +FOUND_ROWS()
>> > +500
>>
>> JL: Wow... I didn't realize until now that we actually do sorting even
>> if LIMIT 0 is requested. This makes me wonder if you should have a
>> small optimization in Bounded_queue::push to not create and push
>> element keys to the underlying queue if the LIMIT is 0. Counting
>> # records should be enough.
>>
>
> Done, with a new unit test for it.
> But, for a real query with LIMIT 0; I got coredump,
> so even if we are not sending any data back to the client, there has to
> be some data there to "unpack".

Ok, it was worth a try.

>> JL: Have order_by_all and order_by_icp_mrr intentionally not been
>> updated (mtr --record)?
>
> They will be updated once this is merged to opt-backporting.
> This worklog will most likely go though a staging tree, into trunk, into
> trunk-bugfixing, and finally opt-backporting.
> (by that time, maybe you are merge captain)

Yay!

>> > +
>> > + if (options & OPTION_FOUND_ROWS)
>> > + num_rows= found_rows;
>>
>> JL: I thought that
>> * num_rows is the number of records returned by filesort, and
>> * found_rows is the number of records that would have been returned if
>> there was no LIMIT.
>>
>> Why this assignment? Are you changing the meaning of the return value
>> based on whether or not OPTION_FOUND_ROWS is specified?
>>
>> If OPTION_FOUND_ROWS is set, then retval means total number of
>> records, otherwise retval means number of records returned? If so, I
>> think you should rather add another out-parameter for found_rows.
>> Also, the documentation for retval does not mention this.
>
> Rewritten with an extra output argument instead.

Thank you. I think it's much better now :)

>> > +
>> > + if (num_rows < num_available_keys)
>> > + {
>> > + /* The whole source set fits into memory. */
>> > + if (param->max_rows < num_rows/PQ_slowness )
>> > + {
>> > + make_char_array(filesort_info,
>> > + param->num_keys_per_buffer, param->rec_length);
>> > + if (filesort_info->sort_keys)
>> > + DBUG_RETURN(true);
>> > + }
>> > + else
>> > + {
>> > + /* PQ will be slower */
>> > + DBUG_RETURN(false);
>> > + }
>> > + }
>> > +
>> > + if (param->num_keys_per_buffer < num_available_keys)
>> > + {
>>
>> JL: Comment this block like you did for the "whole set fits in memory"
>> block above
>>
>
> OK

I think you forgot this.

-- 
Jørgen Løland | Senior Software Engineer | +47 73842138
Oracle MySQL
Trondheim, Norway