From: Luís Soares
Date: October 29 2010 12:47pm
Subject: Re: bzr commit into mysql-trunk-bugfixing branch (luis.soares:3288)
 Bug#53386
List-Archive: http://lists.mysql.com/commits/122304
Message-Id: <4CCAC25A.7010206@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit

Hi Mats,
   The new patch is at: http://lists.mysql.com/commits/122303

   I have some replies inline.

Thanks,
Luís


On 10/29/2010 01:28 PM, Mats Kindahl wrote:
> 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.

Ok, added.

> /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?

Yes. Test added.

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

Somehow I knew you would like it ;).

>>         }
>>
>>         /*
>>
>> === 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?

I was just following the pattern. Anyway, I agree with
you and removed it in the new patch.

>>       break;
>>     }
>>     case MYSQL_TYPE_TINY_BLOB:
>>
>>
>>
>>
>>
>>
>
>