maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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@xxxxxxxxxxx