Hi Tatiana
This patch is rejected,
see below for comments.
Regards,
-- Marc
Tatiana A. Nurnberg wrote:
> #At file:///misc/mysql/forest/43746/51-43746/ based on
> revid:azundris@stripped
>
> 2932 Tatiana A. Nurnberg 2009-06-24
> Bug#43746: YACC return wrong query string when parse 'load data infile' sql
> statement
>
> LOAD DATA-handling relied on literal command-string (rather than particles)
> too much, and subsequently got confused when implementation details (such
> as handling of SQL_MODE="IGNORE_SPACES") behaved in original ways.
>
> No longer affected by IGNORE_SPACES.
> Also handles "creative" use of versioned code in LOAD DATA correctly.
> @ mysql-test/suite/rpl/r/rpl_loaddatalocal.result
> Extend test to show that IGNORE_SPACES or creative comments
> do not marr our handling of LOAD DATA LOCAL ...
> @ mysql-test/suite/rpl/r/rpl_stm_log.result
> update results
> @ mysql-test/suite/rpl/t/rpl_loaddatalocal.test
> Extend test to show that IGNORE_SPACES or creative comments
> do not marr our handling of LOAD DATA LOCAL ...
> @ sql/sql_lex.cc
> Two wrongs don't make a right, but three lefts do.
I don't see this as a proper change log comment.
> @ sql/sql_lex.h
> We need to be able to call consume_comment() from sql_yacc.yy, alas.
No, this is a serious flaw.
> @ sql/sql_yacc.yy
> Use tok_start() instead of ptr() where possible.
> Handle the -- somewhat esoteric -- case of someone putting "DATA" or "INTO"
> of "LOAD DATA LOCAL INFILE INTO" into versioned code-comments correctly.
> You call it dumb, we call it orthogonal.
I call it unacceptable for a change set comment.
>
> modified:
> mysql-test/suite/rpl/r/rpl_loaddatalocal.result
> mysql-test/suite/rpl/r/rpl_stm_log.result
> mysql-test/suite/rpl/t/rpl_loaddatalocal.test
> sql/log_event.cc
> sql/sql_lex.cc
> sql/sql_lex.h
> sql/sql_yacc.yy
> === modified file 'mysql-test/suite/rpl/r/rpl_loaddatalocal.result'
> --- a/mysql-test/suite/rpl/r/rpl_loaddatalocal.result 2009-03-16 08:21:29 +0000
> +++ b/mysql-test/suite/rpl/r/rpl_loaddatalocal.result 2009-06-24 09:06:27 +0000
> @@ -54,3 +54,29 @@ a
> [on master]
> DROP TABLE t1;
> [on slave]
> +
> +Bug #43746:
> +"return wrong query string when parse 'load data infile' sql statement"
> +
> +[master]
> +SELECT @@SESSION.sql_mode INTO @old_mode;
> +SET sql_mode='ignore_space';
> +CREATE TABLE t1(a int);
> +INSERT INTO t1 VALUES (1), (2), (3), (4);
> +SELECT * INTO OUTFILE 'MYSQLD_DATADIR/bug43746.sql' FROM t1;
> +TRUNCATE TABLE t1;
> +LOAD DATA LOCAL INFILE 'MYSQLD_DATADIR/bug43746.sql' INTO TABLE t1;
> +LOAD/* look mum, with comments in weird places! */DATA/* oh hai */LOCAL INFILE
> 'MYSQLD_DATADIR/bug43746.sql'/* we are */INTO/* from the internets */TABLE t1;
> +LOAD DATA/*!10000 LOCAL */INFILE 'MYSQLD_DATADIR/bug43746.sql' INTO TABLE t1;
> +LOAD DATA LOCAL INFILE 'MYSQLD_DATADIR/bug43746.sql' /*!10000 INTO */ TABLE t1;
> +LOAD DATA/*!10000 LOCAL */INFILE 'MYSQLD_DATADIR/bug43746.sql'/*!10000 INTO*/TABLE
> t1;
> +LOAD DATA/*!10000 LOCAL */INFILE 'MYSQLD_DATADIR/bug43746.sql'/* empty */INTO TABLE
> t1;
> +LOAD DATA/*!10000 LOCAL */INFILE 'MYSQLD_DATADIR/bug43746.sql' INTO/* empty */TABLE
> t1;
> +LOAD/*!99999 special comments that do not expand */DATA/*!99999 code from the future
> */LOCAL INFILE 'MYSQLD_DATADIR/bug43746.sql'/*!99999 have flux capacitor */INTO/*!99999
> will travel */TABLE t1;
> +SET
> sql_mode='PIPES_AS_CONCAT,ANSI_QUOTES,NO_KEY_OPTIONS,NO_TABLE_OPTIONS,NO_FIELD_OPTIONS,STRICT_TRANS_TABLES,STRICT_ALL_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,TRADITIONAL,NO_AUTO_CREATE_USER';
> +LOAD DATA LOCAL INFILE 'MYSQLD_DATADIR/bug43746.sql' INTO TABLE t1;
> +[slave]
> +[master]
> +DROP TABLE t1;
> +SET SESSION sql_mode=@old_mode;
> +[slave]
>
> === modified file 'mysql-test/suite/rpl/r/rpl_stm_log.result'
> --- a/mysql-test/suite/rpl/r/rpl_stm_log.result 2009-06-08 19:01:36 +0000
> +++ b/mysql-test/suite/rpl/r/rpl_stm_log.result 2009-06-24 09:06:27 +0000
> @@ -218,7 +218,7 @@ slave-bin.000001 # Query 1 # use `test`;
> slave-bin.000001 # Query 1 # use `test`; drop table t1
> slave-bin.000001 # Query 1 # use `test`; create table t1 (word char(20) not
> null)ENGINE=MyISAM
> slave-bin.000001 # Begin_load_query 1 # ;file_id=1;block_len=581
> -slave-bin.000001 # Execute_load_query 1 # use `test`; load data INFILE
> '../../tmp/SQL_LOAD-2-1-1.data' INTO table t1 ignore 1 lines ;file_id=1
> +slave-bin.000001 # Execute_load_query 1 # use `test`; load data INFILE
> '../../tmp/SQL_LOAD-2-1-1.data' INTO table t1 ignore 1 lines ;file_id=1
> slave-bin.000001 # Query 1 # use `test`; create table t3 (a int)ENGINE=MyISAM
> slave-bin.000001 # Rotate 2 # slave-bin.000002;pos=4
> show binlog events in 'slave-bin.000002' from 4;
>
> === modified file 'mysql-test/suite/rpl/t/rpl_loaddatalocal.test'
> --- a/mysql-test/suite/rpl/t/rpl_loaddatalocal.test 2009-03-16 08:21:29 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_loaddatalocal.test 2009-06-24 09:06:27 +0000
> @@ -98,3 +98,67 @@ DROP TABLE t1;
> --echo [on slave]
> sync_slave_with_master;
>
> +--echo
> +--echo Bug #43746:
> +--echo "return wrong query string when parse 'load data infile' sql statement"
> +--echo
> +
> +--echo [master]
> +connection master;
> +let $MYSQLD_DATADIR= `select @@datadir`;
> +SELECT @@SESSION.sql_mode INTO @old_mode;
> +
> +SET sql_mode='ignore_space';
> +
> +CREATE TABLE t1(a int);
> +INSERT INTO t1 VALUES (1), (2), (3), (4);
> +
> +--replace_result $MYSQLD_DATADIR MYSQLD_DATADIR
> +eval SELECT * INTO OUTFILE '$MYSQLD_DATADIR/bug43746.sql' FROM t1;
> +TRUNCATE TABLE t1;
> +
> +--replace_result $MYSQLD_DATADIR MYSQLD_DATADIR
> +eval LOAD DATA LOCAL INFILE '$MYSQLD_DATADIR/bug43746.sql' INTO TABLE t1;
> +
> +--replace_result $MYSQLD_DATADIR MYSQLD_DATADIR
> +eval LOAD/* look mum, with comments in weird places! */DATA/* oh hai */LOCAL INFILE
> '$MYSQLD_DATADIR/bug43746.sql'/* we are */INTO/* from the internets */TABLE t1;
> +
> +--replace_result $MYSQLD_DATADIR MYSQLD_DATADIR
> +eval LOAD DATA/*!10000 LOCAL */INFILE '$MYSQLD_DATADIR/bug43746.sql' INTO TABLE t1;
> +
> +--replace_result $MYSQLD_DATADIR MYSQLD_DATADIR
> +eval LOAD DATA LOCAL INFILE '$MYSQLD_DATADIR/bug43746.sql' /*!10000 INTO */ TABLE
> t1;
> +
> +--replace_result $MYSQLD_DATADIR MYSQLD_DATADIR
> +eval LOAD DATA/*!10000 LOCAL */INFILE '$MYSQLD_DATADIR/bug43746.sql'/*!10000
> INTO*/TABLE t1;
> +
> +--replace_result $MYSQLD_DATADIR MYSQLD_DATADIR
> +eval LOAD DATA/*!10000 LOCAL */INFILE '$MYSQLD_DATADIR/bug43746.sql'/* empty */INTO
> TABLE t1;
> +
> +--replace_result $MYSQLD_DATADIR MYSQLD_DATADIR
> +eval LOAD DATA/*!10000 LOCAL */INFILE '$MYSQLD_DATADIR/bug43746.sql' INTO/* empty
> */TABLE t1;
> +
> +--replace_result $MYSQLD_DATADIR MYSQLD_DATADIR
> +eval LOAD/*!99999 special comments that do not expand */DATA/*!99999 code from the
> future */LOCAL INFILE '$MYSQLD_DATADIR/bug43746.sql'/*!99999 have flux capacitor
> */INTO/*!99999 will travel */TABLE t1;
> +
> +SET
> sql_mode='PIPES_AS_CONCAT,ANSI_QUOTES,NO_KEY_OPTIONS,NO_TABLE_OPTIONS,NO_FIELD_OPTIONS,STRICT_TRANS_TABLES,STRICT_ALL_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,TRADITIONAL,NO_AUTO_CREATE_USER';
> +
> +--replace_result $MYSQLD_DATADIR MYSQLD_DATADIR
> +eval LOAD DATA LOCAL INFILE '$MYSQLD_DATADIR/bug43746.sql' INTO TABLE t1;
> +
> +--echo [slave]
> +sync_slave_with_master;
> +
> +# cleanup
> +
> +--remove_file $MYSQLD_DATADIR/bug43746.sql
> +
> +--echo [master]
> +connection master;
> +DROP TABLE t1;
> +SET SESSION sql_mode=@old_mode;
> +
> +--echo [slave]
> +sync_slave_with_master;
> +
> +connection master;
>
> === modified file 'sql/log_event.cc'
> --- a/sql/log_event.cc 2009-06-08 19:01:36 +0000
> +++ b/sql/log_event.cc 2009-06-24 09:06:27 +0000
> @@ -6764,7 +6764,7 @@ Execute_load_query_log_event::do_apply_e
> /* Ordinary load data */
> break;
> }
> - p= strmake(p, STRING_WITH_LEN(" INTO"));
> + p= strmake(p, STRING_WITH_LEN(" INTO "));
> p= strmake(p, query+fn_pos_end, q_len-fn_pos_end);
>
> error= Query_log_event::do_apply_event(rli, buf, p-buf);
>
> === modified file 'sql/sql_lex.cc'
> --- a/sql/sql_lex.cc 2009-06-08 19:01:36 +0000
> +++ b/sql/sql_lex.cc 2009-06-24 09:06:27 +0000
> @@ -939,16 +939,7 @@ int MYSQLlex(void *arg, void *yythd)
> If we find a space then this can't be an identifier. We notice this
> below by checking start != lex->ptr.
> */
> -
> - uchar prvs_c;
> -
> - for (prvs_c= c; state_map[c] == MY_LEX_SKIP ; c= lip->yyGet())
> - prvs_c= c;
> - if ((prvs_c == ' ') && (c != '('))
> - {
> - lip->yyUnget();
> - c= prvs_c;
> - }
> + for (; state_map[c] == MY_LEX_SKIP ; c= lip->yyGet());
> }
> if (start == lip->get_ptr() && c == '.' &&
> ident_map[lip->yyPeek()])
> lip->next_state=MY_LEX_IDENT_SEP;
>
I can not find code using the prvs_c variable in sql_lex.cc,
so that at this point I don't even have confidence about what was and
what was not changed.
I looked in:
- mysql-5.1
- mysql-5.1-bugteam
as of 2009-07-07
When posting the next patch, please clarify what code base the patch
will be based on.
> === modified file 'sql/sql_lex.h'
> --- a/sql/sql_lex.h 2009-05-27 15:19:44 +0000
> +++ b/sql/sql_lex.h 2009-06-24 09:06:27 +0000
> @@ -1969,6 +1969,7 @@ extern void lex_end(LEX *lex);
> extern int MYSQLlex(void *arg, void *yythd);
>
> extern void trim_whitespace(CHARSET_INFO *cs, LEX_STRING *str);
> +extern bool consume_comment(Lex_input_stream *lip, int
> remaining_recursions_permitted);
No, this should not be exposed.
>
> extern bool is_lex_native_function(const LEX_STRING *name);
>
>
> === modified file 'sql/sql_yacc.yy'
> --- a/sql/sql_yacc.yy 2009-06-01 12:43:16 +0000
> +++ b/sql/sql_yacc.yy 2009-06-24 09:06:27 +0000
> @@ -10411,7 +10411,14 @@ load:
> my_error(ER_SP_BADSTATEMENT, MYF(0), "LOAD DATA");
> MYSQL_YYABORT;
> }
> - lex->fname_start= lip->get_ptr();
> + if (lip->in_comment != NO_COMMENT)
> + {
> + consume_comment(lip, 1);
> + lex->fname_start= lip->get_ptr();
> + lip->in_comment= NO_COMMENT;
> + }
> + else
> + lex->fname_start= lip->get_tok_start() + lip->yyLength() +
> 1;
Calling lexer code like consume_comment() to skip comments / whitespace
from the parser is by definition broken.
If the need is to get the query text, with comments pre processed, then
please use the get_cpp_ptr() APIs and friends, instead of get_ptr().
> }
> load_data
> {}
> @@ -10445,7 +10452,15 @@ load_data:
> }
> opt_duplicate INTO
> {
> - Lex->fname_end= YYLIP->get_ptr();
> + Lex_input_stream *lip= YYLIP;
> + if (lip->in_comment != NO_COMMENT)
> + {
> + consume_comment(lip, 1);
> + Lex->fname_end= lip->get_ptr();
> + lip->in_comment= NO_COMMENT;
> + }
> + else
> + Lex->fname_end= lip->get_tok_start() + lip->yyLength() + 1;
Likewise
> }
> TABLE_SYM table_ident
> {
>
>
>
> ------------------------------------------------------------------------
>
>
