List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:April 26 2011 8:30am
Subject:Re: bzr commit into mysql-trunk branch (guilhem.bichot:3348) Bug#11766522
View as plain text  
Hello,

Øystein Grøvlen a écrit, Le 06.04.2011 10:45:
> Hi Guilhem,
> 
> Thanks for the patch and for a detailed explanation in commit comments
> about why this fails.
> 
> However, it would be good with an "executive summary" in the
> non-file-specific part of the commit comments that sums up the issue
> and what has been done to fix it.  To me it was actually not quite
> clear from the commit comments what changes have actually been made.
> It kind of "drowned" in the detailed specification of the problem.

ok, changed the global comment to:

Fix for BUG#11766522 "59651: ASSERTION `TABLE_REF->HAS_RECORD' FAILED 
WITH JOIN_CACHE_LEVEL=3":
during execution, access to last-in-plan tables spoiled TABLE::status of 
first tables:
some save/restore logic of TABLE::status was:
- not recursion-safe (causing the bug)
- not needed (every handler "read" function sets TABLE::status)
- logically absurd (why would reading a last-in-plan table affect the status
of a previous table?).


> IIUC, the basic message is that the described save/restore logic is
> not necessary since all engines sets table->status, anyway.

Not only is this not necessary, but more importantly, it's wrong, 
because it is not recursion-safe.

>  I think
> this could be said more explicitly.

done

> I also wonder why the reported test case only fails for
> join_cache_level=3.  Is this because buffers were not filled up when
> incremental buffers were used?

yes. Incremental buffers are more compact.

> I have only minor comments to the actual code (see inline), so patch
> is approved.

Thanks. Replies below.

> 
> On 04/ 1/11 11:09 AM, Guilhem Bichot wrote:
>  > #At file:///home/mysql_src/bzrrepos_new/59651/ based on 
> revid:roy.lyseng@stripped
>  >
>  >   3348 Guilhem Bichot    2011-04-01
>  >        Fix for BUG#11766522 "59651: ASSERTION `TABLE_REF->HAS_RECORD' 
> FAILED WITH JOIN_CACHE_LEVEL=3":
>  >        during execution, access to last-in-plan tables spoiled 
> TABLE::status of first tables.
>  >       @ mysql-test/include/join_cache.inc
>  >          test for bug. Without the code fix, join_cache_jcl3 would 
> hit an assertion failure in join_read_key2.
>  >       @ mysql-test/r/join_cache_jcl1.result
>  >          correct result (32 rows)
>  >       @ mysql-test/r/join_cache_jcl2.result
>  >          correct result (32 rows)
>  >       @ mysql-test/r/join_cache_jcl3.result
>  >          correct result (32 rows)
>  >       @ mysql-test/r/join_cache_jcl4.result
>  >          correct result (32 rows)
>  >       @ mysql-test/r/join_cache_jcl5.result
>  >          correct result (32 rows)
>  >       @ mysql-test/r/join_cache_jcl6.result
>  >          correct result (32 rows)
>  >       @ mysql-test/r/join_cache_jcl7.result
>  >          correct result (32 rows)
>  >       @ mysql-test/r/join_cache_jcl8.result
>  >          correct result (32 rows)
>  >       @ sql/handler.h
>  >          Removing the assignments to table->status
>  >          from ha_innodb.cc, ha_myisam.cc, ha_heap.cc causes test 
> failures,
>  >          which shows that engines must set table->status.
>  >          So tell this brutal truth, in comments: engines must set 
> table->status.
> 
> I think it should be made more explicit that only comments have been
> changed/added in this file.

done

>  >          All those which I could check, do so (including PBXT).
>  >          Question for reviewers: most of the affected handler functions
>  >          have ha_* non-virtual wrappers (ha_index_next_same(), etc);
>  >          we could, in a follow-up patch:
>  >          - remove the requirement from the engine (don't set 
> table->status
>  >          in their functions anymore)
>  >          - set table->status in the ha_* wrappers.
>  >          - make STATUS_ * flags invisible to engines
>  >          - change them to an enum, and replace the magic value "0"
>  >          with an enum value.
> 
> Sounds like very good ideas to me.

I postponed them to an "undefined future".

>  >       @ sql/sql_join_cache.cc
>  >          Deleting broken code which caused the bug. It was broken for 
> two reasons.
>  >          First, it affected JOIN_TABs located _before_ 'join_tab' in 
> the plan;
>  >          there is no reason why reading rows from 'join_tab' should 
> affect
>  >          TABLE::status of previous JOIN_TABs.
>  >          Second, the save/restore logic was not recursion-safe; 
> consider:
>  >          int save_x, x; // global variables
>  >          f()
>  >          {
>  >            save_x= x; // save
>  >            f();
>  >            x= save_x; // restore
>  >          }
>  >          the inner call to f() will overwrite the value of save_x,
>  >          in the end x will not get its original value back.
>  >          That is what caused the assertion failure. In the bug's 
> testcase,
>  >          the last tables of the plan are t7, t8, t6. t7 is doing ref 
> access,
>  >          t8 and t6 are doing join buffering. During execution,
>  >          at some point, no row is found in t7 so t7's table->status 
> is set to
>  >          STATUS_NOT_FOUND // because no record found
>  >          | STATUS_NULL_ROW // because NULL-complemented (LEFT JOIN)
>  >          Then we continue into t8: at some point the join buffer of 
> t8 is full,
>  >          so t8 cannot accumulate more partial records sent by t7, so
>  >          join_records() is called for t8, to generate records (of t8 
> joined
>  >          with its buffer), in order to be able to later empty t8's 
> buffer.
>  >          Those records are generated, which backs up t7's table->status
>  >          into t7's tab->status, and changes table->status to 0.
>  >          Generated records are passed to t6. t6's buffer gets full. So
>  >          join_records() is called for t6. Which backs up t7's 
> table->status
>  >          (0) into t7's tab->status. From then on, things are wrong, 
> as when
>  >          the two join_records() finally end, t7's table->status is not
>  >          restored to its original value 
> (STATUS_NOT_FOUND|STATUS_NULL_ROW)
>  >          but to 0 ("save_x" issue above). After that, more rows come 
> from tables
>  >          before t7, so we have another ref access to t7 i.e. a call 
> to join_read_key2(),
>  >          which has this code:
>  >            if (cmp_buffer_with_ref(tab->join->thd, table, table_ref) ||
>  >                (table->status&  (STATUS_GARBAGE | STATUS_NULL_ROW)))
>  >            {
>  >          ...
>  >            }
>  >            else if (table->status == 0)
>  >            {
>  >              DBUG_ASSERT(table_ref->has_record);
>  >              table_ref->use_count++;
>  >            }
>  >          The "else" branch is expected to be for when:
>  >          - the lookup value is the same as the previous lookup value 
> (see cmp_buffer_with_ref())
>  >          - the previous lookup did find a record (i.e. table->status==0)
>  >          - in which case there should be a record ready for reusal
>  >          (which is asserted with table_ref->has_record) and a new 
> lookup can be avoided.
>  >          But in our case, though the lookup value is the same, there
>  >          is no record; if t7's table->status was correct it would still
>  >          have STATUS_NULL_ROW and we wouldn't enter the "else" block.
>  >          Because t7's table->status is wrongly 0, we enter the "else"
>  >          and the assertion fires, because there is no record actually.
>  >
>  >          This code dates from 2000 at the latest (when code was 
> imported into bitkeeper).
>  >
>  >          It is unlikely that this causes a real bug in 5.1 or 5.5 
> because:
>  >          - those older versions do join buffering only for inner join
>  >          - in an inner join, if the "ref" access finds no rows, we don't
>  >          go into the "next" tables, so have no chance to corrupt 
> table->status
>  >          of the table doing "ref" access.
>  >       @ sql/sql_select.cc
>  >          unused flag (was never set)
> 
> This is no longer the only change.

fixed

>  >       @ sql/sql_select.h
>  >          JOIN_TAB::status not needed anymore
>  >       @ sql/structs.h
>  >          Best-effort documentation of STATUS_ flags, to the extent of 
> what I understood.
>  >          Deleting STATUS_PARENT and STATUS_NOT_READ: they were never 
> set, and this is sure:
> 
> It should be STATUS_NO_PARENT

fixed

>  >          Deleting STATUS_NO_RECORD.
> 
> I think the above line is misplaced since the below lines is
> associated with the previous line.

fixed

>  I think you should also explain
> why STATUS_NO_RECORD could be deleted.

fixed

>  >          I temporarily changed TABLE::status to an enum so that any
>  >          table->status= 8; /* using 8 instead of STATUS_NOT_READ */
>  >          would be caught by the compiler.
>  >
>  >      modified:
>  >        mysql-test/include/join_cache.inc
>  >        mysql-test/r/join_cache_jcl1.result
>  >        mysql-test/r/join_cache_jcl2.result
>  >        mysql-test/r/join_cache_jcl3.result
>  >        mysql-test/r/join_cache_jcl4.result
>  >        mysql-test/r/join_cache_jcl5.result
>  >        mysql-test/r/join_cache_jcl6.result
>  >        mysql-test/r/join_cache_jcl7.result
>  >        mysql-test/r/join_cache_jcl8.result
>  >        sql/handler.h
>  >        sql/item_cmpfunc.cc
>  >        sql/sql_base.h
>  >        sql/sql_join_cache.cc
>  >        sql/sql_select.cc
>  >        sql/sql_select.h
>  >        sql/structs.h
>  >        sql/table.cc
>  > === modified file 'mysql-test/include/join_cache.inc'
>  > --- a/mysql-test/include/join_cache.inc    2010-08-11 13:28:19 +0000
>  > +++ b/mysql-test/include/join_cache.inc    2011-04-01 09:09:15 +0000
>  > @@ -1577,3 +1577,72 @@ eval explain $query;
>  >   eval $query;
>  >
>  >   drop table t1,t2;
>  > +
>  > +--echo
>  > +--echo # Bug#11766522 "59651: ASSERTION `TABLE_REF->HAS_RECORD' FAILED
>  > +--echo # WITH JOIN_CACHE_LEVEL=3"
>  > +--echo
>  > +
>  > +CREATE TABLE t1 (
>  > +  b varchar(20)
>  > +)  ;
>  > +INSERT INTO t1 VALUES ('1'),('1');
>  > +
>  > +CREATE TABLE t4 (
>  > +  col253 text
>  > +)  ;
>  > +INSERT INTO t4 VALUES (''),('pf');
>  > +
>  > +CREATE TABLE t6 (
>  > +  col282 timestamp
>  > +)  ;
>  > +INSERT INTO t6 VALUES ('2010-11-07 01:04:45'),('2010-12-13 01:36:32');
>  > +
>  > +CREATE TABLE t7 (
>  > +  col319 timestamp NOT NULL,
>  > +  UNIQUE KEY idx263 (col319)
>  > +)  ;
>  > +# zero rows would do, if there was no const-table optimization
>  > +insert into t7 values("2000-01-01"),("2000-01-02");
>  > +
>  > +CREATE TABLE t3 (
>  > +  col582 char(230) CHARACTER SET utf8 DEFAULT NULL
>  > +)  ;
>  > +# one single row would do, if there was no const-table optimization
>  > +INSERT INTO t3 VALUES ('cymej'),('spb');
>  > +
>  > +CREATE TABLE t5 (
>  > +  col712 time
>  > +)  ;
>  > +# zero rows would do, if there was no const-table optimization
>  > +insert into t5 values(0),(0);
>  > +
>  > +CREATE TABLE t8 (
>  > +  col804 char(169),
>  > +  col805 varchar(51)
>  > +)  ;
>  > +INSERT INTO t8 VALUES ('tmqcb','pwk');
>  > +
>  > +CREATE TABLE t2 (
>  > +  col841 varchar(10)
>  > +)  ;
>  > +# one single row would do, if there was no const-table optimization
>  > +INSERT INTO t2 VALUES (''),('');
>  > +
>  > +# small buffer, to trigger "full buffer" in both caches of t8 and t6
>  > +set join_buffer_size=1;
>  > +select @@join_buffer_size;
> 
> I think you should explain that buffer size will not be 1, bit lowest
> possible, and that a warning is expected.

done

>  > +
>  > +--disable_warnings
>  > +select 1 from
>  > +(t1 join t2 join t3)
>  > +left join t4 on 1
>  > +left join t5 on 1 like t4.col253
>  > +left join t6 on t5.col712 is null
>  > +left join t7 on t1.b<=>t7.col319
>  > +left join t8 on t3.col582<=  1;
>  > +--enable_warnings
>  > +
>  > +drop table t1,t2,t3,t4,t5,t6,t7,t8;
>  > +
>  > +set @@join_buffer_size=default;
>  >
>  > === modified file 'mysql-test/r/join_cache_jcl1.result'
>  > --- a/mysql-test/r/join_cache_jcl1.result    2011-02-17 14:42:21 +0000
>  > +++ b/mysql-test/r/join_cache_jcl1.result    2011-04-01 09:09:15 +0000
>  > @@ -2250,5 +2250,91 @@ col_int_key    col_datetime
>  >   0    2000-09-26 07:45:57
>  >   2    2003-02-11 21:19:41
>  >   drop table t1,t2;
>  > +
>  > +# Bug#11766522 "59651: ASSERTION `TABLE_REF->HAS_RECORD' FAILED
>  > +# WITH JOIN_CACHE_LEVEL=3"
>  > +
>  > +CREATE TABLE t1 (
>  > +b varchar(20)
>  > +)  ;
>  > +INSERT INTO t1 VALUES ('1'),('1');
>  > +CREATE TABLE t4 (
>  > +col253 text
>  > +)  ;
>  > +INSERT INTO t4 VALUES (''),('pf');
>  > +CREATE TABLE t6 (
>  > +col282 timestamp
>  > +)  ;
>  > +INSERT INTO t6 VALUES ('2010-11-07 01:04:45'),('2010-12-13 01:36:32');
>  > +CREATE TABLE t7 (
>  > +col319 timestamp NOT NULL,
>  > +UNIQUE KEY idx263 (col319)
>  > +)  ;
>  > +insert into t7 values("2000-01-01"),("2000-01-02");
>  > +CREATE TABLE t3 (
>  > +col582 char(230) CHARACTER SET utf8 DEFAULT NULL
>  > +)  ;
>  > +INSERT INTO t3 VALUES ('cymej'),('spb');
>  > +CREATE TABLE t5 (
>  > +col712 time
>  > +)  ;
>  > +insert into t5 values(0),(0);
>  > +CREATE TABLE t8 (
>  > +col804 char(169),
>  > +col805 varchar(51)
>  > +)  ;
>  > +INSERT INTO t8 VALUES ('tmqcb','pwk');
>  > +CREATE TABLE t2 (
>  > +col841 varchar(10)
>  > +)  ;
>  > +INSERT INTO t2 VALUES (''),('');
>  > +set join_buffer_size=1;
>  > +Warnings:
>  > +Warning    1292    Truncated incorrect join_buffer_size value: '1'
>  > +select @@join_buffer_size;
>  > +@@join_buffer_size
>  > +128
>  > +select 1 from
>  > +(t1 join t2 join t3)
>  > +left join t4 on 1
>  > +left join t5 on 1 like t4.col253
>  > +left join t6 on t5.col712 is null
>  > +left join t7 on t1.b<=>t7.col319
>  > +left join t8 on t3.col582<=  1;
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
>  > +1
> 
> This result is a bit "boring" ;-) I suggest using "select count(*)"
> instead of "select 1".  Also makes it easier to spot differences
> between result files.

done. And I verified that without the bugfix, the count is wrong.

>  > +drop table t1,t2,t3,t4,t5,t6,t7,t8;
>  > +set @@join_buffer_size=default;
>  >   set optimizer_join_cache_level = default;
>  >   set optimizer_switch = default;
>  >
> 
> ...
> 
>  > === modified file 'sql/handler.h'
>  > --- a/sql/handler.h    2011-01-26 21:12:56 +0000
>  > +++ b/sql/handler.h    2011-04-01 09:09:15 +0000
>  > @@ -1799,9 +1799,12 @@ public:
>  >     }
>  >     /**
>  >        @brief
>  > -     Positions an index cursor to the index specified in the handle. 
> Fetches the
>  > -     row if available. If the key value is null, begin at the first 
> key of the
>  > -     index.
>  > +     Positions an index cursor to the index specified in the handle
>  > +     ('active_index'). Fetches the row if available. If the key 
> value is null,
>  > +     begin at the first key of the index.
>  > +     To indicate success (found a record), function returns 0 _and_
>  > +     sets table->status to 0. Otherwise, returns non-zero and sets
>  > +     table->status to STATUS_NOT_FOUND.
> 
> I suggest adding a @returns tag here.

done

>  >     */
>  >     virtual int index_read_map(uchar * buf, const uchar * key,
>  >                                key_part_map keypart_map,
>  > @@ -1813,27 +1816,34 @@ public:
>  >   protected:
>  >     /**
>  >        @brief
>  > -     Positions an index cursor to the index specified in the handle. 
> Fetches the
>  > -     row if available. If the key value is null, begin at the first 
> key of the
>  > -     index.
>  > +     Positions an index cursor to the index specified in parameter. 
> Fetches
>  > +     the row if available. If the key value is null, begin at the 
> first key of
>  > +     the index.
>  > +     @returns as index_read_map().
> 
> At first, the above line was a bit confusing to me.  I suggest "@see
> index_read_map(), instead"

Changed to:
@returns @see index_read_map().

>  >     */
>  >     virtual int index_read_idx_map(uchar * buf, uint index, const 
> uchar * key,
>  >                                    key_part_map keypart_map,
>  >                                    enum ha_rkey_function find_flag);
>  > +  /// @returns as index_read_map().
>  >     virtual int index_next(uchar * buf)
>  >      { return  HA_ERR_WRONG_COMMAND; }
>  > +  /// @returns as index_read_map().
>  >     virtual int index_prev(uchar * buf)
>  >      { return  HA_ERR_WRONG_COMMAND; }
>  > +  /// @returns as index_read_map().
>  >     virtual int index_first(uchar * buf)
>  >      { return  HA_ERR_WRONG_COMMAND; }
>  > +  /// @returns as index_read_map().
>  >     virtual int index_last(uchar * buf)
>  >      { return  HA_ERR_WRONG_COMMAND; }
>  >   public:
>  > +  /// @returns as index_read_map().
>  >     virtual int index_next_same(uchar *buf, const uchar *key, uint 
> keylen);
>  >     /**
>  >        @brief
>  >        The following functions works like index_read, but it find the 
> last
>  >        row with the current key value or prefix.
>  > +     @returns as index_read_map().
>  >     */
>  >     virtual int index_read_last_map(uchar * buf, const uchar * key,
>  >                                     key_part_map keypart_map)
>  > @@ -1853,7 +1863,9 @@ public:
>  >       { return NULL; }
>  >     virtual int ft_read(uchar *buf) { return HA_ERR_WRONG_COMMAND; }
>  >   protected:
>  > +  /// @returns as index_read_map().
>  >     virtual int rnd_next(uchar *buf)=0;
>  > +  /// @returns as index_read_map().
>  >     virtual int rnd_pos(uchar * buf, uchar *pos)=0;
>  >   public:
>  >     /**
>  >
>  > === modified file 'sql/item_cmpfunc.cc'
>  > --- a/sql/item_cmpfunc.cc    2011-02-08 15:54:12 +0000
>  > +++ b/sql/item_cmpfunc.cc    2011-04-01 09:09:15 +0000
>  > @@ -430,7 +430,8 @@ static bool convert_constant_item(THD *t
>  >       */
>  >       bool save_field_value= (field_item->depended_from&&
>  >                               (field_item->const_item() ||
>  > -                             !(field->table->status& 
> STATUS_NO_RECORD)));
>  > +                             !(field->table->status&
>  > +                               (STATUS_GARBAGE | STATUS_NOT_FOUND))));
> 
> I think a helper function that returned (STATUS_GARBAGE |
> STATUS_NOT_FOUND) would be good so one did not have to remember to
> test for both all over the place.

Mmmm
STATUS_NO_RECORD used to be defined as STATUS_GARBAGE | STATUS_NOT_FOUND.
Roy asked me to delete STATUS_NO_RECORD and use STATUS_GARBAGE | 
STATUS_NOT_FOUND instead.
If I introduce a helper function, it's like resurrecting 
STATUS_NO_RECORD...?
I think Roy's intention is that in the long run, STATUS_GARBAGE should 
be deleted/renamed so that code becomes more natural.

>  >       if (save_field_value)
>  >         orig_field_val= field->val_int();
>  >       if (!(*item)->is_null()&&  !(*item)->save_in_field(field,
> 1))
>  >
>  > === modified file 'sql/sql_base.h'
>  > --- a/sql/sql_base.h    2010-11-23 22:37:59 +0000
>  > +++ b/sql/sql_base.h    2011-04-01 09:09:15 +0000
>  > @@ -323,7 +323,7 @@ inline void setup_table_map(TABLE *table
>  >     table->used_fields= 0;
>  >     table->const_table= 0;
>  >     table->null_row= 0;
>  > -  table->status= STATUS_NO_RECORD;
>  > +  table->status= STATUS_GARBAGE | STATUS_NOT_FOUND;
>  >     table->maybe_null= table_list->outer_join;
>  >     TABLE_LIST *embedding= table_list->embedding;
>  >     while (!table->maybe_null&&  embedding)
>  >
>  > === modified file 'sql/sql_join_cache.cc'
>  > --- a/sql/sql_join_cache.cc    2010-09-02 07:34:10 +0000
>  > +++ b/sql/sql_join_cache.cc    2011-04-01 09:09:15 +0000
>  > @@ -1761,7 +1761,6 @@ enum_nested_loop_state JOIN_CACHE_BNL::j
>  >   {
>  >     uint cnt;
>  >     int error;
>  > -  JOIN_TAB *tab;
>  >     READ_RECORD *info;
>  >     enum_nested_loop_state rc= NESTED_LOOP_OK;
>  >     SQL_SELECT *select= join_tab->cache_select;
>  > @@ -1788,18 +1787,9 @@ enum_nested_loop_state JOIN_CACHE_BNL::j
>  >       join_tab->select->quick= 0;
>  >     }
>  >
>  > -  for (tab= join->join_tab; tab != join_tab ; tab++)
>  > -  {
>  > -    tab->status= tab->table->status;
>  > -    tab->table->status= 0;
>  > -  }
>  > -
>  >     /* Start retrieving all records of the joined table */
>  >     if ((error= join_init_read_record(join_tab)))
>  > -  {
>  > -    rc= error<  0 ? NESTED_LOOP_NO_MORE_ROWS: NESTED_LOOP_ERROR;
>  > -    goto finish;
>  > -  }
>  > +    return error<  0 ? NESTED_LOOP_NO_MORE_ROWS: NESTED_LOOP_ERROR;
>  >
>  >     info=&join_tab->read_record;
>  >     do
>  > @@ -1811,8 +1801,7 @@ enum_nested_loop_state JOIN_CACHE_BNL::j
>  >       {
>  >         /* The user has aborted the execution of the query */
>  >         join->thd->send_kill_message();
>  > -      rc= NESTED_LOOP_KILLED;
>  > -      goto finish;
>  > +      return NESTED_LOOP_KILLED;
>  >       }
>  >
>  >       /*
>  > @@ -1826,10 +1815,7 @@ enum_nested_loop_state JOIN_CACHE_BNL::j
>  > (!select->skip_record(join->thd,&skip_record)&&
>  >                                 !skip_record));
>  >         if (select&&  join->thd->is_error())
>  > -      {
>  > -        rc= NESTED_LOOP_ERROR;
>  > -        goto finish;
>  > -      }
>  > +        return NESTED_LOOP_ERROR;
>  >         if (consider_record)
>  >         {
>  >           /* Prepare to read records from the join buffer */
>  > @@ -1847,7 +1833,7 @@ enum_nested_loop_state JOIN_CACHE_BNL::j
>  >               get_record();
>  >               rc= generate_full_extensions(get_curr_rec());
>  >               if (rc != NESTED_LOOP_OK&&  rc != 
> NESTED_LOOP_NO_MORE_ROWS)
>  > -              goto finish;
>  > +              return rc;
>  >             }
>  >           }
>  >         }
>  > @@ -1856,9 +1842,6 @@ enum_nested_loop_state JOIN_CACHE_BNL::j
>  >
>  >     if (error>  0)                // Fatal error
>  >       rc= NESTED_LOOP_ERROR;
>  > -finish:
>  > -  for (tab= join->join_tab; tab != join_tab ; tab++)
>  > -    tab->table->status= tab->status;
>  >     return rc;
>  >   }
>  >
>  > @@ -2299,7 +2282,7 @@ enum_nested_loop_state JOIN_CACHE_BKA::j
>  >
>  >     rc= init_join_matching_records(&seq_funcs, records);
>  >     if (rc != NESTED_LOOP_OK)
>  > -    goto finish;
>  > +    return rc;
>  >
>  >     while (!(error= file->multi_range_read_next((char **)&rec_ptr)))
>  >     {
>  > @@ -2307,8 +2290,7 @@ enum_nested_loop_state JOIN_CACHE_BKA::j
>  >       {
>  >         /* The user has aborted the execution of the query */
>  >         join->thd->send_kill_message();
>  > -      rc= NESTED_LOOP_KILLED;
>  > -      goto finish;
>  > +      return NESTED_LOOP_KILLED;
>  >       }
>  >       if (join_tab->keep_current_rowid)
>  >        
> join_tab->table->file->position(join_tab->table->record[0]);
>  > @@ -2323,14 +2305,13 @@ enum_nested_loop_state JOIN_CACHE_BKA::j
>  >         get_record_by_pos(rec_ptr);
>  >         rc= generate_full_extensions(rec_ptr);
>  >         if (rc != NESTED_LOOP_OK&&  rc != NESTED_LOOP_NO_MORE_ROWS)
>  > -    goto finish;
>  > +        return rc;
>  >       }
>  >     }
>  >
>  >     if (error>  0&&  error != HA_ERR_END_OF_FILE)   
>  >       return NESTED_LOOP_ERROR;
>  > -finish:
>  > -  return end_join_matching_records(rc);
>  > +  return rc;
>  >   }
>  >
>  >
>  > @@ -2375,12 +2356,6 @@ JOIN_CACHE_BKA::init_join_matching_recor
>  >     /* Dynamic range access is never used with BKA */
>  >     DBUG_ASSERT(join_tab->use_quick != 2);
>  >
>  > -  for (JOIN_TAB *tab =join->join_tab; tab != join_tab ; tab++)
>  > -  {
>  > -    tab->status= tab->table->status;
>  > -    tab->table->status= 0;
>  > -  }
>  > -
>  >     init_mrr_buff();
>  >
>  >     /*
>  > @@ -2398,31 +2373,6 @@ JOIN_CACHE_BKA::init_join_matching_recor
>  >
>  >
>  >   /*
>  > -  Finish searching for records that match records from the join buffer
>  > -
>  > -  SYNOPSIS
>  > -    end_join_matching_records()
>  > -      rc      return code passed by the join_matching_records function
>  > -
>  > -  DESCRIPTION
>  > -    This function perform final actions on searching for all matches 
> for
>  > -    the records from the join buffer and building all full join 
> extensions
>  > -    of the records with these matches.
>  > -
>  > -  RETURN
>  > -    return code rc passed to the function as a parameter
>  > -*/
>  > -
>  > -enum_nested_loop_state
>  > -JOIN_CACHE_BKA::end_join_matching_records(enum_nested_loop_state rc)
>  > -{
>  > -  for (JOIN_TAB *tab=join->join_tab; tab != join_tab ; tab++)
>  > -    tab->table->status= tab->status;
>  > -  return rc;
>  > -}
>  > -
>  > -
>  > -/*
>  >     Get the key built over the next record from BKA join buffer
>  >
>  >     SYNOPSIS
>  > @@ -3192,7 +3142,7 @@ JOIN_CACHE_BKA_UNIQUE::join_matching_rec
>  >
>  >     rc= init_join_matching_records(&seq_funcs, key_entries);
>  >     if (rc != NESTED_LOOP_OK)
>  > -    goto finish;
>  > +    return rc;
>  >
>  >     while (!(error= file->multi_range_read_next((char 
> **)&key_chain_ptr)))
>  >     {
>  > @@ -3227,8 +3177,7 @@ JOIN_CACHE_BKA_UNIQUE::join_matching_rec
>  >         {
>  >           /* The user has aborted the execution of the query */
>  >           join->thd->send_kill_message();
>  > -        rc= NESTED_LOOP_KILLED;
>  > -        goto finish;
>  > +        return NESTED_LOOP_KILLED;
>  >         }
>  >         /*
>  >           If only the first match is needed and it has been already 
> found
>  > @@ -3241,7 +3190,7 @@ JOIN_CACHE_BKA_UNIQUE::join_matching_rec
>  >           get_record_by_pos(rec_ptr);
>  >           rc= generate_full_extensions(rec_ptr);
>  >           if (rc != NESTED_LOOP_OK&&  rc != NESTED_LOOP_NO_MORE_ROWS)
>  > -      goto finish;
>  > +          return rc;
>  >         }
>  >       }
>  >       while (next_rec_ref_ptr != last_rec_ref_ptr);
>  > @@ -3249,8 +3198,7 @@ JOIN_CACHE_BKA_UNIQUE::join_matching_rec
>  >
>  >     if (error>  0&&  error != HA_ERR_END_OF_FILE)   
>  >       return NESTED_LOOP_ERROR;
>  > -finish:
>  > -  return end_join_matching_records(rc);
>  > +  return rc;
>  >   }
>  >
>  >
>  >
>  > === modified file 'sql/sql_select.cc'
>  > --- a/sql/sql_select.cc    2011-03-03 09:43:14 +0000
>  > +++ b/sql/sql_select.cc    2011-04-01 09:09:15 +0000
>  > @@ -11168,7 +11168,7 @@ make_join_readinfo(JOIN *join, ulonglong
>  >       */
>  >       tab->sorted= sorted;
>  >       sorted= 0;                                  // only first must 
> be sorted
>  > -    table->status=STATUS_NO_RECORD;
>  > +    table->status= STATUS_GARBAGE | STATUS_NOT_FOUND;
>  >       pick_table_access_method (tab);
>  >
>  >       if (tab->loosescan_match_tab)
>  > @@ -17960,7 +17960,7 @@ join_read_const_table(JOIN_TAB *tab, POS
>  >     TABLE *table=tab->table;
>  >     table->const_table=1;
>  >     table->null_row=0;
>  > -  table->status=STATUS_NO_RECORD;
>  > +  table->status= STATUS_GARBAGE | STATUS_NOT_FOUND;
>  >
>  >     if (tab->type == JT_SYSTEM)
>  >     {
>  > @@ -18154,7 +18154,7 @@ join_read_key2(JOIN_TAB *tab, TABLE *tab
>  >       indices on NOT NULL columns (see create_ref_for_key()).
>  >     */
>  >     if (cmp_buffer_with_ref(tab->join->thd, table, table_ref) ||
>  > -      (table->status&  (STATUS_GARBAGE | STATUS_NO_PARENT | 
> STATUS_NULL_ROW)))
>  > +      (table->status&  (STATUS_GARBAGE | STATUS_NULL_ROW)))
>  >     {
>  >       if (table_ref->key_err)
>  >       {
>  >
>  > === modified file 'sql/sql_select.h'
>  > --- a/sql/sql_select.h    2011-03-01 14:57:53 +0000
>  > +++ b/sql/sql_select.h    2011-04-01 09:09:15 +0000
>  > @@ -296,7 +296,6 @@ typedef struct st_join_table : public Sq
>  >
>  >     table_map    dependent,key_dependent;
>  >     uint        index;
>  > -  uint        status;                ///<  Save status for cache
>  >     uint        used_fields,used_fieldlength,used_blobs;
>  >     uint          used_null_fields;
>  >     uint          used_rowid_fields;
>  > @@ -470,7 +469,6 @@ st_join_table::st_join_table()
>  >       dependent(0),
>  >       key_dependent(0),
>  >       index(0),
>  > -    status(0),
>  >       used_fields(0),
>  >       used_fieldlength(0),
>  >       used_blobs(0),
>  > @@ -1032,9 +1030,6 @@ protected:
>  >     enum_nested_loop_state init_join_matching_records(RANGE_SEQ_IF 
> *seq_funcs,
>  >                                                       uint ranges);
>  >
>  > -  /* Finish searching for records that match records from the join 
> buffer */
>  > -  enum_nested_loop_state 
> end_join_matching_records(enum_nested_loop_state rc);
>  > -
>  >   public:
>  >
>  >     /*
>  >
>  > === modified file 'sql/structs.h'
>  > --- a/sql/structs.h    2010-10-21 09:49:16 +0000
>  > +++ b/sql/structs.h    2011-04-01 09:09:15 +0000
>  > @@ -231,13 +231,20 @@ typedef struct  user_conn {
>  >   #define REG_SKIP_DUP        256
>  >
>  >       /* Bits in form->status */
>  > -#define STATUS_NO_RECORD    (1+2)    /* Record isn't usably */
>  >   #define STATUS_GARBAGE        1
>  > -#define STATUS_NOT_FOUND    2    /* No record in database when 
> needed */
>  > -#define STATUS_NO_PARENT    4    /* Parent record wasn't found */
>  > -#define STATUS_NOT_READ        8    /* Record isn't read */
>  > -#define STATUS_UPDATED        16    /* Record is updated by formula */
>  > -#define STATUS_NULL_ROW        32    /* table->null_row is set */
>  > +/**
>  > +   Means we were searching for a row and didn't find it. This is 
> used by
>  > +   storage engines (@see handler::index_read_map()) and the Server 
> layer.
>  > +*/
>  > +#define STATUS_NOT_FOUND    2
>  > +/// Reserved for use by multi-table update. Means the row has been 
> updated.
>  > +#define STATUS_UPDATED        16
>  > +/**
>  > +   Means that table->null_row is set. This is an artificial 
> NULL-filled row
>  > +   (one example: in outer join, if no match has been found in inner 
> table).
>  > +*/
>  > +#define STATUS_NULL_ROW        32
>  > +/// Reserved for use by multi-table delete. Means the row has been 
> deleted.
>  >   #define STATUS_DELETED        64
> 
> I suggest removing the TABs above.

done

>  >
>  >   /*
>  >
>  > === modified file 'sql/table.cc'
>  > --- a/sql/table.cc    2011-02-09 15:17:14 +0000
>  > +++ b/sql/table.cc    2011-04-01 09:09:15 +0000
>  > @@ -3301,7 +3301,7 @@ void TABLE::init(THD *thd, TABLE_LIST *t
>  >     force_index= 0;
>  >     force_index_order= 0;
>  >     force_index_group= 0;
>  > -  status= STATUS_NO_RECORD;
>  > +  status= STATUS_GARBAGE | STATUS_NOT_FOUND;
> 
> I think it would suffice to initialize status to STATUS_GARBAGE.
> AFAICT, there is no test for STATUS_NOT_FOUND that does not also check
> STATUS_GARBAGE.  Also, comments indicate that STATUS_GARBAGE means
> first read.  (Same goes for other places that initialize
> table->status.)

I think Roy suggested the same. Here's what I replied (alas this mail 
didn't go to the list for some reason).

<quoting myself>

Observations about how TABLE::status is updated (I did a grep in the 
entire tree):
A) we most often update it using assignments of constants "x=const" or 
"x= const | const"
B) we never turn bits down using "&=" or "z= x & ..."
C) we turn bits on using "|=" or "z= x | ..." , but only 
STATUS_DELETED/UPDATED/NULL_ROW
D) we don't test STATUS_NOT_FOUND with bit tests like "x & STATUS_NOT_FOUND"
E) we have no tests for the presence of a single flag and absence of 
others, like "(x & (several constants)) == STATUS_GARBAGE"

So I wonder whether
STATUS_NO_RECORD
i.e.
STATUS_GARBAGE | STATUS_NOT_FOUND
could be just STATUS_GARBAGE.         <<<Oystein: same as your idea
The reasoning is the following.
A status is set to STATUS_GARBAGE | STATUS_NOT_FOUND. Let's look only at 
those two bits. They will stay on, together, until an assignment to a 
constant or union of constants (due to (B)). Let's study the period 
before this final assignment. The status may be tested. Does it matter 
that it's only STATUS_GARBAGE instead of STATUS_GARBAGE | 
STATUS_NOT_FOUND? It matters only if there are tests that 
STATUS_NOT_FOUND is on, or that both are on, or that only STATUS_GARBAGE 
is on. And there are no such tests (D, E). But I'm not sure of this 
reasoning.

Still I would prefer to not do the changes above. It would require an 
understanding which I cannot acquire unless I spend several days of 
concentrated code study on it. It looks like a little refactoring task 
but too big for the time alloted for this bugfix. Maybe we could write 
it on some list of small refactoring tasks to do one day?

<end of quoting myself>

>  >     insert_values= 0;
>  >     fulltext_searched= 0;
>  >     file->ft_handler= 0;
>  >
>  >
>  >
>  >
>  >
> 
> 


-- 
Mr. Guilhem Bichot <guilhem.bichot@stripped>
Oracle / MySQL / Optimizer team, Lead Software Engineer
Bordeaux, France
www.oracle.com / www.mysql.com
Thread
bzr commit into mysql-trunk branch (guilhem.bichot:3348) Bug#11766522Guilhem Bichot1 Apr
  • Re: bzr commit into mysql-trunk branch (guilhem.bichot:3348) Bug#11766522Øystein Grøvlen6 Apr
    • Re: bzr commit into mysql-trunk branch (guilhem.bichot:3348) Bug#11766522Guilhem Bichot26 Apr
      • Re: bzr commit into mysql-trunk branch (guilhem.bichot:3348) Bug#11766522Guilhem Bichot26 Apr
  • Re: bzr commit into mysql-trunk branch (guilhem.bichot:3348) Bug#11766522Roy Lyseng8 Apr
    • Re: bzr commit into mysql-trunk branch (guilhem.bichot:3348) Bug#11766522Guilhem Bichot26 Apr