List:Commits« Previous MessageNext Message »
From:Roy Lyseng Date:December 13 2010 1:43pm
Subject:Re: bzr commit into mysql-5.1 branch (ole.john.aske:3477) Bug#58628
View as plain text  
Hi Ole John,

thank you for working on this.

The bugfix solves the problem. I am unsure of the bigger impact on the system, 
however, so I need to rely on a second reviewer evaluating this.

On 01.12.10 13.20, Ole John Aske wrote:
> #Atfile:///net/fimafeng09/export/home/tmp/oleja/mysql/mysql-5.1/  based
> onrevid:georgi.kodinov@stripped
>
>   3477 Ole John Aske	2010-12-01
>        Fix for bug#58628: Incorrect result for 'WHERE NULL NOT IN (<subquery>)
>
>        create_ref_for_key() allowed any constant part of a REF key to be evaluated
>        and stored into the 'key_buff' during ::optimize(). These 'store_key*' was
>        *not* kept in ref.key_copy[] as they where constant and we assumed we
>        would not have to reevaluate them during JOIN::exec()
>
>        However, during execute NULL values in REF key has to be detected as they may
>        need special attention - as in 'Full scan on NULL key'. This is done by
>        subselect_uniquesubquery_engine::copy_ref_key() which check if any keyparts
>        evaluated to a NULL-value.
>
>        As we didn't keep a store_key for a constant value, a NULL-constant was not
> detected
>        by subselect_uniquesubquery_engine::copy_ref_key() !
>
>        This fixs modifies create_ref_for_key() to check if a NULL-value constant
>        was produced - In these cases it keeps the store_key, which then will be
>        reevaluated in JOIN::exec() and trigger correct handling of NULL-valued keys.
>
>      modified:
>        mysql-test/r/func_in.result
>        mysql-test/t/func_in.test
>        sql/sql_select.cc
>        sql/sql_select.h
> === modified file 'mysql-test/r/func_in.result'
> --- a/mysql-test/r/func_in.result	2010-06-22 18:53:08 +0000
> +++ b/mysql-test/r/func_in.result	2010-12-01 12:20:46 +0000
> @@ -770,4 +770,40 @@ CASE a WHEN a THEN a END
>   NULL
>   DROP TABLE t1;
>   #
> +# Bug#58628: Incorrect result for 'WHERE NULL NOT IN (<subquery>)
> +#
> +CREATE TABLE t1 (pk INT NOT NULL, i INT);
> +INSERT INTO t1 VALUES (0,NULL), (1,NULL), (2,NULL), (3,NULL);
> +CREATE TABLE subq (pk INT NOT NULL, i INT NOT NULL, PRIMARY KEY(i,pk));
> +INSERT INTO subq VALUES (0,0), (1,1), (2,2), (3,3);
> +SELECT * FROM t1
> +WHERE t1.i NOT IN
> +(SELECT i FROM subq WHERE subq.pk = t1.pk);
> +pk	i
> +SELECT * FROM t1
> +WHERE t1.i IN
> +(SELECT i FROM subq WHERE subq.pk = t1.pk) IS UNKNOWN;
> +pk	i
> +0	NULL
> +1	NULL
> +2	NULL
> +3	NULL
> +SELECT * FROM t1
> +WHERE NULL NOT IN
> +(SELECT i FROM subq WHERE subq.pk = t1.pk);
> +pk	i
> +SELECT * FROM t1
> +WHERE NULL IN
> +(SELECT i FROM subq WHERE subq.pk = t1.pk) IS UNKNOWN;
> +pk	i
> +0	NULL
> +1	NULL
> +2	NULL
> +3	NULL
> +SELECT * FROM t1
> +WHERE 1+NULL NOT IN
> +(SELECT i FROM subq WHERE subq.pk = t1.pk);
> +pk	i
> +DROP TABLE t1,subq;
> +#
>   End of 5.1 tests
>
> === modified file 'mysql-test/t/func_in.test'
> --- a/mysql-test/t/func_in.test	2010-06-22 18:53:08 +0000
> +++ b/mysql-test/t/func_in.test	2010-12-01 12:20:46 +0000
> @@ -555,5 +555,50 @@ SELECT CASE a WHEN a THEN a END FROM t1
>   DROP TABLE t1;
>
>   --echo #
> +--echo # Bug#58628: Incorrect result for 'WHERE NULL NOT IN (<subquery>)
> +--echo #
> +
> +CREATE TABLE t1 (pk INT NOT NULL, i INT);
> +INSERT INTO t1 VALUES (0,NULL), (1,NULL), (2,NULL), (3,NULL);
> +
> +CREATE TABLE subq (pk INT NOT NULL, i INT NOT NULL, PRIMARY KEY(i,pk));
> +INSERT INTO subq VALUES (0,0), (1,1), (2,2), (3,3);
> +
> +## Baseline queries: t1.i contains only NULL and should effectively
> +## be evaluated as 'WHERE NULL IN'
> +## .. These return correct resultset !
> +
> +--sorted_result
> +SELECT * FROM t1
> +  WHERE t1.i NOT IN
> +    (SELECT i FROM subq WHERE subq.pk = t1.pk);
> +
> +--sorted_result
> +SELECT * FROM t1
> +  WHERE t1.i IN
> +    (SELECT i FROM subq WHERE subq.pk = t1.pk) IS UNKNOWN;
> +
> +## Replaced 't1.i' with some constant expression which
> +## also evaluates to NULL. Expected to return same result as above:
> +
> +--sorted_result
> +SELECT * FROM t1
> +  WHERE NULL NOT IN
> +    (SELECT i FROM subq WHERE subq.pk = t1.pk);
> +
> +--sorted_result
> +SELECT * FROM t1
> +  WHERE NULL IN
> +    (SELECT i FROM subq WHERE subq.pk = t1.pk) IS UNKNOWN;
> +
> +--sorted_result
> +SELECT * FROM t1
> +  WHERE 1+NULL NOT IN
> +    (SELECT i FROM subq WHERE subq.pk = t1.pk);
> +
> +
> +DROP TABLE t1,subq;
> +
> +--echo #
>
>   --echo End of 5.1 tests
>
> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2010-10-29 08:23:06 +0000
> +++ b/sql/sql_select.cc	2010-12-01 12:20:46 +0000
> @@ -5798,22 +5798,36 @@ static bool create_ref_for_key(JOIN *joi
>         if (keyuse->null_rejecting)
>           j->ref.null_rejecting |= 1<<  i;
>         keyuse_uses_no_tables= keyuse_uses_no_tables&& 
> !keyuse->used_tables;
> -      if (!keyuse->used_tables&&
> -	  !(join->select_options&  SELECT_DESCRIBE))
> -      {					// Compare against constant
> -	store_key_item tmp(thd, keyinfo->key_part[i].field,
> -                           key_buff + maybe_null,
> -                           maybe_null ?  key_buff : 0,
> -                           keyinfo->key_part[i].length, keyuse->val);
> -	if (thd->is_fatal_error)
> -	  DBUG_RETURN(TRUE);
> -	tmp.copy();
> +
> +      store_key* key= get_store_key(thd,
> +				    keyuse,join->const_table_map,
> +				&keyinfo->key_part[i],
> +				    key_buff, maybe_null);
> +      if (unlikely(!key || thd->is_fatal_error))
> +        DBUG_RETURN(TRUE);
> +
> +      if (keyuse->used_tables || join->select_options&  SELECT_DESCRIBE)
> +      {
> +	*ref_key++= key; // Always evaluate/explain in JOIN::exec()
>         }
>         else
> -	*ref_key++= get_store_key(thd,
> -				  keyuse,join->const_table_map,
> -				&keyinfo->key_part[i],
> -				  key_buff, maybe_null);
> +      {
> +        /* key is constant, copy value now and possibly skip it while ::exec() */
> +        enum store_key::store_key_result result= key->copy();
> +
> +        /* Depending on 'result' it should be reevaluated in ::exec(), if either:
> +         *  1) '::copy()' failed, in case we reevaluate - and refail in
> +         *       JOIN::exec() where the error can be handled.
> +         *  2)  Constant evaluated to NULL value which we might need to
> +         *      handle as a special case during JOIN::exec()
> +         *      (As in : 'Full scan on NULL key')
> +         */
Please follow rule for comments:
  - Comment starts on next line, two columns indented from start.
  - No '*' characters at start of each line.
  - End line aligned with first line.
> +        if (result!=store_key::STORE_KEY_OK  ||    // 1)

Please insert spaces around the != operator.
> +            key->null_key)                         // 2)
> +        {
> +	  *ref_key++= key;  // Reevaluate in JOIN::exec()
TAB character on this line.
> +        }
> +      }

Please check for too long lines above.

It is possible to rearrange this code to perform the increment only once, by 
inverting logic and using continue statement, but not sure if it is worthwhile...
>         /*
>   	Remember if we are going to use REF_OR_NULL
>   	But only if field _really_ can be null i.e. we force JT_REF
>
> === modified file 'sql/sql_select.h'
> --- a/sql/sql_select.h	2010-08-24 10:44:15 +0000
> +++ b/sql/sql_select.h	2010-12-01 12:20:46 +0000
> @@ -753,9 +753,8 @@ public:
>     store_key_const_item(THD *thd, Field *to_field_arg, uchar *ptr,
>   		       uchar *null_ptr_arg, uint length,
>   		       Item *item_arg)
> -    :store_key_item(thd, to_field_arg,ptr,
> -		    null_ptr_arg ? null_ptr_arg : item_arg->maybe_null ?
> -		&err : (uchar*) 0, length, item_arg), inited(0)
> +    :store_key_item(thd, to_field_arg, ptr,
> +                    null_ptr_arg, length, item_arg), inited(0)
>     {
>     }
>     const char *name() const { return "const"; }
> @@ -763,23 +762,13 @@ public:
>   protected:
>     enum store_key_result copy_inner()
>     {
> -    int res;
>       if (!inited)
>       {
>         inited=1;
> -      if ((res= item->save_in_field(to_field, 1)))
> -      {
> -        if (!err)
> -          err= res<  0 ? 1 : res; /* 1=STORE_KEY_FATAL */
> -      }
> -      /*
> -        Item::save_in_field() may call Item::val_xxx(). And if this is a subquery
> -        we need to check for errors executing it and react accordingly
> -        */
> -      if (!err&&  to_field->table->in_use->is_error())
> -        err= 1; /* STORE_KEY_FATAL */
> +      int res= store_key_item::copy_inner();
> +      if (res&&  !err)
> +        err= res;
>       }
> -    null_key= to_field->is_null() || item->null_value;
>       return (err>  2 ? STORE_KEY_FATAL : (store_key_result) err);
>     }
>   };

Thank you,
Roy
Thread
bzr commit into mysql-5.1 branch (ole.john.aske:3477) Bug#58628Ole John Aske1 Dec
  • Re: bzr commit into mysql-5.1 branch (ole.john.aske:3477) Bug#58628Roy Lyseng13 Dec