WL#1034 ongoing updates after review

sql/event.cc:
  -my_error() as close as possible to the place where the error
   occurs.
  -a thought how to replicate events
  -use close_thread_tables() in some cases and for others rely on
   this call being done in sql_parse.cc::do_command()
sql/event.h:
  remove redundant defines
sql/event_executor.cc:
  - reenable the compilation again
  - don't backup the open_tables_state, it's not needed
sql/event_timed.cc:
  - inline a bit
  - comment added
sql/mysqld.cc:
  - start mysqld with --event-scheduler=0 by default
sql/share/errmsg.txt:
  3 new messages
sql/sql_parse.cc:
  remove now obsolete error checking - the errors are reported
  as closer as possible to the place where they are detected
sql/sql_yacc.yy:
  add WARNING message. fix a bug that was corrupting 
  thd->client_capabilites ->
  select count(*) from mysql.event
  was reporting : "Unknown table test.event"!!!
  Using temporal variable is nice but IMO quite error-prone.

sql/event.cc

@@ -31,6 +31,18 @@
 
  - CREATE EVENT should not go into binary log! Does it now? The SQL statements
    issued by the EVENT are replicated.
+   I have an idea how to solve the problem at failover. So the status field
+   will be ENUM('DISABLED', 'ENABLED', 'SLAVESIDE_DISABLED').
+   In this case when CREATE EVENT is replicated it should go into the binary
+   as SLAVESIDE_DISABLED if it is ENABLED, when it's created as DISABLEd it
+   should be replicated as disabled. If an event is ALTERed as DISABLED the
+   query should go untouched into the binary log, when ALTERed as enable then
+   it should go as SLAVESIDE_DISABLED. This is regarding the SQL interface.
+   TT routines however modify mysql.event internally and this does not go the log
+   so in this case queries has to be injected into the log...somehow... or
+   maybe a solution is RBR for this case, because the event may go only from
+   ENABLED to DISABLED status change and this is safe for replicating. As well
+   an event may be deleted which is also safe for RBR.
 
  - Add a lock and use it for guarding access to events_array dynamic array.
 
@@ -225,13 +237,13 @@ evex_db_find_routine_aux(THD *thd, const LEX_STRING dbname,
   */
   if (rname.length > table->field[1]->field_length)
     DBUG_RETURN(SP_KEY_NOT_FOUND);
+
   table->field[0]->store(dbname.str, dbname.length, &my_charset_bin);
   table->field[1]->store(rname.str, rname.length, &my_charset_bin);
   key_copy(key, table->record[0], table->key_info, table->key_info->key_length);
 
-  if (table->file->index_read_idx(table->record[0], 0,
-				  key, table->key_info->key_length,
-				  HA_READ_KEY_EXACT))
+  if (table->file->index_read_idx(table->record[0], 0, key,
+                                  table->key_info->key_length,HA_READ_KEY_EXACT))
     DBUG_RETURN(SP_KEY_NOT_FOUND);
 
   DBUG_RETURN(0);
@@ -256,11 +268,13 @@ static int
 evex_fill_row(THD *thd, TABLE *table, event_timed *et, my_bool is_update)
 {
   DBUG_ENTER("evex_fill_row");
-  int ret=0;
 
   if (table->s->fields != EVEX_FIELD_COUNT)
+  {
+    my_error(ER_EVENT_COL_COUNT_DOESNT_MATCH, MYF(0), "mysql", "event");
     DBUG_RETURN(EVEX_GET_FIELD_FAILED);
-
+  }
+  
   DBUG_PRINT("info", ("m_db.len=%d",et->m_db.length));  
   DBUG_PRINT("info", ("m_name.len=%d",et->m_name.length));  
 
@@ -361,27 +375,24 @@ db_create_event(THD *thd, event_timed *et)
   if (!(table= evex_open_event_table(thd, TL_WRITE)))
   {
     my_error(ER_EVENT_OPEN_TABLE_FAILED, MYF(0));
-    goto done;
+    goto err;
   }
 
   DBUG_PRINT("info", ("check existance of an event with the same name"));
   if (!evex_db_find_routine_aux(thd, et->m_db, et->m_name, table))
   {
     my_error(ER_EVENT_ALREADY_EXISTS, MYF(0), et->m_name.str);
-    goto done;    
+    goto err;    
   }
 
   DBUG_PRINT("info", ("non-existant, go forward"));
-  if ((ret= sp_use_new_db(thd, et->m_db.str, olddb, sizeof(olddb),
-			  0, &dbchanged)))
+  if ((ret= sp_use_new_db(thd, et->m_db.str,olddb, sizeof(olddb),0, &dbchanged)))
   {
-    DBUG_PRINT("info", ("cannot use_new_db. code=%d", ret));
     my_error(ER_BAD_DB_ERROR, MYF(0));
-    DBUG_RETURN(0);
+    goto err;
   }
   
   restore_record(table, s->default_values); // Get default values for fields
-  strxmov(definer, et->m_definer_user.str, "@", et->m_definer_host.str, NullS);
 
 /* TODO : Uncomment these and add handling in sql_parse.cc or here
 
@@ -400,23 +411,29 @@ db_create_event(THD *thd, event_timed *et)
   {
     DBUG_PRINT("error", ("neither m_expr nor m_execute_as are set!"));
     my_error(ER_EVENT_NEITHER_M_EXPR_NOR_M_AT, MYF(0));
-    goto done;
+    
+    goto err;
   }
 
+  strxmov(definer, et->m_definer_user.str, "@", et->m_definer_host.str, NullS);
   if (table->field[EVEX_FIELD_DEFINER]->
-       store(definer, (uint)strlen(definer), system_charset_info))
+       store(definer, et->m_definer_user.length + 1 + et->m_definer_host.length,
+             system_charset_info))
   {
     my_error(ER_EVENT_STORE_FAILED, MYF(0), et->m_name.str);
-    goto done;
+    goto err;
   }
 
   ((Field_timestamp *)table->field[EVEX_FIELD_CREATED])->set_time();
   if ((ret= evex_fill_row(thd, table, et, false)))
-    goto done; 
+    goto err; 
 
   ret= EVEX_OK;
   if (table->file->write_row(table->record[0]))
+  {
     my_error(ER_EVENT_STORE_FAILED, MYF(0), et->m_name.str);
+    goto err;
+  }
   else if (mysql_bin_log.is_open())
   {
     thd->clear_error();
@@ -425,12 +442,15 @@ db_create_event(THD *thd, event_timed *et)
     mysql_bin_log.write(&qinfo);
   }
 
-done:
   // No need to close the table, it will be closed in sql_parse::do_command
+  if (dbchanged)
+    (void) mysql_change_db(thd, olddb, 1);
+  DBUG_RETURN(EVEX_OK);
 
+err:
   if (dbchanged)
     (void) mysql_change_db(thd, olddb, 1);
-  DBUG_RETURN(ret);
+  DBUG_RETURN(EVEX_GENERAL_ERROR);
 }
 
 
@@ -462,20 +482,23 @@ db_update_event(THD *thd, sp_name *name, event_timed *et)
   if (!(table= evex_open_event_table(thd, TL_WRITE)))
   {
     my_error(ER_EVENT_OPEN_TABLE_FAILED, MYF(0));
-    goto done;
+    goto err;
   }
 
   if (evex_db_find_routine_aux(thd, et->m_db, et->m_name, table) == SP_KEY_NOT_FOUND)
   {
     my_error(ER_EVENT_DOES_NOT_EXIST, MYF(0), et->m_name.str);
-    goto done;    
+    goto err;    
   }
   
   store_record(table,record[1]);
   
-  table->timestamp_field_type= TIMESTAMP_NO_AUTO_SET; // Don't update create on row update.
+  // Don't update create on row update.
+  table->timestamp_field_type= TIMESTAMP_NO_AUTO_SET;
+
+  // evex_fill_row() calls my_error() in case of error so no need to handle it here
   if ((ret= evex_fill_row(thd, table, et, true)))
-    goto done;
+    goto err;
    
   if (name)
   {    
@@ -485,14 +508,20 @@ db_update_event(THD *thd, sp_name *name, event_timed *et)
       store(name->m_name.str, name->m_name.length, system_charset_info);
   }
 
-  if ((ret= table->file->update_row(table->record[1],table->record[0])))
+  if ((ret= table->file->update_row(table->record[1], table->record[0])))
   {
     my_error(ER_EVENT_STORE_FAILED, MYF(0), et->m_name.str);
-    goto done;
+    goto err;
   }
-done:
+
+  // close mysql.event or we crash later when loading the event from disk
   close_thread_tables(thd);
-  DBUG_RETURN(ret);
+  DBUG_RETURN(0);
+
+err:
+  if (table)
+    close_thread_tables(thd);
+  DBUG_RETURN(EVEX_GENERAL_ERROR);
 }
 
 
@@ -500,7 +529,7 @@ db_update_event(THD *thd, sp_name *name, event_timed *et)
     Use sp_name for look up, return in **ett if found
 */
 static int
-db_find_event(THD *thd, sp_name *name, event_timed **ett)
+db_find_event(THD *thd, sp_name *name, event_timed **ett, TABLE *tbl)
 {
   TABLE *table;
   int ret;
@@ -508,27 +537,35 @@ db_find_event(THD *thd, sp_name *name, event_timed **ett)
   char *ptr;
   event_timed *et;  
   DBUG_ENTER("db_find_event");
-  DBUG_PRINT("enter", ("name: %*s",
-		       name->m_name.length, name->m_name.str));
-
+  DBUG_PRINT("enter", ("name: %*s", name->m_name.length, name->m_name.str));
 
-  if (!(table= evex_open_event_table(thd, TL_READ)))
+  if (tbl)
+    table= tbl;
+  else if (!(table= evex_open_event_table(thd, TL_READ)))
   {
     my_error(ER_EVENT_OPEN_TABLE_FAILED, MYF(0));
+    ret= EVEX_GENERAL_ERROR;
     goto done;
   }
 
   if ((ret= evex_db_find_routine_aux(thd, name->m_db, name->m_name, table)))
-    goto done;
-
+  {
+    my_error(ER_EVENT_DOES_NOT_EXIST, MYF(0), et->m_name.str);
+    goto done;    
+  }
   et= new event_timed;
   
   /*
-    The table should not be closed beforehand. 
-    ::load_from_row only loads and does not compile
+    1)The table should not be closed beforehand.  ::load_from_row() only loads
+      and does not compile
+
+    2)::load_from_row() is silent on error therefore we emit error msg here
   */
   if ((ret= et->load_from_row(&evex_mem_root, table)))
+  {
+    my_error(ER_EVENT_CANNOT_LOAD_FROM_TABLE, MYF(0));
     goto done;
+  }
 
 done:
   if (ret && et)
@@ -536,7 +573,9 @@ db_find_event(THD *thd, sp_name *name, event_timed **ett)
     delete et;
     et= 0;
   }
-  close_thread_tables(thd);
+  // don't close the table if we haven't opened it ourselves
+  if (!tbl)
+    close_thread_tables(thd);
   *ett= et;
   DBUG_RETURN(ret);
 }
@@ -555,22 +594,16 @@ evex_load_and_compile_event(THD * thd, sp_name *spn, bool use_lock)
   tmp_mem_root= thd->mem_root;
   thd->mem_root= &evex_mem_root;
 
-  if (db_find_event(thd, spn, &ett))
-  {
-    ret= EVEX_GENERAL_ERROR;
+  // no need to use my_error() here because db_find_event() has done it
+  if ((ret= db_find_event(thd, spn, &ett, NULL)))
     goto done;
-  }
 
   /*
-    allocate on evex_mem_root. call without evex_mem_root and
-    and m_sphead will not be cleared!
+    allocate on evex_mem_root. if you call without evex_mem_root
+    then m_sphead will not be cleared!
   */
-
   if ((ret= ett->compile(thd, &evex_mem_root)))
-  {
-    thd->mem_root= tmp_mem_root;
     goto done;
-  }
   
   ett->compute_next_execution_time();
   if (use_lock)
@@ -589,7 +622,7 @@ evex_load_and_compile_event(THD * thd, sp_name *spn, bool use_lock)
   delete ett;
 
   /*
-    We find where the first element resides in the arraay. And then do a
+    We find where the first element resides in the array. And then do a
     qsort of events_array.elements (the current number of elements).
     We know that the elements are stored in a contiguous block w/o holes.
   */
@@ -613,7 +646,6 @@ static int
 evex_remove_from_cache(LEX_STRING *db, LEX_STRING *name, bool use_lock)
 {
   uint i;
-  int ret= 0;
   bool need_second_pass= true;
 
   DBUG_ENTER("evex_remove_from_cache");
@@ -690,7 +722,7 @@ evex_remove_from_cache(LEX_STRING *db, LEX_STRING *name, bool use_lock)
   if (use_lock)
     VOID(pthread_mutex_unlock(&LOCK_event_arrays));
 
-  DBUG_RETURN(ret);
+  DBUG_RETURN(0);
 }
 
 
@@ -711,8 +743,6 @@ evex_remove_from_cache(LEX_STRING *db, LEX_STRING *name, bool use_lock)
    NOTES
      - in case there is an event with the same name (db) and 
        IF NOT EXISTS is specified, an warning is put into the W stack.
-   TODO
-     - Add code for in-memory structures - caching & uncaching.
 */
 
 int
@@ -779,8 +809,6 @@ evex_create_event(THD *thd, event_timed *et, uint create_options)
      et contains data about dbname and event name. 
      name is the new name of the event, if not null this means
      that RENAME TO was specified in the query.
-   TODO
-     - Add code for in-memory structures - caching & uncaching.
 */
 
 int
@@ -800,16 +828,19 @@ evex_update_event(THD *thd, sp_name *name, event_timed *et)
   {}  
   VOID(pthread_mutex_unlock(&LOCK_evex_running));
 */
-
+  /*
+    db_update_event() opens & closes the table to prevent
+    crash later in the code when loading and compiling the new definition
+  */
   if ((ret= db_update_event(thd, name, et)))
-    goto done_no_evex;
+    goto done;
 
   VOID(pthread_mutex_lock(&LOCK_evex_running));
   if (!evex_is_running)
   {
     // not running - therefore no memory structures
     VOID(pthread_mutex_unlock(&LOCK_evex_running));
-    goto done_no_evex;
+    goto done;
   }
   VOID(pthread_mutex_unlock(&LOCK_evex_running));
 
@@ -828,12 +859,9 @@ evex_update_event(THD *thd, sp_name *name, event_timed *et)
       ret= evex_load_and_compile_event(thd, spn, false);
       delete spn;
     }
-
-done:
   VOID(pthread_mutex_unlock(&LOCK_event_arrays));
 
-done_no_evex:
-  // No need to close the table, it will be closed in sql_parse::do_command
+done:
 
   DBUG_RETURN(ret);
 }
@@ -848,8 +876,6 @@ evex_update_event(THD *thd, sp_name *name, event_timed *et)
      et              event's name
      drop_if_exists  if set and the event not existing => warning onto the stack
           
- TODO
-   Update in-memory structures
 */
 
 int
@@ -875,9 +901,9 @@ evex_drop_event(THD *thd, event_timed *et, bool drop_if_exists)
 
   if (ret == EVEX_OK)
   {
-    if (table->file->delete_row(table->record[0]))
+    if (ret= table->file->delete_row(table->record[0]))
     { 	
-      ret= EVEX_DELETE_ROW_FAILED;
+      my_error(ER_EVENT_CANNOT_DELETE, MYF(0));
       goto done;
     }
   }
@@ -897,7 +923,10 @@ evex_drop_event(THD *thd, event_timed *et, bool drop_if_exists)
   VOID(pthread_mutex_unlock(&LOCK_evex_running));
 
 done:  
-  // No need to close the table, it will be closed in sql_parse::do_command
+  /*
+     No need to close the table, it will be closed in sql_parse::do_command()
+     and evex_remove_from_cache does not try to open a table
+  */
 
   DBUG_RETURN(ret);
 }

sql/event.h

@@ -40,17 +40,6 @@ extern ulong opt_event_executor;
 #define EVENT_EXEC_NO_MORE      (1L << 0)
 #define EVENT_NOT_USED          (1L << 1)
 
-#define SP_OK                 0
-#define SP_KEY_NOT_FOUND     -1
-#define SP_OPEN_TABLE_FAILED -2
-#define SP_WRITE_ROW_FAILED  -3
-#define SP_DELETE_ROW_FAILED -4
-#define SP_GET_FIELD_FAILED  -5
-#define SP_PARSE_ERROR       -6
-#define SP_INTERNAL_ERROR    -7
-#define SP_NO_DB_ERROR       -8
-#define SP_BAD_IDENTIFIER    -9
-#define SP_BODY_TOO_LONG    -10
 
 extern ulong opt_event_executor;
 

sql/event_executor.cc

@@ -80,8 +80,6 @@ init_events()
   event_executor_running_global_var= false;
   VOID(pthread_mutex_unlock(&LOCK_evex_running));
 
-  DBUG_RETURN(0);
-/*
 #ifndef DBUG_FAULTY_THR
   //TODO Andrey: Change the error code returned!
   if (pthread_create(&th, NULL, event_executor_main, (void*)NULL))
@@ -91,7 +89,6 @@ init_events()
 #endif
 
   DBUG_RETURN(0);
-*/
 }
 
 
@@ -109,7 +106,6 @@ shutdown_events()
   DBUG_VOID_RETURN;
 }
 
-#ifdef ANDREY_0
 
 static int
 init_event_thread(THD* thd)
@@ -479,13 +475,11 @@ evex_load_events_from_db(THD *thd)
   TABLE *table;
   READ_RECORD read_record_info;
   MYSQL_LOCK *lock;
-  Open_tables_state open_tables_state_backup;
   int ret= -1;
   
   DBUG_ENTER("evex_load_events_from_db");  
 
-  if (!(table= open_proc_type_table_for_read(thd, &open_tables_state_backup,
-                                             "event", &mysql_event_table_exists)))
+  if (!(table= evex_open_event_table(thd, TL_READ)))
     DBUG_RETURN(SP_OPEN_TABLE_FAILED);
 
   VOID(pthread_mutex_lock(&LOCK_event_arrays));
@@ -541,18 +535,16 @@ evex_load_events_from_db(THD *thd)
 
 end:
   close_thread_tables(thd);
-  thd->restore_backup_open_tables_state(&open_tables_state_backup);
 
   DBUG_PRINT("evex_load_events_from_db",
                     ("Events loaded from DB. Status code %d", ret));
   DBUG_RETURN(ret);
 }
 
-#endif
+
 
 bool sys_var_event_executor::update(THD *thd, set_var *var)
 {
-#ifdef ANDREY_0
   // here start the thread if not running.
   VOID(pthread_mutex_lock(&LOCK_evex_running));
   if ((my_bool) var->save_result.ulong_value && !evex_is_running) {
@@ -560,7 +552,7 @@ bool sys_var_event_executor::update(THD *thd, set_var *var)
     init_events();
   } else 
     VOID(pthread_mutex_unlock(&LOCK_evex_running));
-#endif  
+
   return sys_var_bool_ptr::update(thd, var);
 }
 

sql/event_timed.cc

@@ -322,10 +322,8 @@ event_timed::init_comment(THD *thd, LEX_STRING *comment)
 {
   DBUG_ENTER("event_timed::init_comment");
 
-  MEM_ROOT *root= thd->mem_root;
-  m_comment.length= comment->length;
-  m_comment.str= strmake_root(root, comment->str, comment->length);
-  DBUG_PRINT("m_comment", ("len=%d",m_comment.length));
+  m_comment.str= strmake_root(thd->mem_root, comment->str,
+                              m_comment.length= comment->length);
 
   DBUG_VOID_RETURN;
 }
@@ -359,6 +357,11 @@ event_timed::init_definer(THD *thd)
  
  SYNOPSIS
    event_timed::load_from_row()
+   
+ REMARKS
+   This method is silent on errors and should behave like that. Callers
+   should handle throwing of error messages. The reason is that the class
+   should not know about how to deal with communication.
 */
 
 int

sql/mysqld.cc

@@ -4810,7 +4810,7 @@ Disable with --skip-bdb (will save memory).",
    0, GET_BOOL, NO_ARG, 0, 0, 0, 0, 0, 0},
   {"event-scheduler", OPT_EVENT_EXECUTOR, "Enable/disable the event scheduler.",
    (gptr*) &opt_event_executor, (gptr*) &opt_event_executor, 0, GET_BOOL, NO_ARG,
-   1/*default*/, 0/*min-value*/, 1/*max-value*/, 0, 0, 0},
+   0/*default*/, 0/*min-value*/, 1/*max-value*/, 0, 0, 0},
   {"exit-info", 'T', "Used for debugging;  Use at your own risk!", 0, 0, 0,
    GET_LONG, OPT_ARG, 0, 0, 0, 0, 0, 0},
   {"external-locking", OPT_USE_LOCKING, "Use system (external) locking.  With this option enabled you can run myisamchk to test (not repair) tables while the MySQL server is running.",

sql/share/errmsg.txt

@@ -5741,3 +5741,9 @@ ER_EVENT_OPEN_TABLE_FAILED
         eng "Failed to open mysql.event"
 ER_EVENT_NEITHER_M_EXPR_NOR_M_AT
         eng "No datetime expression provided"
+ER_EVENT_COL_COUNT_DOESNT_MATCH
+        eng "Column count of %s.%s is wrong. Table probably corrupted"
+ER_EVENT_CANNOT_LOAD_FROM_TABLE
+        eng "Cannot load from mysql.event. Table probably corrupted"
+ER_EVENT_CANNOT_DELETE
+        eng "Failed to delete the event from mysql.event"

sql/sql_parse.cc

@@ -3689,21 +3689,8 @@ mysql_execute_command(THD *thd)
     int result;
     uint create_options= lex->create_info.options;
     res= (result= evex_create_event(thd, lex->et, create_options));
-    switch (result) {
-    case EVEX_OK:
+    if (result == EVEX_OK)
       send_ok(thd, 1);
-      break;
-    case EVEX_WRITE_ROW_FAILED:
-      my_error(ER_EVENT_ALREADY_EXISTS, MYF(0), lex->et->m_name.str);
-      break;
-    case EVEX_NO_DB_ERROR:
-      my_error(ER_BAD_DB_ERROR, MYF(0), lex->et->m_db.str);
-      break;
-    default:
-      //includes EVEX_PARSE_ERROR
-      my_error(ER_EVENT_STORE_FAILED, MYF(0), lex->et->m_name.str);
-      break;
-    }
     /* lex->unit.cleanup() is called outside, no need to call it here */
     delete lex->et;
     lex->et= 0;

sql/sql_yacc.yy

@@ -1312,6 +1312,10 @@ create:
 	    Lex->sql_command = SQLCOM_CREATE_USER;
           }
 	| CREATE EVENT_SYM opt_if_not_exists sp_name
+          /* 
+             BE CAREFUL when you add a new rule to update the block where 
+             YYTHD->client_capabilities is set back to original value
+          */
           {
             LEX *lex=Lex;
 
@@ -1351,12 +1355,19 @@ create:
           DO_SYM ev_sql_stmt
           {
             /*
-              sql_command is set here because some rules in ev_sql_stmt
-              can overwrite it
+              Restore flag if it was cleared above
+              $1 - CREATE
+              $2 - EVENT_SYM
+              $3 - opt_if_not_exists
+              $4 - sp_name
+              $5 - the block above
             */
-            // Restore flag if it was cleared above
             YYTHD->client_capabilities |= $<ulong_num>5;
 
+            /*
+              sql_command is set here because some rules in ev_sql_stmt
+              can overwrite it
+            */
             Lex->sql_command= SQLCOM_CREATE_EVENT;
           }
       ;
@@ -4192,6 +4203,10 @@ alter:
 	  view_list_opt AS view_select view_check_option
 	  {}
 	| ALTER EVENT_SYM sp_name
+          /* 
+             BE CAREFUL when you add a new rule to update the block where 
+             YYTHD->client_capabilities is set back to original value
+          */
           {
             LEX *lex=Lex;
             event_timed *et;
@@ -4233,11 +4248,18 @@ alter:
           ev_comment
           ev_opt_sql_stmt
           {
+            /*
+              $1 - ALTER
+              $2 - EVENT_SYM
+              $3 - sp_name
+              $4 - the block above
+            */
+            YYTHD->client_capabilities |= $<ulong_num>4;
+
             /*
               sql_command is set here because some rules in ev_sql_stmt
               can overwrite it
             */
-            YYTHD->client_capabilities |= $<ulong_num>3;
             Lex->sql_command= SQLCOM_ALTER_EVENT;
           }	  
       ;