List:Commits« Previous MessageNext Message »
From:Luís Soares Date:September 28 2010 11:40am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3514)
Bug#46110
View as plain text  
Hi Dao-Gang,

  Thanks for the review. See some comments inline.

On Tue, 2010-09-28 at 18:52 +0800, Daogang Qu wrote:
> Hi Luis,
> Nice work. See comments below.
> 
> STATUS
> ------
> Not Approved!
> 
> REQUIRED CHANGES
> ----------------
> RC1.
> Function error.
> The create/drop/alter database will be replicated by default.
> But the replication of the created/dropped database will be
> prevented, if slave configured with:
> --replicate-wild-do-table=db%.t1
> It's not a right behavior.

This is a well known and documented limitation. What we are doing
while fixing this bug is allowing the user to overcome this
limitation by making --replicate-do-db to take precedence over
--repicate-wild-do-table. Ie, making database level filtering be
evaluated before the table level rules. All in all, this gives
the user the ability to override the side effects of the
wild-do-table filters.

We could allow the user to configure explicitly *database level*
statement filtering using *table level* filters (such as
wild-do-table), but that would be just adding more confusion to
this.

Yes, as it is today, it's confusing that wild-do-table rules
filter out table level statements even if we use a db% as part of
db%.table1, but then, this is documented, has been like that for
ages and users are pretty much aware of this limitation:

http://dev.mysql.com/doc/refman/5.1/en/replication-options-slave.html#option_mysqld_replicate-wild-do-table

I say we don't touch wild-do-table rules behavior with respect to
database level statements when the former are specified alone.

So while discussing this with Geert, I basically, presented two
approaches:

  1. http://lists.mysql.com/commits/118617  -- pretty much this one
  2. http://lists.mysql.com/commits/118483  -- and one that ignores
                                               the table part while
                                               evaluating the wild
                                               filter one.

We opted to go with the first one. See comment:

  [21 Sep 12:18] Geert Vanderkelen

Convinced?

> REQUESTS
> --------
> 
> R1.
> Make the code more readable as following:
> 
> +  bool filtered= (!(rpl_filter->get_do_db()->is_empty() &&
> +                    rpl_filter->get_ignore_db()->is_empty())) ?       /*
> filters exist in ignore/do_db ? */
> +                  rpl_filter->db_ok(db) :                            /* then just
> check them */
> +                  rpl_filter->db_ok_with_wild_table(db);             /* if not,
> then check wild do table */
> +
> +  DBUG_RETURN(filtered);


OK, I'll rewrite like this:


db_ok= (rpl_filter->get_do_db()->is_empty() &&
        rpl_filter->get_ignore_db()->is_empty()) ?
       rpl_filter->db_ok_with_wild_table(db) :
       rpl_filter->db_ok(db));

DBUG_RETURN(db_ok);

Do you agree with such change?

> R2.
> Please add comments for rpl_filter_bug.test.

OK.

Regards,
Luís

> Best Regards,
> 
> Daogang
> 
> 
> 2010-09-20 23:42, Luis Soares wrote:
> > #At file:///home/lsoares/Workspace/bzr/work/bugfixing/46110/mysql-5.1-bugteam/
> based on revid:alfranio.correia@stripped
> >
> >  3514 Luis Soares	2010-09-20
> >       BUG#46110: --replicate-wild-do-table invalidates 
> >                  --replicate-do-db matching rule
> >       
> >         When using --replicate-do-db and --replicate-ignore-db with
> >       --replicate-wild-do-table, some database level statements, eg
> >       DROP DATABASE or CREATE DATABASE, might not be replayed even if
> >       the DATABASE name was inline with the rules set in
> >       --replicate-do-db and/or --replicate-ignore-db. The following
> >       example would not replicate correctly:
> >       
> >         SLAVE, configured with: 
> >         --replicate-do-db=dbx --replicate-wild-do-table=db%.t1
> >       
> >         MASTER, dba issues:
> >         MASTER> CREATE DATABASE dbx;
> >       
> >         Although, at first sight, there is nothing in the filter rules
> >       that might prevent this statement to be replayed, the slave would
> >       not execute it. In fact, "replicate-wild-do-table=db%.t1" 
> >       invalidates the "OK" from "replicate-do-db=dbx", making the event
> >       to be skipped/filtered out.
> >       
> >         We fix this, by making --replicate-do/ignore-db rules take
> >       precedence over replicate-wild-do-table for *database* level
> >       statements only:
> >       
> >         - CREATE/DROP/ALTER DATABASE ...
> >       
> >         As such, when replicating such statements, we only look into
> >       replicate-wild-do-table if and only if replicate-do/ignore-db 
> >       rules are void. Otherwise, --replicate-do/ignore-db rules 
> >       dictate the fate of the statement: either filtered out or 
> >       executed.
> >
> >     added:
> >       mysql-test/suite/rpl/r/rpl_filter_bug.result
> >       mysql-test/suite/rpl/t/rpl_filter_bug-slave.opt
> >       mysql-test/suite/rpl/t/rpl_filter_bug.test
> >     modified:
> >       sql/sql_parse.cc
> > === added file 'mysql-test/suite/rpl/r/rpl_filter_bug.result'
> > --- a/mysql-test/suite/rpl/r/rpl_filter_bug.result	1970-01-01 00:00:00 +0000
> > +++ b/mysql-test/suite/rpl/r/rpl_filter_bug.result	2010-09-20 15:42:12 +0000
> > @@ -0,0 +1,6 @@
> > +stop slave;
> > +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> > +reset master;
> > +reset slave;
> > +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> > +start slave;
> >
> > === added file 'mysql-test/suite/rpl/t/rpl_filter_bug-slave.opt'
> > --- a/mysql-test/suite/rpl/t/rpl_filter_bug-slave.opt	1970-01-01 00:00:00 +0000
> > +++ b/mysql-test/suite/rpl/t/rpl_filter_bug-slave.opt	2010-09-20 15:42:12 +0000
> > @@ -0,0 +1 @@
> > +--replicate-do-db=dbx --replicate-wild-do-table=db%.t1
> >
> > === added file 'mysql-test/suite/rpl/t/rpl_filter_bug.test'
> > --- a/mysql-test/suite/rpl/t/rpl_filter_bug.test	1970-01-01 00:00:00 +0000
> > +++ b/mysql-test/suite/rpl/t/rpl_filter_bug.test	2010-09-20 15:42:12 +0000
> > @@ -0,0 +1,76 @@
> > +#
> > +# BUG#46110: --replicate-wild-do-table invalidates --replicate-do-db matching
> rule
> > +#
> > +
> > +-- source include/master-slave.inc
> > +
> > +## Checking CREATE DATABASE
> > +
> > +-- disable_query_log
> > +CREATE DATABASE dbx DEFAULT CHARACTER SET=latin1;
> > +-- enable_query_log
> > +
> > +if (!`SELECT count(*) = 1 FROM information_schema.SCHEMATA WHERE schema_name =
> 'dbx'`)
> > +{
> > +  -- echo unexpected default character set for database: dbx
> > +  -- source include/show_rpl_debug_info.inc
> > +  -- die
> > +}
> > +
> > +-- sync_slave_with_master
> > +
> > +if (!`SELECT count(*) = 1 FROM information_schema.SCHEMATA WHERE schema_name =
> 'dbx'`)
> > +{
> > +  -- echo unexpected default character set for database: dbx
> > +  -- source include/show_rpl_debug_info.inc
> > +  -- die
> > +}
> > +
> > +## Checking ALTER DATABASE
> > +
> > +-- connection master
> > +
> > +-- disable_query_log
> > +ALTER DATABASE dbx DEFAULT CHARACTER SET=latin5;
> > +-- enable_query_log
> > +
> > +if (!`SELECT DEFAULT_CHARACTER_SET_NAME='latin5' FROM
> information_schema.SCHEMATA WHERE schema_name = 'dbx'`)
> > +{
> > +  -- echo unexpected default character set for database: dbx
> > +  -- source include/show_rpl_debug_info.inc
> > +  -- die
> > +}
> > +
> > +-- sync_slave_with_master
> > +
> > +if (!`SELECT DEFAULT_CHARACTER_SET_NAME='latin5' FROM
> information_schema.SCHEMATA WHERE schema_name = 'dbx'`)
> > +{
> > +  -- echo unexpected default character set for database: dbx
> > +  -- source include/show_rpl_debug_info.inc
> > +  -- die
> > +}
> > +
> > +## Checking DROP DATABASE
> > +
> > +-- connection master
> > +-- disable_query_log
> > +DROP DATABASE dbx;
> > +-- enable_query_log
> > +
> > +if (!`SELECT count(*) = 0 FROM information_schema.SCHEMATA WHERE schema_name =
> 'dbx'`)
> > +{
> > +  -- echo unexpected default character set for database: dbx
> > +  -- source include/show_rpl_debug_info.inc
> > +  -- die
> > +}
> > +
> > +-- sync_slave_with_master
> > +
> > +if (!`SELECT count(*) = 0 FROM information_schema.SCHEMATA WHERE schema_name =
> 'dbx'`)
> > +{
> > +  -- echo unexpected default character set for database: dbx
> > +  -- source include/show_rpl_debug_info.inc
> > +  -- die
> > +}
> > +
> > +-- source include/master-slave-end.inc
> >
> > === modified file 'sql/sql_parse.cc'
> > --- a/sql/sql_parse.cc	2010-08-18 04:56:06 +0000
> > +++ b/sql/sql_parse.cc	2010-09-20 15:42:12 +0000
> > @@ -217,6 +217,34 @@ inline bool all_tables_not_ok(THD *thd, 
> >    return rpl_filter->is_on() && tables && !thd->spcont
> &&
> >           !rpl_filter->tables_ok(thd->db, tables);
> >  }
> > +
> > +inline bool db_stmt_db_ok(THD *thd, char* db)
> > +{
> > +  DBUG_ENTER("db_stmt_db_ok");
> > +
> > +  if (!thd->slave_thread)
> > +    DBUG_RETURN(TRUE);
> > +
> > +  /*
> > +
> > +    Checking whether the event for the given database, db, should
> > +    be ignored or not, is done by checking whether there are
> > +    active rules in ignore_db or in do_db containers. If there
> > +    are, then check if there is a match, if not then check the
> > +    wild_do rules.
> > +      
> > +    This means that when using this function replicate-do-db and 
> > +    replicate-ignore-db take precedence over wild do rules.
> > +
> > +   */
> > +
> > +  bool filtered= (!(rpl_filter->get_do_db()->is_empty() &&
> > +                    rpl_filter->get_ignore_db()->is_empty())) ?       /*
> filters exist in ignore/do_db ? */
> > +                  !rpl_filter->db_ok(db) :                            /*
> then just check them */
> > +                  !rpl_filter->db_ok_with_wild_table(db);             /* if
> not, then check wild do table */
> > +
> > +  DBUG_RETURN(!filtered);
> > +}
> >  #endif
> >  
> >  
> > @@ -3646,9 +3674,7 @@ end_with_restore_list:
> >        above was not called. So we have to check rules again here.
> >      */
> >  #ifdef HAVE_REPLICATION
> > -    if (thd->slave_thread && 
> > -	(!rpl_filter->db_ok(lex->name.str) ||
> > -	 !rpl_filter->db_ok_with_wild_table(lex->name.str)))
> > +    if (!db_stmt_db_ok(thd, lex->name.str))
> >      {
> >        my_message(ER_SLAVE_IGNORED_TABLE, ER(ER_SLAVE_IGNORED_TABLE), MYF(0));
> >        break;
> > @@ -3681,9 +3707,7 @@ end_with_restore_list:
> >        above was not called. So we have to check rules again here.
> >      */
> >  #ifdef HAVE_REPLICATION
> > -    if (thd->slave_thread && 
> > -	(!rpl_filter->db_ok(lex->name.str) ||
> > -	 !rpl_filter->db_ok_with_wild_table(lex->name.str)))
> > +    if (!db_stmt_db_ok(thd, lex->name.str))
> >      {
> >        my_message(ER_SLAVE_IGNORED_TABLE, ER(ER_SLAVE_IGNORED_TABLE), MYF(0));
> >        break;
> > @@ -3710,9 +3734,7 @@ end_with_restore_list:
> >        break;
> >      }
> >  #ifdef HAVE_REPLICATION
> > -    if (thd->slave_thread && 
> > -       (!rpl_filter->db_ok(db->str) ||
> > -        !rpl_filter->db_ok_with_wild_table(db->str)))
> > +    if (!db_stmt_db_ok(thd, lex->name.str))
> >      {
> >        res= 1;
> >        my_message(ER_SLAVE_IGNORED_TABLE, ER(ER_SLAVE_IGNORED_TABLE), MYF(0));
> > @@ -3764,9 +3786,7 @@ end_with_restore_list:
> >        above was not called. So we have to check rules again here.
> >      */
> >  #ifdef HAVE_REPLICATION
> > -    if (thd->slave_thread &&
> > -	(!rpl_filter->db_ok(db->str) ||
> > -	 !rpl_filter->db_ok_with_wild_table(db->str)))
> > +    if (!db_stmt_db_ok(thd, lex->name.str))
> >      {
> >        my_message(ER_SLAVE_IGNORED_TABLE, ER(ER_SLAVE_IGNORED_TABLE), MYF(0));
> >        break;
> >
> >   
> >
> >
> >
> >   
> 

-- 
Luís

Thread
bzr commit into mysql-5.1-bugteam branch (luis.soares:3514) Bug#46110Luis Soares20 Sep
  • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3514) Bug#46110Alfranio Correia25 Sep
    • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3514) Bug#46110Alfranio Correia25 Sep
    • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3514)Bug#46110Luís Soares27 Sep
      • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3514) Bug#46110Alfranio Correia27 Sep
        • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3514)Bug#46110Luís Soares6 Oct
  • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3514)Bug#46110Daogang Qu28 Sep
    • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3514)Bug#46110Luís Soares28 Sep
      • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3514)Bug#46110Daogang Qu29 Sep
    • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3514)Bug#46110Daogang Qu28 Sep