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))
>>
>>
>> ------------------------------------------------------------------------
>>
>>
>>
>>
>
>
