Hi Leonard,
Patch seems good. Find below a few suggestions (nothing really show
stopper).
Regards,
Luís
On Tue, 2009-03-17 at 19:23 +0800, Leonard Zhou wrote:
> #At file:///home/zhl/mysql/rep/5.0/bug35515/
>
> 2714 Leonard Zhou 2009-03-17
> BUG#35515 Aliases of variables in binary log are ignored with NAME_CONST.
>
> When add an aliase name after NAME_CONST, the aliase name will be ignored.
>
> Our solution is that set the column to aliase name if the column name is not
> autogenerated.
> That means if we have an aliase name after NAME_CONST, we will set aliase
> name.
[SUGGESTION]
I find this message a bit confusing. Please, try to rewrite it in a more
clear way.
> added:
> mysql-test/r/rpl_name_const.result
> mysql-test/t/rpl_name_const.test
> modified:
> sql/item.cc
>
> per-file messages:
> mysql-test/r/rpl_name_const.result
> Test result.
> mysql-test/t/rpl_name_const.test
> Test case.
> sql/item.cc
> Don't set item name if the name is autogenerated, that mean without aliase.
> === added file 'mysql-test/r/rpl_name_const.result'
> --- a/mysql-test/r/rpl_name_const.result 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/r/rpl_name_const.result 2009-03-17 11:21:29 +0000
> @@ -0,0 +1,32 @@
> +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;
> +==== Initialize ====
> +[on master]
> +drop database if exists testdb;
> +create database testdb;
> +use testdb;
> +create table table1 (id int);
> +==== create a procedure that has a column alias in a subquery ====
> +drop procedure if exists test_procedure;
> +create procedure test_procedure(_id int)
> +begin
> +insert into table1 (id)
> +select a.id
> +from
> +( select _id as id ) a;
> +end;$$
> +==== enable the binary log, then call the procedure ====
> +call test_procedure(1234);
> +[on slave]
> +use testdb;
> +select * from table1 order by id;
> +id
> +1234
> +==== Clean up ====
> +[on master]
> +drop table table1;
> +drop database testdb;
>
> === added file 'mysql-test/t/rpl_name_const.test'
> --- a/mysql-test/t/rpl_name_const.test 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/t/rpl_name_const.test 2009-03-17 11:21:29 +0000
> @@ -0,0 +1,54 @@
> +# ==== Purpose ====
> +#
> +# Test that aliases of variables in binary log aren't ignored with NAME_CONST.
> +#
> +# ==== Method ====
> +#
> +# Create a procedure with aliases of variables, then replicate it to slave.
> +# BUG#35515 Aliases of variables in binary log are ignored with NAME_CONST.
> +#
> +
> +source include/master-slave.inc;
> +
> +--echo ==== Initialize ====
> +
> +--echo [on master]
> +--connection master
> +
> +--disable_warnings
> +drop database if exists testdb;
> +--enable_warnings
> +create database testdb;
> +use testdb;
[SUGGESTION]
I can see that the test is based on the "how to repeat" section from the
bug report. Nonetheless, I believe that there is nothing in the test
case preventing from using the default test database (meaning, there is
no need to create and use a separate 'testdb' for this, just use the
default one).
> +create table table1 (id int);
[SUGGESTION]
Although there is nothing wrong with the table1 name, I would suggest to
follow the convention and use 't1'.
Reference:
http://dev.mysql.com/doc/mysqltest/en/writing-tests-naming-conventions.html
> +
> +--echo ==== create a procedure that has a column alias in a subquery ====
> +--disable_warnings
> +drop procedure if exists test_procedure;
> +--enable_warnings
> +delimiter $$;
> +create procedure test_procedure(_id int)
> +begin
> +insert into table1 (id)
> +select a.id
> +from
> +( select _id as id ) a;
> +end;$$
> +delimiter ;$$
> +
> +--echo ==== enable the binary log, then call the procedure ====
> +call test_procedure(1234);
> +
> +
> +--echo [on slave]
> +sync_slave_with_master;
> +use testdb;
> +select * from table1 order by id;
> +
> +--echo ==== Clean up ====
> +
> +--echo [on master]
> +connection master;
> +drop table table1;
> +drop database testdb;
[SUGGESTION]
If you go for the default database, then you should probably replace the
'drop database testdb' with 'drop procedure test_procedure'.
> +
>
> === modified file 'sql/item.cc'
> --- a/sql/item.cc 2009-02-20 09:42:35 +0000
> +++ b/sql/item.cc 2009-03-17 11:21:29 +0000
> @@ -1282,7 +1282,10 @@ bool Item_name_const::fix_fields(THD *th
> my_error(ER_RESERVED_SYNTAX, MYF(0), "NAME_CONST");
> return TRUE;
> }
> - set_name(item_name->ptr(), (uint) item_name->length(),
> system_charset_info);
> + if (is_autogenerated_name)
> + {
> + set_name(item_name->ptr(), (uint) item_name->length(),
> system_charset_info);
> + }
> collation.set(value_item->collation.collation, DERIVATION_IMPLICIT);
> max_length= value_item->max_length;
> decimals= value_item->decimals;
>
>
--
Luís
