MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Alfranio Correia Date:September 21 2009 7:18pm
Subject:Re: bzr commit into mysql-5.4 branch (luis.soares:2813) Bug#47200
View as plain text  
Hi Luis,

Find some comments in-line.

Cheers.

Alfranio Correia wrote:
> Hi Luis,
>
> Great work.
>
> The patch seems ok.
> However, I will formally approve after reviewing the worklog.
>
> Cheers.
>
> 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. 
See comment below.
>> This is
>>       noticed for InnoDB engine, which provides HA_PARTIAL_COLUMN_READ,
>>       but it may also fail with other engines providing the same
>>       flag.
Remove the sentence above.
>>  The failure happens when searching the row on the slave to
>>       be updated.
>>     
ok.
>>       
>>       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.
>>     
Maybe we can use the setnence "This is noticed for..." in here.
What do you think?
>>       
>>       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, 
Explain better the case above and write a test case for it.
>> the slave
>>       will fail comparison - it will do a memory comparison of row with
>>       PK + default values against a row with only the PK.
>>     
So, in this case the slave has a pk, doesn't it?
I think it does not have but the description is misleading. Please improve
it in order to avoid the impression that the slave has a pk.
If it has a pk in such case I did not get anything and you should change
the first sentence on the changeset's comment. :)
>>       
>>       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;
>> +
>> +-- 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
>>
>> === 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)))
>>    {
>>      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)) &&
>>        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)))
>>    {
>>      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)) &&
>>        memcmp(table->null_flags,
>>               table->null_flags+table->s->rec_buff_length,
>>               table->s->null_bytes))
>>
>>   
>> ------------------------------------------------------------------------
>>
>>
>>   
>>     
>
>   

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