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. 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;
> +
> +-- 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))
>
>
> ------------------------------------------------------------------------
>
>
>
