From: Date: November 17 2008 11:32pm
Subject: Re: bzr commit into mysql-5.1 branch (kristofer.pettersson:2668)
 Bug#39843
List-Archive: http://lists.mysql.com/commits/59010
Message-Id: <4921F115.7000803@sun.com>
MIME-Version: 1.0
Content-Type: text/plain; format=flowed; charset=us-ascii
Content-Transfer-Encoding: 7BIT


Hi Kristofer

See some comments below.

Kristofer Pettersson wrote:
> #At file:///home/thek/Development/cpp/mysqlbzr/mysql-5.1-bug39843/
> 
>  2668 Kristofer Pettersson	2008-10-20
>       Bug#39843 DELETE requires write access to table in subquery in where clause
>       
>       An unnecessarily restrictive lock were taken on sub-SELECTs during DELETE.
>       
>       During parsing a global structure is reused for sub-SELECTs and the variable
>       controlling lock options were not reset properly. This patch sets the default
>       value so that a sub-SELECT will try to acquire a READ lock if possible
>       instead of a WRITE lock as inherited from the outer DELETE statement.
> modified:
>   mysql-test/r/lock.result
>   mysql-test/t/lock.test
>   sql/sql_parse.cc
> 
> per-file messages:
>   sql/sql_parse.cc
>     * Reset lock_option to a deafult value for SELECT statements.
> === modified file 'mysql-test/r/lock.result'
> --- a/mysql-test/r/lock.result	2007-08-02 09:59:02 +0000
> +++ b/mysql-test/r/lock.result	2008-10-20 09:51:24 +0000
> @@ -166,4 +166,26 @@ ERROR HY000: View's SELECT refers to a t
>  Cleanup.
>  
>  drop table t2, t3;
> +#
> +# Bug#39843 DELETE requires write access to table in subquery in where clause
> +#
> +DROP TABLE IF EXISTS t1,t2;
> +CREATE TABLE t1 (
> +table1_rowid SMALLINT NOT NULL
> +);
> +CREATE TABLE t2 (
> +table2_rowid SMALLINT NOT NULL
> +);
> +INSERT INTO t1 VALUES (1);
> +INSERT INTO t2 VALUES (1);
> +LOCK TABLES t1 WRITE, t2 READ;
> +DELETE FROM t1 
> +WHERE EXISTS 
> +( 
> +SELECT 'x' 
> +FROM t2
> +WHERE t1.table1_rowid = t2.table2_rowid
> +) ;
> +UNLOCK TABLES;
> +DROP TABLE t1,t2;

Ok

>  End of 5.1 tests.
> 
> === modified file 'mysql-test/t/lock.test'
> --- a/mysql-test/t/lock.test	2007-08-02 09:59:02 +0000
> +++ b/mysql-test/t/lock.test	2008-10-20 09:51:24 +0000
> @@ -214,4 +214,31 @@ create view v_bug5719 as select * from t
>  --echo
>  drop table t2, t3;
>  
> +--echo #
> +--echo # Bug#39843 DELETE requires write access to table in subquery in where clause
> +--echo #
> +
> +--disable_warnings
> +DROP TABLE IF EXISTS t1,t2;
> +--enable_warnings
> +
> +CREATE TABLE t1 (
> +	table1_rowid SMALLINT NOT NULL
> +);
> +CREATE TABLE t2 (
> +	table2_rowid SMALLINT NOT NULL
> +);
> +INSERT INTO t1 VALUES (1);
> +INSERT INTO t2 VALUES (1);
> +LOCK TABLES t1 WRITE, t2 READ; 
> +DELETE FROM t1 
> +WHERE EXISTS 
> +( 
> +	SELECT 'x' 
> +	FROM t2
> + 	WHERE t1.table1_rowid = t2.table2_rowid
> +) ; 
> +UNLOCK TABLES;
> +DROP TABLE t1,t2;
> +

Ok

>  --echo End of 5.1 tests.
> 
> === modified file 'sql/sql_parse.cc'
> --- a/sql/sql_parse.cc	2008-10-13 10:22:36 +0000
> +++ b/sql/sql_parse.cc	2008-10-20 09:51:24 +0000
> @@ -5525,6 +5525,12 @@ mysql_new_select(LEX *lex, bool move_dow
>    select_lex->parent_lex= lex; /* Used in init_query. */
>    select_lex->init_query();
>    select_lex->init_select();
> +  /*
> +    Lock_option is passed to st_select_lex::add_table_to_list during
> +    parsing and will determine which lock level a table lock will attempt to
> +    acquire.
> +  */
> +  lex->lock_option= TL_READ_DEFAULT;

In other words, st_select_lex::add_table_to_list() did work by side 
effects and was broken in a special case, and this fix adds another side 
effect to compensate.

Sorry but no, I know this is a 1 line fix, and it's rejected.

LEX::lock_option is a global property for the entire statement, which is 
set for example for DELETE, UPDATE statements ...

The fact that calls to st_select_lex::add_table_to_list() did re-use 
LEX::lock_option would be ok if this property did not change for the 
entire statement, but this is not the case.

It seems that, when sub selects query fragments are present in a query, 
there is a need for a different lock_option value for each sub select.
In that case, I would much rather prefer to see the code:
- define a st_select_lex::lock_option member, which represent the lock 
option property *for this sub select*,
- leave LEX::lock_option alone,
- always call st_select_lex::add_tables_to_list() with 
st_select_lex::lock_option, or even better not pass a lock_option 
parameter at all if it's already a member of the st_select_lex class ...

This patch identified the root cause of the problem (the lock_option 
value was wrong for the use case reported), which is good, but I don't 
agree with the solution.

This solution is especially risky since it's extremely hard to predict, 
in a bison parser and given how our code is written, when exactly 
LEX::lock_option is set, and in which order different rules actions are 
executed.
We should simply abandon the idea of trying to maintain a global state 
variable in an ascendant parser.

Please investigate the st_select_lex::lock_option suggestion.
Also, I would like to see some doxygen comments in LEX::lock_option 
then, that explains what this attribute is for, since you found out ;)

Thanks for looking into this.

Regards,
-- Marc.