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;
>
>
> ------------------------------------------------------------------------
>
>
