Hi Luis,
I would the test extended to insert a string that is really more than
255 *bytes* long and see that it actually replicates correctly. Apart
from that, I have nothing that prevents it from being approved. You can
read my comments below anyway.
/Matz
On 10/29/2010 02:08 PM, Luis Soares wrote:
> #At file:///home/lsoares/Workspace/bzr/work/bugfixing/53386/mysql-trunk-bugfixing/
> based on revid:alfranio.correia@stripped
>
> 3288 Luis Soares 2010-10-29
> BUG#53386: HA_ERR_CORRUPT_EVENT or debug assertion with different
> table on slave
>
> When the number of raw bytes used for a CHAR field exceeds 255
> two bytes are actually used while packing the length of the
> field. CHAR fields can exceed the raw 255 byte length, for
> instance, if using multi-byte character sets.
>
> The problem is that when calculating the raw length of the field,
> at unpacking time, only one byte was assumed to be used
> always. Therefore, the number of bytes used would not add up,
> triggering the assertion.
>
> We fix this, by making the member function that calculates the
> raw byte length for a given field to take into account the actual
> number of bytes used while packing the field length.
> @ mysql-test/extra/rpl_tests/rpl_extraMaster_Col.test
> Extended the test case with one based on the bug report entry.
> @ sql/rpl_record.cc
> Added assertion so that calc_field_size always matches the
> size returned by field->unpack.
> @ sql/rpl_utility.cc
> Changed calc_field_size so that either one or two bytes are
> taken into account, depending on the maximum field size.
>
> Additionally, fixed one wrong assertion that was deployed
> when calculating the field size of a varchar type.
>
> modified:
> mysql-test/extra/rpl_tests/rpl_extraMaster_Col.test
> mysql-test/suite/rpl/r/rpl_extraColmaster_innodb.result
> mysql-test/suite/rpl/r/rpl_extraColmaster_myisam.result
> sql/rpl_record.cc
> sql/rpl_utility.cc
> === modified file 'mysql-test/extra/rpl_tests/rpl_extraMaster_Col.test'
> --- a/mysql-test/extra/rpl_tests/rpl_extraMaster_Col.test 2010-06-22 09:34:59 +0000
> +++ b/mysql-test/extra/rpl_tests/rpl_extraMaster_Col.test 2010-10-29 12:08:34 +0000
> @@ -1029,4 +1029,34 @@ sync_slave_with_master;
>
> # END of 5.1 tests case
>
> +#
> +# BUG#53386: HA_ERR_CORRUPT_EVENT or debug assertion with different table on slave
> +#
> +
> +-- connection master
> +-- source include/master-slave-reset.inc
> +-- connection master
> +
> +# CREATE Different tables on master and slave
> +SET SQL_LOG_BIN=0;
> +-- eval CREATE TABLE t1 (c1 INT NOT NULL, c2 CHAR(255) CHARACTER SET UTF8 NOT NULL)
> ENGINE=$engine_type
> +SET SQL_LOG_BIN=1;
> +-- connection slave
> +-- eval CREATE TABLE t1 (c1 INT) ENGINE=$engine_type
> +
> +# insert empty row (c2 gets an empty string)
> +-- connection master
> +
> +-- disable_warnings
> +INSERT INTO t1 VALUES ();
>
What about a row with a string that is longer than 255 bytes here? Is it
skipped correctly?
> +-- enable_warnings
> +
> +SELECT * FROM t1;
> +-- sync_slave_with_master
> +SELECT * FROM t1;
> +
> +# clean up
> +-- connection master
> +DROP TABLE t1;
> +-- sync_slave_with_master
>
>
> === modified file 'mysql-test/suite/rpl/r/rpl_extraColmaster_innodb.result'
> --- a/mysql-test/suite/rpl/r/rpl_extraColmaster_innodb.result 2010-06-22 09:34:59
> +0000
> +++ b/mysql-test/suite/rpl/r/rpl_extraColmaster_innodb.result 2010-10-29 12:08:34
> +0000
> @@ -876,3 +876,21 @@ c1 hex(c4) c5
> 3 6231623162316231 QA
> DROP TABLE t5;
>
> +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;
> +SET SQL_LOG_BIN=0;
> +CREATE TABLE t1 (c1 INT NOT NULL, c2 CHAR(255) CHARACTER SET UTF8 NOT NULL)
> ENGINE='InnoDB';
> +SET SQL_LOG_BIN=1;
> +CREATE TABLE t1 (c1 INT) ENGINE='InnoDB';
> +INSERT INTO t1 VALUES ();
> +SELECT * FROM t1;
> +c1 c2
> +0
> +SELECT * FROM t1;
> +c1
> +0
> +DROP TABLE t1;
>
> === modified file 'mysql-test/suite/rpl/r/rpl_extraColmaster_myisam.result'
> --- a/mysql-test/suite/rpl/r/rpl_extraColmaster_myisam.result 2010-06-22 09:34:59
> +0000
> +++ b/mysql-test/suite/rpl/r/rpl_extraColmaster_myisam.result 2010-10-29 12:08:34
> +0000
> @@ -876,3 +876,21 @@ c1 hex(c4) c5
> 3 6231623162316231 QA
> DROP TABLE t5;
>
> +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;
> +SET SQL_LOG_BIN=0;
> +CREATE TABLE t1 (c1 INT NOT NULL, c2 CHAR(255) CHARACTER SET UTF8 NOT NULL)
> ENGINE='MyISAM';
> +SET SQL_LOG_BIN=1;
> +CREATE TABLE t1 (c1 INT) ENGINE='MyISAM';
> +INSERT INTO t1 VALUES ();
> +SELECT * FROM t1;
> +c1 c2
> +0
> +SELECT * FROM t1;
> +c1
> +0
> +DROP TABLE t1;
>
> === modified file 'sql/rpl_record.cc'
> --- a/sql/rpl_record.cc 2010-07-24 13:47:42 +0000
> +++ b/sql/rpl_record.cc 2010-10-29 12:08:34 +0000
> @@ -324,6 +324,13 @@ unpack_row(Relay_log_info const *rli,
> f->field_name, metadata,
> (ulong) old_pack_ptr, (ulong) pack_ptr,
> (int) (pack_ptr - old_pack_ptr)));
> +
> + /*
> + The raw size of the field, as calculated in calc_field_size,
> + should match the one reported by Field_*::unpack.
> + */
> + DBUG_ASSERT(tabledef->calc_field_size(i, (uchar *) old_pack_ptr) ==
> + (pack_ptr - old_pack_ptr));
>
Nice assertion. :)
> }
>
> /*
>
> === modified file 'sql/rpl_utility.cc'
> --- a/sql/rpl_utility.cc 2010-07-16 21:00:50 +0000
> +++ b/sql/rpl_utility.cc 2010-10-29 12:08:34 +0000
> @@ -225,9 +225,12 @@ uint32 table_def::calc_field_size(uint c
> /*
> We are reading the actual size from the master_data record
> because this field has the actual lengh stored in the first
> - byte.
> + one or two bytes.
> */
> - length= (uint) *master_data + 1;
> + length= max_display_length_for_field(MYSQL_TYPE_STRING, m_field_metadata[col])
> > 255 ? 2 : 1;
> +
> + /* As in Field_string::unpack */
> + length+= ((length == 1) ? *master_data : uint2korr(master_data));
> DBUG_ASSERT(length != 0);
> }
> break;
> @@ -284,8 +287,8 @@ uint32 table_def::calc_field_size(uint c
> case MYSQL_TYPE_VARCHAR:
> {
> length= m_field_metadata[col] > 255 ? 2 : 1; // c&p of
> Field_varstring::data_length()
> - DBUG_ASSERT(uint2korr(master_data) > 0);
> length+= length == 1 ? (uint32) *master_data : uint2korr(master_data);
> + DBUG_ASSERT(length != 0);
>
Assertions are meant to state that something is required for the code to
function correctly: either that the input parameter is correct (the code
has assumptions on the input which, if violated, lead to incorrect
results or a crash) or that the output parameter is correct. This
assertion seems to not be needed, since length is guaranteed to be 1 or
2 plus an unsigned number: hence it cannot even by mistake be 0.
Maybe it is something else that you want to assert?
> break;
> }
> case MYSQL_TYPE_TINY_BLOB:
>
>
>
>
>
>
