From: Alexander Nozdrin
Date: June 9 2011 8:32am
Subject: Re: bzr commit into mysql-5.1 branch (Dmitry.Shulga:3662) Bug#57156
 Bug#11764334
List-Archive: http://lists.mysql.com/commits/138927
Message-Id: <4DF08528.9010608@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit

Hello,

thank you for the patch.

The patch is Ok to push after fixing minor issues and considering
a couple of suggestions.

On 04/19/11 16:01, Dmitry Shulga wrote:
> #At file:///Users/shulga/projects/mysql/mysql-5.1-bug11764334/ based on revid:sergey.glukhov@stripped
> 
>   3662 Dmitry Shulga	2011-04-19
>        Fixed bug#11764334 (formerly bug#57156): ALTER EVENT CHANGES
>        THE EVENT STATUS.
> 
>        The problem was that when ALTER EVENT statement being processed
>        a value of status field is saved uncoditionnaly even if it wasn't

s/uncoditionnaly/unconditionally/

>        specified in ALTER STATEMENT. As a consequence this field
>        is set to default value that corresponds to ENABLE value.
> 
>        The solution is to check if status field was specified in
>        ALTER EVENT statememt before make assignment of new value to

s/statememt/statement/

>        this field.
> === modified file 'sql/event_db_repository.cc'
> --- a/sql/event_db_repository.cc	2011-03-21 16:02:47 +0000
> +++ b/sql/event_db_repository.cc	2011-04-19 12:01:16 +0000
> @@ -226,9 +226,15 @@ mysql_event_fill_row(THD *thd,
>     if (fields[f_num= ET_FIELD_NAME]->store(et->name.str, et->name.length, scs))
>       goto err_truncate;
> 
> -  /* both ON_COMPLETION and STATUS are NOT NULL thus not calling set_notnull()*/
> +  /* ON_COMPLETION are NOT NULL thus not calling set_notnull()*/

set_notnull()*/ -> set_notnull(). */

> +  /*
> +    set STATUS value unconditionally in case of CREATE EVENT processing.
> +    For ALTER EVENT sets this field only if it was changed.

How about change the wording to be more consistent/clear:

For CREATE EVENT: the STATUS value is set unconditionally.
For ALTER EVENT: the STATUS value is set if it has been specified
in the statement.

> === modified file 'sql/event_parse_data.cc'
> --- a/sql/event_parse_data.cc	2009-02-13 16:41:47 +0000
> +++ b/sql/event_parse_data.cc	2011-04-19 12:01:16 +0000
> @@ -46,9 +46,8 @@ Event_parse_data::new_instance(THD *thd)
> 
>   Event_parse_data::Event_parse_data()
>     :on_completion(Event_parse_data::ON_COMPLETION_DEFAULT),
> -  status(Event_parse_data::ENABLED),
> -  do_not_create(FALSE),
> -  body_changed(FALSE),
> +  status(Event_parse_data::ENABLED), status_changed(false),
> +  do_not_create(FALSE), body_changed(FALSE),

I've never understood the idea of minimizing the number of lines
in the source code, but it's up to you.

>     item_starts(NULL), item_ends(NULL), item_execute_at(NULL),
>     starts_null(TRUE), ends_null(TRUE), execute_at_null(TRUE),
>     item_expression(NULL), expression(0)