List:Commits« Previous MessageNext Message »
From:Sergey Vojtovich Date:November 29 2010 2:15pm
Subject:Re: bzr commit into mysql-trunk branch (Li-Bing.Song:3350)
Bug#48183
View as plain text  
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
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