List:Commits« Previous MessageNext Message »
From:Olav Sandstaa Date:January 9 2011 10:12pm
Subject:Re: bzr commit into mysql-trunk branch (olav.sandstaa:3461) Bug#58816
View as plain text  
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


Thread
bzr commit into mysql-trunk branch (olav.sandstaa:3461) Bug#58816Olav Sandstaa6 Jan
  • Re: bzr commit into mysql-trunk branch (olav.sandstaa:3461) Bug#58816Jorgen Loland7 Jan
    • Re: bzr commit into mysql-trunk branch (olav.sandstaa:3461) Bug#58816Roy Lyseng7 Jan
      • Re: bzr commit into mysql-trunk branch (olav.sandstaa:3461) Bug#58816Jorgen Loland7 Jan
    • Re: bzr commit into mysql-trunk branch (olav.sandstaa:3461) Bug#58816Olav Sandstaa9 Jan