Hello Andrei!
Here is my review for your patch:
* Andrei Elkin <aelkin@stripped> [07/09/14 19:59]:
> ChangeSet@stripped, 2007-09-14 17:01:53+03:00, aelkin@stripped +3 -0
> Bug #26199 Replication Failure on Slave when using stored procs with bit-type
> parameters.
>
> The value of the actual argument of BIT-type-arg stored procedure was binlogged as
> non-espaped sequence of
> bytes corresponding to internal representation of the bit value.
non-escaped :)
Also in future please keep lines in ChangeSet comment shorter than 80
characters to make it more convinient for e-mail quoting.
> The patch enforces binlogging of the bit-argument as a valid literal: prefixing the
> quoted bytes sequence with
> _binary.
I think it makes sense to mention here and below that this value is also
escaped if it is needed (and not only quoted).
...
> sql/sp_head.cc@stripped, 2007-09-14 17:01:43+03:00, aelkin@stripped +3 -2
> Treating BIT field type specially to for its value to be prefixed and quoted.
...
> diff -Nrup a/mysql-test/t/rpl_sp_effects.test b/mysql-test/t/rpl_sp_effects.test
> --- a/mysql-test/t/rpl_sp_effects.test 2005-09-07 18:39:41 +03:00
> +++ b/mysql-test/t/rpl_sp_effects.test 2007-09-14 17:01:43 +03:00
> @@ -195,9 +195,44 @@ sync_slave_with_master;
> connection slave;
> select 'slave', a from t1;
>
> +# bug#26199 Replication Failure on Slave when using stored procs with bit-type
> parameters
> +
> connection master;
> -drop table t1;
> +
> +create table t2 (b BIT(7));
> +delimiter //;
> +create procedure sp_bug26199(bitvalue BIT(7))
> +begin
> + insert into t2 set b = bitvalue;
> +end //
> +
> +create function sf_bug26199(b BIT(7)) returns int
> +begin
> + insert into t2 values(b);
> + return 0;
> +end//
I think it is better not to mix this test with cleanup section for the
previous test case. Also IMO it makes sense to stick to the style which
is used in other tests where we use bug-specific table/routine names and
try to drop objects before creating them (e.g. see sp.test).
> +
> +DELIMITER ;//
> +
> +
> +
> +call sp_bug26199(b'1110');
> +select sf_bug26199(b'1111111');
> +select sf_bug26199(b'101111111');
> +select hex(b) from t2;
How about testing value which will require escaping in addition
to the quoting (like '\0' or '\'') ?
> +
> +sync_slave_with_master;
> +#connection slave;
> +select hex(b) from t2;
> +
> +
> +
> +
> +connection master;
> +drop table t1,t2;
> drop function f1;
> drop function f2;
> drop procedure p1;
> +drop procedure sp_bug26199;
> +drop function sf_bug26199;
> sync_slave_with_master;
> diff -Nrup a/sql/sp_head.cc b/sql/sp_head.cc
> --- a/sql/sp_head.cc 2007-07-31 15:23:23 +03:00
> +++ b/sql/sp_head.cc 2007-09-14 17:01:43 +03:00
> @@ -100,8 +100,9 @@ sp_get_item_value(THD *thd, Item *item,
> case REAL_RESULT:
> case INT_RESULT:
> case DECIMAL_RESULT:
> - return item->val_str(str);
> -
> + if (item->field_type() != MYSQL_TYPE_BIT)
> + return item->val_str(str);
> + else {/* Bit type is handled as binary string */}
> case STRING_RESULT:
> {
> String *result= item->val_str(str);
Hmm... in future we might need more generic way for handling of such
'strange' types, but such solution is probably OK at the moment.
I think it is OK to push this patch after fixing/discussing minor
issues mentioned above.
--
Dmitri Lenev, Software Developer
MySQL AB, www.mysql.com
Are you MySQL certified? http://www.mysql.com/certification
