From: Roy Lyseng
Date: January 13 2011 8:28am
Subject: Re: bzr commit into mysql-trunk branch (roy.lyseng:3322) Bug#58561
List-Archive: http://lists.mysql.com/commits/128593
Message-Id: <4D2EB7B1.4070409@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

Hi Evgeny,

thank you for the review.

On 12.01.11 17.33, Evgeny Potemkin wrote:
> Hi Roy,
>
> Ok to push with minor code style changes. See below.
>
> Regards, Evgen.

>> if (save_join_if_explain())
>> - DBUG_RETURN(1); /* purecov: inspected */
>> + {
>> + rc= 1;
>> + goto exit;
>> + }
>> if (item->engine_changed)
>> {
>> - DBUG_RETURN(1);
>> + rc= 1;
>> + goto exit;
>> }
> IMHO it would be more readable to write:
> if (save_join_if_explain() || item->engine_changed)
> {
>
> rc= 1;
> goto exit;
>
> }
>
> But it's up to you.

I think that I want to keep it this way. As Øystein says, the first one is an 
error return, the second is signalling that a retry is needed. Unfortunately, 
both use the same return value.
>
>> }
>> if (select_lex->uncacheable&&
>> @@ -2331,9 +2335,8 @@ int subselect_single_select_engine::exec
>> {
>> if (join->reinit())
>> {
>> - thd->where= save_where;
>> - thd->lex->current_select= save_select;
>> - DBUG_RETURN(1);
>> + rc= 1;
>> + goto exit;
>> }
>> item->reset();
>> item->assigned((executed= 0));
>> @@ -2389,14 +2392,15 @@ int subselect_single_select_engine::exec
>> tab->read_first_record= tab->save_read_first_record;
>> tab->read_record.read_record= tab->save_read_record;
>> }
>> - executed= 1;
>> - thd->where= save_where;
>> - thd->lex->current_select= save_select;
>> - DBUG_RETURN(join->error||thd->is_fatal_error);
>> + executed= true;
>> +
>> + rc= join->error||thd->is_fatal_error;
> Please add spaces around ||.

Done.

>> }
>> +
>> +exit:
>> thd->where= save_where;
>> thd->lex->current_select= save_select;
>> - DBUG_RETURN(0);
>> + DBUG_RETURN(rc);
>> }
>>
>> int subselect_union_engine::exec()
>>
>> === modified file 'sql/sql_select.cc'
>> --- a/sql/sql_select.cc 2010-12-29 00:38:59 +0000
>> +++ b/sql/sql_select.cc 2011-01-12 16:08:16 +0000
>> @@ -6166,8 +6166,8 @@ update_ref_and_keys(THD *thd, DYNAMIC_AR
>> substitutions.
>> */
>> sz= max(sizeof(KEY_FIELD),sizeof(SARGABLE_PARAM))*
>> - (((thd->lex->current_select->cond_count+1)*2 +
>> - thd->lex->current_select->between_count)*m+1);
>> + (((select_lex->cond_count+1)*2 +
>> + select_lex->between_count)*m+1);
> Please add spaces around * and +.

Done (although this calculation scares me a bit...)

>> if (!(key_fields=(KEY_FIELD*) thd->alloc(sz)))
>> return TRUE; /* purecov: inspected */
>> and_level= 0;
>>
>>
>>
>>
>
>

Thanks,
Roy