List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:January 17 2011 9:18am
Subject:Re: bzr commit into mysql-trunk-bugfixing branch (alexander.nozdrin:3421)
Bug#55847
View as plain text  
Hi Alexander,

On 12/11/10 6:17 PM, Alexander Nozdrin wrote:
> #At file:///home/alik/MySQL/bzr/00/bug55847/mysql-trunk-bugfixing-bug55847.2/ based
> on revid:luis.soares@stripped
>
>   3421 Alexander Nozdrin	2010-12-11
>        A patch for Bug#55847: SHOW WARNINGS returns empty result set
>        when SQLEXCEPTION is active.
>
>        The problem was in a hackish THD::no_warnings_for_error attribute.
>        When it was set, an error was not written to Warning_info -- only
>        Diagnostics_area state was changed.

What was the consequence? I mean, I agree this is hack, nonetheless, it 
would be nice to have an explanation of the specific problem.

>        The fix is to remove that hack.
>
>      modified:
>        mysql-test/r/warnings.result
>        mysql-test/t/warnings.test
>        sql/sql_admin.cc
>        sql/sql_class.cc
>        sql/sql_class.h
>        sql/sql_error.h
>        sql/sql_parse.cc
>        sql/sql_show.cc
>        sql/sql_trigger.cc

[..]

>
> === modified file 'sql/sql_admin.cc'
> --- a/sql/sql_admin.cc	2010-11-18 16:34:56 +0000
> +++ b/sql/sql_admin.cc	2010-12-11 20:17:36 +0000
> @@ -336,13 +336,36 @@ static bool mysql_admin_table(THD* thd,
>           so any errors opening the table are logical errors.
>           In these cases it makes sense to report them.
>         */
> -      if (!thd->locked_tables_mode)
> -        thd->no_warnings_for_error= no_warnings_for_error;
> +      Diagnostics_area da;
> +      Warning_info wi(thd->query_id);
> +      Diagnostics_area *da_saved= thd->stmt_da;
> +      Warning_info *wi_saved= thd->warning_info;

Don't we just need to save and restore warning_info?

> +      if (!thd->locked_tables_mode && no_warnings_for_error)
> +      {
> +        thd->stmt_da=&da;
> +        thd->  warning_info=&wi;

Space after "thd->".

> +      }
> +
>         if (view_operator_func == NULL)
>           table->required_type=FRMTYPE_TABLE;
>
>         open_error= open_and_lock_tables(thd, table, TRUE, 0);
> -      thd->no_warnings_for_error= 0;
> +
> +      if (!thd->locked_tables_mode&&  no_warnings_for_error)
> +      {
> +        thd->stmt_da= da_saved;
> +        thd->warning_info= wi_saved;
> +
> +        if (da.is_error())
> +        {
> +          thd->stmt_da->set_error_status(thd,
> +                                         da.sql_errno(),
> +                                         da.message(),
> +                                         da.get_sqlstate());

If we just saved warning_info, this wouldn't be needed.

> +        }
> +      }
> +
>         table->next_global= save_next_global;
>         table->next_local= save_next_local;
>         thd->open_options&= ~extra_open_options;
>
> === modified file 'sql/sql_class.cc'
> --- a/sql/sql_class.cc	2010-11-18 16:34:56 +0000
> +++ b/sql/sql_class.cc	2010-12-11 20:17:36 +0000
> @@ -584,7 +584,7 @@ THD::THD()
>     client_capabilities= 0;                       // minimalistic client
>     ull=0;
>     system_thread= NON_SYSTEM_THREAD;
> -  cleanup_done= abort_on_warning= no_warnings_for_error= 0;
> +  cleanup_done= abort_on_warning= 0;
>     peer_port= 0;					// For SHOW PROCESSLIST
>     transaction.m_pending_rows_event= 0;
>     transaction.on= 1;
> @@ -857,10 +857,6 @@ MYSQL_ERROR* THD::raise_condition(uint s
>
>     query_cache_abort(&query_cache_tls);
>
> -  /* FIXME: broken special case */
> -  if (no_warnings_for_error&&  (level == MYSQL_ERROR::WARN_LEVEL_ERROR))
> -    DBUG_RETURN(NULL);
> -
>     /* When simulating OOM, skip writing to error log to avoid mtr errors */
>     DBUG_EXECUTE_IF("simulate_out_of_memory", DBUG_RETURN(NULL););
>
>
> === modified file 'sql/sql_class.h'
> --- a/sql/sql_class.h	2010-11-29 16:27:58 +0000
> +++ b/sql/sql_class.h	2010-12-11 20:17:36 +0000
> @@ -2118,7 +2118,6 @@ public:
>     bool       enable_slow_log;   /* enable slow log for current statement */
>     bool	     abort_on_warning;
>     bool 	     got_warning;       /* Set on call to push_warning() */
> -  bool	     no_warnings_for_error; /* no warnings on call to my_error() */
>     /* set during loop of derived table processing */
>     bool       derived_tables_processing;
>     my_bool    tablespace_op;	/* This is TRUE in DISCARD/IMPORT TABLESPACE */
>
> === modified file 'sql/sql_error.h'
> --- a/sql/sql_error.h	2010-11-18 16:34:56 +0000
> +++ b/sql/sql_error.h	2010-12-11 20:17:36 +0000
> @@ -398,6 +398,12 @@ public:
>       }
>     }
>
> +  void remove_warning(const MYSQL_ERROR *warning)
> +  {
> +    m_warn_count[warning->get_level()]--;
> +    m_statement_warn_count--;
> +  }
> +
>     /**
>       Conditional merge of related warning information areas.
>     */
>
> === modified file 'sql/sql_parse.cc'
> --- a/sql/sql_parse.cc	2010-11-29 11:28:55 +0000
> +++ b/sql/sql_parse.cc	2010-12-11 20:17:36 +0000
> @@ -7267,6 +7267,7 @@ bool parse_sql(THD *thd,
>     /* Check that if MYSQLparse() failed, thd->is_error() is set. */
>
>     DBUG_ASSERT(!mysql_parse_status ||
> +              (mysql_parse_status&&  thd->get_internal_handler()) ||

Why was the assert changed? Please write per-file comments.

>                 (mysql_parse_status&&  thd->is_error()));
>
>     /* Reset parser state. */
>
> === modified file 'sql/sql_show.cc'
> --- a/sql/sql_show.cc	2010-12-06 13:12:51 +0000
> +++ b/sql/sql_show.cc	2010-12-11 20:17:36 +0000
> @@ -3403,6 +3403,27 @@ end:
>   }
>
>
> +class Trigger_error_handler : public Internal_error_handler
> +{
> +public:
> +  bool handle_condition(THD *thd,
> +                        uint sql_errno,
> +                        const char* sqlstate,
> +                        MYSQL_ERROR::enum_warning_level level,
> +                        const char* msg,
> +                        MYSQL_ERROR ** cond_hdl)
> +  {
> +    if (sql_errno == ER_PARSE_ERROR ||
> +        sql_errno == ER_TRG_NO_DEFINER ||
> +        sql_errno == ER_TRG_NO_CREATION_CTX)
> +      return true;
> +
> +    return false;
> +  }
> +};
> +
> +
> +
>   /**
>     @brief          Fill I_S tables whose data are retrieved
>                     from frm files and storage engine
> @@ -3552,7 +3573,6 @@ int get_all_tables(THD *thd, TABLE_LIST
>           acl_get(sctx->host, sctx->ip, sctx->priv_user, db_name->str,
> 0))
>   #endif
>       {
> -      thd->no_warnings_for_error= 1;
>         List<LEX_STRING>  table_names;
>         int res= make_table_name_list(thd,&table_names, lex,
>                                       &lookup_field_vals,
> @@ -3601,9 +3621,16 @@ int get_all_tables(THD *thd, TABLE_LIST
>               if (!(table_open_method&  ~OPEN_FRM_ONLY)&&
>                   !with_i_schema)
>               {
> -              if (!fill_schema_table_from_frm(thd, tables, schema_table, db_name,
> -                                              table_name, schema_table_idx,
> -                                              can_deadlock))
> +              Trigger_error_handler err_handler;
> +              thd->push_internal_handler(&err_handler);
> +
> +              int res= fill_schema_table_from_frm(thd, tables, schema_table,
> db_name,
> +                                                  table_name, schema_table_idx,
> +                                                  can_deadlock);

I don't understand this change.

Now it's safe to generate warning messages, except for those that are 
handled? What happens if one of these errors is raised but are ignored? 
It seems they weren't being exactly ignored before.

> +              thd->pop_internal_handler();

Please add a test case for this change.

> +              if (!res)
>                   continue;
>               }
>
> @@ -3613,7 +3640,6 @@ int get_all_tables(THD *thd, TABLE_LIST
>                 Set the parent lex of 'sel' because it is needed by
>                 sel.init_query() which is called inside make_table_list.
>               */
> -            thd->no_warnings_for_error= 1;
>               sel.parent_lex= lex;
>               /* db_name can be changed in make_table_list() func */
>               if (!thd->make_lex_string(&orig_db_name, db_name->str,
> @@ -6676,7 +6702,6 @@ bool get_schema_tables_result(JOIN *join
>     bool result= 0;
>     DBUG_ENTER("get_schema_tables_result");
>
> -  thd->no_warnings_for_error= 1;
>     for (JOIN_TAB *tab= join->join_tab; tab<  tmp_join_tab; tab++)
>     {
>       if (!tab->table || !tab->table->pos_in_table_list)
> @@ -6727,8 +6752,58 @@ bool get_schema_tables_result(JOIN *join
>         else
>           table_list->table->file->stats.records= 0;
>
> -      if (table_list->schema_table->fill_table(thd, table_list,
> -                                               tab->select_cond))
> +      bool res;
> +
> +      {

Move this block to a function.

> +        Diagnostics_area da;
> +        Diagnostics_area *da_saved= thd->stmt_da;
> +        Warning_info wi(thd->query_id);
> +        Warning_info *wi_saved= thd->warning_info;
> +
> +        thd->stmt_da=&da;
> +        thd->warning_info=&wi;
> +
> +        res= table_list->schema_table->fill_table(thd, table_list,
> +          tab->select_cond);
> +
> +        thd->stmt_da= da_saved;
> +        thd->warning_info= wi_saved;
> +
> +        // Pass an error if any.
> +
> +        if (da.is_error())
> +        {
> +          thd->warning_info->push_warning(thd,
> +                                          da.sql_errno(),
> +                                          da.get_sqlstate(),
> +                                          MYSQL_ERROR::WARN_LEVEL_ERROR,
> +                                          da.message());
> +
> +          thd->stmt_da->set_error_status(thd,
> +                                         da.sql_errno(),
> +                                         da.message(),
> +                                         da.get_sqlstate());

If we pass the error status, there is no need to save the diagnostics area.

> +        }
> +
> +        // Pass warnings (if any).
> +
> +        List_iterator<MYSQL_ERROR>  it(wi.warn_list());
> +        while (true)
> +        {
> +          MYSQL_ERROR *err = it++;
> +
> +          if (!err)
> +            break;
> +
> +          if (err->get_level() != MYSQL_ERROR::WARN_LEVEL_ERROR)
> +            continue;
> +
> +          wi.remove_warning(err);
> +          it.remove();
> +        }

You could make this into a visitor pattern...

> +        thd->warning_info->append_warnings(thd,&wi.warn_list());

which could be passed through append_warnings.

Regards,

Davi Arnaut
Thread
bzr commit into mysql-trunk-bugfixing branch (alexander.nozdrin:3421)Bug#55847Alexander Nozdrin11 Dec
  • Re: bzr commit into mysql-trunk-bugfixing branch (alexander.nozdrin:3421)Bug#55847Davi Arnaut17 Jan
    • Re: bzr commit into mysql-trunk-bugfixing branch (alexander.nozdrin:3421)Bug#55847Alexander Nozdrin19 Jan