List:Commits« Previous MessageNext Message »
From:Øystein Grøvlen Date:November 10 2010 1:35pm
Subject:Re: bzr commit into mysql-5.5-bugteam branch (oystein.grovlen:3258)
Bug#57704
View as plain text  
Hi Olav,

Thanks for the review.

On 11/ 8/10 11:20 PM, Olav Sandstaa wrote:
> Hi Oystein,
>
> I think the patch looks fine and I have only two issues that I think you
> should consider:
>
> Oystein Grovlen wrote:
...
>> === modified file 'mysql-test/t/subselect.test'
>> --- a/mysql-test/t/subselect.test 2010-06-25 13:32:47 +0000
>> +++ b/mysql-test/t/subselect.test 2010-11-04 10:36:24 +0000
>> @@ -3946,3 +3946,21 @@ DROP TABLE t1,t2;
>> --enable_result_log
>>
>> --echo End of 5.1 tests
>> +
>> +--echo #
>> +--echo # Bug #57704: Cleanup code dies with void
>> TABLE::set_keyread(bool): +--echo # Assertion `file' failed.
>> +--echo #
>> +
>> +CREATE TABLE t1 (a INT);
>> +
>> +--error 1241
>
> 1. Instead of using the error number here would it be better to use the
> error name? (In this case ER_OPERAND_COLUMNS).

Yes, of course.  Thanks for reminding me of this.

...
>> === modified file 'sql/sql_union.cc'
>> --- a/sql/sql_union.cc 2010-07-27 12:42:36 +0000
>> +++ b/sql/sql_union.cc 2010-11-04 10:36:24 +0000
>> @@ -443,6 +443,7 @@ bool st_select_lex_unit::prepare(THD *th
>>
>> err:
>> thd_arg->lex->current_select= lex_select_save;
>> + cleanup();
>
> 2. I replaced your call to cleanup() with the following code (due to
> that I considered adding "always check return values" to your "avoiding
> regressions" list) :
>
> bool xxx= cleanup();
> DBUG_ASSERT(xxx == false);
>
> and with this change I hit this assert when running the subselect test
> (not in your test case but an earlier test in the same file).
> Given that we already have run into an "error situation" and are trying
> to clean up after it it might be expected that cleanup() will not always
> return without error. Still, I think it is worth consider if this is the
> case or not (I do not know).
>
> If you think this is fine, then it is OK to push this patch.

AFAICT, cleanup() will return TRUE if an underlying JOIN object has 
recorded an error.  I guess such an error will be an normal reason for 
why cleanup() is needed. Note that the return value is also ignored by 
the "normal" cleanup at end of mysql_execute_command().

--
Øystein

Thread
bzr commit into mysql-5.5-bugteam branch (oystein.grovlen:3258) Bug#57704Oystein Grovlen4 Nov
Re: bzr commit into mysql-5.5-bugteam branch (oystein.grovlen:3258)Bug#57704Olav Sandstaa8 Nov
  • Re: bzr commit into mysql-5.5-bugteam branch (oystein.grovlen:3258)Bug#57704Øystein Grøvlen10 Nov