Hi Sergey,
Thanks for your comments.
On Mon, 2010-11-29 at 17:15 +0300, Sergey Vojtovich wrote:
> 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.
So the description should be:
"After this patch, any statement which calls MATCH...AGAINST to
full-text search data is marked unsafe unless it is calling mysql
default fulltext plugin."
>
>
> ...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?
The test sources have_simple_parser.inc, so the whole test will be
skipped if mypluglib.so is not available.
> ...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."
OK.
>
> > 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.
>
Good point.
> 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?
binlog_query() calls issue_unsafe_warnings(). issue_unsafe_warnings()
checks this flag and then generates different unsafe warnings.
>
> ...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.
I will remove the comma.
>
> ...skip...
>
> Regards,
> Sergey
--
Your Sincerely,
Libing Song
==================================
MySQL Replication Team
Software Engineer
Email : Li-Bing.Song@stripped
Skype : libing.song
MSN : slb_database@stripped
Phone : +86 010-6505-4020 ext. 319
Mobile: +86 138-1144-2038
==================================
