List:Commits« Previous MessageNext Message »
From:Mats Kindahl Date:December 10 2009 10:45am
Subject:Re: bzr commit into mysql-5.1 branch (mats:3180) WL#5151
View as plain text  

Luís Soares wrote:
> Hi Mats,
> 
>   Nice Work, I really don't have much to say... which is good.
>   Please find my review comments below.
> 
> STATUS
> ------
> 
>  Not Approved.
> 
> REQUIRED CHANGES
> ----------------
>  
>   RC1. Please, change the WL so that it matches the
>        implementation regarding BLOBS. If a lossy conversion is
>        used and the slave's blob field is "smaller", then it gets
>        truncated, not set to NULL.
> 
>        If it was set to NULL then we could loose the ability to
>        replicate correctly when blob had a prefix part of a PK,
>        eg:
> 
>        MASTER> CREATE TABLE t1 (c1 BLOB, c2 int, primary key(c1(4),
> c2));
>        SLAVE> CREATE TABLE t1 (c1 TINYBLOB, c2 int, primary key(c1(4),
> c2));
>        MASTER> INSERT INTO t1 VALUES ('aaaa', 1);
>        MASTER> INSERT INTO t1 VALUES ('bbbbb', 1);

Fixed.

> 
>   RC2. I can't build mysql server after applying the patches. I get:
> 
>        (...)   
>        set_var.cc:120: error: cannot convert ‘size_t*’ to
> ‘unsigned
> int*’ in initialization
>        make[3]: *** [set_var.o] Error 1
>        make[3]: *** Waiting for unfinished jobs....
>        (...)
> 
>        Details:
>          arch: x86_64
>            OS: Ubuntu Linux 9.04
>           cmd: ./BUILD/compile-pentium64-debug-max

Fixed.

> 
> REQUESTS
> --------
> 
>   R1. While unpacking, I think there wont be problems when
>       copying back Field_bit from the conversion table, correct?
>       I am just thinking that some parts of the bit fields are
>       stored in the null_bytes, which may not match between
>       conversion and destination table.

No, that should not be a problem at all, even in the case that the tables are
using Field_bit and Field_bit_as_char.

>       Also, running the following test case shows something I was
>       not expecting:
> 
> -- source include/master-slave.inc
> -- source include/have_binlog_format_row.inc
> -- source include/have_innodb.inc
> 
> -- connection slave
> SET GLOBAL SLAVE_TYPE_CONVERSIONS='ALL_LOSSY';
> 
> -- source include/stop_slave.inc
> -- source include/start_slave.inc
> 
> -- connection master
> 
> SET SQL_LOG_BIN=0;
> CREATE TABLE t1 (c1 bit(3), a int);
> SET SQL_LOG_BIN=1;
> 
> -- connection slave
> CREATE TABLE t1 (c1 bit(2), a int);
> 
> -- connection master
> 
> INSERT INTO t1 VALUES (b'101', 1);
> 
> SELECT hex(c1) FROM t1;
> -- sync_slave_with_master
> SELECT hex(c1) FROM t1;
> 
> -- connection master
> DROP TABLE t1;
> -- sync_slave_with_master
> 
> SET GLOBAL SLAVE_TYPE_CONVERSIONS='ALL_NON_LOSSY';
> 
> -- exit
> 
>      The SELECT hex(c1) in the slave, shows 0x03... Shouldn't it
>      show 0x01, given that the least significant bits from the
>      master are taken and inserted into the slave table?

It should be 0x3, since this is how the server normally handles such a
conversion. We have already discussed this and I fixed the worklog to match with
the true behavior.

> 
> 
> SUGGESTIONS
> -----------
>  
>   S1. ALL_NON_LOSSY, ALL_LOSSY and empty value... This caused a
>       bit of confusion to me at first. It seems that empty value
>       is something one would use when no conversions are allowed
>       at all, correct? So in other words, empty feels the most
>       strict option one has...
> 
>       Can you make this clear in the help text in mysqld.cc?

Fixed.

> 
> DETAILS 
> -------
>   
>   See one more comment inline.

That was really hard to find.

[snip]

>> +static bool
>> +can_convert_field_to(Field *field, enum_field_types source_type, uint16
> metadata,
>> +                     Relay_log_info *rli, int *order_var)
>> +{
>> +  DBUG_ENTER(__FUNCTION__);
>> +#ifndef DBUG_OFF
>> +  char field_type_buf[MAX_FIELD_WIDTH];
>> +  String field_type(field_type_buf, sizeof(field_type_buf),
> field->charset());
>> +  field->sql_type(field_type);
>> +  DBUG_PRINT("enter", ("field_type: %s, source_type: %d, source_metadata:
> 0x%x",
>> +                       field_type.c_ptr(), source_type, metadata));
>> +#endif
>> +  /*
>> +    If the real type is the same, we need to check the metadata to
>> +    decide if conversions are allowed.
>> +   */
>> +  if (field->real_type() == source_type)
>> +  {
>> +    DBUG_PRINT("debug", ("Base types are identical, doing field size
> comparison"));
>> +    if (const_cast<Field*>(field)->compatible_field_size(metadata, rli,
> order_var))
>> +      DBUG_RETURN(is_conversion_ok(*order_var, rli));
>> +    else
>> +      DBUG_RETURN(false);
>> +  }
> 
> Why not add here (so that we avoid the switch and processing below)?
> 
>      else if (!slave_type_conversions_options)
> 	DBUG_RETURN(false);

Good idea. I'll add that, and maybe in some more places.

/Matz

-- 
Mats Kindahl
Senior Software Engineer
Database Technology Group
Sun Microsystems
Thread
bzr commit into mysql-5.1 branch (mats:3180) WL#5151Mats Kindahl4 Dec
  • Re: bzr commit into mysql-5.1 branch (mats:3180) WL#5151Alfranio Correia10 Dec
    • Re: bzr commit into mysql-5.1 branch (mats:3180) WL#5151Sergei Golubchik10 Dec
    • Re: bzr commit into mysql-5.1 branch (mats:3180) WL#5151Mats Kindahl10 Dec
      • Re: bzr commit into mysql-5.1 branch (mats:3180) WL#5151Alfranio Correia10 Dec
        • Re: bzr commit into mysql-5.1 branch (mats:3180) WL#5151Mats Kindahl10 Dec
  • Re: bzr commit into mysql-5.1 branch (mats:3180) WL#5151Luís Soares10 Dec
    • Re: bzr commit into mysql-5.1 branch (mats:3180) WL#5151Mats Kindahl10 Dec