Hi Luís!
Here are my comments.
Luis Soares wrote:
> #At file:///stuff/workspace/mysql-server/bugfix/39753/5.1-bugteam/
>
> 2765 Luis Soares 2008-12-20
> Bug #39753: Replication failure on MIXED + bit + myisam + no PK
>
> When using mixed mode the record values stored inside the storage
> engine differed from the ones computed from the row event. This
> happened because the prepare_record function was calling
> empty_record macro causing some don't care bits to be left set.
>
> Replacing the empty_record with restore_record to restore the
> record default values fixes this (as suggested by Mats).
> added:
> mysql-test/suite/rpl/r/rpl_mixed_bit_pk.result
> mysql-test/suite/rpl/t/rpl_mixed_bit_pk.test
> modified:
> sql/rpl_record.cc
>
> === added file 'mysql-test/suite/rpl/r/rpl_mixed_bit_pk.result'
> --- a/mysql-test/suite/rpl/r/rpl_mixed_bit_pk.result 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/rpl/r/rpl_mixed_bit_pk.result 2008-12-20 18:43:30 +0000
> @@ -0,0 +1,11 @@
> +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;
> +DROP TABLE IF EXISTS t1;
> +CREATE TABLE t1 (`bit_key` bit, `bit` bit, key (`bit_key` )) ENGINE=MyISAM;
> +INSERT INTO `t1` ( `bit` ) VALUES ( 0 );
> +DELETE FROM `t1` WHERE `bit` < 2 LIMIT 4;
> +DROP TABLE t1;
>
> === added file 'mysql-test/suite/rpl/t/rpl_mixed_bit_pk.test'
> --- a/mysql-test/suite/rpl/t/rpl_mixed_bit_pk.test 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_mixed_bit_pk.test 2008-12-20 18:43:30 +0000
> @@ -0,0 +1,53 @@
> +#
> +# BUG
> +# ---
> +# BUG#39753: Replication failure on MIXED + bit + myisam + no PK
> +#
> +# Description
> +# -----------
> +# Simple statements against a bit column cause failure in mixed-mode
> +# replication.
> +#
> +# Implementation is as follows:
> +# i) A table with two bit fields is created. One of them is a key.
> +# ii) A record is inserted without specifying the key value.
> +# iii) The record is deleted using a where clause that matches it.
> +# iv) The slave is synchronized with master
> +# v) The table is dropped on master and the slave is re-synchronized
> +# with master.
> +#
> +# Step iv) made the bug evident before the patch, as the slave would
> +# fail to find the correspondent row in its database (although it did
> +# the insert in step ii) ).
> +#
> +# Obs
> +# ---
> +# This test is based on the "how to repeat" section from the bug report.
> +#
> +#
> +
> +--source include/master-slave.inc
> +
> +--disable_warnings
> +
> +# setup
> +
> +DROP TABLE IF EXISTS t1;
> +CREATE TABLE t1 (`bit_key` bit, `bit` bit, key (`bit_key` )) ENGINE=MyISAM;
> +
> +# insert and delete
> +INSERT INTO `t1` ( `bit` ) VALUES ( 0 );
> +DELETE FROM `t1` WHERE `bit` < 2 LIMIT 4;
> +
> +--enable_warnings
> +
> +save_master_pos;
> +connection slave;
> +sync_with_master 0;
> +
> +# clean up
> +connection master;
> +DROP TABLE t1;
> +save_master_pos;
> +connection slave;
> +sync_with_master 0;
>
> === modified file 'sql/rpl_record.cc'
> --- a/sql/rpl_record.cc 2008-02-05 13:52:20 +0000
> +++ b/sql/rpl_record.cc 2008-12-20 18:43:30 +0000
> @@ -297,7 +297,7 @@ unpack_row(Relay_log_info const *rli,
> /**
> Fills @c table->record[0] with default values.
>
> - First @c empty_record() is called and then, additionally, fields are
> + First @c restore_record() is called and then, additionally, fields are
It is quite clear what empty_record() does, but could you extend the comment
explaining why restore_record() is called, i.e., something like:
First, @c restore_record() is called to initialize the working record with the
default values for the table, ...
Also, the text following is hard to understand, could you expand it to work with
the changed comment?
> initialized explicitly with a call to @c set_default().
>
> For optimization reasons, the explicit initialization can be skipped for
> @@ -321,7 +321,7 @@ int prepare_record(TABLE *const table,
> DBUG_ENTER("prepare_record");
>
> int error= 0;
> - empty_record(table);
> + restore_record(table, s->default_values);
>
> if (skip >= table->s->fields) // nothing to do
> DBUG_RETURN(0);
The loop following this code seems redundant (at least the calls to
set_default()). Is it still necessary to call set_default() for each field after
the skipped prefix?
Just my few cents,
Mats Kindahl
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
