From: Roy Lyseng
Date: September 22 2010 11:13am
Subject: Re: bzr commit into mysql-next-mr-bugfixing branch (roy.lyseng:3245)
List-Archive: http://lists.mysql.com/commits/118789
Message-Id: <4C99E4F3.1000705@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit

On 21.09.10 16.17, Guilhem Bichot wrote:
> Hello,
>
> Roy Lyseng a écrit, Le 21.09.2010 13:41:
>> #At file:///home/rl136806/mysql/repo/mysql-review/ based on
>> revid:roy.lyseng@stripped
>>
>> 3245 Roy Lyseng 2010-09-21
>> Refactoring: Make members positions and best_positions of class JOIN
>> dynamically allocated instead of fixed-size arrays.
>> This saves approx. 20 kbyte memory during optimization and execution
>> for normal-sized join operations.
>
> Good catch!

Anyone doing print sizeof(JOIN) would go huntin' for this one ;)
>
>> === modified file 'sql/sql_select.cc'
>> --- a/sql/sql_select.cc 2010-09-20 14:06:02 +0000
>> +++ b/sql/sql_select.cc 2010-09-21 11:36:51 +0000
>> @@ -4490,6 +4490,14 @@ make_join_statistics(JOIN *join, TABLE_L
>> if (!stat || !stat_ref || !table_vector)
>> DBUG_RETURN(1); // Eom /* purecov: inspected */
>>
>> + if (!(join->positions=
>> + (POSITION *)join->thd->alloc(sizeof(POSITION)*(table_count+1))))
>> + DBUG_RETURN(TRUE);
>> +
>> + if (!(join->best_positions=
>> + (POSITION *)join->thd->alloc(sizeof(POSITION)*(table_count+1))))
>> + DBUG_RETURN(TRUE);
>> +
>
> this, or like is done in the previous lines for stat/stat_ref/table_vector:
> just do a bunch of
> x= join->thd->alloc()
> and one final test
> if (!x || !y || !z || !t || etc)
> DBUG_RETURN(TRUE);

I prefer to backout early when an error is detected, so I would rather stick to 
my original fix.

A more elegant way is:

   if (!(a= allocate_a()) ||
       !(b= allocate_b()) ||
       !(c= allocate_c()))
     DBUG_RETURN(TRUE);

but the reason that I do not like to use it is that it becomes difficult to step 
into the different allocate_X functions in a debugger. Besides, you do not know 
which of the functions that caused the error.
>
> Ok to push.
>
> As this patch is touching memory allocation and using MEM_ROOT (via
> thd->alloc()), I suggest running all optimizer tests with --ps-protocol
> --valgrind to see whether there's a problem before pushing.

Yes, good advice. Validated that no more valgrind warnings occur after my fix :(

> Thanks.