Hi Luís,
Comments inline below...
Best wishes,
Mats Kindahl
Luis Soares wrote:
> #At file:///home/lsoares/Workspace/mysql-server/bugfix/39753/5.1-bt/ based on
> revid:patrick.crews@stripped
>
> 2768 Luis Soares 2009-02-10
> 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.
> 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 2009-02-10 11:21:26 +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 2009-02-10 11:21:26 +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;
Could you extend the test case to check that the default value different widths
of bits are set correctly. I think it is sufficient to check cases where the
extra bits are 1, 7, and something in between, as well as have a field with
exactly 8 bits (that is, with no extra bits in the NULL bits) and a case with
more than 8 bits and with extra bits as above (1, 7, and something between).
> +
> +# 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;
There is a sync_slave_with_master command that is pretty useful here. Otherwise
somebody will wonder why you used "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 2009-02-10 11:21:26 +0000
> @@ -297,21 +297,16 @@ 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
> - initialized explicitly with a call to @c set_default().
> -
> - For optimization reasons, the explicit initialization can be skipped for
> - first @c skip fields. This is useful if later we are going to fill these
> - fields from other source (e.g. from a Rows replication event).
> -
> - If @c check is true, fields are explicitly initialized only if they have
> - default value or can be NULL. Otherwise error is reported.
> + First @c restore_record() is called to restore the default values for
> + record concerning the given table. Then, if @c check is true,
> + a check is performed to see if fields are have default value or can
> + be NULL. Otherwise error is reported.
>
> @param table Table whose record[0] buffer is prepared.
> - @param skip Number of columns for which default value initialization
> + @param skip Number of columns for which default/nullable check
> should be skipped.
> - @param check Indicates if errors should be checked when setting default
> - values.
> + @param check Indicates if errors should be raised when checking
> + default/nullable field properties.
>
> @returns 0 on success or a handler level error code
> */
> @@ -321,12 +316,12 @@ 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);
Could you add a comment that this skip "optimization" needs to be "fixed" when
moving the patch to 6.0. This optimization is incorrect in 6.0, since there
might be "holes" in the read and write-sets.
>
> - /* Explicit initialization of fields */
> + /* Checking if exists default/nullable fields in the default values. */
>
> for (Field **field_ptr= table->field+skip ; *field_ptr ; ++field_ptr)
> {
> @@ -338,8 +333,6 @@ int prepare_record(TABLE *const table,
> my_error(ER_NO_DEFAULT_FOR_FIELD, MYF(0), f->field_name);
> error = HA_ERR_ROWS_EVENT_APPLY;
> }
> - else
> - f->set_default();
> }
>
> DBUG_RETURN(error);
>
>
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
