Hi Li-Bing,
a few comments inline.
On Tue, Nov 23, 2010 at 09:19:49AM +0000, Li-Bing.Song@stripped wrote:
> #At file:///home/anders/Work/bzrwork/wt4/mysql-trunk-bugfixing/ based on
> revid:dao-gang.qu@stripped
>
> 3350 Li-Bing.Song@stripped 2010-11-23
> Bug#48183 statements using fulltext parser plugins should be marked unsafe
>
> Any use of plugins that could affect rows in a table should be marked unsafe
> because it cannot be determined that the statement will have the same effect
> on the slave. For example, UDFs are currently marked unsafe. However, use of
> fulltext parser plugins is currently not marked as unsafe.
>
> After this patch, any statement which uses fulltext parser plugins is marked
> unsafe.
Strictly speaking that's not true. Because parser plugins are used internally
by MyISAM even if there is no MATCH clause. You're marking MATCH with parser
plugin, not any statement.
...skip...
> === modified file 'mysql-test/suite/binlog/r/binlog_stm_unsafe_warning.result'
> --- a/mysql-test/suite/binlog/r/binlog_stm_unsafe_warning.result 2010-08-10 11:58:46
> +0000
> +++ b/mysql-test/suite/binlog/r/binlog_stm_unsafe_warning.result 2010-11-23 09:19:35
> +0000
> @@ -94,3 +94,21 @@ Note 1592 Unsafe statement written to th
> DROP FUNCTION sf_bug50192;
> DROP TRIGGER tr_bug50192;
> DROP TABLE t1, t2;
> +
> +# BUG#48183 statements using fulltext parser plugins should be marked unsafe
> +INSTALL PLUGIN simple_parser SONAME 'mypluglib.so';
> +
> +CREATE TABLE t1 (a TEXT, FULLTEXT(a) WITH PARSER simple_parser);
> +CREATE TABLE t2 (a TEXT, FULLTEXT(a));
> +CREATE TABLE t3 (a TEXT);
> +
> +# It should generate an unsafe warning
> +INSERT INTO t3 SELECT * FROM t1 WHERE MATCH (a) AGAINST ('test');
> +Warnings:
> +Note 1592 Unsafe statement written to the binary log using statement format since
> BINLOG_FORMAT = STATEMENT. Statement is unsafe because it uses a plugin which may not
> return the same value on the slave.
> +
> +# It should not generate an unsafe warning
> +INSERT INTO t3 SELECT * FROM t2 WHERE MATCH (a) AGAINST ('test');
> +
> +DROP TABLE t1, t2, t3;
> +UNINSTALL PLUGIN simple_parser;
What if simple_parser is not available?
Did you try to remove mypluglib.so and run this test?
...skip...
> === modified file 'sql/sql_base.cc'
> --- a/sql/sql_base.cc 2010-10-25 12:13:13 +0000
> +++ b/sql/sql_base.cc 2010-11-23 09:19:35 +0000
> @@ -8791,7 +8791,7 @@ int setup_ftfuncs(SELECT_LEX *select_lex
> List_iterator<Item_func_match> li(*(select_lex->ftfunc_list)),
> lj(*(select_lex->ftfunc_list));
> Item_func_match *ftf, *ftf2;
> -
> + THD *thd= current_thd;
> while ((ftf=li++))
> {
> if (ftf->fix_index())
> @@ -8802,6 +8802,12 @@ int setup_ftfuncs(SELECT_LEX *select_lex
> if (ftf->eq(ftf2,1) && !ftf2->master)
> ftf2->master=ftf;
> }
> +
> + /* Just like UDF, we don't know if plugin's funtions will output same value
> + on both master and slave, So the statement is marked unsafe.
> + */
According to coding style: "When writing multi-line comments please put the
'/*' and '*/' on their own lines, put the '*/' below the '/*', put a line
break and a two-space indent after the '/*', do not use additional asterisks
on the left of the comment."
> i if (ftf->table->key_info[ftf->key].parser)
> + thd->set_binlog_unsafe_warning_flags(LEX::BINLOG_STMT_UNSAFE_PLUGIN);
> }
>
> return 0;
>
Probably more proper way to do it is to check flags:
key_info.flags & HA_USES_PARSER. But it doesn't seem to matter
these days - key_info.parser is indeed initialized to NULL.
Probably it is a good idea to use ftf->table->in_use instead of
current_thd.
Also, being non-replication guy, I'm a bit confused: you set unsafe
warning flag, but you don't really mark statement unsafe? If it is
done implicitely, could you point me to that code?
...skip...
> === modified file 'sql/sql_lex.cc'
> --- a/sql/sql_lex.cc 2010-11-10 11:26:45 +0000
> +++ b/sql/sql_lex.cc 2010-11-23 09:19:35 +0000
> @@ -58,7 +58,8 @@ Query_tables_list::binlog_stmt_unsafe_er
> ER_BINLOG_UNSAFE_SYSTEM_FUNCTION,
> ER_BINLOG_UNSAFE_NONTRANS_AFTER_TRANS,
> ER_BINLOG_UNSAFE_MULTIPLE_ENGINES_AND_SELF_LOGGING_ENGINE,
> - ER_BINLOG_UNSAFE_MIXED_STATEMENT
> + ER_BINLOG_UNSAFE_MIXED_STATEMENT,
> + ER_BINLOG_UNSAFE_PLUGIN,
> };
Are you sure this form is portable? I mean ending comma.
...skip...
Regards,
Sergey
--
Sergey Vojtovich <svoj@stripped>
MySQL AB, Software Engineer
Izhevsk, Russia, www.mysql.com
