From: Roy Lyseng
Date: May 20 2011 2:26pm
Subject: Re: bzr commit into mysql-trunk branch (olav.sandstaa:3377) Bug#12355958
List-Archive: http://lists.mysql.com/commits/137795
Message-Id: <4DD67A01.70108@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi Olav,

thanks for fixing this bug.

I think that you need to consider the case for Item_ref as described below, 
otherwise this patch is approved.

I wonder also if with_subselect should be private and accessed through a 
function, like used_tables_cache. I think this might be correct handling for 
Item_ref objects. However I do not consider this a necessary task for this bugfix.

Please also consider the comment about source code comments...

On 20.05.11 13.59, Olav Sandstaa wrote:
> #At file:///export/home/tmp/mysql2/icp-subquery-bug-fix/ based on revid:mayank.prasad@stripped
>
>   3377 Olav Sandstaa	2011-05-20
>        Fix for bug Bug#12355958 "FAILING ASSERTION: TRX->LOCK.N_ACTIVE_THRS == 1".
>
>        The assert that crashes in InnoDB is caused by the ICP code pushing
>        down an index condition that contains a subquery. When InnoDB
>        evaluates this index condition the subquery is executed and this results in
>        a second access to data in InnoDB. InnoDB does not allow a transaction
>        to have more than one concurrent executing table operation and this
>        causes the assert to trigger.
>
>        The ICP code should not have pushed down this index condition to InnoDB
>        since it contains a subquery. The existing ICP code checks for subqueries in
>        the condition by checking the value of the condition's with_subselect
>        member. In this case the value for the Item_func condition's with_subselect
>        is false even though one of the arguments for the Item_func is a subquery.
>        This situation can occur when we initially create the item tree
>        for a condition and then later substitutes non-subquery parts of it
>        with a subquery. In the test case for this bug this occurs when
>        optimize_conds() replaces a field reference with a subquery due to the
>        subquery will be const when it is evaluated. This replacement is done
>        without updating the Item_func node's with_subselect to true.
>
>        The fix for this would be easy if it was possible to update the
>        with_subselect member locally when doing changes. What makes this
>        more complex is that this information needs to be propagated
>        upwards in the item tree. Since we have no "parent reference" in
>        the item tree this can not be done locally. In order to be able
>        to propagate this information upwards in the item tree this patch
>        extends Item::update_uses_tables() to also update
typo: Item::update_used_tables()
>        the value of with_subselect when traversing the item tree.
>       @ mysql-test/include/icp_tests.inc
>          Test for Bug#12355958 "FAILING ASSERTION: TRX->LOCK.N_ACTIVE_THRS == 1".
>       @ mysql-test/r/innodb_icp.result
>          Test for Bug#12355958 "FAILING ASSERTION: TRX->LOCK.N_ACTIVE_THRS == 1".
>       @ mysql-test/r/innodb_icp_all.result
>          Test for Bug#12355958 "FAILING ASSERTION: TRX->LOCK.N_ACTIVE_THRS == 1".
>       @ mysql-test/r/innodb_icp_none.result
>          Test for Bug#12355958 "FAILING ASSERTION: TRX->LOCK.N_ACTIVE_THRS == 1".
>       @ mysql-test/r/myisam_icp.result
>          Test for Bug#12355958 "FAILING ASSERTION: TRX->LOCK.N_ACTIVE_THRS == 1".
>       @ mysql-test/r/myisam_icp_all.result
>          Test for Bug#12355958 "FAILING ASSERTION: TRX->LOCK.N_ACTIVE_THRS == 1".
>       @ mysql-test/r/myisam_icp_none.result
>          Test for Bug#12355958 "FAILING ASSERTION: TRX->LOCK.N_ACTIVE_THRS == 1".
>       @ sql/item.h
>          Extend Item_ref::update_used_tables() to propagate the value of
>          with_subselect.
>       @ sql/item_cmpfunc.cc
>          Extend Item_cond::update_used_tables(),
>          Item_is_not_null_test::update_used_tables(), and
>          Item_equal::update_used_tables() to update its value
>          for with_subselect based on the value in its arguments.
>       @ sql/item_cmpfunc.h
>          Extend Item_func_isnull::update_used_tables() to propagate
>          the value of with_subselect in the item tree.
>       @ sql/item_func.cc
>          Extend Item_func::update_used_tables() to update its value
>          for with_subselect based on the value in its arguments.
>       @ sql/item_row.cc
>          Extend Item_row::update_used_tables() to update its value
>          for with_subselect based on the value in its arguments.
>       @ sql/item_strfunc.cc
>          Extend Item_func_make_set::update_used_tables() to update its
>          value for with_subselect based on the value in its item object.
>       @ sql/item_sum.cc
>          Extend Item_sum::update_used_tables() to update its value
>          for with_subselect based on the value in its arguments.
>
>      modified:
>        mysql-test/include/icp_tests.inc
>        mysql-test/r/innodb_icp.result
>        mysql-test/r/innodb_icp_all.result
>        mysql-test/r/innodb_icp_none.result
>        mysql-test/r/myisam_icp.result
>        mysql-test/r/myisam_icp_all.result
>        mysql-test/r/myisam_icp_none.result
>        sql/item.h
>        sql/item_cmpfunc.cc
>        sql/item_cmpfunc.h
>        sql/item_func.cc
>        sql/item_row.cc
>        sql/item_strfunc.cc
>        sql/item_sum.cc
> === modified file 'mysql-test/include/icp_tests.inc'
>
> === modified file 'mysql-test/include/icp_tests.inc'
> --- a/mysql-test/include/icp_tests.inc	2011-03-24 13:16:36 +0000
> +++ b/mysql-test/include/icp_tests.inc	2011-05-20 11:58:59 +0000
> @@ -924,3 +924,27 @@
>   eval $query;
>
>   DROP TABLE t1, t2, t3;
> +
> +--echo #
> +--echo # Bug#12355958 "FAILING ASSERTION: TRX->LOCK.N_ACTIVE_THRS == 1"
> +--echo #
> +
> +CREATE TABLE t1 (
> +  pk INTEGER PRIMARY KEY,
> +  a INTEGER NOT NULL,
> +  b CHAR(1),
> +  KEY(b)
> +);
> +
> +INSERT INTO t1 VALUES (23,5,'d');
> +
> +let query=
> +SELECT a1.pk
> +FROM t1 AS a1 JOIN (SELECT * FROM t1 LIMIT 1) AS a2 ON a2.b = a1.b
> +WHERE a1.a = (SELECT pk FROM t1 LIMIT 1)
> +      AND (a1.a != a2.a OR a1.b IS NULL);
> +
> +eval EXPLAIN $query;
> +eval $query;
> +
> +DROP TABLE t1;
>
> === modified file 'mysql-test/r/innodb_icp.result'
> --- a/mysql-test/r/innodb_icp.result	2011-03-24 13:16:36 +0000
> +++ b/mysql-test/r/innodb_icp.result	2011-05-20 11:58:59 +0000
> @@ -871,5 +871,30 @@
>   1
>   2
>   DROP TABLE t1, t2, t3;
> +#
> +# Bug#12355958 "FAILING ASSERTION: TRX->LOCK.N_ACTIVE_THRS == 1"
> +#
> +CREATE TABLE t1 (
> +pk INTEGER PRIMARY KEY,
> +a INTEGER NOT NULL,
> +b CHAR(1),
> +KEY(b)
> +);
> +INSERT INTO t1 VALUES (23,5,'d');
> +EXPLAIN SELECT a1.pk
> +FROM t1 AS a1 JOIN (SELECT * FROM t1 LIMIT 1) AS a2 ON a2.b = a1.b
> +WHERE a1.a = (SELECT pk FROM t1 LIMIT 1)
> +AND (a1.a != a2.a OR a1.b IS NULL);
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
> +1	PRIMARY	<derived2>	system	NULL	NULL	NULL	NULL	1	
> +1	PRIMARY	a1	ref	b	b	2	const	1	Using where
> +3	SUBQUERY	t1	index	NULL	b	2	NULL	1	Using index
> +2	DERIVED	t1	ALL	NULL	NULL	NULL	NULL	1	
> +SELECT a1.pk
> +FROM t1 AS a1 JOIN (SELECT * FROM t1 LIMIT 1) AS a2 ON a2.b = a1.b
> +WHERE a1.a = (SELECT pk FROM t1 LIMIT 1)
> +AND (a1.a != a2.a OR a1.b IS NULL);
> +pk
> +DROP TABLE t1;
>   set default_storage_engine= @save_storage_engine;
>   set optimizer_switch=default;
>
> === modified file 'mysql-test/r/innodb_icp_all.result'
> --- a/mysql-test/r/innodb_icp_all.result	2011-03-29 08:10:26 +0000
> +++ b/mysql-test/r/innodb_icp_all.result	2011-05-20 11:58:59 +0000
> @@ -871,5 +871,30 @@
>   1
>   2
>   DROP TABLE t1, t2, t3;
> +#
> +# Bug#12355958 "FAILING ASSERTION: TRX->LOCK.N_ACTIVE_THRS == 1"
> +#
> +CREATE TABLE t1 (
> +pk INTEGER PRIMARY KEY,
> +a INTEGER NOT NULL,
> +b CHAR(1),
> +KEY(b)
> +);
> +INSERT INTO t1 VALUES (23,5,'d');
> +EXPLAIN SELECT a1.pk
> +FROM t1 AS a1 JOIN (SELECT * FROM t1 LIMIT 1) AS a2 ON a2.b = a1.b
> +WHERE a1.a = (SELECT pk FROM t1 LIMIT 1)
> +AND (a1.a != a2.a OR a1.b IS NULL);
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
> +1	PRIMARY	<derived2>	system	NULL	NULL	NULL	NULL	1	
> +1	PRIMARY	a1	ref	b	b	2	const	1	Using where
> +3	SUBQUERY	t1	index	NULL	b	2	NULL	1	Using index
> +2	DERIVED	t1	ALL	NULL	NULL	NULL	NULL	1	
> +SELECT a1.pk
> +FROM t1 AS a1 JOIN (SELECT * FROM t1 LIMIT 1) AS a2 ON a2.b = a1.b
> +WHERE a1.a = (SELECT pk FROM t1 LIMIT 1)
> +AND (a1.a != a2.a OR a1.b IS NULL);
> +pk
> +DROP TABLE t1;
>   set default_storage_engine= @save_storage_engine;
>   set optimizer_switch=default;
>
> === modified file 'mysql-test/r/innodb_icp_none.result'
> --- a/mysql-test/r/innodb_icp_none.result	2011-03-24 13:16:36 +0000
> +++ b/mysql-test/r/innodb_icp_none.result	2011-05-20 11:58:59 +0000
> @@ -870,5 +870,30 @@
>   1
>   2
>   DROP TABLE t1, t2, t3;
> +#
> +# Bug#12355958 "FAILING ASSERTION: TRX->LOCK.N_ACTIVE_THRS == 1"
> +#
> +CREATE TABLE t1 (
> +pk INTEGER PRIMARY KEY,
> +a INTEGER NOT NULL,
> +b CHAR(1),
> +KEY(b)
> +);
> +INSERT INTO t1 VALUES (23,5,'d');
> +EXPLAIN SELECT a1.pk
> +FROM t1 AS a1 JOIN (SELECT * FROM t1 LIMIT 1) AS a2 ON a2.b = a1.b
> +WHERE a1.a = (SELECT pk FROM t1 LIMIT 1)
> +AND (a1.a != a2.a OR a1.b IS NULL);
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
> +1	PRIMARY	<derived2>	system	NULL	NULL	NULL	NULL	1	
> +1	PRIMARY	a1	ref	b	b	2	const	1	Using where
> +3	SUBQUERY	t1	index	NULL	b	2	NULL	1	Using index
> +2	DERIVED	t1	ALL	NULL	NULL	NULL	NULL	1	
> +SELECT a1.pk
> +FROM t1 AS a1 JOIN (SELECT * FROM t1 LIMIT 1) AS a2 ON a2.b = a1.b
> +WHERE a1.a = (SELECT pk FROM t1 LIMIT 1)
> +AND (a1.a != a2.a OR a1.b IS NULL);
> +pk
> +DROP TABLE t1;
>   set default_storage_engine= @save_storage_engine;
>   set optimizer_switch=default;
>
> === modified file 'mysql-test/r/myisam_icp.result'
> --- a/mysql-test/r/myisam_icp.result	2011-03-24 13:16:36 +0000
> +++ b/mysql-test/r/myisam_icp.result	2011-05-20 11:58:59 +0000
> @@ -868,4 +868,28 @@
>   1
>   2
>   DROP TABLE t1, t2, t3;
> +#
> +# Bug#12355958 "FAILING ASSERTION: TRX->LOCK.N_ACTIVE_THRS == 1"
> +#
> +CREATE TABLE t1 (
> +pk INTEGER PRIMARY KEY,
> +a INTEGER NOT NULL,
> +b CHAR(1),
> +KEY(b)
> +);
> +INSERT INTO t1 VALUES (23,5,'d');
> +EXPLAIN SELECT a1.pk
> +FROM t1 AS a1 JOIN (SELECT * FROM t1 LIMIT 1) AS a2 ON a2.b = a1.b
> +WHERE a1.a = (SELECT pk FROM t1 LIMIT 1)
> +AND (a1.a != a2.a OR a1.b IS NULL);
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
> +1	PRIMARY	NULL	NULL	NULL	NULL	NULL	NULL	NULL	Impossible WHERE noticed after reading const tables
> +3	SUBQUERY	t1	system	NULL	NULL	NULL	NULL	1	
> +2	DERIVED	t1	system	NULL	NULL	NULL	NULL	1	
> +SELECT a1.pk
> +FROM t1 AS a1 JOIN (SELECT * FROM t1 LIMIT 1) AS a2 ON a2.b = a1.b
> +WHERE a1.a = (SELECT pk FROM t1 LIMIT 1)
> +AND (a1.a != a2.a OR a1.b IS NULL);
> +pk
> +DROP TABLE t1;
>   set optimizer_switch=default;
>
> === modified file 'mysql-test/r/myisam_icp_all.result'
> --- a/mysql-test/r/myisam_icp_all.result	2011-03-29 08:10:26 +0000
> +++ b/mysql-test/r/myisam_icp_all.result	2011-05-20 11:58:59 +0000
> @@ -868,4 +868,28 @@
>   1
>   2
>   DROP TABLE t1, t2, t3;
> +#
> +# Bug#12355958 "FAILING ASSERTION: TRX->LOCK.N_ACTIVE_THRS == 1"
> +#
> +CREATE TABLE t1 (
> +pk INTEGER PRIMARY KEY,
> +a INTEGER NOT NULL,
> +b CHAR(1),
> +KEY(b)
> +);
> +INSERT INTO t1 VALUES (23,5,'d');
> +EXPLAIN SELECT a1.pk
> +FROM t1 AS a1 JOIN (SELECT * FROM t1 LIMIT 1) AS a2 ON a2.b = a1.b
> +WHERE a1.a = (SELECT pk FROM t1 LIMIT 1)
> +AND (a1.a != a2.a OR a1.b IS NULL);
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
> +1	PRIMARY	NULL	NULL	NULL	NULL	NULL	NULL	NULL	Impossible WHERE noticed after reading const tables
> +3	SUBQUERY	t1	system	NULL	NULL	NULL	NULL	1	
> +2	DERIVED	t1	system	NULL	NULL	NULL	NULL	1	
> +SELECT a1.pk
> +FROM t1 AS a1 JOIN (SELECT * FROM t1 LIMIT 1) AS a2 ON a2.b = a1.b
> +WHERE a1.a = (SELECT pk FROM t1 LIMIT 1)
> +AND (a1.a != a2.a OR a1.b IS NULL);
> +pk
> +DROP TABLE t1;
>   set optimizer_switch=default;
>
> === modified file 'mysql-test/r/myisam_icp_none.result'
> --- a/mysql-test/r/myisam_icp_none.result	2011-03-24 13:16:36 +0000
> +++ b/mysql-test/r/myisam_icp_none.result	2011-05-20 11:58:59 +0000
> @@ -867,4 +867,28 @@
>   1
>   2
>   DROP TABLE t1, t2, t3;
> +#
> +# Bug#12355958 "FAILING ASSERTION: TRX->LOCK.N_ACTIVE_THRS == 1"
> +#
> +CREATE TABLE t1 (
> +pk INTEGER PRIMARY KEY,
> +a INTEGER NOT NULL,
> +b CHAR(1),
> +KEY(b)
> +);
> +INSERT INTO t1 VALUES (23,5,'d');
> +EXPLAIN SELECT a1.pk
> +FROM t1 AS a1 JOIN (SELECT * FROM t1 LIMIT 1) AS a2 ON a2.b = a1.b
> +WHERE a1.a = (SELECT pk FROM t1 LIMIT 1)
> +AND (a1.a != a2.a OR a1.b IS NULL);
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
> +1	PRIMARY	NULL	NULL	NULL	NULL	NULL	NULL	NULL	Impossible WHERE noticed after reading const tables
> +3	SUBQUERY	t1	system	NULL	NULL	NULL	NULL	1	
> +2	DERIVED	t1	system	NULL	NULL	NULL	NULL	1	
> +SELECT a1.pk
> +FROM t1 AS a1 JOIN (SELECT * FROM t1 LIMIT 1) AS a2 ON a2.b = a1.b
> +WHERE a1.a = (SELECT pk FROM t1 LIMIT 1)
> +AND (a1.a != a2.a OR a1.b IS NULL);
> +pk
> +DROP TABLE t1;
>   set optimizer_switch=default;
>
> === modified file 'sql/item.h'
> --- a/sql/item.h	2011-04-08 13:41:38 +0000
> +++ b/sql/item.h	2011-05-20 11:58:59 +0000
> @@ -2623,8 +2623,13 @@
>     }
>     void update_used_tables()
>     {
> -    if (!depended_from)
> -      (*ref)->update_used_tables();
> +    if (!depended_from)
> +    {
> +      (*ref)->update_used_tables();
> +
> +      // Propagate information about sub-queries
> +      with_subselect= (*ref)->with_subselect;

I am not sure about the correctness of this sentence. It does not seem that 
Item_ref::fix_fields() propagates the flag from the ref object. On the other 
hand, we should test that this really works with Item_ref objects, such as when 
the referenced column is from a view.
> +    }
>     }
>     virtual table_map resolved_used_tables() const;
>     table_map not_null_tables() const { return (*ref)->not_null_tables(); }
>
> === modified file 'sql/item_cmpfunc.cc'
> --- a/sql/item_cmpfunc.cc	2011-05-18 10:43:46 +0000
> +++ b/sql/item_cmpfunc.cc	2011-05-20 11:58:59 +0000
> @@ -4615,11 +4615,15 @@
>
>     used_tables_cache=0;
>     const_item_cache=1;
> +  with_subselect= false;
>     while ((item=li++))
>     {
>       item->update_used_tables();
>       used_tables_cache|= item->used_tables();
>       const_item_cache&= item->const_item();
> +
> +    // Propagate information about sub-queries
> +    with_subselect|= item->with_subselect;

I think this code line is pretty self-explanatory, so I consider the comment 
above to be redundant.
This comment applies to all similar statements in the patch.

>     }
>   }
>
> @@ -4803,6 +4807,10 @@
>     else
>     {
>       args[0]->update_used_tables();
> +
> +    // Propagate information about sub-queries
> +    with_subselect= args[0]->with_subselect;
> +
>       if (!(used_tables_cache=args[0]->used_tables())&&  !with_subselect)
>       {
>         /* Remember if the value is always NULL or never NULL */
> @@ -5827,12 +5835,16 @@
>     not_null_tables_cache= used_tables_cache= 0;
>     if ((const_item_cache= cond_false))
>       return;
> +  with_subselect= false;
>     while ((item=li++))
>     {
>       item->update_used_tables();
>       used_tables_cache|= item->used_tables();
>       /* see commentary at Item_equal::update_const() */
>       const_item_cache&= item->const_item()&&  !item->is_outer_field();
> +
> +    // Propagate information about sub_queries
> +    with_subselect|= item->with_subselect;
>     }
>   }
>
>
> === modified file 'sql/item_cmpfunc.h'
> --- a/sql/item_cmpfunc.h	2011-03-22 11:44:40 +0000
> +++ b/sql/item_cmpfunc.h	2011-05-20 11:58:59 +0000
> @@ -1363,6 +1363,10 @@
>       else
>       {
>         args[0]->update_used_tables();
> +
> +      // Propagate information about sub-queries
> +      with_subselect= args[0]->with_subselect;
> +
>         if ((const_item_cache= !(used_tables_cache= args[0]->used_tables())&&
>             !with_subselect))
>         {
>
> === modified file 'sql/item_func.cc'
> --- a/sql/item_func.cc	2011-05-12 18:44:56 +0000
> +++ b/sql/item_func.cc	2011-05-20 11:58:59 +0000
> @@ -405,11 +405,15 @@
>   {
>     used_tables_cache=0;
>     const_item_cache=1;
> +  with_subselect= false;
>     for (uint i=0 ; i<  arg_count ; i++)
>     {
>       args[i]->update_used_tables();
>       used_tables_cache|=args[i]->used_tables();
>       const_item_cache&=args[i]->const_item();
> +
> +    // Propagate information about sub-queries
> +    with_subselect|= args[i]->with_subselect;
>     }
>   }
>
>
> === modified file 'sql/item_row.cc'
> --- a/sql/item_row.cc	2011-05-06 13:32:53 +0000
> +++ b/sql/item_row.cc	2011-05-20 11:58:59 +0000
> @@ -126,11 +126,15 @@
>   {
>     used_tables_cache= 0;
>     const_item_cache= 1;
> +  with_subselect= false;
>     for (uint i= 0; i<  arg_count; i++)
>     {
>       items[i]->update_used_tables();
>       used_tables_cache|= items[i]->used_tables();
>       const_item_cache&= items[i]->const_item();
> +
> +    // Propagate information about sub-queries
> +    with_subselect|= items[i]->with_subselect;
>     }
>   }
>
>
> === modified file 'sql/item_strfunc.cc'
> --- a/sql/item_strfunc.cc	2011-05-06 13:32:53 +0000
> +++ b/sql/item_strfunc.cc	2011-05-20 11:58:59 +0000
> @@ -2571,6 +2571,9 @@
>     item->update_used_tables();
>     used_tables_cache|=item->used_tables();
>     const_item_cache&=item->const_item();
> +
> +  // Propagage information about sub-queries
> +  with_subselect= item->with_subselect;
>   }
>
>
>
> === modified file 'sql/item_sum.cc'
> --- a/sql/item_sum.cc	2011-04-12 10:31:30 +0000
> +++ b/sql/item_sum.cc	2011-05-20 11:58:59 +0000
> @@ -529,10 +529,14 @@
>     if (!forced_const)
>     {
>       used_tables_cache= 0;
> +    with_subselect= false;
>       for (uint i=0 ; i<  arg_count ; i++)
>       {
>         args[i]->update_used_tables();
>         used_tables_cache|= args[i]->used_tables();
> +
> +      // Propagate information about sub-queries
> +      with_subselect|= args[i]->with_subselect;
>       }
>
>       used_tables_cache&= PSEUDO_TABLE_BITS;

Thanks,
Roy