MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Mats Kindahl Date:September 22 2009 9:10am
Subject:Re: bzr commit into mysql-5.4 branch (luis.soares:2813) Bug#47200
View as plain text  
Hi Luis,

Here are my comments regarding the patch.

Best wishes,
Mats Kindahl

Luis Soares wrote:
> #At file:///home/lsoares/Workspace/bzr/work/bugfixing/47200/mysql-next-bugfixing/
> based on revid:luis.soares@stripped
> 
>  2813 Luis Soares	2009-09-10
>       BUG#47200: RBR: Absence of PK on slave leads to slave stop.
>       
>       If the same table is declared with a PK on the master but without
>       the PK on the slave, row based replication may break. This is
>       noticed for InnoDB engine, which provides HA_PARTIAL_COLUMN_READ,
>       but it may also fail with other engines providing the same
>       flag. The failure happens when searching the row on the slave to
>       be updated.
>       
>       When the slave thread performs a scan (table or index), the
>       search procedure is basically fetching rows from slave engine and
>       comparing them against the row in the event before image. If the
>       engine provides HA_PARTIAL_COLUMN_READ, then a memory comparison
>       is performed for both rows, otherwise, a column by column (for
>       only those marked in the read_set) comparison is done. There can
>       be two problems with this:
>       
>       First, engine does not honor HA_PARTIAL_COLUMN_READ for some
>       cases. When fetching the row from the slave engine, and although
>       it provides HA_PARTIAL_COLUMN_READ, the table->read_set (which
>       marks only the columns as in the master PK) is ignored. Thence,
>       the engine returns the full row instead of only the columns
>       needed for comparison. In this case, the comparison will fail.
>       
>       Second, when the RBR event row is unpacked it is filled with
>       default values for the missing columns. So, it can be the case
>       the event before image is filled with default values, on the
>       slave at unpack time. Consequently, when searching the record,
>       and even if the engine honors HA_PARTIAL_COLUMN_READ, the slave
>       will fail comparison - it will do a memory comparison of row with
>       PK + default values against a row with only the PK.
>       
>       This patch fixes this issue by removing the part of the
>       comparison that takes into account the HA_PARTIAL_COLUMN_READ. In
>       such cases it performs a column by column comparison (for those
>       marked in the read_set).
> 
>     modified:
>       mysql-test/suite/rpl/r/rpl_row_record_find_innodb.result
>       mysql-test/suite/rpl/t/rpl_row_record_find_innodb.test
>       sql/log_event.cc
>       sql/log_event_old.cc
> === modified file 'mysql-test/suite/rpl/r/rpl_row_record_find_innodb.result'
> --- a/mysql-test/suite/rpl/r/rpl_row_record_find_innodb.result	2009-04-29 16:48:59
> +0000
> +++ b/mysql-test/suite/rpl/r/rpl_row_record_find_innodb.result	2009-09-10 09:37:43
> +0000
> @@ -826,3 +826,15 @@ DELETE FROM t WHERE c1 = 8;
>  DELETE FROM t;
>  Comparing tables master:test.t and slave:test.t
>  DROP TABLE t;
> +SET SQL_LOG_BIN=0;
> +CREATE TABLE t (c1 int, c2 char(1), c3 char(1), 
> +c4 char(1), primary key(c1,c3)) engine=
> +InnoDB;
> +SET SQL_LOG_BIN=1;
> +SET SQL_LOG_BIN=0;
> +CREATE TABLE t (c1 int, c2 char(1), c3 char(1), 
> +c4 char(1)) engine= InnoDB;
> +SET SQL_LOG_BIN=1;
> +INSERT INTO t VALUES (1, '2', '3', '4' );
> +UPDATE t SET c4 = '7';
> +DROP TABLE t;
> 
> === modified file 'mysql-test/suite/rpl/t/rpl_row_record_find_innodb.test'
> --- a/mysql-test/suite/rpl/t/rpl_row_record_find_innodb.test	2009-04-29 16:48:59
> +0000
> +++ b/mysql-test/suite/rpl/t/rpl_row_record_find_innodb.test	2009-09-10 09:37:43
> +0000
> @@ -44,3 +44,35 @@ let $master_engine= MyISAM;
>  let $slave_engine= InnoDB;
>  
>  -- source extra/rpl_tests/rpl_row_record_find.test
> +
> +#
> +# BUG#47200: RBR: Absence of PK on slave leads to slave stop.
> +# 
> +
> +-- connection master
> +SET SQL_LOG_BIN=0;
> +CREATE TABLE t (c1 int, c2 char(1), c3 char(1), 
> +                c4 char(1), primary key(c1,c3)) engine=
> +InnoDB;
> +SET SQL_LOG_BIN=1;
> +
> +-- connection slave
> +SET SQL_LOG_BIN=0;
> +CREATE TABLE t (c1 int, c2 char(1), c3 char(1), 
> +                c4 char(1)) engine= InnoDB;
> +SET SQL_LOG_BIN=1;

Although it does not harm, I do not see any reason to turn the binlog on and off
all the time.

> +-- connection master
> +INSERT INTO t VALUES (1, '2', '3', '4' );
> +
> +-- sync_slave_with_master
> +
> +-- connection master
> +UPDATE t SET c4 = '7';
> +
> +-- sync_slave_with_master
> +-- connection master
> +
> +DROP TABLE t;
> +
> +-- sync_slave_with_master

Can you add the same test for the other engines we have?

Could you add the DELETE as well, since you already have tests for INSERT and
UPDATE.

Could you also test the following combinations, to get a good coverage:

- INDEX on master, nothing on slave
- PK on master, INDEX on slave (same columns)
- PK on master, INDEX on slave where slave index has a subset of the columns on
  the master.
- PK on master, INDEX on slave with additional columns compared to master.
- PK on master, *different* PK on slave (data in table should satisfy both
  constraints).

> === modified file 'sql/log_event.cc'
> --- a/sql/log_event.cc	2009-08-13 14:29:55 +0000
> +++ b/sql/log_event.cc	2009-09-10 09:37:43 +0000
> @@ -8735,16 +8735,14 @@ static bool record_compare(TABLE *table,
>       ii) all columns were read or the slave SE provides partial reads
>    */
>    if ((table->s->blob_fields + table->s->varchar_fields == 0)
> &&
> -      (bitmap_is_set_all(cols_in_readset) || 
> -      (table->file->ha_table_flags() & HA_PARTIAL_COLUMN_READ)))
> +      (bitmap_is_set_all(cols_in_readset)))

Looks OK, but I think it is a pity that we cannot improve performance in the
cases where the storage engine touches only the columns in the read_set. Maybe
we can add some extra flag later to indicate that: I think that, for example,
NDB would like that.

>    {
>      result= cmp_record(table,record[1]);
>      goto record_compare_exit;
>    }
>  
>    /* Compare null bits only if all fields were read or slave SE has partial reads
> */
> -  if ((bitmap_is_set_all(cols_in_readset) || 
> -       (table->file->ha_table_flags() & HA_PARTIAL_COLUMN_READ))
> && 
> +  if ((bitmap_is_set_all(cols_in_readset)) &&

OK.

>        memcmp(table->null_flags,
>  	     table->null_flags+table->s->rec_buff_length,
>  	     table->s->null_bytes))
> 
> === modified file 'sql/log_event_old.cc'
> --- a/sql/log_event_old.cc	2009-08-13 14:29:55 +0000
> +++ b/sql/log_event_old.cc	2009-09-10 09:37:43 +0000
> @@ -320,16 +320,14 @@ static bool record_compare(TABLE *table,
>        ii) all columns were read or the slave SE provides partial reads
>    */
>    if ((table->s->blob_fields + table->s->varchar_fields == 0)
> &&
> -      (bitmap_is_set_all(cols_in_readset) || 
> -      (table->file->ha_table_flags() & HA_PARTIAL_COLUMN_READ)))
> +      (bitmap_is_set_all(cols_in_readset)))

OK

>    {
>      result= cmp_record(table,record[1]);
>      goto record_compare_exit;
>    }
>  
>    /* Compare null bits only if all fields were read or slave SE has partial reads
> */
> -  if ((bitmap_is_set_all(cols_in_readset) || 
> -        (table->file->ha_table_flags() & HA_PARTIAL_COLUMN_READ))
> && 
> +  if ((bitmap_is_set_all(cols_in_readset)) &&

OK.

>        memcmp(table->null_flags,
>               table->null_flags+table->s->rec_buff_length,
>               table->s->null_bytes))
> 
> 
> 
> ------------------------------------------------------------------------
> 
> 

-- 
Mats Kindahl
Senior Software Engineer
Database Technology Group
Sun Microsystems
Thread
bzr commit into mysql-5.4 branch (luis.soares:2813) Bug#47200Luis Soares10 Sep
  • Re: bzr commit into mysql-5.4 branch (luis.soares:2813) Bug#47200Alfranio Correia21 Sep
    • Re: bzr commit into mysql-5.4 branch (luis.soares:2813) Bug#47200Alfranio Correia21 Sep
  • Re: bzr commit into mysql-5.4 branch (luis.soares:2813) Bug#47200Mats Kindahl22 Sep