MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:He Zhenxing Date:June 5 2009 11:16am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:2874)
Bug#42941
View as plain text  
Hi Luis,

Nice work, patch approved, only some minor comments.

Luis Soares wrote:
> #At file:///home/lsoares/Workspace/mysql-server/bugfix/b42941/5.1-bt/ based on
> revid:mats@stripped
> 
>  2874 Luis Soares	2009-06-05
>       BUG#42941: --database paramater to mysqlbinlog fails with RBR

type: parameter

>                   
>       mysqlbinlog --database parameter was being ignored when processing
>       row events. As such no event filtering would take place.
>                   
>       This patch addresses this by deploying a call to shall_skip_database
>       when table_map_events are handled (as these contain also the name of
>       the database). All other rows events referencing the table id for the
>       filtered map event, will also be skipped.
>      @ client/mysqlbinlog.cc
>         Added shall_skip_database call to the part of the code that handles 
>         Table_map_log_events. It inspects the database name and decides whether
>         to filter the event or not. Furthermore, if table map event is filtered
>         next events referencing the table id in the table map event, will also
>         be filtered.
>      @ mysql-test/suite/binlog/t/binlog_row_mysqlbinlog_db_filter.test
>         Test case that checks if row events are actually filtered out.
>      @ sql/log_event.h
>         Added a map for holding the currently ignored table map events.
>         Table map events are inserted when they shall be skipped and removed
>         once the last row event in the statement is processed.
> 
>     added:
>       mysql-test/suite/binlog/r/binlog_row_mysqlbinlog_db_filter.result
>       mysql-test/suite/binlog/t/binlog_row_mysqlbinlog_db_filter.test
>     modified:
>       client/mysqlbinlog.cc
>       sql/log_event.h
> === modified file 'client/mysqlbinlog.cc'
> --- a/client/mysqlbinlog.cc	2009-05-08 17:24:15 +0000
> +++ b/client/mysqlbinlog.cc	2009-06-05 07:49:39 +0000
> @@ -681,6 +681,7 @@ Exit_status process_event(PRINT_EVENT_IN
>  {
>    char ll_buff[21];
>    Log_event_type ev_type= ev->get_type_code();
> +  my_bool destroy_evt= TRUE;
>    DBUG_ENTER("process_event");
>    print_event_info->short_form= short_form;
>    Exit_status retval= OK_CONTINUE;
> @@ -871,12 +872,61 @@ Exit_status process_event(PRINT_EVENT_IN
>        break;
>      }
>      case TABLE_MAP_EVENT:
> +    {
> +      Table_map_log_event *map= ((Table_map_log_event *)ev);
> +      if (shall_skip_database(map->get_db_name()))
> +      {
> +        print_event_info->m_table_map_ignored.set_table(map->get_table_id(),
> map);
> +        destroy_evt= FALSE;
> +        goto end;
> +      }
> +    }
>      case WRITE_ROWS_EVENT:
>      case DELETE_ROWS_EVENT:
>      case UPDATE_ROWS_EVENT:
>      case PRE_GA_WRITE_ROWS_EVENT:
>      case PRE_GA_DELETE_ROWS_EVENT:
>      case PRE_GA_UPDATE_ROWS_EVENT:
> +    {
> +      if (ev_type != TABLE_MAP_EVENT)
> +      {
> +        Rows_log_event *e= (Rows_log_event*) ev;
> +        Table_map_log_event *ignored_map= 
> +          print_event_info->m_table_map_ignored.get_table(e->get_table_id());
> +        bool skip_event= (ignored_map != NULL);
> +

only a suggestion :) I think you can remove the ignored_map variable,
it's not used anywhere else.

> +        /* 
> +           end of statement check:
> +             i) destroy/free ignored maps
> +            ii) if skip event, flush cache now
> +         */
> +        if (e->get_flags(Rows_log_event::STMT_END_F))
> +        {
> +          /* 
> +            now is safe to destroy the original ignored map events
> +            for this statement. 
> +          */
> +          if (print_event_info->m_table_map_ignored.count() > 0)
> +            print_event_info->m_table_map_ignored.clear_tables();
> +
> +          /* 
> +             one needs to take into account event that gets filtered
> +             but was last event in the statement. Cache still needs to
> +             be written to file. (Especially needed on multi table updates
> +             from different databases).
> +          */
> +          if (skip_event)
> +          {
> +            if
> ((copy_event_cache_to_file_and_reinit(&print_event_info->head_cache, result_file)
> ||
> +               
> copy_event_cache_to_file_and_reinit(&print_event_info->body_cache, result_file)))
> +              goto err;
> +          }
> +        }
> +
> +        /* skip the event check */
> +        if (skip_event)
> +          goto end;
> +      }
>        /*
>          These events must be printed in base64 format, if printed.
>          base64 format requires a FD event to be safe, so if no FD
> @@ -900,6 +950,7 @@ Exit_status process_event(PRINT_EVENT_IN
>          goto err;
>        }
>        /* FALL THROUGH */
> +    }
>      default:
>        ev->print(result_file, print_event_info);
>      }
> @@ -919,7 +970,8 @@ end:
>    {
>      if (remote_opt)
>        ev->temp_buf= 0;
> -    delete ev;
> +    if (destroy_evt) /* destroy it later if not set (ignored table map) */
> +      delete ev;
>    }
>    DBUG_RETURN(retval);
>  }
> 
> === added file 'mysql-test/suite/binlog/r/binlog_row_mysqlbinlog_db_filter.result'
> --- a/mysql-test/suite/binlog/r/binlog_row_mysqlbinlog_db_filter.result	1970-01-01
> 00:00:00 +0000
> +++ b/mysql-test/suite/binlog/r/binlog_row_mysqlbinlog_db_filter.result	2009-06-05
> 07:49:39 +0000
> @@ -0,0 +1,43 @@
> +RESET MASTER;
> +CREATE TABLE t1 (id int);
> +CREATE TABLE t2 (id int);
> +CREATE TABLE t3 (txt TEXT);
> +CREATE TABLE t4 (a int) ENGINE= InnoDB;
> +INSERT INTO t1 VALUES (1);
> +INSERT INTO t1 VALUES (2);
> +INSERT INTO t2 VALUES (1);
> +INSERT INTO t2 VALUES (2);
> +INSERT INTO t1 VALUES (3);
> +LOAD DATA INFILE 'MYSQLTEST_VARDIR/std_data/words.dat' INTO TABLE t3;
> +INSERT INTO t1 VALUES (4);
> +CREATE DATABASE b42941;
> +use b42941;
> +CREATE TABLE t1 (id int);
> +CREATE TABLE t2 (id int);
> +CREATE TABLE t3 (txt TEXT);
> +CREATE TABLE t4 (a int) ENGINE= InnoDB;
> +INSERT INTO t1 VALUES (1);
> +INSERT INTO t1 VALUES (2);
> +INSERT INTO t2 VALUES (1);
> +INSERT INTO t2 VALUES (2);
> +INSERT INTO t1 VALUES (3);
> +LOAD DATA INFILE 'MYSQLTEST_VARDIR/std_data/words.dat' INTO TABLE t3;
> +INSERT INTO t1 VALUES (4);
> +INSERT INTO test.t1 VALUES (5);
> +FLUSH LOGS;
> +UPDATE test.t1 t11, b42941.t1 t12 SET t11.id=10, t12.id=100;
> +BEGIN;
> +INSERT INTO test.t4 VALUES (1);
> +INSERT INTO b42941.t4 VALUES (1);
> +UPDATE test.t4 tn4, b42941.t4 tt4 SET tn4.a= 10, tt4.a= 100;
> +COMMIT;
> +FLUSH LOGS;
> +SET @b42941_output.1= LOAD_FILE('MYSQLTEST_VARDIR/tmp/b42941-mysqlbinlog.1');
> +SET @b42941_output.2= LOAD_FILE('MYSQLTEST_VARDIR/tmp/b42941-mysqlbinlog.2');
> +SET @b42941_output.1= LOAD_FILE('MYSQLTEST_VARDIR/tmp/b42941-mysqlbinlog.1');
> +SET @b42941_output.2= LOAD_FILE('MYSQLTEST_VARDIR/tmp/b42941-mysqlbinlog.2');
> +SET @b42941_output.1= LOAD_FILE('MYSQLTEST_VARDIR/tmp/b42941-mysqlbinlog.1');
> +SET @b42941_output.2= LOAD_FILE('MYSQLTEST_VARDIR/tmp/b42941-mysqlbinlog.2');
> +DROP DATABASE b42941;
> +use test;
> +DROP TABLE t1, t2, t3, t4;
> 
> === added file 'mysql-test/suite/binlog/t/binlog_row_mysqlbinlog_db_filter.test'
> --- a/mysql-test/suite/binlog/t/binlog_row_mysqlbinlog_db_filter.test	1970-01-01
> 00:00:00 +0000
> +++ b/mysql-test/suite/binlog/t/binlog_row_mysqlbinlog_db_filter.test	2009-06-05
> 07:49:39 +0000
> @@ -0,0 +1,143 @@
> +# BUG#42941: --database paramater to mysqlbinlog fails with RBR
> +#

typo: parameter

> +# WHAT
> +# ====
> +#
> +#  This test aims at checking whether a rows log event is printed or
> +#  not when --database parameter is used to filter events from one
> +#  given database.
> +#
> +# HOW
> +# ===
> +#
> +#  The test is implemented as follows: 
> +#
> +#    i) Some operations are done in two different databases:
> +#       'test' and 'b42941';
> +#   ii) mysqlbinlog is used to dump the contents of the binlog file
> +#       filtering only events from 'b42941'. The result of the dump is
> +#       stored in a temporary file. (This is done with and without
> +#       --verbose/hexdump flag);
> +#  iii) The contents of the dump are loaded into a session variable;
> +#   iv) The variable contents are searched for 'test' and 'b42941';
> +#    v) Should 'test' be found, an ERROR is reported. Should 'b42941' be
> +#       absent, an ERROR is reported.
> +
> +-- source include/have_log_bin.inc
> +-- source include/have_binlog_format_row.inc
> +-- source include/have_innodb.inc
> +
> +RESET MASTER;
> +-- let $MYSQLD_DATADIR= `select @@datadir`
> +
> +CREATE TABLE t1 (id int);
> +CREATE TABLE t2 (id int);
> +CREATE TABLE t3 (txt TEXT);
> +CREATE TABLE t4 (a int) ENGINE= InnoDB;
> +INSERT INTO t1 VALUES (1);
> +INSERT INTO t1 VALUES (2);
> +INSERT INTO t2 VALUES (1);
> +INSERT INTO t2 VALUES (2);
> +INSERT INTO t1 VALUES (3);
> +-- replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR
> +-- eval LOAD DATA INFILE '$MYSQLTEST_VARDIR/std_data/words.dat' INTO TABLE t3
> +INSERT INTO t1 VALUES (4);
> +
> +CREATE DATABASE b42941;
> +use b42941;
> +CREATE TABLE t1 (id int);
> +CREATE TABLE t2 (id int);
> +CREATE TABLE t3 (txt TEXT);
> +CREATE TABLE t4 (a int) ENGINE= InnoDB;
> +INSERT INTO t1 VALUES (1);
> +INSERT INTO t1 VALUES (2);
> +INSERT INTO t2 VALUES (1);
> +INSERT INTO t2 VALUES (2);
> +INSERT INTO t1 VALUES (3);
> +-- replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR
> +-- eval LOAD DATA INFILE '$MYSQLTEST_VARDIR/std_data/words.dat' INTO TABLE t3
> +INSERT INTO t1 VALUES (4);
> +
> +INSERT INTO test.t1 VALUES (5);
> +
> +FLUSH LOGS;
> +
> +UPDATE test.t1 t11, b42941.t1 t12 SET t11.id=10, t12.id=100;
> +
> +BEGIN;
> +INSERT INTO test.t4 VALUES (1);
> +INSERT INTO b42941.t4 VALUES (1);
> +UPDATE test.t4 tn4, b42941.t4 tt4 SET tn4.a= 10, tt4.a= 100;
> +COMMIT;
> +
> +FLUSH LOGS;
> +
> +-- let $log_file1= $MYSQLD_DATADIR/master-bin.000001
> +-- let $log_file2= $MYSQLD_DATADIR/master-bin.000002
> +-- let $outfile= $MYSQLTEST_VARDIR/tmp/b42941-mysqlbinlog
> +-- let $cmd= $MYSQL_BINLOG
> +
> +let $i= 3;
> +while($i)
> +{
> +  -- let $flags=--database=b42941
> +
> +  # construct CLI for mysqlbinlog
> +  if(`SELECT $i=3`)
> +  {
> +    -- let $flags= $flags --verbose --hexdump
> +  }
> +
> +  if(`SELECT $i=2`)
> +  {
> +    -- let $flags= $flags --verbose
> +  }
> +
> +  if(`SELECT $i=0`)
> +  {
> +    # do nothing
> +  }

I think you mean `SELECT $i=1`, and I think this is redundant, just add
a comment about it.

> +
> +  # execute mysqlbinlog on the two available master binlog files
> +  -- exec $cmd $flags $log_file1 > $outfile.1
> +  -- exec $cmd $flags $log_file2 > $outfile.2
> +
> +  # load outputs into a variable
> +  -- replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR
> +  -- eval SET @b42941_output.1= LOAD_FILE('$outfile.1')
> +
> +  -- replace_result $MYSQLTEST_VARDIR MYSQLTEST_VARDIR
> +  -- eval SET @b42941_output.2= LOAD_FILE('$outfile.2')
> +
> +  # remove unecessary files
> +  -- remove_file $outfile.1
> +  -- remove_file $outfile.2
> +
> +  # assertion: events for database test are filtered
> +  if (`SELECT INSTR(@b42941_output.1, 'test')`)
> +  {
> +    -- echo **** ERROR **** Database name 'test' FOUND in mysqlbinlog output ($flags
> $outfile.1).
> +  }
> +
> +  if (`SELECT INSTR(@b42941_output.2, 'test')`)
> +  {
> +    -- echo **** ERROR **** Database name 'test' FOUND in mysqlbinlog output ($flags
> $outfile.2).
> +  }
> +
> +  # assertion: events for database b42941 are not filtered
> +  if (!`SELECT INSTR(@b42941_output.1, 'b42941')`)
> +  {
> +    -- echo **** ERROR **** Database name 'b42941' NOT FOUND in mysqlbinlog output
> ($flags $outfile.1).
> +  }
> +
> +  if (!`SELECT INSTR(@b42941_output.2, 'b42941')`)
> +  {
> +    -- echo **** ERROR **** Database name 'b42941' NOT FOUND in mysqlbinlog output
> ($flags $outfile.2).
> +  }
> +
> +  dec $i;
> +}
> +
> +DROP DATABASE b42941;
> +use test;
> +DROP TABLE t1, t2, t3, t4;
> 
> === modified file 'sql/log_event.h'
> --- a/sql/log_event.h	2009-04-08 23:42:51 +0000
> +++ b/sql/log_event.h	2009-06-05 07:49:39 +0000
> @@ -676,6 +676,7 @@ typedef struct st_print_event_info
>  #ifdef MYSQL_CLIENT
>    uint verbose;
>    table_mapping m_table_map;
> +  table_mapping m_table_map_ignored;
>  #endif
>  
>    /*
> 

Thread
bzr commit into mysql-5.1-bugteam branch (luis.soares:2874) Bug#42941Luis Soares5 Jun
  • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:2874)Bug#42941He Zhenxing5 Jun
    • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:2874)Bug#42941Luís Soares7 Jun
  • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:2874)Bug#42941Alfranio Correia7 Jun
    • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:2874)Bug#42941Luís Soares7 Jun
      • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:2874)Bug#42941Alfranio Correia7 Jun
        • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:2874)Bug#42941Luís Soares7 Jun
          • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:2874)Bug#42941Alfranio Correia7 Jun
            • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:2874)Bug#42941Luís Soares8 Jun
            • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:2874)Bug#42941Luís Soares8 Jun