Hi, Alexey! In general the patch is pretty good. But I've had few comments, mostly about bad choice of error messages. It's a historical behavoir that you decided not to change. If you'd prefer to leave it as is, that's ok, but then, please, create a new MDEV about these error messages. And add test cases :) On Mar 20, Alexey Botchkov wrote:
revision-id: 29e99a701817da56499df9126830d9533e4d66f3 (mariadb-10.1.12-26-g29e99a7) parent(s): 7cb16dc2a369058b31df1b3999989f6beff94d07 committer: Alexey Botchkov timestamp: 2016-03-20 23:57:26 +0400 message:
MDEV-717 LP:1003679 - Wrong binlog order on concurrent DROP schema and CREATE function. The cause of the issue is when DROP DATABASE takes metadata lock and is in progress through it's execution, a concurrently running CREATE FUNCTION checks for the existence of database which it succeeds and then it waits on the metadata lock. Once DROP DATABASE writes to BINLOG and finally releases the metadata lock on schema object, the CREATE FUNCTION waiting on metadata lock gets in it's code path and succeeds and writes to binlog.
Please structure the commit comment as <first line - the subject> <empty line> <the rest - the body> That's the convention and various tools, for example, github itself, use it to parse and display the commit subject and body.
--- sql/events.cc | 29 ++++++++++++++------ sql/sp.cc | 81 +++++++++++++++++++++++++++++++++++++------------------- sql/sp.h | 2 +- sql/sql_parse.cc | 56 +++++++--------------------------------
What, no test cases at all? Please, add tests for everything you've fixed: - create event - update event with moving to a new database
diff --git a/sql/sp.cc b/sql/sp.cc index 6ec5914..11ba186 100644 --- a/sql/sp.cc +++ b/sql/sp.cc @@ -1040,7 +1045,22 @@ sp_create_routine(THD *thd, stored_procedure_type type, sp_head *sp)
/* Grab an exclusive MDL lock. */ if (lock_object_name(thd, mdl_type, sp->m_db.str, sp->m_name.str)) - DBUG_RETURN(SP_OPEN_TABLE_FAILED); + { + my_error(ER_SP_STORE_FAILED, MYF(0), SP_TYPE_STRING(type), sp->m_name.str);
No better error message here? May be ER_BAD_DB_ERROR? I understand that the old behavior is ER_SP_STORE_FAILED here.
+ DBUG_RETURN(TRUE); + } + + /* + Check that a database directory with this name + exists. Design note: This won't work on virtual databases + like information_schema. + */ + if (check_db_dir_existence(sp->m_db.str)) + { + my_error(ER_BAD_DB_ERROR, MYF(0), sp->m_db.str); + DBUG_RETURN(TRUE); + } +
/* Reset sql_mode during data dictionary operations. */ thd->variables.sql_mode= 0; @@ -1049,7 +1069,9 @@ sp_create_routine(THD *thd, stored_procedure_type type, sp_head *sp) thd->count_cuted_fields= CHECK_FIELD_WARN;
if (!(table= open_proc_table_for_update(thd))) - ret= SP_OPEN_TABLE_FAILED; + { + my_error(ER_SP_STORE_FAILED, MYF(0), SP_TYPE_STRING(type),sp->m_name.str);
better error message, may be?
+ } else { /* Checking if the routine already exists */ @@ -1268,7 +1289,8 @@ sp_create_routine(THD *thd, stored_procedure_type type, sp_head *sp) &(thd->lex->definer->host), saved_mode)) { - ret= SP_INTERNAL_ERROR; + my_error(ER_SP_STORE_FAILED, MYF(0), + SP_TYPE_STRING(type), sp->m_name.str);
That's OOM, can be a better error message too. Btw, could you reverse the return value of show_create_sp() please?
goto done; } /* restore sql_mode when binloging */ @@ -1277,9 +1299,14 @@ sp_create_routine(THD *thd, stored_procedure_type type, sp_head *sp) if (thd->binlog_query(THD::STMT_QUERY_TYPE, log_query.ptr(), log_query.length(), FALSE, FALSE, FALSE, 0)) - ret= SP_INTERNAL_ERROR; + { + my_error(ER_SP_STORE_FAILED, MYF(0), + SP_TYPE_STRING(type), sp->m_name.str);
that's very confusing. "Failed to CREATE %s %s" when the creation has actually succeeded. The same applies to the OOM error above.
+ goto done; + } thd->variables.sql_mode= 0; } + ret= FALSE;
Regards, Sergei Chief Architect MariaDB and security@mariadb.org