Leonard, hello.
> 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?
I think you really need to prove that non-default sql_mode is present
in binlog as it does for the CREATE table in the bug's description:
SET @@session.sql_mode=4/*!*/;
As to how to make that, I'd suggest
to start a fresh binlog file with reset master or flush logs;
to change sql_mode to `your_preference';
to CREATE one intance of each trigger, sp, sf;
to parse the binlog file e.g using a method deployed for
Bug #37313 BINLOG Contains Incorrect server id
in mysqlbinlog.test, or binlog_killed.test, you may find some
better.
to print out
SET @@session.sql_mode=`your_preference'
EOF
>
>>
>>> #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.
I did not doubt that!
A test case is often needs "cementing" one's
fixes that none could hurt them unless that one removes the test as well :-)
>
>>
>>
>>
>>> 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.
tnx
>>
>> Please provide your feedback, and i'll review your new patch asap.
>>
>> cheers,
>>
best wishes,
Andrei
