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
