Hi Jørgen,
Thanks for the review.
Jorgen Loland wrote:
> Olav,
>
> The patch looks good, but I have a few suggestions for you to consider
> inline.
>
> On 01/06/2011 04:36 PM, Olav Sandstaa wrote:
>> #At file:///export/home/tmp/mysql2/opt-bug58816/ based on
>> revid:nirbhay.choubey@stripped
>>
>> 3461 Olav Sandstaa 2011-01-06
>> Fix for Bug#58816 Extra temporary duplicate rows in result set
>> when
>> switching ICP off.
>>
>> === modified file 'mysql-test/include/icp_tests.inc'
>> --- a/mysql-test/include/icp_tests.inc 2010-12-13 15:22:45 +0000
>> +++ b/mysql-test/include/icp_tests.inc 2011-01-06 15:36:46 +0000
>> @@ -653,3 +653,28 @@
>> insert into t1 values ('',1);
>> select 1 from t1 where b<= 1 and a<> '';
>> drop table t1;
>> +
>> +--echo #
>> +--echo # Bug #58816 "Extra temporary duplicate rows in result set when
>> +--echo # switching ICP off"
>> +--echo #
>> +
>> +CREATE TABLE t1 (
>> + pk INT NOT NULL,
>> + c1 INT NOT NULL,
>> + PRIMARY KEY (pk)
>> +);
>> +
>> +INSERT INTO t1 VALUES (1,9);
>> +INSERT INTO t1 VALUES (2,7);
>> +INSERT INTO t1 VALUES (3,6);
>> +INSERT INTO t1 VALUES (4,3);
>> +INSERT INTO t1 VALUES (5,1);
>
> You could do INSERT INTO t1 VALUES (1,9),(2,7),...
Yes, I will rewrite this to make it more compact.
>
>> === modified file 'sql/handler.cc'
>> --- a/sql/handler.cc 2010-12-16 19:36:57 +0000
>> +++ b/sql/handler.cc 2011-01-06 15:36:46 +0000
>> @@ -5820,7 +5820,7 @@
>> */
>> int handler::ha_reset()
>> {
>> - DBUG_ENTER("ha_reset");
>> + DBUG_ENTER("handler::ha_reset");
>> /* Check that we have called all proper deallocation functions */
>> DBUG_ASSERT((uchar*) table->def_read_set.bitmap +
>> table->s->column_bitmap_size ==
>> @@ -5833,6 +5833,11 @@
>> free_io_cache(table);
>> /* reset the bitmaps to point to defaults */
>> table->default_column_bitmaps();
>> + /* Reset information about pushed index conditions */
>> + pushed_idx_cond= NULL;
>> + pushed_idx_cond_keyno= MAX_KEY;
>> + in_range_check_pushed_down= false;
>> +
>> const int retval= reset();
>> DBUG_RETURN(retval);
>> }
>
> Looks good, but I wonder if other variables should be reset as well
> for bonus-points. How about pushed_cond for instance?
>
Yes, definitively and as Roy already has mentioned there is a bug report
and a patch for this (Bug#58553).
And there might be more variables that should be reset in the same way.
The best would maybe be do do a small refactoring of this and make some
common code/function for the handler's constructor and ha_reset for
initializing these variables. It seems like there is evidence for that
variables get added to the handler class and initialized in the
constructor but not considered for resetting in ha_reset.
Olav
