Hi Alfranio,
Thanks for your good comments. I have updated the code according to your
most comments. Please review the newest patch. Thanks!
Best Regards,
Daogang
> Hi Dao-Gang,
>
> Make sure that you run MTR before pushing the patch.
> There are some test cases that are failing: See one more
> comment in-line.
>
> Cheers,
>
> Alfranio Correia wrote:
>
>> Hi Dao-Gang,
>>
>> Nice work.
>>
>> Find some comments in-line.
>>
>> Cheers.
>>
>> Dao-Gang.Qu@stripped wrote:
>>
>>
>>> #At file:///home/daogangq/mysql/bzrwork/bug44331/5.1-bugteam/ based on
> revid:joro@stripped
>>>
>>> 2994 Dao-Gang.Qu@stripped 2009-07-10
>>> Bug #44331 Restore of database with events produces warning in
> replication
>>>
>>>
>>>
>> I think the description of the problem is wrong. And assuming that
>> I did not understand the problem the description of the solution does
>> not seem to fix the problem ;).
>>
>>
Right. corrected it.
>>>
>>> The real problem is that If the user create EVENT without DEFINER
> clause set explicitly,
>>> the CURRENT_USER is set to DEFINER in current thread. The CURRENT_USER
> is mysqld startup
>>> user on master, but the CURRENT_USER is empty on slave for replication.
> So the definer
>>> is not coincident between master and slave.
>>>
>>>
>>>
>>>
>>>
>> The description below is fine, although there are some English typos.
>>
>>
>>> For fixing the problem, the CURRENT_USER will be written into the
> binary log as
>>> default definer for replication on master, If the user create EVENT
> without
>>> DEFINER clause set explicitly.
>>>
>>>
>>>
>>
>>
>>> @ mysql-test/suite/rpl/r/rpl_restore_event_warning.result
>>> Add test result for bug#44331
>>> @ mysql-test/suite/rpl/t/rpl_restore_event_warning.test
>>> Add test file for bug#44331
>>> @ sql/events.cc
>>> The "create_query_string(...)" is added for generating the CREATE
> query string
>>> from the thd information.
>>> Added a new parameter "definer_exist" to 'Events::create_event(...)'
> function for
>>> handling the case that the user create EVENT without DEFINER clause
> set explicitly.
>>> the CURRENT_USER will be written into the binary log as default
> definer for replication.
>>> @ sql/events.h
>>> Added a new parameter "definer_exist" with default actual parameter
> to
>>> 'Events::create_event(...)' function declaration.
>>> @ sql/sql_parse.cc
>>> Add a new local variable "definer_exist" to
> "mysql_execute_command(THD *thd)" function
>>> to indicate if the definer has been specified by user.
>>>
>>> added:
>>> mysql-test/suite/rpl/r/rpl_restore_event_warning.result
>>> mysql-test/suite/rpl/t/rpl_restore_event_warning.test
>>> modified:
>>> sql/events.cc
>>> sql/events.h
>>> sql/sql_parse.cc
>>> === added file 'mysql-test/suite/rpl/r/rpl_restore_event_warning.result'
>>> --- a/mysql-test/suite/rpl/r/rpl_restore_event_warning.result 1970-01-01
> 00:00:00 +0000
>>> +++ b/mysql-test/suite/rpl/r/rpl_restore_event_warning.result 2009-07-10
> 08:47:20 +0000
>>> @@ -0,0 +1,19 @@
>>> +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;
>>> +drop database if exists mysqltest;
>>> +CREATE DATABASE mysqltest;
>>> +CREATE TABLE mysqltest.t1(details CHAR(30));
>>> +CREATE EVENT mysqltest.e1 ON SCHEDULE AT CURRENT_TIMESTAMP
>>> +ON COMPLETION PRESERVE DISABLE
>>> +DO INSERT INTO mysqltest.t1 VALUES('event e1 fired');
>>> +select EVENT_SCHEMA, EVENT_NAME, DEFINER from information_schema.events;
>>> +EVENT_SCHEMA EVENT_NAME DEFINER
>>> +mysqltest e1 root@localhost
>>> +select EVENT_SCHEMA, EVENT_NAME, DEFINER from information_schema.events;
>>> +EVENT_SCHEMA EVENT_NAME DEFINER
>>> +mysqltest e1 root@localhost
>>> +drop database mysqltest;
>>>
>>>
>>>
>>>
>> This is a small test can you merge it with other test?
>> Maybe: ./suite/rpl/t/rpl_events.test
>>
>>
Right. I have merged it into rpl_events.test.
>>> === added file 'mysql-test/suite/rpl/t/rpl_restore_event_warning.test'
>>> --- a/mysql-test/suite/rpl/t/rpl_restore_event_warning.test 1970-01-01
> 00:00:00 +0000
>>> +++ b/mysql-test/suite/rpl/t/rpl_restore_event_warning.test 2009-07-10
> 08:47:20 +0000
>>> @@ -0,0 +1,28 @@
>>> +#
>>> +# BUG#44331
>>> +# This test verifies if the default definer is coincident between master and
> slave,
>>> +# when the user create EVENT without DEFINER clause set explicitly.
>>> +#
>>> +
>>> +source include/master-slave.inc;
>>> +--disable_warnings
>>> +drop database if exists mysqltest;
>>> +--enable_warnings
>>> +
>>> +connection master;
>>> +CREATE DATABASE mysqltest;
>>> +CREATE TABLE mysqltest.t1(details CHAR(30));
>>> +CREATE EVENT mysqltest.e1 ON SCHEDULE AT CURRENT_TIMESTAMP
>>> + ON COMPLETION PRESERVE DISABLE
>>> + DO INSERT INTO mysqltest.t1 VALUES('event e1 fired');
>>> +select EVENT_SCHEMA, EVENT_NAME, DEFINER from information_schema.events;
>>> +
>>> +sync_slave_with_master;
>>> +connection slave;
>>> +select EVENT_SCHEMA, EVENT_NAME, DEFINER from information_schema.events;
>>> +
>>> +connection master;
>>> +drop database mysqltest;
>>> +save_master_pos;
>>> +connection slave;
>>> +sync_with_master;
>>>
>>>
>>>
>>
>>
>>> === modified file 'sql/events.cc'
>>> --- a/sql/events.cc 2009-04-09 06:22:06 +0000
>>> +++ b/sql/events.cc 2009-07-10 08:47:20 +0000
>>> @@ -340,6 +340,41 @@ common_1_lev_code:
>>> return 0;
>>> }
>>>
>>> +/**
>>> + Generates the CREATE query string from the thd information.
>>> + Set the specified definer to the default value, which is the
>>> + current user in the thread.
>>> +
>>> + @param[in] thd thread handler
>>> + @param[in] buf query string
>>> + @return
>>> + Returns TRUE on success, FALSE on (alloc) failure.
>>> +*/
>>> +static bool
>>> +create_query_string(THD *thd, String *buf)
>>> +{
>>> + int userlen= strlen(thd->lex->definer->user.str);
>>> + int hostlen= strlen(thd->lex->definer->host.str);
>>> + char *indexofspace= strstr(thd->query, " ");
>>> +
>>> + /* Make some room to begin with */
>>> + if (buf->alloc(100 + userlen + hostlen + thd->query_length +
>>> + 15 /* length of " DEFINER=''@'' "*/))
>>>
>>>
>>>
>> ./sql/sql_show.cc:1614:void append_definer(THD *thd, String *buffer ...
>>
>> Use the function above, it seems to be default.
>>
>>
Yes. it's better.
>>> + return FALSE;
>>> + /* Append the "CREATE" part of thd->query */
>>> + buf->append(thd->query, indexofspace - thd->query);
>>> + buf->append(STRING_WITH_LEN(" DEFINER='"));
>>> + /* Append the definer user */
>>> + buf->append(thd->lex->definer->user.str);
>>> + buf->append(STRING_WITH_LEN("'@'"));
>>> + /* Append the definer host*/
>>> + buf->append(thd->lex->definer->host.str);
>>> + buf->append(STRING_WITH_LEN("'"));
>>> + /* Append the left part of thd->query */
>>> + buf->append(indexofspace);
>>> +
>>> + return TRUE;
>>> +}
>>>
>>> /**
>>> Create a new event.
>>> @@ -359,7 +394,7 @@ common_1_lev_code:
>>>
>>> bool
>>> Events::create_event(THD *thd, Event_parse_data *parse_data,
>>> - bool if_not_exists)
>>> + bool if_not_exists, bool definer_exist)
>>> {
>>> int ret;
>>> DBUG_ENTER("Events::create_event");
>>> @@ -439,7 +474,15 @@ Events::create_event(THD *thd, Event_par
>>> {
>>> /* Binlog the create event. */
>>> DBUG_ASSERT(thd->query && thd->query_length);
>>> - write_bin_log(thd, TRUE, thd->query, thd->query_length);
>>> +
>>> + String log_query;
>>> + /* if the definer does not exist, CURRENT_USER will be written
>>> + into the binary log for replication */
>>> + if (!definer_exist && create_query_string(thd,
> &log_query))
>>> + write_bin_log(thd, TRUE, log_query.c_ptr(), log_query.length());
>>> + else
>>> + write_bin_log(thd, TRUE, thd->query, thd->query_length);
>>> +
>>> }
>>> }
>>> pthread_mutex_unlock(&LOCK_event_metadata);
>>>
>>> === modified file 'sql/events.h'
>>> --- a/sql/events.h 2007-08-15 15:08:44 +0000
>>> +++ b/sql/events.h 2009-07-10 08:47:20 +0000
>>> @@ -107,7 +107,7 @@ public:
>>> switch_event_scheduler_state(enum enum_opt_event_scheduler new_state);
>>>
>>> static bool
>>> - create_event(THD *thd, Event_parse_data *parse_data, bool if_exists);
>>>
>>>
>>>
>> Please, don't use default values.
>>
>>
Yes. updated it.
>>> + create_event(THD *thd, Event_parse_data *parse_data, bool if_exists, bool
> definer_exist= TRUE);
>>>
>>>
>>> static bool
>>> update_event(THD *thd, Event_parse_data *parse_data,
>>>
>>> === modified file 'sql/sql_parse.cc'
>>> --- a/sql/sql_parse.cc 2009-07-01 12:32:04 +0000
>>> +++ b/sql/sql_parse.cc 2009-07-10 08:47:20 +0000
>>>
>>>
>>>
>> If for some reason the request above is hard to address,
>> you can keep the signature and access the lex->definer through the THD
>> in the Events::create.
>>
>>
the lex->definer can't carry the mode of the definer is set. So we have to
add a new parameter for the "create_event" function.
>>> @@ -3707,6 +3707,10 @@ end_with_restore_list:
>>> "function calls as part of this statement");
>>> break;
>>> }
>>> +
>>> + bool definer_exist= TRUE;
>>> + if (!lex->definer)
>>> + definer_exist= FALSE;
>>>
>>> res= sp_process_definer(thd);
>>> if (res)
>>> @@ -3715,9 +3719,10 @@ end_with_restore_list:
>>> switch (lex->sql_command) {
>>> case SQLCOM_CREATE_EVENT:
>>> {
>>> +
>>> bool if_not_exists= (lex->create_info.options &
>>> HA_LEX_CREATE_IF_NOT_EXISTS);
>>> - res= Events::create_event(thd, lex->event_parse_data,
> if_not_exists);
>>>
>>>
> Maybe it would be better to replace definer_exist by if_definer_exists.
>
Yes. I think definer_set_mode is better. what do you think?
>>> + res= Events::create_event(thd, lex->event_parse_data,
> if_not_exists, definer_exist);
>>>
>>>
>
>
>>> break;
>>> }
>>> case SQLCOM_ALTER_EVENT:
>>>
>>>
>>> ------------------------------------------------------------------------
>>>
>>>
>>>
>>>
>>>
>>
>>
>
>
