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
