List:Commits« Previous MessageNext Message »
From:Luís Soares Date:December 11 2009 10:16am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3247)
Bug#49481
View as plain text  
Hi Mats,

  Thanks for the review.
  I have some explanations below.

On Fri, 2009-12-11 at 09:00 +0100, Mats Kindahl wrote:
> Hi Luis,
> 
> Patch approved, but I have a minor comment below about extending the test.
> 
> Just my few cents,
> Mats Kindahl
> 
> Luis Soares wrote:
> > #At file:///home/lsoares/Workspace/bzr/work/bugfixing/49482/mysql-5.1-bugteam/
> based on revid:davi.arnaut@stripped
> > 
> >  3247 Luis Soares	2009-12-06
> >       BUG#49481: RBR: MyISAM and bit fields may cause slave to stop on delete: 
> >       cant find record
> >       
> >       BUG#49482: RBR: Replication may break on deletes when MyISAM tables + 
> >       char field are used
> >       
> >       When using MyISAM tables, despite the fact that the null bit is
> >       set for some fields, their old value is still in the row. This
> >       can cause the comparison of records to fail when the slave is
> >       doing an index or range scan.
> >       
> >       We fix this by avoiding memcmp for MyISAM tables when comparing
> >       records. Additionally, when comparing field by field, we first
> >       check if both fields are not null and if so, then we compare
> >       them. If just one field is null we return failure immediately. If
> >       both fields are null, we move on to the next field.
> > 
> >     added:
> >       mysql-test/suite/rpl/r/rpl_myisam_null_values.result
> >       mysql-test/suite/rpl/t/rpl_myisam_null_values.test
> >     modified:
> >       sql/log_event.cc
> > === added file 'mysql-test/suite/rpl/r/rpl_myisam_null_values.result'
> > --- a/mysql-test/suite/rpl/r/rpl_myisam_null_values.result	1970-01-01 00:00:00
> +0000
> > +++ b/mysql-test/suite/rpl/r/rpl_myisam_null_values.result	2009-12-06 04:51:12
> +0000
> > @@ -0,0 +1,24 @@
> > +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;
> > +CREATE TABLE t1 (c1 BIT, c2 INT);
> > +INSERT INTO `t1` VALUES ( 1, 1 );
> > +UPDATE t1 SET c1=NULL where c2=1;
> > +Comparing tables master:test.t1 and slave:test.t1
> > +DELETE FROM t1 WHERE c2=1 LIMIT 1;
> > +Comparing tables master:test.t1 and slave:test.t1
> > +DROP TABLE t1;
> > +CREATE TABLE t1 (c1 CHAR);
> > +INSERT INTO t1 ( c1 ) VALUES ( 'w' ) ;
> > +SELECT * FROM t1;
> > +c1
> > +w
> > +# should trigger switch to row due to LIMIT
> 
> How do you know it did?

I assume that this is valid:
http://dev.mysql.com/doc/refman/5.1/en/replication-features-limit.html

> > +UPDATE t1 SET c1=NULL WHERE c1='w' LIMIT 2;
> > +Comparing tables master:test.t1 and slave:test.t1
> > +DELETE FROM t1 LIMIT 2;
> > +Comparing tables master:test.t1 and slave:test.t1
> > +DROP TABLE t1;
> > 
> > === added file 'mysql-test/suite/rpl/t/rpl_myisam_null_values.test'
> > --- a/mysql-test/suite/rpl/t/rpl_myisam_null_values.test	1970-01-01 00:00:00
> +0000
> > +++ b/mysql-test/suite/rpl/t/rpl_myisam_null_values.test	2009-12-06 04:51:12
> +0000
> > @@ -0,0 +1,53 @@
> > +# BUG#49481: RBR: MyISAM and bit fields may cause slave to stop on delete cant
> find record
> > +# BUG#49482: RBR: Replication may break on deletes when MyISAM tables + char
> field are used
> > +
> > +-- source include/master-slave.inc
> > +-- source include/have_binlog_format_mixed_or_row.inc
> 
> Since you're testing a row-format feature, I see no reason to run this test
> under mixed.

In general this is not an issue that is strictly related to RBR. It is
related to finding a row when executing an UPDATE or DELETE rows event.
For a row to be found, it first needs to be inserted (and/or updated).
We can insert/update/delete either by executing a statement or 
by executing a row event. That's what mixed mode does, it logs
statements and row events together, intermixed.

Therefore, we can face this issue in MIXED mode (check the statement
with LIMIT below). That's why I have made the test to also run in 
mixed, to prove that in general even if mixed mode is affected, the
patch also fixes it.

> > +
> > +-- connection master
> > +CREATE TABLE t1 (c1 BIT, c2 INT);
> > +INSERT INTO `t1` VALUES ( 1, 1 );
> > +UPDATE t1 SET c1=NULL where c2=1;
> > +-- sync_slave_with_master
> > +
> > +-- let $diff_table_1=master:test.t1
> > +-- let $diff_table_2=slave:test.t1
> > +-- source include/diff_tables.inc
> > +
> > +-- connection master
> > +DELETE FROM t1 WHERE c2=1 LIMIT 1;
> > +-- sync_slave_with_master
> > +
> > +-- let $diff_table_1=master:test.t1
> > +-- let $diff_table_2=slave:test.t1
> > +-- source include/diff_tables.inc
> > +
> > +-- connection master
> > +DROP TABLE t1;
> > +-- sync_slave_with_master
> > +
> > +-- connection master
> > +
> > +CREATE TABLE t1 (c1 CHAR);
> > +
> > +INSERT INTO t1 ( c1 ) VALUES ( 'w' ) ;
> > +SELECT * FROM t1;
> > +-- echo # should trigger switch to row due to LIMIT
> 
> Remove this comment if you only run under row.
> 
> > +UPDATE t1 SET c1=NULL WHERE c1='w' LIMIT 2;
> > +-- sync_slave_with_master
> > +
> > +-- let $diff_table_1=master:test.t1
> > +-- let $diff_table_2=slave:test.t1
> > +-- source include/diff_tables.inc
> > +
> > +-- connection master
> > +DELETE FROM t1 LIMIT 2;
> > +-- sync_slave_with_master
> > +
> > +-- let $diff_table_1=master:test.t1
> > +-- let $diff_table_2=slave:test.t1
> > +-- source include/diff_tables.inc
> > +
> > +-- connection master
> > +DROP TABLE t1;
> > +-- sync_slave_with_master
> > 
> 
> Can you check this for InnoDB as well? Just to make sure that there are no
> issues with it either.

I did already it's in the bug report. See how to repeat section 
(step 6.).

No issues were found in InnoDB.

> > === modified file 'sql/log_event.cc'
> > --- a/sql/log_event.cc	2009-11-21 13:02:18 +0000
> > +++ b/sql/log_event.cc	2009-12-06 04:51:12 +0000
> > @@ -8708,6 +8708,20 @@ static bool record_compare(TABLE *table)
> >      }
> >    }
> >  
> > +  /**
> > +    Check if we are using MyISAM.
> > +
> > +    If this is a myisam table, then we cannot do a memcmp
> > +    right away because for some nulled fields there can still
> > +    be old values in the row (null bits for bit fields or values
> > +    for char like fields) despite the fact that the null bit is
> > +    set. As such, plain memory contents comparison cannot be
> > +    assured to work. See: BUG#49482 and BUG#49481.
> > +  */
> > +  
> > +  if (table->file->ht->db_type == DB_TYPE_MYISAM)
> > +    goto record_compare_field_by_field;
> > +
> 
> Hum... so what if you replicate from MyISAM to InnoDB? Since the complete row is
> sent, isn't it possible that MyISAM is reading up data to null fields and then
> pass it to the slave?
> 
> No, because we do not store data in the binlog for null fields.
> 
> OK.

OK, I'll add this to the comment.

> >    if (table->s->blob_fields + table->s->varchar_fields == 0)
> >    {
> >      result= cmp_record(table,record[1]);
> > @@ -8723,14 +8737,33 @@ static bool record_compare(TABLE *table)
> >      goto record_compare_exit;
> >    }
> >  
> > +record_compare_field_by_field:
> > +
> >    /* Compare updated fields */
> >    for (Field **ptr=table->field ; *ptr ; ptr++)
> >    {
> > -    if ((*ptr)->cmp_binary_offset(table->s->rec_buff_length))
> > +    Field *f= *ptr;
> > +
> > +    /* if just one of the fields is null then there is no match */
> > +    if ((f->is_null_in_record(table->record[0])) ==
> > +         !(f->is_null_in_record(table->record[1])))
> >      {
> >        result= TRUE;
> >        goto record_compare_exit;
> >      }
> > +
> > +    /* if both fields are not null then we can compare */
> > +    if (!(f->is_null_in_record(table->record[0])) &&
> > +        !(f->is_null_in_record(table->record[1])))
> > +    {
> > +      if (f->cmp_binary_offset(table->s->rec_buff_length))
> > +      {
> > +        result= TRUE;
> > +        goto record_compare_exit;
> > +      }
> > +    }
> > +
> > +    /* if both fields are null then there is a match. compare next field */
> >    }
> >  
> >  record_compare_exit:
> > 
> > 
> > 
> > ------------------------------------------------------------------------
> > 
> > 
> 
> -- 
> Mats Kindahl
> Senior Software Engineer
> Database Technology Group
> Sun Microsystems
> 

Regards,
Luís

Thread
bzr commit into mysql-5.1-bugteam branch (luis.soares:3247) Bug#49481Luis Soares6 Dec
  • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3247)Bug#49481Mats Kindahl11 Dec
    • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3247)Bug#49481Luís Soares11 Dec
  • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3247)Bug#49481Alfranio Correia29 Dec