Hi Chuck!
I've looked at the patch, and I think there are a few things we need to do in
order to avoid code duplication. See my comments below.
Just my few cents,
Mats Kindahl
On Tuesday 31 July 2007 23.20.48 cbell@stripped wrote:
> Below is the list of changes that have just been committed into a local
> 5.1 repository of cbell. When cbell does a push these changes will
> be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>
> ChangeSet@stripped, 2007-07-31 17:20:17-04:00, cbell@mysql_cab_desk. +8 -0
> BUG#22086 : Extra Slave Col: Char(5) on slave and Char(10) on master
> cause mysqld crash
>
> This patch adds functionality to row-based replication to ensure the
> slave's column sizes are >= to that of the master.
>
> mysql-test/extra/rpl_tests/rpl_extraSlave_Col.test@stripped, 2007-07-31
> 17:19:32-04:00, cbell@mysql_cab_desk. +41 -32 BUG#22086 : Extra Slave Col:
> Char(5) on slave and Char(10) on master cause mysqld crash
>
> Removed commented out portion of test referenced in bug report. This
> test supports the original request of the bug report.
>
> mysql-test/include/test_fieldsize.inc@stripped, 2007-07-31 17:19:36-04:00,
> cbell@mysql_cab_desk. +48 -0 BUG#22086 : Extra Slave Col: Char(5) on slave
> and Char(10) on master cause mysqld crash
>
> Sub unit file to test each variable type that relies on field
> metadata from the master.
>
> mysql-test/include/test_fieldsize.inc@stripped, 2007-07-31 17:19:36-04:00,
> cbell@mysql_cab_desk. +0 -0
>
> mysql-test/suite/rpl/r/rpl_extraCol_innodb.result@stripped, 2007-07-31
> 17:19:33-04:00, cbell@mysql_cab_desk. +67 -0 BUG#22086 : Extra Slave Col:
> Char(5) on slave and Char(10) on master cause mysqld crash
>
> New result file for additional test.
>
> mysql-test/suite/rpl/r/rpl_extraCol_myisam.result@stripped, 2007-07-31
> 17:19:33-04:00, cbell@mysql_cab_desk. +67 -0 BUG#22086 : Extra Slave Col:
> Char(5) on slave and Char(10) on master cause mysqld crash
>
> New result file for additional test.
>
> mysql-test/suite/rpl/r/rpl_row_colSize.result@stripped, 2007-07-31
> 17:19:35-04:00, cbell@mysql_cab_desk. +726 -0 BUG#22086 : Extra Slave Col:
> Char(5) on slave and Char(10) on master cause mysqld crash
>
> New result file for additional test.
>
> mysql-test/suite/rpl/r/rpl_row_colSize.result@stripped, 2007-07-31
> 17:19:35-04:00, cbell@mysql_cab_desk. +0 -0
>
> mysql-test/suite/rpl/t/rpl_row_colSize.test@stripped, 2007-07-31
> 17:19:36-04:00, cbell@mysql_cab_desk. +150 -0 BUG#22086 : Extra Slave Col:
> Char(5) on slave and Char(10) on master cause mysqld crash
>
> Added a test file to test each variable type that relies on field
> metadata from the master.
>
> mysql-test/suite/rpl/t/rpl_row_colSize.test@stripped, 2007-07-31
> 17:19:36-04:00, cbell@mysql_cab_desk. +0 -0
>
> mysql-test/suite/rpl_ndb/r/rpl_ndb_extraCol.result@stripped, 2007-07-31
> 17:19:34-04:00, cbell@mysql_cab_desk. +67 -0 BUG#22086 : Extra Slave Col:
> Char(5) on slave and Char(10) on master cause mysqld crash
>
> New result file for additional test.
>
> sql/rpl_utility.cc@stripped, 2007-07-31 17:19:34-04:00, cbell@mysql_cab_desk.
> +102 -0 BUG#22086 : Extra Slave Col: Char(5) on slave and Char(10) on
> master cause mysqld crash
>
> This patch adds a method to check the size of the field on the master
> using the field metadata from WL#3228. Each column is checked to ensure
> the slave's column is >= to the master's column in size.
[snip]
> diff -Nrup a/mysql-test/include/test_fieldsize.inc
> b/mysql-test/include/test_fieldsize.inc --- /dev/null Wed Dec 31 16:00:00
> 196900
> +++ b/mysql-test/include/test_fieldsize.inc 2007-07-31 17:19:36 -04:00
> @@ -0,0 +1,48 @@
> +#
> +# include/test_fieldsize.inc
> +#
> +# This include file is designed to create a table with one column
> +# whose size on the master is greater than that on the slave. The
> +# test should fail with an error on the slave.
> +#
> +
> +--echo Test_fieldsize BEGIN
> +connection master;
> +--echo Dropping table t1
The statements are visible in the result file, there is no need to echo extra
information.
It *is* however a good idea to add a unique line for each section, since that
helps diff3 to generate good diffs.
> +DROP TABLE IF EXISTS t1;
> +
> +sync_slave_with_master;
> +STOP SLAVE;
> +RESET SLAVE;
> +--echo Creating table on slave
Dito
> +eval $test_table_slave;
> +
> +connection master;
> +--echo Creating table on master
Here too.
> +eval $test_table_master;
> +RESET MASTER;
> +
> +--echo Inserting a row on master
> +eval $test_insert;
Same here.
> +
> +connection slave;
> +--echo Restarting slave. Should see error 1532 here.
> +START SLAVE;
> +wait_for_slave_to_stop;
> +--replace_result $MASTER_MYPORT MASTER_PORT
> +--replace_column 1 # 4 # 7 # 8 # 9 # 22 # 23 # 33 #
> +--query_vertical SHOW SLAVE STATUS
> +
> +# The following should be 0
> +--echo Checking row count on slave. Should be 0.
> +SELECT COUNT(*) FROM t1;
> +STOP SLAVE;
> +RESET SLAVE;
> +
> +connection master;
> +RESET MASTER;
> +
> +connection slave;
> +START SLAVE;
> +--echo Test_fieldsize END
[snip]
> diff -Nrup a/mysql-test/suite/rpl/t/rpl_row_colSize.test
> b/mysql-test/suite/rpl/t/rpl_row_colSize.test --- /dev/null Wed Dec 31
> 16:00:00 196900
> +++ b/mysql-test/suite/rpl/t/rpl_row_colSize.test 2007-07-31 17:19:36
> -04:00 @@ -0,0 +1,150 @@
> +##################################################################
> +# rpl_colSize #
> +# #
> +# This test is designed to test the changes included in WL#3228. #
> +# The changes include the ability to replicate with the master #
> +# having columns that are smaller (shorter) than the slave. #
> +##################################################################
> +
> +-- source include/master-slave.inc
> +-- source include/have_binlog_format_row.inc
> +
> +--disable_warnings
> +DROP TABLE IF EXISTS t1;
> +--enable_warnings
> +
> +
> +--echo **** Testing WL#3228 changes. ****
> +--echo *** Create "wider" table on slave ***
> +sync_slave_with_master;
> +
> +#
> +# Check each column type to verify error 1532 fires (BUG#22086)
> +# This check covers only those fields that require additional
> +# metadata from the master to be replicated to the slave. These
> +# field types are:
> +# MYSQL_TYPE_NEWDECIMAL:
> +# MYSQL_TYPE_FLOAT:
> +# MYSQL_TYPE_BIT:
> +# MYSQL_TYPE_SET:
> +# MYSQL_TYPE_STRING:
> +# MYSQL_TYPE_ENUM:
> +# MYSQL_TYPE_VARCHAR:
> +# MYSQL_TYPE_BLOB:
> +
> +#
> +# Test: Checking MYSQL_TYPE_NEWDECIMAL fields
> +#
> +let $test_table_master = CREATE TABLE t1 (a DECIMAL(20, 10));
> +let $test_table_slave = CREATE TABLE t1 (a DECIMAL(5,2));
> +let $test_insert = INSERT INTO t1 VALUES (901251.90125);
> +--source include/test_fieldsize.inc
> +
> +let $test_table_master = CREATE TABLE t1 (a NUMERIC(20, 10));
> +let $test_table_slave = CREATE TABLE t1 (a NUMERIC(5,2));
> +let $test_insert = INSERT INTO t1 VALUES (901251.90125);
> +--source include/test_fieldsize.inc
> +
> +#
> +# Test: Checking MYSQL_TYPE_FLOAT fields
> +#
> +let $test_table_master = CREATE TABLE t1 (a FLOAT(47));
> +let $test_table_slave = CREATE TABLE t1 (a FLOAT(20));
> +let $test_insert = INSERT INTO t1 VALUES (901251.90125);
> +--source include/test_fieldsize.inc
> +
> +#
> +# Test: Checking MYSQL_TYPE_BIT fields
> +#
> +let $test_table_master = CREATE TABLE t1 (a BIT(64));
> +let $test_table_slave = CREATE TABLE t1 (a BIT(5));
> +let $test_insert = INSERT INTO t1 VALUES (B'10101');
> +--source include/test_fieldsize.inc
> +
> +#
> +# Test: Checking MYSQL_TYPE_SET fields
> +#
> +let $test_table_master = CREATE TABLE t1 (a
> SET('1','2','3','4','5','6','7','8','9')); +let $test_table_slave = CREATE
> TABLE t1 (a SET('4'));
> +let $test_insert = INSERT INTO t1 VALUES ('4');
> +--source include/test_fieldsize.inc
use the form "source include/test_fieldsize.inc;" since that avoid problems in
the case of misspellings.
> +
> +#
> +# Test: Checking MYSQL_TYPE_STRING fields
> +#
> +let $test_table_master = CREATE TABLE t1 (a CHAR(20));
> +let $test_table_slave = CREATE TABLE t1 (a CHAR(10));
> +let $test_insert = INSERT INTO t1 VALUES ('This is a test.');
> +--source include/test_fieldsize.inc
here too.
> +
> +#
> +# Test: Checking MYSQL_TYPE_ENUM fields
> +#
> +let $test_table_master = CREATE TABLE t1 (a ENUM(
> + '01','02','03','04','05','06','07','08','09',
> + '11','12','13','14','15','16','17','18','19',
> + '21','22','23','24','25','26','27','28','29',
> + '31','32','33','34','35','36','37','38','39',
> + '41','42','43','44','45','46','47','48','49',
> + '51','52','53','54','55','56','57','58','59',
> + '61','62','63','64','65','66','67','68','69',
> + '71','72','73','74','75','76','77','78','79',
> + '81','82','83','84','85','86','87','88','89',
> + '91','92','93','94','95','96','97','98','99',
> + '101','102','103','104','105','106','107','108','109',
> + '111','112','113','114','115','116','117','118','119',
> + '121','122','123','124','125','126','127','128','129',
> + '131','132','133','134','135','136','137','138','139',
> + '141','142','143','144','145','146','147','148','149',
> + '151','152','153','154','155','156','157','158','159',
> + '161','162','163','164','165','166','167','168','169',
> + '171','172','173','174','175','176','177','178','179',
> + '181','182','183','184','185','186','187','188','189',
> + '191','192','193','194','195','196','197','198','199',
> + '201','202','203','204','205','206','207','208','209',
> + '211','212','213','214','215','216','217','218','219',
> + '221','222','223','224','225','226','227','228','229',
> + '231','232','233','234','235','236','237','238','239',
> + '241','242','243','244','245','246','247','248','249',
> + '251','252','253','254','255','256','257','258','259',
> + '261','262','263','264','265','266','267','268','269',
> + '271','272','273','274','275','276','277','278','279',
> + '281','282','283','284','285','286','287','288','289',
> + '291','292','293','294','295','296','297','298','299'
> + ));
> +let $test_table_slave = CREATE TABLE t1 (a ENUM('44','54'));
> +let $test_insert = INSERT INTO t1 VALUES ('44');
> +--source include/test_fieldsize.inc
here too.
> +
> +#
> +# Test: Checking MYSQL_TYPE_VARCHAR fields
> +#
> +let $test_table_master = CREATE TABLE t1 (a VARCHAR(2000));
> +let $test_table_slave = CREATE TABLE t1 (a VARCHAR(100));
> +let $test_insert = INSERT INTO t1 VALUES ('This is a test.');
> +--source include/test_fieldsize.inc
> +
> +let $test_table_master = CREATE TABLE t1 (a VARCHAR(200));
> +let $test_table_slave = CREATE TABLE t1 (a VARCHAR(10));
> +let $test_insert = INSERT INTO t1 VALUES ('This is a test.');
> +--source include/test_fieldsize.inc
> +
> +let $test_table_master = CREATE TABLE t1 (a VARCHAR(2000));
> +let $test_table_slave = CREATE TABLE t1 (a VARCHAR(1000));
> +let $test_insert = INSERT INTO t1 VALUES ('This is a test.');
> +--source include/test_fieldsize.inc
> +
> +#
> +# Test: Checking MYSQL_TYPE_BLOB fields
> +#
> +let $test_table_master = CREATE TABLE t1 (a LONGBLOB);
> +let $test_table_slave = CREATE TABLE t1 (a TINYBLOB);
> +let $test_insert = INSERT INTO t1 VALUES ('This is a test.');
> +--source include/test_fieldsize.inc
> +
> +--echo *** Cleanup ***
> +connection master;
> +DROP TABLE IF EXISTS t1;
> +sync_slave_with_master;
> +# END 5.1 Test Case
> +
[snip]
> diff -Nrup a/sql/rpl_utility.cc b/sql/rpl_utility.cc
> --- a/sql/rpl_utility.cc 2007-07-30 17:39:49 -04:00
> +++ b/sql/rpl_utility.cc 2007-07-31 17:19:34 -04:00
> @@ -124,6 +124,102 @@ uint32 table_def::calc_field_size(uint c
> return length;
> }
>
> +/**
> + Compare slave field size to that of the master. Error if slave < master.
What does it mean that slave < master? Suggest to use a more elaborate
wording.
> +
> + This method gets the field metadata as pertaining to how the field
> + is stored in the raw data from the master. This can be used to determine
> + if the sizes are compatible in either max size (length) or if the
> + packed size is compatible.
> +
> + @param type Field type of the master
> + @param col The field number (column) being checked
> + @param table The TABLE variable
> + @param master_size The value from the field metadata in the table map
> + @param tsh The table share (used for reporting)
> + @param rli Relay log information (used for reporting)
> +
> + @retval 0 No error: Slave col size >= master col size
> + @retval 1 Error: Slave col size < master col size
> +*/
> +int check_slave_field_size(uint32 type, int col, TABLE *table,
> + uint master_size, TABLE_SHARE const *tsh,
> + RELAY_LOG_INFO const *rli)
> +{
> + uint slave_value= 0;
> + uint master_value= master_size;
Call it master_metadata and slave_metadata?
There are quite a few parameter passed, and they have a set of constraints on
themselves that you need to check. The code assumes that:
- That table is non-null
- That type == table->field[col]->type()
- That table->s == tsh
- That table->field[col] is non-null and references valid memory
It is usually prudent to check the constraints on the input parameters with
assertions, since users of the function easily can make mistakes that
introduce subtle errors.
An alternative is to remove the type parameter and the col parameter, and
instead pass the table (since you need it to get access to the table share)
and the field (since you need to access it for the field data), and compute
everything else inside the function. That follows the maxim that "the
precondition of a function should be as weak as possible, but not weaker".
Also, when writing functions to be as efficient as possible: try to pass
frequently used parameters before less frequently used parameters since that
usually puts the frequently used parameters in registers, and also in correct
registers for further processing when calling functions internally.
> + int error= 0;
> +
> + /*
> + Return 0 if there is no metadata from the master.
> + */
> + if (!master_size)
> + return error;
> +
> + switch (type) {
Although this is correct it's starting to become quite a few switches for
checking the field types.
If we had a virtual Field::field_metadata() function, we could do something
like this:
if (master_value < slave_field->field_metadata())
{
...
}
You could move the code that computes the metadata into virtual functions (as
we have discussed previously) but since you introduced code in
WL#3228/WL#3915 to transform the fields of a table to field metadata and
added it to Table_map_log_event, I suggest that you use the following
approach to avoid code duplication:
Table_map_log_event tmap(thd, table, /* dummy */ 0, /* dummy */ 0);
tmap.save_field_metadata();
.
.
.
if (master_value < tmap.get_field_metadata(col))
{
...
}
That will remove the switch entirely, and will in addition avoid code
duplication, which is always a maintenance risk. It appears that the
get_field_metadata() function is mentioned in comments, but does not exist,
but it is easy to add that function to Table_map_log_event.
An alternative is, of course, to factor out the code in save_field_metadata()
into virtual functions in the Field hierarchy, but in that case, make sure to
have a non-virtual public function that calls a (pure) virtual private
function, since it might be interesting to introduce caching to speed up use
of this piece of data.
> + case MYSQL_TYPE_NEWDECIMAL:
> + {
> + uint master_precision= (master_size >> 8U) & 0x00ff;
> + uint master_decimal= master_size & 0x00ff;
Use const qualifier on these since you do not intend to change them. It's a
good habit. It doesn't make the code more efficient, but it makes it easier
to avoid programming mistakes.
> + slave_value= table->field[col]->pack_length();
> + master_value= my_decimal_get_binary_size(master_precision,
> master_decimal); + break;
> + }
> + case MYSQL_TYPE_DOUBLE:
> + case MYSQL_TYPE_FLOAT:
> + slave_value= table->field[col]->pack_length();
> + break;
> + case MYSQL_TYPE_BIT:
> + {
> + slave_value= ((Field_bit *)table->field[col])->bytes_in_rec +
> + ((((Field_bit *)table->field[col])->bit_len > 0) ? 1 :
> 0); + uint from_len= (master_size >> 8U) & 0x00ff;
> + uint from_bit_len= master_size & 0x00ff;
> + master_value= from_len + ((from_bit_len > 0) ? 1 : 0);
> + break;
> + }
> + case MYSQL_TYPE_SET:
> + case MYSQL_TYPE_STRING:
> + case MYSQL_TYPE_ENUM:
> + {
> + if (((master_size & 0xff00) == (MYSQL_TYPE_SET << 8)) ||
> + ((master_size & 0xff00) == (MYSQL_TYPE_ENUM << 8)))
> + master_value= master_size & 0x00ff;
> + else
> + master_value= master_size & 0x00ff;
> + if ((table->field[col]->real_type() == MYSQL_TYPE_SET) ||
> + (table->field[col]->real_type() == MYSQL_TYPE_ENUM))
> + slave_value= table->field[col]->pack_length();
> + else
> + slave_value= table->field[col]->field_length + 1;
> + break;
> + }
> + case MYSQL_TYPE_VARCHAR:
> + slave_value= table->field[col]->field_length;
> + break;
> + case MYSQL_TYPE_TINY_BLOB:
> + case MYSQL_TYPE_MEDIUM_BLOB:
> + case MYSQL_TYPE_LONG_BLOB:
> + case MYSQL_TYPE_BLOB:
> + slave_value= ((Field_blob *)table->field[col])->pack_length_no_ptr();
> + break;
> + default:
> + slave_value= master_value;
> + }
> + if (slave_value < master_value)
> + {
> + error= 1;
> + char buf[256];
> + my_snprintf(buf, sizeof(buf), "Column %d size mismatch - "
> + "master has size %d, %s.%s on slave has size %d",
Also give information on what the constraint is, i.e., that the slave's width
has to be greater or equal to the master's width.
> + col, master_value, tsh->db.str,
> + tsh->table_name.str, slave_value);
> + rli->report(ERROR_LEVEL, ER_BINLOG_ROW_WRONG_TABLE_DEF,
> + ER(ER_BINLOG_ROW_WRONG_TABLE_DEF), buf);
> + }
> + return (error);
> +}
> +
> /*
> Is the definition compatible with a table?
>
> @@ -156,6 +252,12 @@ table_def::compatible_with(RELAY_LOG_INF
> rli->report(ERROR_LEVEL, ER_BINLOG_ROW_WRONG_TABLE_DEF,
> ER(ER_BINLOG_ROW_WRONG_TABLE_DEF), buf);
> }
> + /*
> + Check the slave's field size against that of the master.
> + */
> + if (!error)
> + error= check_slave_field_size(table->field[col]->type(), col, table,
> + m_field_metadata[col], tsh, rli);
> }
>
> return error;
