Hi Andrei,
Thanks for your review.
I've put comments inline below.
Andrei Elkin ??:
> Leonard, hello.
>
> The patch is good.
> I have some comments.
>
> Very important is to have test cases covering all situations the bug
> is possible. I would suggest suite/rpl/t/rpl_sp.test as the tests
> placeholder or a like.
It's difficult to test an inter-process variable.
What i did to check correct 'sql_mode' is checking binlog file and check
debug log file.
Do you have a good idea to do that?
>
>> #At file:///home/zhl/mysql/rep/5.1/bug39526/
>>
>> 2793 Leonard Zhou 2009-02-12
>> Bug#39526 Correct sql_mode in binary log for CREATE PROCEDURE
>>
>> sql_mode can't correctly write into binary log when creating a
>> procedure.
>
> I would say the sql_mode was just missed out. Could you please
> formulate circumstances when it's so. In my view these are:
> non-default sql_mode and one of CREATE trigger, stored function or
> stored procedure.
>
> That would be wonderful if check also if other CREATEable objects are
> vulnerable (e.g EVENT) while you are within the current context.
I have checked the source code for CREATE FUNCTION/EVENT/TRIGGERS. Seems
sql_mode is set in right value before binloging.
>
>
>
>>
>> restore sql_mode before write events into log file when
>> creating a procedure.
>
> Ok.
>
> Still I would comment that more specific. E.g restoring the current
> session sql_mode right before generating the binlog event.
>
>
>> modified:
>> sql/sp.cc
>>
>> per-file messages:
>> sql/sp.cc
>> restore sql_mode before write events into log file
>> === modified file 'sql/sp.cc'
>> --- a/sql/sp.cc 2008-07-15 01:43:12 +0000
>> +++ b/sql/sp.cc 2009-02-12 10:00:21 +0000
>> @@ -937,6 +937,7 @@ sp_create_routine(THD *thd, int type, sp
>> goto done;
>> }
>>
>> + thd->variables.sql_mode= saved_mode;
>> /* Such a statement can always go directly to binlog, no trans cache */
>> thd->binlog_query(THD::MYSQL_QUERY_TYPE,
>> log_query.c_ptr(), log_query.length(),
>> FALSE, FALSE);
>
> How about to return the mode back to the temporary value
>
> + thd->variables.sql_mode= 0;
>
> in the same block which would look parenthesis-like?
>
> In the end there will be always
>
> thd->variables.sql_mode= saved_mode;
>
> but we don't know future: if some more code will be added right
> after `else if (mysql_bin_log.is_open()) {...}' lines that code
> would assume thd->variables.sql_mode == 0 (overlooking your currently
> correct restore).
OK. I will do this change.
>
> Please provide your feedback, and i'll review your new patch asap.
>
> cheers,
>
> Andrei
--
Leonard (Li) Zhou, Software Engineer, MySQL Replication
Sun Microsystems, www.sun.com
Office: 86-10-65054020 Ext: 299
