maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Holyfoot!

On Sep 24, holyfoot@xxxxxxxxxxxx wrote:
> revision-id: 1d92131613770b505fa462b33c1091e00d9475be (mariadb-10.1.7-61-g1d92131)
> parent(s): e5418942609833edb681d16c4e2705f8c338bfee
> committer: Alexey Botchkov
> timestamp: 2015-09-24 14:31:06 +0500
> message:
> 
> MDEV-4829 BEFORE INSERT triggers dont issue 1406 error.
> Fixed as it's done in MySQL 5.7.
> The Strict_error_handler introduced to intercept such states and is activated
> in the sp_head::execute_trigger()

Hmm...
This is, basically, a new implementation of the strict mode.
But enabled only for triggers, which is kind of weird.
And potentially very fragile and inconsistent, as it's a strict mode
we're talking about, it's confusing and not very consistent on itself.
Having two different implementations of it doesn't help to make it clear
or consistent.

If you're introducing a new implementation - use it consistently
everywhere. If that won't change existing intentional behavior.
And please talk to Bar about his recent experiences in this area.

There were also few comments about the code, see below.

> diff --git a/sql/sp_head.cc b/sql/sp_head.cc
> index a117963..ff6d2b4 100644
> --- a/sql/sp_head.cc
> +++ b/sql/sp_head.cc
> @@ -54,6 +54,35 @@
>  
>  #include <my_user.h>
>  
> +/**
> +   A simple RAII wrapper around Strict_error_handler.
> +*/
> +class Strict_error_handler_wrapper
> +{
> +  THD *m_thd;
> +  Strict_error_handler m_strict_handler;
> +  bool m_active;
> +
> +public:
> +  Strict_error_handler_wrapper(THD *thd, sp_head *sp_head)
> +    : m_thd(thd), m_active(false)
> +  {
> +    if (sp_head->m_sql_mode & (MODE_STRICT_ALL_TABLES |
> +                               MODE_STRICT_TRANS_TABLES))
> +    {
> +      m_thd->push_internal_handler(&m_strict_handler);
> +      m_active= true;
> +    }
> +  }
> +
> +  ~Strict_error_handler_wrapper()
> +  {
> +    if (m_active)
> +      m_thd->pop_internal_handler();
> +  }
> +};

Oh, really? A wrapper per handler? Make it a template, please, so that
it could be used for any error handler. Like this:

  Error_handler_wraper<Strict_error_handler> strict_error_handler(thd, this);

Also, it would be good if you do a second commit using this wrapper in
other places, where appropriate.

> @@ -1576,6 +1605,9 @@ sp_head::execute_trigger(THD *thd,
>    DBUG_ENTER("sp_head::execute_trigger");
>    DBUG_PRINT("info", ("trigger %s", m_name.str));
>  
> +  // Push Strict_error_handler if the SP was created in STRICT mode.
> +  Strict_error_handler_wrapper strict_wrapper(thd, this);
> +
>  #ifndef NO_EMBEDDED_ACCESS_CHECKS
>    Security_context *save_ctx= NULL;
>  
> diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> index 9162969..ca0699c 100644
> --- a/sql/sql_base.cc
> +++ b/sql/sql_base.cc
> @@ -96,6 +96,74 @@ bool No_such_table_error_handler::safely_trapped_errors()
>  
>  
>  /**
> +  Implementation of STRICT mode.
> +  Upgrades a set of given conditions from warning to error.
> +*/
> +bool Strict_error_handler::handle_condition_ext(THD *thd,
> +       uint sql_errno,
> +       const char *sqlstate,
> +       Sql_condition::enum_warning_level *level,
> +       const char *msg,
> +       Sql_condition **cond_hdl)
> +{
> +  /* STRICT MODE should affect only the below statements */

Really?

> +  switch (thd->lex->sql_command)
> +  {
> +  case SQLCOM_CREATE_TABLE:
> +  case SQLCOM_DROP_INDEX:
> +  case SQLCOM_INSERT:
> +  case SQLCOM_REPLACE:
> +  case SQLCOM_REPLACE_SELECT:
> +  case SQLCOM_INSERT_SELECT:
> +  case SQLCOM_UPDATE:
> +  case SQLCOM_UPDATE_MULTI:
> +  case SQLCOM_DELETE:
> +  case SQLCOM_DELETE_MULTI:
> +  case SQLCOM_ALTER_TABLE:
> +  case SQLCOM_LOAD:
> +  case SQLCOM_CALL:
> +  case SQLCOM_END:
> +  case SQLCOM_SET_OPTION:
> +  case SQLCOM_SELECT:
> +    break;
> +  default:
> +    return false;
> +  }
> +
> +  switch (sql_errno)
> +  {
> +  case ER_TRUNCATED_WRONG_VALUE:
> +  case ER_WRONG_VALUE_FOR_TYPE:
> +  case ER_WARN_DATA_OUT_OF_RANGE:
> +  case ER_DIVISION_BY_ZERO:
> +  case ER_TRUNCATED_WRONG_VALUE_FOR_FIELD:
> +  case WARN_DATA_TRUNCATED:
> +  case ER_DATA_TOO_LONG:
> +  case ER_BAD_NULL_ERROR:
> +  case ER_NO_DEFAULT_FOR_FIELD:
> +  case ER_TOO_LONG_KEY:
> +  case ER_WRONG_ARGUMENTS:
> +  case ER_NO_DEFAULT_FOR_VIEW_FIELD:
> +  case ER_WARN_NULL_TO_NOTNULL:
> +  case ER_CUT_VALUE_GROUP_CONCAT:
> +  case ER_DATETIME_FUNCTION_OVERFLOW:
> +  case ER_WARN_TOO_FEW_RECORDS:
> +    if ((*level == Sql_condition::WARN_LEVEL_WARN) &&
> +        thd->transaction.stmt.m_unsafe_rollback_flags == 0 ||
> +        (thd->variables.sql_mode & MODE_STRICT_ALL_TABLES))
> +    {
> +      (*level)= Sql_condition::WARN_LEVEL_ERROR;
> +      thd->killed= KILL_BAD_DATA;
> +    }
> +    break;
> +  default:
> +    break;
> +  }
> +  return false;
> +}
> +
> +
> +/**
>    This internal handler is used to trap ER_NO_SUCH_TABLE and
>    ER_WRONG_MRG_TABLE errors during CHECK/REPAIR TABLE for MERGE
>    tables.
> diff --git a/sql/sql_base.h b/sql/sql_base.h
> index 1c0029d..c5a76c5 100644
> --- a/sql/sql_base.h
> +++ b/sql/sql_base.h
> @@ -676,5 +676,30 @@ class No_such_table_error_handler : public Internal_error_handler
>    int m_unhandled_errors;
>  };
>  
> +/**
> +  This internal handler implements upgrade from SL_WARNING to SL_ERROR

"SL_ERROR"? "SL_WARNING"?

> +  for the error codes affected by STRICT mode. Currently STRICT mode does
> +  not affect SELECT statements.
> +*/
> +
> +class Strict_error_handler : public Internal_error_handler
> +{
> +protected:
> +  bool handle_condition(THD *thd,
> +                        uint sql_errno,
> +                        const char* sqlstate,
> +                        Sql_condition::enum_warning_level level,
> +                        const char* msg,
> +                        Sql_condition ** cond_hdl)
> +  { DBUG_ASSERT(FALSE); return 0; }
> +public:
> +  bool handle_condition_ext(THD *thd,
> +                            uint sql_errno,
> +                            const char* sqlstate,
> +                            Sql_condition::enum_warning_level *level,
> +                            const char* msg,
> +                            Sql_condition ** cond_hdl);

1. why not to change handle_condition() ?
2. strict mode may change the sql_errno, handle_condition_ext doesn't
   support it.

> +};
> +
>  
>  #endif /* SQL_BASE_INCLUDED */

Regards,
Sergei