From: Martin Hansson
Date: September 28 2010 7:23am
Subject: Re: bzr commit into mysql-5.1-bugteam branch (Sergey.Glukhov:3510)
 Bug#54484
List-Archive: http://lists.mysql.com/commits/119221
Message-Id: <4CA197EF.7040804@oracle.com>
MIME-Version: 1.0
Content-Type: multipart/alternative;
 boundary="------------070204030304010100010304"

--------------070204030304010100010304
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

  Hi,
I see a risk with your patch in that you introduce a new field that must 
be written to every time the old field is written to. This forces you 
(and anyone writing new code that writes to TABLE_LIST::outer_join) to 
copy-paste the same code. This is not very safe.

I suggest

- Since ::outer_join is only updated in one place, I suggest you copy 
the value to orig_outer_join there instead.

- Alternatively, create a method set_outer_join_type(uint join_type) { 
orig_outer_join= outer_join; outer_join= join_type; }.

Both of these approaches would make your patch smaller and easier to 
merge. Some comments are inlined below, otherwise the patch is fine.

Best Regards

Martin

Sergey Glukhov wrote:
> #Atfile:///home/gluh/MySQL/mysql-5.1-bugteam/  based on revid:mikael@dator8-20100916065358-em2dzakdx3n5z4kz
>
>   3510 Sergey Glukhov	2010-09-16
>        Bug#54484 explain + prepared statement: crash and Got error -1 from storage engine
>        If outer join is replaced with inner join
>        TABLE_LIST::outer_join value is changed in
>        simplify_joins() function. On second
>        execution of PS TABLE::maybe_null is initialized
>        with changed TABLE_LIST::outer_join value.
>        It affects list_contains_unique_index() behaviour
>        and optimizer incorrectly decides to optimize
>        away GROUP BY clause(see JOIN::optimize).
>        The fix is to use original outer_join value
>        for TABLE::maybe_null initialization.
>    
Can you please make the lines just a little longer? 80 characters is 
what most people use.
>       @ mysql-test/r/fulltext.result
>          test case
>       @ mysql-test/t/fulltext.test
>          test case
>       @ sql/mysql_priv.h
>          use original value for initialization
>       @ sql/sql_parse.cc
>          save original outer_join value
>       @ sql/sql_yacc.yy
>          save original outer_join value
>       @ sql/table.h
>          new field orig_outer_join
>
>      modified:
>        mysql-test/r/fulltext.result
>        mysql-test/t/fulltext.test
>        sql/mysql_priv.h
>        sql/sql_parse.cc
>        sql/sql_yacc.yy
>        sql/table.h
> === modified file 'mysql-test/r/fulltext.result'
> --- a/mysql-test/r/fulltext.result	2010-03-25 12:08:21 +0000
> +++ b/mysql-test/r/fulltext.result	2010-09-16 11:24:57 +0000
> @@ -644,4 +644,39 @@ Table	Op	Msg_type	Msg_text
>   test.t1	repair	status	OK
>   SETmyisam_sort_buffer_size=@@global.myisam_sort_buffer_size;
>   DROP TABLE t1;
> +#
> +# Bug#54484 explain + prepared statement: crash and Got error -1 from storage engine
> +#
> +CREATE TABLE t1(f1 VARCHAR(6) NOT NULL,
> +FULLTEXT KEY(f1),UNIQUE(f1));
> +INSERT INTO t1 VALUES ('test');
> +PREPARE stmt FROM
> +'SELECT 1 FROM t1 WHERE 1>
> + ALL((SELECT 1 FROM t1 RIGHT OUTER JOIN t1 a
> + ON (MATCH(t1.f1) against (""))
> + WHERE t1.f1 GROUP BY t1.f1)) xor f1 or f1>  1';
> +EXECUTE stmt;
> +1
> +1
> +EXECUTE stmt;
> +1
> +1
> +DEALLOCATE PREPARE stmt;
> +PREPARE stmt FROM
> +'EXPLAIN SELECT 1 FROM t1
> + WHERE 1>  ALL((SELECT 1 FROM t1 RIGHT OUTER JOIN t1 a
> + ON (MATCH(t1.f1) AGAINST (""))
> + WHERE t1.f1 GROUP BY t1.f1)) XOR f1 or f1>1';
> +EXECUTE stmt;
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
> +1	PRIMARY	t1	system	f1_2,f1	NULL	NULL	NULL	1	
> +2	DEPENDENT SUBQUERY	a	system	NULL	NULL	NULL	NULL	1	Using temporary; Using filesort
> +2	DEPENDENT SUBQUERY	t1	fulltext	f1	f1	0		1	Using where
> +EXECUTE stmt;
> +id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
> +1	PRIMARY	t1	system	f1_2,f1	NULL	NULL	NULL	1	
> +2	DEPENDENT SUBQUERY	a	system	NULL	NULL	NULL	NULL	1	Using temporary; Using filesort
> +2	DEPENDENT SUBQUERY	t1	fulltext	f1	f1	0		1	Using where
> +DEALLOCATE PREPARE stmt;
> +DROP TABLE t1;
>   End of 5.1 tests
>
> === modified file 'mysql-test/t/fulltext.test'
> --- a/mysql-test/t/fulltext.test	2010-03-25 12:08:21 +0000
> +++ b/mysql-test/t/fulltext.test	2010-09-16 11:24:57 +0000
> @@ -585,4 +585,36 @@ REPAIR TABLE t1;
>   SETmyisam_sort_buffer_size=@@global.myisam_sort_buffer_size;
>   DROP TABLE t1;
>
> +--echo #
> +--echo # Bug#54484 explain + prepared statement: crash and Got error -1 from storage engine
> +--echo #
> +
> +CREATE TABLE t1(f1 VARCHAR(6) NOT NULL,
> +FULLTEXT KEY(f1),UNIQUE(f1));
> +INSERT INTO t1 VALUES ('test');
> +
> +PREPARE stmt FROM
> +'SELECT 1 FROM t1 WHERE 1>
> + ALL((SELECT 1 FROM t1 RIGHT OUTER JOIN t1 a
> + ON (MATCH(t1.f1) against (""))
> + WHERE t1.f1 GROUP BY t1.f1)) xor f1 or f1>  1';
Is the test case really simplified as much as possible? If so, do you 
know why a fulltext key is needed? Or XOR?
> +
> +EXECUTE stmt;
> +EXECUTE stmt;
> +
> +DEALLOCATE PREPARE stmt;
> +
> +PREPARE stmt FROM
> +'EXPLAIN SELECT 1 FROM t1
> + WHERE 1>  ALL((SELECT 1 FROM t1 RIGHT OUTER JOIN t1 a
> + ON (MATCH(t1.f1) AGAINST (""))
> + WHERE t1.f1 GROUP BY t1.f1)) XOR f1 or f1>1';
> +
> +EXECUTE stmt;
> +EXECUTE stmt;
> +
> +DEALLOCATE PREPARE stmt;
> +
> +DROP TABLE t1;
> +
>   --echo End of 5.1 tests
>
> === modified file 'sql/mysql_priv.h'
> --- a/sql/mysql_priv.h	2010-07-29 03:00:57 +0000
> +++ b/sql/mysql_priv.h	2010-09-16 11:24:57 +0000
> @@ -2391,7 +2391,7 @@ inline void setup_table_map(TABLE *table
>     table->const_table= 0;
>     table->null_row= 0;
>     table->status= STATUS_NO_RECORD;
> -  table->maybe_null= table_list->outer_join;
> +  table->maybe_null= table_list->orig_outer_join;
>     TABLE_LIST *embedding= table_list->embedding;
>     while (!table->maybe_null&&  embedding)
>     {
>
> === modified file 'sql/sql_parse.cc'
> --- a/sql/sql_parse.cc	2010-08-18 04:56:06 +0000
> +++ b/sql/sql_parse.cc	2010-09-16 11:24:57 +0000
> @@ -6632,7 +6632,7 @@ TABLE_LIST *st_select_lex::convert_right
>
>     join_list->push_front(tab2);
>     join_list->push_front(tab1);
> -  tab1->outer_join|= JOIN_TYPE_RIGHT;
> +  tab1->orig_outer_join= tab1->outer_join|= JOIN_TYPE_RIGHT;
>
>     DBUG_RETURN(tab1);
>   }
>
> === modified file 'sql/sql_yacc.yy'
> --- a/sql/sql_yacc.yy	2010-08-30 22:16:38 +0000
> +++ b/sql/sql_yacc.yy	2010-09-16 11:24:57 +0000
> @@ -8596,7 +8596,7 @@ join_table:
>             {
>               add_join_on($5,$8);
>               Lex->pop_context();
> -            $5->outer_join|=JOIN_TYPE_LEFT;
> +            $5->orig_outer_join= $5->outer_join|=JOIN_TYPE_LEFT;
>               $$=$5;
>               Select->parsing_place= NO_MATTER;
>             }
> @@ -8607,14 +8607,14 @@ join_table:
>             USING '(' using_list ')'
>             {
>               add_join_natural($1,$5,$9,Select);
> -            $5->outer_join|=JOIN_TYPE_LEFT;
> +            $5->orig_outer_join= $5->outer_join|=JOIN_TYPE_LEFT;
>               $$=$5;
>             }
>           | table_ref NATURAL LEFT opt_outer JOIN_SYM table_factor
>             {
>               MYSQL_YYABORT_UNLESS($1&&  $6);
>               add_join_natural($1,$6,NULL,Select);
> -            $6->outer_join|=JOIN_TYPE_LEFT;
> +            $6->orig_outer_join= $6->outer_join|=JOIN_TYPE_LEFT;
>               $$=$6;
>             }
>
>
> === modified file 'sql/table.h'
> --- a/sql/table.h	2010-08-02 07:50:15 +0000
> +++ b/sql/table.h	2010-09-16 11:24:57 +0000
> @@ -1348,7 +1348,7 @@ struct TABLE_LIST
>     /* call back function for asking handler about caching in query cache */
>     qc_engine_callback callback_func;
>     thr_lock_type lock_type;
> -  uint		outer_join;		/* Which join type */
> +  uint		outer_join, orig_outer_join; /* Which join type */
>     uint		shared;			/* Used in multi-upd */
>     size_t        db_length;
>     size_t        table_name_length;
>
>    
> ------------------------------------------------------------------------
>
>


--------------070204030304010100010304--