maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Rucha!

On Sep 28, Rucha Deodhar wrote:
> revision-id: 4156071ebe2 (mariadb-10.6.1-91-g4156071ebe2)
> parent(s): 8dd4794c4e1
> author: Rucha Deodhar
> committer: Rucha Deodhar
> timestamp: 2021-09-26 19:58:33 +0530
> message:
> 
> MDEV-26606: ERROR_INDEX property value isn't passed from inside a stored
> procedure
> 
> Analysis: m_current_row_for_warning is reset to 1 during cleanup phase of
> stored procedure. When we perform a copy because some statement of procedure
> created warning, this reset value os passed to push _warning().
> Hence the output is always 1.
> Fix: Add a parameter in relevant functions to pass correct value of
> error index and don't use m_current_row_for_warning directly.

first thought about test cases, ERROR_INDEX should show the same row
number as in a warning, so let's check whether warning is correct or not
too. In this case we have

  CREATE OR REPLACE TABLE t1 (pk TINYINT PRIMARY KEY);
  CREATE OR REPLACE PROCEDURE sp(a INT) INSERT INTO t1 VALUES (2),(a);
  INSERT INTO t1 VALUES(1);
  --error ER_WARN_DATA_OUT_OF_RANGE
  CALL sp(100000);

prints

  ERROR 22003: Out of range value for column 'a' at row 2

which is fine. Would be good to have it in the test.

Otherwise looks good. One question below.

> diff --git a/sql/sql_class.cc b/sql/sql_class.cc
> index 31f5faa72ed..4d7093763ae 100644
> --- a/sql/sql_class.cc
> +++ b/sql/sql_class.cc
> @@ -1149,7 +1149,8 @@ Sql_condition* THD::raise_condition(uint sql_errno,
>    if (likely(!(is_fatal_error && (sql_errno == EE_OUTOFMEMORY ||
>                                    sql_errno == ER_OUTOFMEMORY))))
>    {
> -    cond= da->push_warning(this, sql_errno, sqlstate, level, ucid, msg);
> +    cond= da->push_warning(this, sql_errno, sqlstate, level, ucid, msg,
> +                           da->current_row_for_warning());
>    }
>    DBUG_RETURN(cond);
>  }
> diff --git a/sql/sql_error.cc b/sql/sql_error.cc
> index 7b6e6a83d2f..ca9da3dbf89 100644
> --- a/sql/sql_error.cc
> +++ b/sql/sql_error.cc
> @@ -663,7 +663,8 @@ void Warning_info::reserve_space(THD *thd, uint count)
>  
>  Sql_condition *Warning_info::push_warning(THD *thd,
>                                            const Sql_condition_identity *value,
> -                                          const char *msg)
> +                                          const char *msg,
> +                                          ulong current_error_index)
>  {
>    Sql_condition *cond= NULL;
>  
> @@ -673,7 +674,7 @@ Sql_condition *Warning_info::push_warning(THD *thd,
>          m_warn_list.elements() < thd->variables.max_error_count)
>      {
>        cond= new (& m_warn_root) Sql_condition(& m_warn_root, *value, msg,
> -                                              m_current_row_for_warning);
> +                                              current_error_index);
>        if (cond)
>          m_warn_list.push_back(cond);
>      }
> @@ -689,7 +690,8 @@ Sql_condition *Warning_info::push_warning(THD *thd,
>                                            const Sql_condition *sql_condition)
>  {
>    Sql_condition *new_condition= push_warning(thd, sql_condition,
> -                                             sql_condition->get_message_text());
> +                                             sql_condition->get_message_text(),
> +                                             sql_condition->m_error_index);
>  
>    if (new_condition)
>      new_condition->copy_opt_attributes(sql_condition);
> diff --git a/sql/sql_error.h b/sql/sql_error.h
> index b6ed7f5061a..a3e00e8419d 100644
> --- a/sql/sql_error.h
> +++ b/sql/sql_error.h
> @@ -747,7 +747,8 @@ class Warning_info
>    */
>    Sql_condition *push_warning(THD *thd,
>                                const Sql_condition_identity *identity,
> -                              const char* msg);
> +                              const char* msg,
> +                              ulong current_error_index);
>  
>    /**
>      Add a new SQL-condition to the current list and increment the respective
> @@ -1178,10 +1179,12 @@ class Diagnostics_area: public Sql_state_errno,
>                                const char* sqlstate,
>                                Sql_condition::enum_warning_level level,
>                                const Sql_user_condition_identity &ucid,
> -                              const char* msg)
> +                              const char* msg,
> +                              ulong current_error_index)
>    {
>      Sql_condition_identity tmp(sql_errno_arg, sqlstate, level, ucid);
> -    return get_warning_info()->push_warning(thd, &tmp, msg);
> +    return get_warning_info()->push_warning(thd, &tmp, msg,
> +                                            current_error_index);
>    }
>  
>    Sql_condition *push_warning(THD *thd,
> @@ -1191,7 +1194,7 @@ class Diagnostics_area: public Sql_state_errno,
>                                const char* msg)
>    {
>      return push_warning(thd, sqlerrno, sqlstate, level,
> -                        Sql_user_condition_identity(), msg);
> +                        Sql_user_condition_identity(), msg, 0);

what is this push_warning used for? does it never need a proper
current_error_index?

>    }
>    void mark_sql_conditions_for_removal()
>    { get_warning_info()->mark_sql_conditions_for_removal(); }
> 
Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx