List:Commits« Previous MessageNext Message »
From:Martin Hansson Date:September 22 2009 7:49am
Subject:bzr commit into mysql-5.1-bugteam branch (martin.hansson:3118) Bug#45235
View as plain text  
#At file:///data0/martin/bzr/bug45235/5.1bt-commit/ based on revid:kristofer.pettersson@stripped

 3118 Martin Hansson	2009-09-22
      Bug#45235: 5.1 does not support 5.0-only syntax triggers in any way
      
      A parse error when opening a trigger file would disallow the user to drop the
      trigger causing it. The most common scenario is a server upgrade, when the
      trigger uses deprecated syntax. The problem is that the trigger name has to be
      parsed out in order to successfully drop the trigger, which obviously cannot
      be guaranteed. For the upgrade case, however, it is worthwhile to analyze the
      parse tree and try to recover it. The error is then silenced, but stored. The
      Trigger Manager then enters an error state, and throws the error whenever a
      trigger on the table is invoked or manipulated. The error state is exited from
      when all broken triggers are dropped.
     @ mysql-test/r/trigger-compat.result
        Bug#45235: Test result.
     @ mysql-test/t/trigger-compat.test
        Bug#45235: Test case.
     @ sql/sql_parse.cc
        Bug#45235: Removed assertion for a state that can't be maintained.
     @ sql/sql_trigger.cc
        Bug#45235: 
        - New class to implement error suppression and recovery of trigger name.
        - Comment correction
        - Errors for trigger manipulation statements
        - More comment correction
        - Fix exploiting new class.
        - indentation correction.
        - Split up a conjunction in an assert.
        - Added code for handling completely broken triggers
        - More errors for trigger manipulation statements. 
        - Method for setting broken triggers flag & error message.
     @ sql/sql_trigger.h
        Bug#45235: New members to handle broken triggers and error messages.

    modified:
      mysql-test/r/trigger-compat.result
      mysql-test/t/trigger-compat.test
      sql/sql_parse.cc
      sql/sql_trigger.cc
      sql/sql_trigger.h
=== modified file 'mysql-test/r/trigger-compat.result'
--- a/mysql-test/r/trigger-compat.result	2009-02-19 23:24:25 +0000
+++ b/mysql-test/r/trigger-compat.result	2009-09-22 07:49:09 +0000
@@ -43,3 +43,84 @@ DROP TABLE t2;
 DROP USER mysqltest_dfn@localhost;
 DROP USER mysqltest_inv@localhost;
 DROP DATABASE mysqltest_db1;
+USE test;
+#
+# Bug#45235: 5.1 does not support 5.0-only syntax triggers in any way
+#
+CREATE TABLE t1 ( a INT );
+CREATE TABLE t2 ( a INT );
+CREATE TABLE t3 ( a INT );
+INSERT INTO t1 VALUES (1), (2), (3);
+INSERT INTO t2 VALUES (1), (2), (3);
+INSERT INTO t3 VALUES (1), (2), (3);
+# We simulate importing a trigger from 5.0 by writing a .TRN file for
+# each trigger plus a .TRG file the way MySQL 5.0 would have done it, 
+# with syntax allowed in 5.0 only.
+#
+# Note that in 5.0 the following lines are missing from t1.TRG:
+#
+# client_cs_names='latin1'
+# connection_cl_names='latin1_swedish_ci'
+# db_cl_names='latin1_swedish_ci'
+# We will get parse errors for most DDL and DML statements when the table
+# has broken triggers. The parse error refers to the first broken 
+# trigger.
+CREATE TRIGGER tr16 AFTER UPDATE ON t1 FOR EACH ROW INSERT INTO t1 VALUES (1);
+ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'a USING t1 a' at line 1
+CREATE TRIGGER tr22 BEFORE INSERT ON t2 FOR EACH ROW DELETE FROM non_existing_table;
+ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'Not allowed syntax here, and trigger name cant be extracted either.' at line 1
+SHOW TRIGGERS;
+Trigger	Event	Table	Statement	Timing	Created	sql_mode	Definer	character_set_client	collation_connection	Database Collation
+tr11	INSERT	t1	DELETE FROM t3	BEFORE	NULL		root@localhost	latin1	latin1_swedish_ci	latin1_swedish_ci
+tr12	INSERT	t1	DELETE FROM t3	AFTER	NULL		root@localhost	latin1	latin1_swedish_ci	latin1_swedish_ci
+tr14	DELETE	t1	DELETE FROM non_existing_table	AFTER	NULL		root@localhost	latin1	latin1_swedish_ci	latin1_swedish_ci
+Warnings:
+Warning	1603	Triggers for table `test`.`t1` have no creation context
+Warning	1603	Triggers for table `test`.`t2` have no creation context
+INSERT INTO t1 VALUES (1);
+ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'a USING t1 a' at line 1
+INSERT INTO t2 VALUES (1);
+ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'Not allowed syntax here, and trigger name cant be extracted either.' at line 1
+DELETE FROM t1;
+ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'a USING t1 a' at line 1
+UPDATE t1 SET a = 1 WHERE a = 1;
+ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'a USING t1 a' at line 1
+SELECT * FROM t1;
+a
+1
+2
+3
+SHOW TRIGGERS;
+Trigger	Event	Table	Statement	Timing	Created	sql_mode	Definer	character_set_client	collation_connection	Database Collation
+tr11	INSERT	t1	DELETE FROM t3	BEFORE	NULL		root@localhost	latin1	latin1_swedish_ci	latin1_swedish_ci
+tr12	INSERT	t1	DELETE FROM t3	AFTER	NULL		root@localhost	latin1	latin1_swedish_ci	latin1_swedish_ci
+tr14	DELETE	t1	DELETE FROM non_existing_table	AFTER	NULL		root@localhost	latin1	latin1_swedish_ci	latin1_swedish_ci
+DROP TRIGGER tr11;
+Warnings:
+Warning	1603	Triggers for table `test`.`t1` have no creation context
+DROP TRIGGER tr12;
+DROP TRIGGER tr13;
+DROP TRIGGER tr14;
+DROP TRIGGER tr15;
+SHOW TRIGGERS;
+Trigger	Event	Table	Statement	Timing	Created	sql_mode	Definer	character_set_client	collation_connection	Database Collation
+# Make sure there is no trigger file left.
+t1.MYD
+t1.MYI
+t1.frm
+t2.MYD
+t2.MYI
+t2.TRG
+t2.frm
+t3.MYD
+t3.MYI
+t3.frm
+# We write the same trigger files one more time to test DROP TABLE.
+DROP TABLE t1;
+Warnings:
+Warning	1603	Triggers for table `test`.`t1` have no creation context
+DROP TABLE t2;
+Warnings:
+Warning	1603	Triggers for table `test`.`t2` have no creation context
+DROP TABLE t3;
+# Make sure there is no trigger file left.

=== modified file 'mysql-test/t/trigger-compat.test'
--- a/mysql-test/t/trigger-compat.test	2009-02-19 23:24:25 +0000
+++ b/mysql-test/t/trigger-compat.test	2009-09-22 07:49:09 +0000
@@ -106,4 +106,134 @@ DROP TABLE t2;
 DROP USER mysqltest_dfn@localhost;
 DROP USER mysqltest_inv@localhost;
 DROP DATABASE mysqltest_db1;
+USE test;
+--echo #
+--echo # Bug#45235: 5.1 does not support 5.0-only syntax triggers in any way
+--echo #
+let $MYSQLD_DATADIR=`SELECT @@datadir`;
+
+CREATE TABLE t1 ( a INT );
+CREATE TABLE t2 ( a INT );
+CREATE TABLE t3 ( a INT );
+INSERT INTO t1 VALUES (1), (2), (3);
+INSERT INTO t2 VALUES (1), (2), (3);
+INSERT INTO t3 VALUES (1), (2), (3);
+
+--echo # We simulate importing a trigger from 5.0 by writing a .TRN file for
+--echo # each trigger plus a .TRG file the way MySQL 5.0 would have done it, 
+--echo # with syntax allowed in 5.0 only.
+--echo #
+--echo # Note that in 5.0 the following lines are missing from t1.TRG:
+--echo #
+--echo # client_cs_names='latin1'
+--echo # connection_cl_names='latin1_swedish_ci'
+--echo # db_cl_names='latin1_swedish_ci'
+
+--write_file $MYSQLD_DATADIR/test/tr11.TRN
+TYPE=TRIGGERNAME
+trigger_table=t1
+EOF
+
+--write_file $MYSQLD_DATADIR/test/tr12.TRN
+TYPE=TRIGGERNAME
+trigger_table=t1
+EOF
+
+--write_file $MYSQLD_DATADIR/test/tr13.TRN
+TYPE=TRIGGERNAME
+trigger_table=t1
+EOF
+
+--write_file $MYSQLD_DATADIR/test/tr14.TRN
+TYPE=TRIGGERNAME
+trigger_table=t1
+EOF
+
+--write_file $MYSQLD_DATADIR/test/tr15.TRN
+TYPE=TRIGGERNAME
+trigger_table=t1
+EOF
+
+--write_file $MYSQLD_DATADIR/test/t1.TRG
+TYPE=TRIGGERS
+triggers='CREATE DEFINER=`root`@`localhost` TRIGGER tr11 BEFORE INSERT ON t1 FOR EACH ROW DELETE FROM t3' 'CREATE DEFINER=`root`@`localhost` TRIGGER tr12 AFTER INSERT ON t1 FOR EACH ROW DELETE FROM t3' 'CREATE DEFINER=`root`@`localhost` TRIGGER tr13 BEFORE DELETE ON t1 FOR EACH ROW DELETE FROM t1 a USING t1 a' 'CREATE DEFINER=`root`@`localhost` TRIGGER tr14 AFTER DELETE ON t1 FOR EACH ROW DELETE FROM non_existing_table' 'CREATE DEFINER=`root`@`localhost` TRIGGER tr15 BEFORE UPDATE ON t1 FOR EACH ROW DELETE FROM non_existing_table a USING non_existing_table a'
+sql_modes=0 0 0 0 0
+definers='root@localhost' 'root@localhost' 'root@localhost' 'root@localhost' 'root@localhost'
+EOF
+
+--write_file $MYSQLD_DATADIR/test/t2.TRG
+TYPE=TRIGGERS
+triggers='Not allowed syntax here, and trigger name cant be extracted either.'
+sql_modes=0
+definers='root@localhost'
+EOF
+
+--echo # We will get parse errors for most DDL and DML statements when the table
+--echo # has broken triggers. The parse error refers to the first broken 
+--echo # trigger.
+--error ER_PARSE_ERROR
+CREATE TRIGGER tr16 AFTER UPDATE ON t1 FOR EACH ROW INSERT INTO t1 VALUES (1);
+--error ER_PARSE_ERROR
+CREATE TRIGGER tr22 BEFORE INSERT ON t2 FOR EACH ROW DELETE FROM non_existing_table;
+SHOW TRIGGERS;
+--error ER_PARSE_ERROR
+INSERT INTO t1 VALUES (1);
+--error ER_PARSE_ERROR
+INSERT INTO t2 VALUES (1);
+--error ER_PARSE_ERROR
+DELETE FROM t1;
+--error ER_PARSE_ERROR
+UPDATE t1 SET a = 1 WHERE a = 1;
+SELECT * FROM t1;
+SHOW TRIGGERS;
+
+DROP TRIGGER tr11;
+DROP TRIGGER tr12;
+DROP TRIGGER tr13;
+DROP TRIGGER tr14;
+DROP TRIGGER tr15;
+
+SHOW TRIGGERS;
+--echo # Make sure there is no trigger file left.
+--list_files $MYSQLD_DATADIR/test/
+
+--echo # We write the same trigger files one more time to test DROP TABLE.
+--write_file $MYSQLD_DATADIR/test/tr11.TRN
+TYPE=TRIGGERNAME
+trigger_table=t1
+EOF
+
+--write_file $MYSQLD_DATADIR/test/tr12.TRN
+TYPE=TRIGGERNAME
+trigger_table=t1
+EOF
+
+--write_file $MYSQLD_DATADIR/test/tr13.TRN
+TYPE=TRIGGERNAME
+trigger_table=t1
+EOF
+
+--write_file $MYSQLD_DATADIR/test/tr14.TRN
+TYPE=TRIGGERNAME
+trigger_table=t1
+EOF
+
+--write_file $MYSQLD_DATADIR/test/tr15.TRN
+TYPE=TRIGGERNAME
+trigger_table=t1
+EOF
+
+--write_file $MYSQLD_DATADIR/test/t1.TRG
+TYPE=TRIGGERS
+triggers='CREATE DEFINER=`root`@`localhost` TRIGGER tr11 BEFORE INSERT ON t1 FOR EACH ROW DELETE FROM t3' 'CREATE DEFINER=`root`@`localhost` TRIGGER tr12 AFTER INSERT ON t1 FOR EACH ROW DELETE FROM t3' 'CREATE DEFINER=`root`@`localhost` TRIGGER tr13 BEFORE DELETE ON t1 FOR EACH ROW DELETE FROM t1 a USING t1 a' 'CREATE DEFINER=`root`@`localhost` TRIGGER tr14 AFTER DELETE ON t1 FOR EACH ROW DELETE FROM non_existing_table' 'CREATE DEFINER=`root`@`localhost` TRIGGER tr15 BEFORE UPDATE ON t1 FOR EACH ROW DELETE FROM non_existing_table a USING non_existing_table a'
+sql_modes=0 0 0 0 0
+definers='root@localhost' 'root@localhost' 'root@localhost' 'root@localhost' 'root@localhost'
+EOF
+
+DROP TABLE t1;
+DROP TABLE t2;
+DROP TABLE t3;
+
+--echo # Make sure there is no trigger file left.
+--list_files $MYSQLD_DATADIR/test/
 

=== modified file 'sql/sql_parse.cc'
--- a/sql/sql_parse.cc	2009-09-10 07:40:57 +0000
+++ b/sql/sql_parse.cc	2009-09-22 07:49:09 +0000
@@ -7809,11 +7809,6 @@ bool parse_sql(THD *thd,
 
   bool mysql_parse_status= MYSQLparse(thd) != 0;
 
-  /* Check that if MYSQLparse() failed, thd->is_error() is set. */
-
-  DBUG_ASSERT(!mysql_parse_status ||
-              (mysql_parse_status && thd->is_error()));
-
   /* Reset parser state. */
 
   thd->m_parser_state= NULL;

=== modified file 'sql/sql_trigger.cc'
--- a/sql/sql_trigger.cc	2009-06-17 14:56:44 +0000
+++ b/sql/sql_trigger.cc	2009-09-22 07:49:09 +0000
@@ -291,6 +291,36 @@ private:
   LEX_STRING *trigger_table_value;
 };
 
+class Deprecated_trigger_syntax_handler : public Internal_error_handler 
+{
+private:
+
+  char m_message[MYSQL_ERRMSG_SIZE];
+
+public:
+  
+  LEX_STRING *m_trigger_name;
+
+  Deprecated_trigger_syntax_handler() : m_trigger_name(NULL) {}
+
+  bool handle_error(uint sql_errno, const char *message,
+                    MYSQL_ERROR::enum_warning_level level, THD *thd)
+  {
+
+    if (sql_errno == ER_PARSE_ERROR)
+    {
+      if(thd->lex->spname)
+        m_trigger_name= &thd->lex->spname->m_name;
+      strcpy(m_message, message);
+      return TRUE;
+    }
+    return FALSE;
+  }
+
+  LEX_STRING *get_trigger_name() { return m_trigger_name; }
+  char *get_error_text() { return m_message; }
+
+};
 
 /**
   Create or drop trigger for table.
@@ -541,13 +571,16 @@ end:
     (SUID/new trigger).
 
   @retval
-    False   success
+    false   success
   @retval
-    True    error
+    true    error
 */
 bool Table_triggers_list::create_trigger(THD *thd, TABLE_LIST *tables,
                                          String *stmt_query)
 {
+  if (check_for_broken_triggers())
+    return TRUE;
+
   LEX *lex= thd->lex;
   TABLE *table= tables->table;
   char file_buff[FN_REFLEN], trigname_buff[FN_REFLEN];
@@ -837,7 +870,7 @@ static bool rm_trigger_file(char *path, 
   @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
@@ -1301,12 +1334,11 @@ bool Table_triggers_list::check_n_load(T
         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
@@ -1317,6 +1349,38 @@ bool Table_triggers_list::check_n_load(T
         */
         lex.set_trg_event_type_for_tables();
 
+        if (parse_error)
+        {
+          if (!triggers->m_has_unparseable_trigger)
+            triggers->set_parse_error(error_handler.get_error_text());
+          /* Currently sphead is always set to NULL in case of a parse error */
+          DBUG_ASSERT(lex.sphead == 0);
+          if (error_handler.get_trigger_name())
+            if (triggers->names_list.push_back(error_handler.get_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);
+            empty->str= NULL;
+            empty->length= 0;
+            if (triggers->names_list.push_back(empty, &table->mem_root) ||
+                triggers->on_table_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;
@@ -1357,8 +1421,8 @@ bool Table_triggers_list::check_n_load(T
 
         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;
 
@@ -1383,9 +1447,8 @@ bool Table_triggers_list::check_n_load(T
         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
@@ -1674,6 +1737,8 @@ bool Table_triggers_list::drop_all_trigg
 
     while ((trigger= it_name++))
     {
+      if (trigger->length == 0)
+        continue;
       if (rm_trigname_file(path, db, trigger->str))
       {
         /*
@@ -1980,6 +2045,9 @@ bool Table_triggers_list::process_trigge
                                            trg_action_time_type time_type,
                                            bool old_row_is_record1)
 {
+  if (check_for_broken_triggers())
+    return TRUE;
+
   bool err_status;
   Sub_statement_state statement_state;
   sp_head *sp_trigger= bodies[event][time_type];
@@ -2055,6 +2123,22 @@ void Table_triggers_list::mark_fields_us
 
 
 /**
+   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(char *error_message)
+{
+  m_has_unparseable_trigger= TRUE;
+  strcpy(m_parse_error_text, error_message);
+}
+
+
+/**
   Trigger BUG#14090 compatibility hook.
 
   @param[in,out] unknown_key       reference on the line with unknown

=== modified file 'sql/sql_trigger.h'
--- a/sql/sql_trigger.h	2009-01-14 14:50:51 +0000
+++ b/sql/sql_trigger.h	2009-09-22 07:49:09 +0000
@@ -62,6 +62,27 @@ class Table_triggers_list: public Sql_al
   */
   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 diplayed 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_text[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(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)
+    {
+      (*error_handler_hook)(ER_PARSE_ERROR, m_parse_error_text, MYF(0));
+      return TRUE;
+    }
+    return FALSE;
+  }
 };
 
 extern const LEX_STRING trg_action_time_type_names[];


Attachment: [text/bzr-bundle] bzr/martin.hansson@sun.com-20090922074909-wxt7kdl4ls6on0ib.bundle
Thread
bzr commit into mysql-5.1-bugteam branch (martin.hansson:3118) Bug#45235Martin Hansson22 Sep