List:Commits« Previous MessageNext Message »
From:Libing Song Date:December 1 2010 7:29am
Subject:Re: bzr commit into mysql-trunk branch (Li-Bing.Song:3350) Bug#48183
View as plain text  
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
==================================

Thread
bzr commit into mysql-trunk branch (Li-Bing.Song:3350) Bug#48183Li-Bing.Song23 Nov
  • Re: bzr commit into mysql-trunk branch (Li-Bing.Song:3350) Bug#48183He Zhenxing29 Nov
  • Re: bzr commit into mysql-trunk branch (Li-Bing.Song:3350)Bug#48183Sergey Vojtovich29 Nov
    • Re: bzr commit into mysql-trunk branch (Li-Bing.Song:3350) Bug#48183Libing Song1 Dec
      • Re: bzr commit into mysql-trunk branch (Li-Bing.Song:3350)Bug#48183Sergey Vojtovich1 Dec
        • Re: bzr commit into mysql-trunk branch (Li-Bing.Song:3350) Bug#48183Libing Song2 Dec
          • Re: bzr commit into mysql-trunk branch (Li-Bing.Song:3350)Bug#48183Sergey Vojtovich2 Dec
            • Re: bzr commit into mysql-trunk branch (Li-Bing.Song:3350) Bug#48183Alfranio Correia2 Dec
              • Re: bzr commit into mysql-trunk branch (Li-Bing.Song:3350)Bug#48183Sergey Vojtovich2 Dec
            • Re: bzr commit into mysql-trunk branch (Li-Bing.Song:3350) Bug#48183Libing Song3 Dec