Hi Sergei, Thanks for your suggestions that you gave in the first review. Please review a new patch for MDEV-7281, which addresses your suggestions.
diff --git a/mysql-test/r/create_drop_event.result b/mysql-test/ /create_drop_event.result
again, same thing. I don't see a test that the command actually worked. please use different event definitions in all three create variants. and add show create event after every create to verify that create if not exists did *not* modify the event, but create or replace did modify it.
Done.
--- a/sql/event_db_repository.cc +++ b/sql/event_db_repository.cc @@ -684,18 +684,28 @@ Event_db_repository::create_event(THD *thd, Event_parse_data *parse_data, DBUG_PRINT("info", ("check existance of an event with the same name")); if (!find_named_event(parse_data->dbname, parse_data->name, table)) { - if (create_if_not) + if (thd->lex->is_create_or_replace()) + { + if ((ret= table->file->ha_delete_row(table->record[0]))) + { + table->file->print_error(ret, MYF(0)); + goto end; + } + } + else if (create_if_not) { *event_already_exists= true; push_warning_printf(thd, Sql_condition::WARN_LEVEL_NOTE, ER_EVENT_ALREADY_EXISTS, ER(ER_EVENT_ALREADY_EXISTS), parse_data->name.str); ret= 0; + goto end; } else + { my_error(ER_EVENT_ALREADY_EXISTS, MYF(0), parse_data->name.str); - - goto end; + goto end; + } } else *event_already_exists= false;
I wonder whether this works as it should. On create-or-replace you delete the old event row and a new one is supposedly inserted. So, the table is fine.
But event_already_exists will stay unset, undefined, actually, because the caller didn't set it either. If you set it to TRUE, then the caller won't update in-memory even list, so your new event won't be enabled. If you set it to FALSE, it will try to insert a new event without deleting the old one and I don't know what will happen (either both will stay active or the insertion fails).
event_already_exists is now set to "false" on OR REPLACE. The in-memory list is updated in events.cc, in the method Events::create_event(...). This is the new code for this: if (thd->lex->create_info.or_replace() && event_queue) event_queue->drop_event(thd, parse_data->dbname, parse_data->name);
Do you have a test to verify that after CREATE OR REPLACE EVENT the old event no longer exists and the new event does exist and is executed as expected?
Added a test for this.
diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 922f0d7..648fd48 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -3995,9 +3997,8 @@ 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); + res= Events::create_event(thd, lex->event_parse_data, + lex->is_create_if_not_exists());
This doesn't seem to be particularly logical. You pass lex->is_create_if_not_exists)() here an argument, but you check thd->lex->is_create_or_replace() in the Event_db_repository::create_event anyway. I'd suggest not to pass lex->is_create_if_not_exists() as an argument, and check both in Event_db_repository::create_event().
The "if not exists" flag is not passed to Events::create_event() any more. Thanks.