Fixed bug#11753738 (formely known as bug#45235) - 5.1 DOES NOT SUPPORT 5.0-ONLY

SYNTAX TRIGGERS IN ANY WAY

Table with triggers which were using deprecated (5.0-only) syntax became
unavailable for any DML and DDL after upgrade to 5.1 version of server.
Attempt to execute any statement on such a table resulted in parsing
error reported. Since this included DROP TRIGGER and DROP TABLE
statements (actually, the latter was allowed but was not functioning
properly for such tables) it was impossible to fix the problem without
manual operations on .TRG and .TRN files in data directory.

The problem was that failure to parse trigger body (due to 5.0-only
syntax) when opening trigger file for a table prevented the table
from being open. This made all operations on the table impossible
(except DROP TABLE which due to peculiarity in its implementation
dropped the table but left trigger files around).

This patch solves this problem by silencing error which occurs when
we parse trigger body during table open. Error message is preserved
for the future use and table is marked as having a broken trigger.
We also try to analyze parse tree to recover trigger name, which
will be needed in order to drop the broken trigger. DML statements
which invoke triggers on the table marked as having broken trigger
are prohibited and emit saved error message. The same happens for
DDL which change triggers except DROP TRIGGER and DROP TABLE which
try their best to do what was requested. Table becomes no longer
marked as having broken trigger when last such trigger is dropped.

mysql-test/r/trigger-compat.result:
  Add results for test case for bug#45235
mysql-test/t/trigger-compat.test:
  Add test case for bug#45235.
sql/sp_head.cc:
  Added protection against MEM_ROOT double restoring to
  sp_head::restore_thd_mem_root() method. Since this
  method can be sometimes called twice during parsing
  of stored routine (the first time during normal flow
  of parsing, and the second time when a syntax error
  is detected) we need to shortcut execution of the
  method to avoid damaging MEM_ROOT by the second
  consecutive call to this method.
sql/sql_trigger.cc:
  Added error handler Deprecated_trigger_syntax_handler to 
  catch non-OOM errors during parsing of trigger body.
  
  Added handling of parse errors into method 
  Table_triggers_list::check_n_load().
sql/sql_trigger.h:
  Added new members to handle broken triggers and error messages.

4 files changed, 191 insertions, 15 deletions

diff --git a/sql/sp_head.cc b/sql/sp_head.cc
index a4dd51d8a4a..e5a06566a7d 100644
--- a/sql/sp_head.cc
+++ b/sql/sp_head.cc
@@ -2354,6 +2354,21 @@ void
 sp_head::restore_thd_mem_root(THD *thd)
 {
   DBUG_ENTER("sp_head::restore_thd_mem_root");
+
+  /*
+   In some cases our parser detects a syntax error and calls
+   LEX::cleanup_lex_after_parse_error() method only after
+   finishing parsing the whole routine. In such a situation
+   sp_head::restore_thd_mem_root() will be called twice - the
+   first time as part of normal parsing process and the second
+   time by cleanup_lex_after_parse_error().
+   To avoid ruining active arena/mem_root state in this case we
+   skip restoration of old arena/mem_root if this method has been
+   already called for this routine.
+  */
+  if (!m_thd)
+    DBUG_VOID_RETURN;
+
   Item *flist= free_list;	// The old list
   set_query_arena(thd);         // Get new free_list and mem_root
   state= INITIALIZED_FOR_SP;
diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
index ecc43f54fa5..b336a06e519 100644
--- a/sql/sql_parse.cc
+++ b/sql/sql_parse.cc
@@ -7972,10 +7972,14 @@ bool parse_sql(THD *thd,
 
   bool mysql_parse_status= MYSQLparse(thd) != 0;
 
-  /* Check that if MYSQLparse() failed, thd->is_error() is set. */
+  /*
+    Check that if MYSQLparse() failed, thd->is_error() is set (unless
+    we have an error handler installed, which might have silenced error).
+  */
 
   DBUG_ASSERT(!mysql_parse_status ||
-              (mysql_parse_status && thd->is_error()));
+              (mysql_parse_status && thd->is_error()) ||
+              (mysql_parse_status && thd->get_internal_handler()));
 
   /* Reset parser state. */
 
diff --git a/sql/sql_trigger.cc b/sql/sql_trigger.cc
index 9348feaa22a..89ee411c8da 100644
--- a/sql/sql_trigger.cc
+++ b/sql/sql_trigger.cc
@@ -19,6 +19,7 @@
 #include "sp_head.h"
 #include "sql_trigger.h"
 #include "parse_file.h"
+#include <mysys_err.h>
 
 /*************************************************************************/
 
@@ -293,6 +294,52 @@ private:
 
 
 /**
+  An error handler that catches all non-OOM errors which can occur during
+  parsing of trigger body. Such errors are ignored and corresponding error
+  message is used to construct a more verbose error message which contains
+  name of problematic trigger. This error message is later emitted when
+  one tries to perform DML or some of DDL on this table.
+  Also, if possible, grabs name of the trigger being parsed so it can be
+  used to correctly drop problematic trigger.
+*/
+class Deprecated_trigger_syntax_handler : public Internal_error_handler 
+{
+private:
+
+  char m_message[MYSQL_ERRMSG_SIZE];
+  LEX_STRING *m_trigger_name;
+
+public:
+
+  Deprecated_trigger_syntax_handler() : m_trigger_name(NULL) {}
+
+  virtual bool handle_error(uint sql_errno, const char *message,
+                            MYSQL_ERROR::enum_warning_level level, THD *thd)
+  {
+    if (sql_errno != EE_OUTOFMEMORY &&
+        sql_errno != ER_OUT_OF_RESOURCES)
+    {
+      if(thd->lex->spname)
+        m_trigger_name= &thd->lex->spname->m_name;
+      if (m_trigger_name)
+        my_snprintf(m_message, sizeof(m_message),
+                    "Trigger '%s' has an error in its body: '%s'",
+                    m_trigger_name->str, message);
+      else
+        my_snprintf(m_message, sizeof(m_message),
+                    "Unknown trigger has an error in its body: '%s'",
+                    message);
+      return true;
+    }
+    return false;
+  }
+
+  LEX_STRING *get_trigger_name() { return m_trigger_name; }
+  char *get_error_message() { return m_message; }
+};
+
+
+/**
   Create or drop trigger for table.
 
   @param thd     current thread context (including trigger definition in LEX)
@@ -575,6 +622,8 @@ bool Table_triggers_list::create_trigger(THD *thd, TABLE_LIST *tables,
   LEX_STRING *trg_connection_cl_name;
   LEX_STRING *trg_db_cl_name;
 
+  if (check_for_broken_triggers())
+    return true;
 
   /* Trigger must be in the same schema as target table. */
   if (my_strcasecmp(table_alias_charset, table->s->db.str,
@@ -848,7 +897,7 @@ static bool rm_trigger_file(char *path, const char *db,
   @param path         char buffer of size FN_REFLEN to be used
                       for constructing path to .TRN file.
   @param db           trigger's database name
-  @param table_name   trigger's name
+  @param trigger_name trigger's name
 
   @retval
     False   success
@@ -1312,12 +1361,11 @@ bool Table_triggers_list::check_n_load(THD *thd, const char *db,
         lex_start(thd);
         thd->spcont= NULL;
 
-        if (parse_sql(thd, & parser_state, creation_ctx))
-        {
-          /* Currently sphead is always deleted in case of a parse error */
-          DBUG_ASSERT(lex.sphead == 0);
-          goto err_with_lex_cleanup;
-        }
+        Deprecated_trigger_syntax_handler error_handler;
+        thd->push_internal_handler(&error_handler);
+        bool parse_error= parse_sql(thd, & parser_state, creation_ctx);
+        thd->pop_internal_handler();
+
         /*
           Not strictly necessary to invoke this method here, since we know
           that we've parsed CREATE TRIGGER and not an
@@ -1328,6 +1376,52 @@ bool Table_triggers_list::check_n_load(THD *thd, const char *db,
         */
         lex.set_trg_event_type_for_tables();
 
+        if (parse_error)
+        {
+          if (!triggers->m_has_unparseable_trigger)
+            triggers->set_parse_error_message(error_handler.get_error_message());
+          /* Currently sphead is always set to NULL in case of a parse error */
+          DBUG_ASSERT(lex.sphead == 0);
+          if (error_handler.get_trigger_name())
+          {
+            LEX_STRING *trigger_name;
+            const LEX_STRING *orig_trigger_name= error_handler.get_trigger_name();
+
+            if (!(trigger_name= alloc_lex_string(&table->mem_root)) ||
+                !(trigger_name->str= strmake_root(&table->mem_root,
+                                                  orig_trigger_name->str,
+                                                  orig_trigger_name->length)))
+              goto err_with_lex_cleanup;
+
+            trigger_name->length= orig_trigger_name->length;
+
+            if (triggers->names_list.push_back(trigger_name,
+                                               &table->mem_root))
+              goto err_with_lex_cleanup;
+          }
+          else
+          {
+            /* 
+               The Table_triggers_list is not constructed as a list of
+               trigger objects as one would expect, but rather of lists of
+               properties of equal length. Thus, even if we don't get the
+               trigger name, we still fill all in all the lists with
+               placeholders as we might otherwise create a skew in the
+               lists. Obviously, this has to be refactored.
+            */
+            LEX_STRING *empty= alloc_lex_string(&table->mem_root);
+            if (!empty)
+              goto err_with_lex_cleanup;
+
+            empty->str= const_cast<char*>("");
+            empty->length= 0;
+            if (triggers->names_list.push_back(empty, &table->mem_root))
+              goto err_with_lex_cleanup;
+          }
+          lex_end(&lex);
+          continue;
+        }
+
         lex.sphead->set_info(0, 0, &lex.sp_chistics, (ulong) *trg_sql_mode);
 
         int event= lex.trg_chistics.event;
@@ -1368,8 +1462,8 @@ bool Table_triggers_list::check_n_load(THD *thd, const char *db,
 
         if (triggers->names_list.push_back(&lex.sphead->m_name,
                                            &table->mem_root))
-            goto err_with_lex_cleanup;
-
+          goto err_with_lex_cleanup;
+        
         if (!(on_table_name= alloc_lex_string(&table->mem_root)))
           goto err_with_lex_cleanup;
 
@@ -1394,9 +1488,8 @@ bool Table_triggers_list::check_n_load(THD *thd, const char *db,
         char fname[NAME_LEN + 1];
         DBUG_ASSERT((!my_strcasecmp(table_alias_charset, lex.query_tables->db, db) ||
                      (check_n_cut_mysql50_prefix(db, fname, sizeof(fname)) &&
-                      !my_strcasecmp(table_alias_charset, lex.query_tables->db, fname))) &&
-                    (!my_strcasecmp(table_alias_charset, lex.query_tables->table_name,
-                                    table_name) ||
+                      !my_strcasecmp(table_alias_charset, lex.query_tables->db, fname))));
+        DBUG_ASSERT((!my_strcasecmp(table_alias_charset, lex.query_tables->table_name, table_name) ||
                      (check_n_cut_mysql50_prefix(table_name, fname, sizeof(fname)) &&
                       !my_strcasecmp(table_alias_charset, lex.query_tables->table_name, fname))));
 #endif
@@ -1680,6 +1773,13 @@ bool Table_triggers_list::drop_all_triggers(THD *thd, char *db, char *name)
 
     while ((trigger= it_name++))
     {
+      /*
+        Trigger, which body we failed to parse during call
+        Table_triggers_list::check_n_load(), might be missing name.
+        Such triggers have zero-length name and are skipped here.
+      */
+      if (trigger->length == 0)
+        continue;
       if (rm_trigname_file(path, db, trigger->str))
       {
         /*
@@ -1903,6 +2003,11 @@ bool Table_triggers_list::change_table_name(THD *thd, const char *db,
   }
   if (table.triggers)
   {
+    if (table.triggers->check_for_broken_triggers())
+    {
+      result= 1;
+      goto end;
+    }
     LEX_STRING old_table_name= { (char *) old_table, strlen(old_table) };
     LEX_STRING new_table_name= { (char *) new_table, strlen(new_table) };
     /*
@@ -1991,6 +2096,9 @@ bool Table_triggers_list::process_triggers(THD *thd,
   sp_head *sp_trigger= bodies[event][time_type];
   SELECT_LEX *save_current_select;
 
+  if (check_for_broken_triggers())
+    return true;
+
   if (sp_trigger == NULL)
     return FALSE;
 
@@ -2070,6 +2178,22 @@ void Table_triggers_list::mark_fields_used(trg_event_type event)
 
 
 /**
+   Signals to the Table_triggers_list that a parse error has occured when
+   reading a trigger from file. This makes the Table_triggers_list enter an
+   error state flagged by m_has_unparseable_trigger == true. The error message
+   will be used whenever a statement invoking or manipulating triggers is
+   issued against the Table_triggers_list's table.
+
+   @param error_message The error message thrown by the parser.
+ */
+void Table_triggers_list::set_parse_error_message(char *error_message)
+{
+  m_has_unparseable_trigger= true;
+  strcpy(m_parse_error_message, error_message);
+}
+
+
+/**
   Trigger BUG#14090 compatibility hook.
 
   @param[in,out] unknown_key       reference on the line with unknown
diff --git a/sql/sql_trigger.h b/sql/sql_trigger.h
index f6754a75284..c077d9567f8 100644
--- a/sql/sql_trigger.h
+++ b/sql/sql_trigger.h
@@ -62,6 +62,27 @@ class Table_triggers_list: public Sql_alloc
   */
   GRANT_INFO        subject_table_grants[TRG_EVENT_MAX][TRG_ACTION_MAX];
 
+  /**
+     This flag indicates that one of the triggers was not parsed successfully,
+     and as a precaution the object has entered a state where all trigger
+     access results in errors until all such triggers are dropped. It is not
+     safe to add triggers since we don't know if the broken trigger has the
+     same name or event type. Nor is it safe to invoke any trigger for the
+     aforementioned reasons. The only safe operations are drop_trigger and
+     drop_all_triggers.
+
+     @see Table_triggers_list::set_parse_error
+   */
+  bool m_has_unparseable_trigger;
+
+  /**
+    This error will be displayed when the user tries to manipulate or invoke
+    triggers on a table that has broken triggers. It will get set only once
+    per statement and thus will contain the first parse error encountered in
+    the trigger file.
+   */
+  char m_parse_error_message[MYSQL_ERRMSG_SIZE];
+
 public:
   /**
     Field responsible for storing triggers definitions in file.
@@ -84,7 +105,7 @@ public:
   /* End of character ser context. */
 
   Table_triggers_list(TABLE *table_arg):
-    record1_field(0), trigger_table(table_arg)
+  record1_field(0), trigger_table(table_arg), m_has_unparseable_trigger(false)
   {
     bzero((char *)bodies, sizeof(bodies));
     bzero((char *)trigger_fields, sizeof(trigger_fields));
@@ -140,6 +161,8 @@ public:
 
   void mark_fields_used(trg_event_type event);
 
+  void set_parse_error_message(char *error_message);
+
   friend class Item_trigger_field;
   friend int sp_cache_routines_and_add_tables_for_triggers(THD *thd, LEX *lex,
                                                             TABLE_LIST *table);
@@ -155,6 +178,16 @@ private:
                                      const char *new_db_name,
                                      LEX_STRING *old_table_name,
                                      LEX_STRING *new_table_name);
+
+  bool check_for_broken_triggers() 
+  {
+    if (m_has_unparseable_trigger)
+    {
+      my_message(ER_PARSE_ERROR, m_parse_error_message, MYF(0));
+      return true;
+    }
+    return false;
+  }
 };
 
 extern const LEX_STRING trg_action_time_type_names[];