maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexey!

On Mar 20, Alexey Botchkov wrote:
> > This looks too complex to me. Why would you extend the base
> > Internal_error_handler class - no other error handler did that or needed
> > that, fwiw.

Here I tried to come up with what I think is a simpler approach. It
basically uses a buffer in the THD, but just like the old code, this
buffer is in the Diagnostic_area, it's the last error message.

There is also one problem with thd->clear_error() that is used
everywhere in sql_show.cc. I did not fix it, only added a comment about
it. Eventually we will need to remove thd->clear_error() completely, it is
conceptually incompatible with the pluggable auditing.

See the attached patch. 

Regards,
Sergei

=== modified file 'mysql-test/suite/plugins/r/sql_error_log.result'
--- mysql-test/suite/plugins/r/sql_error_log.result	2014-01-23 18:21:02 +0000
+++ mysql-test/suite/plugins/r/sql_error_log.result	2014-04-01 22:27:07 +0000
@@ -33,6 +33,8 @@ insert into t1 values ('aa');
 ERROR 22007: Incorrect integer value: 'aa' for column 'id' at row 1
 SET SQL_MODE = '';
 drop table t1;
+SELECT TABLE_NAME FROM information_schema.TABLES WHERE TABLE_SCHEMA = 'not_exists' AND TABLE_NAME = 'not_exists';
+TABLE_NAME
 uninstall plugin SQL_ERROR_LOG;
 Warnings:
 Warning	1620	Plugin is busy and will be uninstalled on shutdown

=== modified file 'mysql-test/suite/plugins/t/sql_error_log.test'
--- mysql-test/suite/plugins/t/sql_error_log.test	2014-01-24 02:07:22 +0000
+++ mysql-test/suite/plugins/t/sql_error_log.test	2014-04-01 22:26:10 +0000
@@ -46,6 +46,8 @@ insert into t1 values ('aa');
 SET SQL_MODE = '';
 drop table t1;
 
+SELECT TABLE_NAME FROM information_schema.TABLES WHERE TABLE_SCHEMA = 'not_exists' AND TABLE_NAME = 'not_exists';
+
 uninstall plugin SQL_ERROR_LOG;
 
 let $MYSQLD_DATADIR= `SELECT @@datadir`;

=== modified file 'sql/sql_class.cc'
--- sql/sql_class.cc	2014-03-26 18:56:23 +0000
+++ sql/sql_class.cc	2014-04-01 22:29:01 +0000
@@ -1145,7 +1145,6 @@ MYSQL_ERROR* THD::raise_condition(uint s
     got_warning= 1;
     break;
   case MYSQL_ERROR::WARN_LEVEL_ERROR:
-    mysql_audit_general(this, MYSQL_AUDIT_GENERAL_ERROR, sql_errno, msg);
     break;
   default:
     DBUG_ASSERT(FALSE);
@@ -1156,6 +1155,8 @@ MYSQL_ERROR* THD::raise_condition(uint s
 
   if (level == MYSQL_ERROR::WARN_LEVEL_ERROR)
   {
+    mysql_audit_general(this, MYSQL_AUDIT_GENERAL_ERROR, sql_errno, msg);
+
     is_slave_error=  1; // needed to catch query errors during replication
 
     if (! stmt_da->is_error())

=== modified file 'sql/sql_class.h'
--- sql/sql_class.h	2014-03-27 21:26:58 +0000
+++ sql/sql_class.h	2014-04-01 12:58:56 +0000
@@ -2608,8 +2608,11 @@ class THD :public Statement,
     Clear the current error, if any.
     We do not clear is_fatal_error or is_fatal_sub_stmt_error since we
     assume this is never called if the fatal error is set.
+
     @todo: To silence an error, one should use Internal_error_handler
-    mechanism. In future this function will be removed.
+    mechanism. Issuing an error that can be possibly later "cleared" is not
+    compatible with other installed error handlers and audit plugins.
+    In future this function will be removed.
   */
   inline void clear_error()
   {

=== modified file 'sql/sql_show.cc'
--- sql/sql_show.cc	2014-03-17 12:04:28 +0000
+++ sql/sql_show.cc	2014-04-01 14:52:39 +0000
@@ -4287,25 +4287,7 @@ static int fill_schema_table_from_frm(TH
 }
 
 
-/**
-  Trigger_error_handler is intended to intercept and silence SQL conditions
-  that might happen during trigger loading for SHOW statements.
-  The potential SQL conditions are:
-
-    - ER_PARSE_ERROR -- this error is thrown if a trigger definition file
-      is damaged or contains invalid CREATE TRIGGER statement. That should
-      not happen in normal life.
-
-    - ER_TRG_NO_DEFINER -- this warning is thrown when we're loading a
-      trigger created/imported in/from the version of MySQL, which does not
-      support trigger definers.
-
-    - ER_TRG_NO_CREATION_CTX -- this warning is thrown when we're loading a
-      trigger created/imported in/from the version of MySQL, which does not
-      support trigger creation contexts.
-*/
-
-class Trigger_error_handler : public Internal_error_handler
+class Warnings_only_error_handler : public Internal_error_handler
 {
 public:
   bool handle_condition(THD *thd,
@@ -4320,12 +4302,16 @@ class Trigger_error_handler : public Int
         sql_errno == ER_TRG_NO_CREATION_CTX)
       return true;
 
-    return false;
+    if (level != MYSQL_ERROR::WARN_LEVEL_ERROR)
+      return false;
+
+    if (!thd->stmt_da->is_error())
+      thd->stmt_da->set_error_status(thd, sql_errno, msg, sqlstate);
+    return true; // handled!
   }
 };
 
 
-
 /**
   @brief          Fill I_S tables whose data are retrieved
                   from frm files and storage engine
@@ -4523,25 +4509,11 @@ int get_all_tables(THD *thd, TABLE_LIST
             if (!(table_open_method & ~OPEN_FRM_ONLY) &&
                 !with_i_schema)
             {
-              /*
-                Here we need to filter out warnings, which can happen
-                during loading of triggers in fill_schema_table_from_frm(),
-                because we don't need those warnings to pollute output of
-                SELECT from I_S / SHOW-statements.
-              */
-
-              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,
-                                                  &open_tables_state_backup,
-                                                  can_deadlock);
-
-              thd->pop_internal_handler();
-
-              if (!res)
+              if (!fill_schema_table_from_frm(thd, tables, schema_table,
+                                              db_name, table_name,
+                                              schema_table_idx,
+                                              &open_tables_state_backup,
+                                              can_deadlock))
                 continue;
             }
 
@@ -7666,92 +7638,6 @@ int make_schema_select(THD *thd, SELECT_
 }
 
 
-/**
-  Fill INFORMATION_SCHEMA-table, leave correct Diagnostics_area /
-  Warning_info state after itself.
-
-  This function is a wrapper around ST_SCHEMA_TABLE::fill_table(), which
-  may "partially silence" some errors. The thing is that during
-  fill_table() many errors might be emitted. These errors stem from the
-  nature of fill_table().
-
-  For example, SELECT ... FROM INFORMATION_SCHEMA.xxx WHERE TABLE_NAME = 'xxx'
-  results in a number of 'Table <db name>.xxx does not exist' errors,
-  because fill_table() tries to open the 'xxx' table in every possible
-  database.
-
-  Those errors are cleared (the error status is cleared from
-  Diagnostics_area) inside fill_table(), but they remain in Warning_info
-  (Warning_info is not cleared because it may contain useful warnings).
-
-  This function is responsible for making sure that Warning_info does not
-  contain warnings corresponding to the cleared errors.
-
-  @note: THD::no_warnings_for_error used to be set before calling
-  fill_table(), thus those errors didn't go to Warning_info. This is not
-  the case now (THD::no_warnings_for_error was eliminated as a hack), so we
-  need to take care of those warnings here.
-
-  @param thd            Thread context.
-  @param table_list     I_S table.
-  @param join_table     JOIN/SELECT table.
-
-  @return Error status.
-  @retval TRUE Error.
-  @retval FALSE Success.
-*/
-static bool do_fill_table(THD *thd,
-                          TABLE_LIST *table_list,
-                          JOIN_TAB *join_table)
-{
-  // NOTE: fill_table() may generate many "useless" warnings, which will be
-  // ignored afterwards. On the other hand, there might be "useful"
-  // warnings, which should be presented to the user. Warning_info usually
-  // stores no more than THD::variables.max_error_count warnings.
-  // The problem is that "useless warnings" may occupy all the slots in the
-  // Warning_info, so "useful warnings" get rejected. In order to avoid
-  // that problem we create a Warning_info instance, which is capable of
-  // storing "unlimited" number of warnings.
-  Warning_info wi(thd->query_id, true);
-  Warning_info *wi_saved= thd->warning_info;
-
-  thd->warning_info= &wi;
-
-  bool res= table_list->schema_table->fill_table(
-    thd, table_list, join_table->select_cond);
-
-  thd->warning_info= wi_saved;
-
-  // Pass an error if any.
-
-  if (thd->stmt_da->is_error())
-  {
-    thd->warning_info->push_warning(thd,
-                                    thd->stmt_da->sql_errno(),
-                                    thd->stmt_da->get_sqlstate(),
-                                    MYSQL_ERROR::WARN_LEVEL_ERROR,
-                                    thd->stmt_da->message());
-  }
-
-  // Pass warnings (if any).
-  //
-  // Filter out warnings with WARN_LEVEL_ERROR level, because they
-  // correspond to the errors which were filtered out in fill_table().
-
-
-  List_iterator_fast<MYSQL_ERROR> it(wi.warn_list());
-  MYSQL_ERROR *err;
-
-  while ((err= it++))
-  {
-    if (err->get_level() != MYSQL_ERROR::WARN_LEVEL_ERROR)
-      thd->warning_info->push_warning(thd, err);
-  }
-
-  return res;
-}
-
-
 /*
   Fill temporary schema tables before SELECT
 
@@ -7773,6 +7659,8 @@ bool get_schema_tables_result(JOIN *join
   bool result= 0;
   DBUG_ENTER("get_schema_tables_result");
 
+  Warnings_only_error_handler err_handler;
+  thd->push_internal_handler(&err_handler);
   for (JOIN_TAB *tab= first_linear_tab(join, WITH_CONST_TABLES); 
        tab; 
        tab= next_linear_tab(join, tab, WITHOUT_BUSH_ROOTS))
@@ -7825,20 +7713,42 @@ bool get_schema_tables_result(JOIN *join
       else
         table_list->table->file->stats.records= 0;
 
-      if (do_fill_table(thd, table_list, tab))
+
+      if (table_list->schema_table->fill_table(thd, table_list,
+                                               tab->select_cond))
       {
         result= 1;
         join->error= 1;
         tab->read_record.table->file= table_list->table->file;
         table_list->schema_table_state= executed_place;
-        if (!thd->is_error())
-          my_error(ER_UNKNOWN_ERROR, MYF(0));
         break;
       }
       tab->read_record.table->file= table_list->table->file;
       table_list->schema_table_state= executed_place;
     }
   }
+  thd->pop_internal_handler();
+  if (thd->is_error())
+  {
+    /*
+      This hack is here, because I_S code uses thd->clear_error() a lot.
+      Which means, a Warnings_only_error_handler cannot handle the error
+      corectly as it does not know whether an error is real (e.g. caused
+      by tab->select_cond->val_int()) or will be cleared later.
+      Thus it ignores all errors, and the real one (that is, the error
+      that was not cleared) is pushed now.
+
+      It also means that an audit plugin cannot process the error correctly
+      either. See also thd->clear_error()
+    */
+    thd->warning_info->push_warning(thd,
+                                    thd->stmt_da->sql_errno(),
+                                    thd->stmt_da->get_sqlstate(),
+                                    MYSQL_ERROR::WARN_LEVEL_ERROR,
+                                    thd->stmt_da->message());
+  }
+  else if (result)
+    my_error(ER_UNKNOWN_ERROR, MYF(0));
   DBUG_RETURN(result);
 }