Bug#23703 (DROP TRIGGER needs an IF EXISTS)

This change set implements the DROP TRIGGER IF EXISTS functionality.

This fix is considered a bug and not a feature, because without it,
there is no known method to write a database creation script that can create
a trigger without failing, when executed on a database that may or may not
contain already a trigger of the same name.

Implementing this functionality closes an orthogonality gap between triggers
and stored procedures / stored functions (which do support the DROP IF
EXISTS syntax).

In sql_trigger.cc, in mysql_create_or_drop_trigger,
the code has been reordered to:
- perform the tests that do not depend on the file system (access()),
- get the locks (wait_if_global_read_lock, LOCK_open)
- call access()
- perform the operation
- write to the binlog
- unlock (LOCK_open, start_waiting_global_read_lock)

This is to ensure that all the code that depends on the presence of the
trigger file is executed in the same critical section,
and prevents race conditions similar to the case fixed by Bug 14262 :

- thread 1 executes DROP TRIGGER IF EXISTS, access() returns a failure
- thread 2 executes CREATE TRIGGER
- thread 2 logs CREATE TRIGGER
- thread 1 logs DROP TRIGGER IF EXISTS

The patch itself is based on code contributed by the MySQL community,
under the terms of the Contributor License Agreement (See Bug 18161).


mysql-test/r/rpl_trigger.result:
  DROP TRIGGER IF EXISTS
mysql-test/r/trigger.result:
  DROP TRIGGER IF EXISTS
mysql-test/t/rpl_trigger.test:
  DROP TRIGGER IF EXISTS
mysql-test/t/trigger.test:
  DROP TRIGGER IF EXISTS
sql/sql_trigger.cc:
  DROP TRIGGER IF EXISTS
sql/sql_yacc.yy:
  DROP TRIGGER IF EXISTS

mysql-test/r/rpl_trigger.result

@@ -941,3 +941,30 @@ c
 ---> Cleaning up...
 DROP TABLE t1;
 DROP TABLE t2;
+drop table if exists t1;
+create table t1(a int, b varchar(50));
+drop trigger not_a_trigger;
+ERROR HY000: Trigger does not exist
+drop trigger if exists not_a_trigger;
+Warnings:
+Note	1360	Trigger does not exist
+create trigger t1_bi before insert on t1
+for each row set NEW.b := "In trigger t1_bi";
+insert into t1 values (1, "a");
+drop trigger if exists t1_bi;
+insert into t1 values (2, "b");
+drop trigger if exists t1_bi;
+Warnings:
+Note	1360	Trigger does not exist
+insert into t1 values (3, "c");
+select * from t1;
+a	b
+1	In trigger t1_bi
+2	b
+3	c
+select * from t1;
+a	b
+1	In trigger t1_bi
+2	b
+3	c
+drop table t1;

mysql-test/r/trigger.result

@@ -1256,4 +1256,26 @@ select @a;
 20
 drop table t1;
 drop function f1;
+drop table if exists t1;
+create table t1(a int, b varchar(50));
+drop trigger not_a_trigger;
+ERROR HY000: Trigger does not exist
+drop trigger if exists not_a_trigger;
+Warnings:
+Note	1360	Trigger does not exist
+create trigger t1_bi before insert on t1
+for each row set NEW.b := "In trigger t1_bi";
+insert into t1 values (1, "a");
+drop trigger if exists t1_bi;
+insert into t1 values (2, "b");
+drop trigger if exists t1_bi;
+Warnings:
+Note	1360	Trigger does not exist
+insert into t1 values (3, "c");
+select * from t1;
+a	b
+1	In trigger t1_bi
+2	b
+3	c
+drop table t1;
 End of 5.0 tests

mysql-test/t/rpl_trigger.test

@@ -423,6 +423,43 @@ DROP TABLE t2;
 --sync_with_master
 --connection master
 
+#
+# BUG#23703: DROP TRIGGER needs an IF EXISTS
+#
+
+connection master;
+
+--disable_warnings
+drop table if exists t1;
+--enable_warnings
+
+create table t1(a int, b varchar(50));
+
+-- error ER_TRG_DOES_NOT_EXIST
+drop trigger not_a_trigger;
+
+drop trigger if exists not_a_trigger;
+
+create trigger t1_bi before insert on t1
+for each row set NEW.b := "In trigger t1_bi";
+
+insert into t1 values (1, "a");
+drop trigger if exists t1_bi;
+insert into t1 values (2, "b");
+drop trigger if exists t1_bi;
+insert into t1 values (3, "c");
+
+select * from t1;
+
+save_master_pos;
+connection slave;
+sync_with_master;
+
+select * from t1;
+
+connection master;
+
+drop table t1;
 
 #
 # End of tests

mysql-test/t/trigger.test

@@ -1519,4 +1519,32 @@ connection default;
 drop table t1;
 drop function f1;
 
+#
+# Bug#23703: DROP TRIGGER needs an IF EXISTS
+#
+
+--disable_warnings
+drop table if exists t1;
+--enable_warnings
+
+create table t1(a int, b varchar(50));
+
+-- error ER_TRG_DOES_NOT_EXIST
+drop trigger not_a_trigger;
+
+drop trigger if exists not_a_trigger;
+
+create trigger t1_bi before insert on t1
+for each row set NEW.b := "In trigger t1_bi";
+
+insert into t1 values (1, "a");
+drop trigger if exists t1_bi;
+insert into t1 values (2, "b");
+drop trigger if exists t1_bi;
+insert into t1 values (3, "c");
+
+select * from t1;
+
+drop table t1;
+
 --echo End of 5.0 tests

sql/sql_trigger.cc

@@ -107,7 +107,9 @@ const LEX_STRING trg_event_type_names[]=
 };
 
 
-static TABLE_LIST *add_table_for_trigger(THD *thd, sp_name *trig);
+static int
+add_table_for_trigger(THD *thd, sp_name *trig, bool if_exists,
+                      TABLE_LIST ** table);
 
 class Handle_old_incorrect_sql_modes_hook: public Unknown_key_hook
 {
@@ -156,6 +158,13 @@ private:
 */
 bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create)
 {
+  /*
+    FIXME: The code below takes too many different paths depending on the
+    'create' flag, so that the justification for a single function
+    'mysql_create_or_drop_trigger', compared to two separate functions
+    'mysql_create_trigger' and 'mysql_drop_trigger' is not apparent.
+    This is a good candidate for a minor refactoring.
+  */
   TABLE *table;
   bool result= TRUE;
   String stmt_query;
@@ -181,10 +190,6 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create)
     DBUG_RETURN(TRUE);
   }
 
-  if (!create &&
-      !(tables= add_table_for_trigger(thd, thd->lex->spname)))
-    DBUG_RETURN(TRUE);
-
   /*
     We don't allow creating triggers on tables in the 'mysql' schema
   */
@@ -194,9 +199,6 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create)
     DBUG_RETURN(TRUE);
   }
 
-  /* We should have only one table in table list. */
-  DBUG_ASSERT(tables->next_global == 0);
-
   /*
     TODO: We should check if user has TRIGGER privilege for table here.
     Now we just require SUPER privilege for creating/dropping because
@@ -211,7 +213,7 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create)
     DROP for example) so we do the check for privileges. For now there is
     already a stronger test right above; but when this stronger test will
     be removed, the test below will hold. Because triggers have the same
-    nature as functions regarding binlogging: their body is implicitely
+    nature as functions regarding binlogging: their body is implicitly
     binlogged, so they share the same danger, so trust_function_creators
     applies to them too.
   */
@@ -222,24 +224,52 @@ bool mysql_create_or_drop_trigger(THD *thd, TABLE_LIST *tables, bool create)
     DBUG_RETURN(TRUE);
   }
 
-  /* We do not allow creation of triggers on temporary tables. */
-  if (create && find_temporary_table(thd, tables->db, tables->table_name))
-  {
-    my_error(ER_TRG_ON_VIEW_OR_TEMP_TABLE, MYF(0), tables->alias);
-    DBUG_RETURN(TRUE);
-  }
-
   /*
     We don't want perform our operations while global read lock is held
-    so we have to wait until its end and then prevent it from occuring
+    so we have to wait until its end and then prevent it from occurring
     again until we are done. (Acquiring LOCK_open is not enough because
-    global read lock is held without helding LOCK_open).
+    global read lock is held without holding LOCK_open).
   */
   if (wait_if_global_read_lock(thd, 0, 1))
     DBUG_RETURN(TRUE);
 
   VOID(pthread_mutex_lock(&LOCK_open));
 
+  if (!create)
+  {
+    bool if_exists= thd->lex->drop_if_exists;
+
+    if (add_table_for_trigger(thd, thd->lex->spname, if_exists, & tables))
+      goto end;
+
+    if (!tables)
+    {
+      DBUG_ASSERT(if_exists);
+      /*
+        Since the trigger does not exist, there is no associated table,
+        and therefore :
+        - no TRIGGER privileges to check,
+        - no trigger to drop,
+        - no table to lock/modify,
+        so the drop statement is successful.
+      */
+      result= FALSE;
+      /* Still, we need to log the query ... */
+      stmt_query.append(thd->query, thd->query_length);
+      goto end;
+    }
+  }
+
+  /* We should have only one table in table list. */
+  DBUG_ASSERT(tables->next_global == 0);
+
+  /* We do not allow creation of triggers on temporary tables. */
+  if (create && find_temporary_table(thd, tables->db, tables->table_name))
+  {
+    my_error(ER_TRG_ON_VIEW_OR_TEMP_TABLE, MYF(0), tables->alias);
+    goto end;
+  }
+
   if (lock_table_names(thd, tables))
     goto end;
 
@@ -1145,13 +1175,17 @@ bool Table_triggers_list::get_trigger_info(THD *thd, trg_event_type event,
     mysql_table_for_trigger()
       thd    - current thread context
       trig   - identifier for trigger
+      if_exists - treat a not existing trigger as a warning if TRUE
+      table - pointer to TABLE_LIST object for the table trigger (output)
 
   RETURN VALUE
-    0 - error
-    # - pointer to TABLE_LIST object for the table
+    0 Success
+    1 Error
 */
 
-static TABLE_LIST *add_table_for_trigger(THD *thd, sp_name *trig)
+static int
+add_table_for_trigger(THD *thd, sp_name *trig, bool if_exists,
+                      TABLE_LIST **table)
 {
   LEX *lex= thd->lex;
   char path_buff[FN_REFLEN];
@@ -1162,6 +1196,7 @@ static TABLE_LIST *add_table_for_trigger(THD *thd, sp_name *trig)
                                           path_buff, &trigname.trigger_table);
   
   DBUG_ENTER("add_table_for_trigger");
+  DBUG_ASSERT(table != NULL);
 
   strxnmov(path_buff, FN_REFLEN, mysql_data_home, "/", trig->m_db.str, "/",
            trig->m_name.str, trigname_file_ext, NullS);
@@ -1170,30 +1205,45 @@ static TABLE_LIST *add_table_for_trigger(THD *thd, sp_name *trig)
 
   if (access(path_buff, F_OK))
   {
+    if (if_exists)
+    {
+      push_warning_printf(thd,
+                         MYSQL_ERROR::WARN_LEVEL_NOTE,
+                         ER_TRG_DOES_NOT_EXIST,
+                         ER(ER_TRG_DOES_NOT_EXIST));
+      *table= NULL;
+      DBUG_RETURN(0);
+    }
+
     my_error(ER_TRG_DOES_NOT_EXIST, MYF(0));
-    DBUG_RETURN(0);
+    DBUG_RETURN(1);
   }
 
   if (!(parser= sql_parse_prepare(&path, thd->mem_root, 1)))
-    DBUG_RETURN(0);
+    DBUG_RETURN(1);
 
   if (!is_equal(&trigname_file_type, parser->type()))
   {
     my_error(ER_WRONG_OBJECT, MYF(0), trig->m_name.str, trigname_file_ext+1,
              "TRIGGERNAME");
-    DBUG_RETURN(0);
+    DBUG_RETURN(1);
   }
 
   if (parser->parse((gptr)&trigname, thd->mem_root,
                     trigname_file_parameters, 1,
                     &trigger_table_hook))
-    DBUG_RETURN(0);
+    DBUG_RETURN(1);
 
   /* We need to reset statement table list to be PS/SP friendly. */
   lex->query_tables= 0;
   lex->query_tables_last= &lex->query_tables;
-  DBUG_RETURN(sp_add_to_query_tables(thd, lex, trig->m_db.str,
-                                     trigname.trigger_table.str, TL_IGNORE));
+  *table= sp_add_to_query_tables(thd, lex, trig->m_db.str,
+                                 trigname.trigger_table.str, TL_IGNORE);
+
+  if (! *table)
+    DBUG_RETURN(1);
+
+  DBUG_RETURN(0);
 }
 
 

sql/sql_yacc.yy

@@ -6098,11 +6098,12 @@ drop:
 	    lex->sql_command= SQLCOM_DROP_VIEW;
 	    lex->drop_if_exists= $3;
 	  }
-        | DROP TRIGGER_SYM sp_name
+        | DROP TRIGGER_SYM if_exists sp_name
           {
             LEX *lex= Lex;
             lex->sql_command= SQLCOM_DROP_TRIGGER;
-            lex->spname= $3;
+            lex->drop_if_exists= $3;
+            lex->spname= $4;
           }
 	;