WL#3337 (Event scheduler new architecture)

Post-review fixes. Mostly whitespace, int-to-bool return value, fixed comments


sql/Makefile.am:
  compile all submodules of Events before compiling the facade
sql/event_data_objects.cc:
  - Use initialization list
  - Clean whitespaces
  - Shorten comments
  - Fix comments
sql/event_data_objects.h:
  - Fix whitespace
sql/event_db_repository.cc:
  - Change return type from int to bool where only one error code is
    returned.
  - Don't use macros but get the maximal number of characters in a column
    from the column
  - Fix  comments
  - Make functions which has return value but it's not used - void.
sql/event_db_repository.h:
  - Methods with only one error code int -> bool return value
  - Remove declaration of fill_schema_events, a function that does not exist
sql/event_queue.cc:
  - Use initialization lists
  - Let find_n_remove_event delete the object thus making the code more robust.
    The caller could forget to destruct the object. In addition, find_n_remove_element()
    does not return a value.
  - Move check_system_tables() to class Events
  - Fix comments
sql/event_queue.h:
  - Whitespace changes
  - init_queue() should allow passing of THD
  - check_system_tables moved to class Events
  - find_n_remove_event() is now void
sql/event_scheduler.cc:
  - Initialize res before use
  - Remove end stop from message
sql/event_scheduler.h:
  Add uninitialized state. The scheduler is in it before init_scheduler()
  is called. The rationale is that otherwise state has no value before
  the call. If the system tables were damaged the scheduler won't be initialized
  but in Events::deinit() Event_scheduler::stop() will be called and this will
  touch state, generating valgrind warning at minimum.
sql/events.cc:
  - Whitespace changes
  - Fix comments
  - Make methods which have only one error code be bool instead of int
  - Create temporarily a THD to be used for the initialization of Event_queue
  - Event_queue::check_system_tables() moved to Events::check_system_tables
  - is_started() is renamed to is_execution_of_events_started()
sql/events.h:
  - Whitespace changes
  - When a method returns only one error code it should be bool, not int
  - is_started() renamed to is_execution_of_events_started()
sql/set_var.cc:
  is_started() is renamed to is_execution_of_events_started()
sql/sql_db.cc:
  The return code is not used, thus don't return anything and drop_schema_events()
  is now void.
sql/sql_yacc.yy:
  - Fix comments
  - Remove unneeded initialization which is performed in lex_init()
sql/share/errmsg.txt:
  New error message
sql/table.cc:
  - Fix comments
  - make table_check_intact() accespt const *table_def
sql/table.h:
  Make table_check_intact() accespt const *table_def

sql/Makefile.am

@@ -105,8 +105,8 @@ mysqld_SOURCES =	sql_lex.cc sql_handler.cc sql_partition.cc \
 			tztime.cc my_time.c my_user.c my_decimal.cc\
 			sp_head.cc sp_pcontext.cc  sp_rcontext.cc sp.cc \
 			sp_cache.cc parse_file.cc sql_trigger.cc \
-                        event_scheduler.cc events.cc event_data_objects.cc \
-                        event_queue.cc event_db_repository.cc \
+                        event_scheduler.cc event_data_objects.cc \
+                        event_queue.cc event_db_repository.cc events.cc \
 			sql_plugin.cc sql_binlog.cc \
 			sql_builtin.cc sql_tablespace.cc partition_info.cc
 

sql/event_data_objects.cc

@@ -59,19 +59,17 @@ Event_parse_data::new_instance(THD *thd)
 */
 
 Event_parse_data::Event_parse_data()
+  :on_completion(ON_COMPLETION_DROP), status(ENABLED),
+   item_starts(NULL), item_ends(NULL), item_execute_at(NULL),
+   starts_null(TRUE), ends_null(TRUE), execute_at_null(TRUE),
+   item_expression(NULL), expression(0)
 {
   DBUG_ENTER("Event_parse_data::Event_parse_data");
 
-  item_execute_at= item_expression= item_starts= item_ends= NULL;
-  status= ENABLED;
-  on_completion= ON_COMPLETION_DROP;
-  expression= 0;
-
   /* Actually in the parser STARTS is always set */
   set_zero_time(&starts, MYSQL_TIMESTAMP_DATETIME);
   set_zero_time(&ends, MYSQL_TIMESTAMP_DATETIME);
   set_zero_time(&execute_at, MYSQL_TIMESTAMP_DATETIME);
-  starts_null= ends_null= execute_at_null= TRUE;
 
   body.str= comment.str= NULL;
   body.length= comment.length= 0;
@@ -736,8 +734,8 @@ Event_timed::~Event_timed()
     Event_job_data::Event_job_data()
 */
 
-Event_job_data::Event_job_data():
-  thd(NULL), sphead(NULL), sql_mode(0)
+Event_job_data::Event_job_data()
+  :thd(NULL), sphead(NULL), sql_mode(0)
 {
 }
 
@@ -1115,14 +1113,16 @@ bool get_next_time(TIME *next, TIME *start, TIME *time_now, TIME *last_exec,
     interval.month= (diff_months / months)*months;
     /*
       Check if the same month as last_exec (always set - prerequisite)
-      An event happens at most once per month so there is no way to schedule
-      it two times for the current month. This saves us from two calls to
-      date_add_interval() if the event was just executed.  But if the scheduler
-      is started and there was at least 1 scheduled date skipped this one does
-      not help and two calls to date_add_interval() will be done, which is a
-      bit more expensive but compared to the rareness of the case is neglectable.
+      An event happens at most once per month so there is no way to
+      schedule it two times for the current month. This saves us from two
+      calls to date_add_interval() if the event was just executed.  But if
+      the scheduler is started and there was at least 1 scheduled date
+      skipped this one does not help and two calls to date_add_interval()
+      will be done, which is a bit more expensive but compared to the
+      rareness of the case is neglectable.
     */
-    if (time_now->year==last_exec->year && time_now->month==last_exec->month)
+    if (time_now->year == last_exec->year &&
+        time_now->month == last_exec->month)
       interval.month+= months;
 
     tmp= *start;
@@ -1454,7 +1454,8 @@ Event_queue_element::drop(THD *thd)
 
   RETURN VALUE
     FALSE   OK
-    TRUE    Error while opening mysql.event for writing or during write on disk
+    TRUE    Error while opening mysql.event for writing or during
+            write on disk
 */
 
 bool
@@ -1645,9 +1646,9 @@ Event_job_data::execute(THD *thd)
   event_change_security_context(thd, definer_user, definer_host, dbname,
                                 &save_ctx);
   /*
-    THD::~THD will clean this or if there is DROP DATABASE in the SP then
-    it will be free there. It should not point to our buffer which is allocated
-    on a mem_root.
+    THD::~THD will clean this or if there is DROP DATABASE in the
+    SP then it will be free there. It should not point to our buffer
+    which is allocated on a mem_root.
   */
   thd->db= my_strdup(dbname.str, MYF(0));
   thd->db_length= dbname.length;
@@ -1719,7 +1720,6 @@ Event_job_data::compile(THD *thd, MEM_ROOT *mem_root)
 
   switch (get_fake_create_event(thd, &show_create)) {
   case EVEX_MICROSECOND_UNSUP:
-    sql_print_error("Scheduler");
     DBUG_RETURN(EVEX_MICROSECOND_UNSUP);
   case 0:
     break;
@@ -1769,7 +1769,8 @@ Event_job_data::compile(THD *thd, MEM_ROOT *mem_root)
       Free lex associated resources
       QQ: Do we really need all this stuff here?
     */
-    sql_print_error("error during compile of %s.%s or thd->is_fatal_error=%d",
+    sql_print_error("SCHEDULER: Error during compilation of %s.%s or "
+                    "thd->is_fatal_error=%d",
                     dbname.str, name.str, thd->is_fatal_error);
 
     lex.unit.cleanup();
@@ -1832,10 +1833,13 @@ event_basic_db_equal(LEX_STRING db, Event_basic *et)
 
 
 /*
-  Checks whether two events are equal by identifiers
+  Checks whether an event has equal `db` and `name`
 
   SYNOPSIS
     event_basic_identifier_equal()
+      db   Schema
+      name Name
+      et   The event object
 
   RETURN VALUE
     TRUE   Equal
@@ -1851,7 +1855,8 @@ event_basic_identifier_equal(LEX_STRING db, LEX_STRING name, Event_basic *b)
 
 
 /*
-  Switches the security context
+  Switches the security context.
+
   SYNOPSIS
     event_change_security_context()
       thd     Thread
@@ -1887,7 +1892,8 @@ event_change_security_context(THD *thd, LEX_STRING user, LEX_STRING host,
 
 
 /*
-  Restores the security context
+  Restores the security context.
+
   SYNOPSIS
     event_restore_security_context()
       thd     Thread

sql/event_data_objects.h

@@ -237,7 +237,7 @@ public:
   new_instance(THD *thd);
 
   bool
-  check_parse_data(THD *);
+  check_parse_data(THD *thd);
 
   void
   init_body(THD *thd);

sql/event_db_repository.h

@@ -47,9 +47,6 @@ events_table_index_read_for_db(THD *thd, TABLE *schema_table,
 int
 events_table_scan_all(THD *thd, TABLE *schema_table, TABLE *event_table);
 
-int
-fill_schema_events(THD *thd, TABLE_LIST *tables, COND * /* cond */);
-
 class Event_basic;
 class Event_parse_data;
 
@@ -58,7 +55,7 @@ class Event_db_repository
 public:
   Event_db_repository(){}
 
-  int
+  bool
   create_event(THD *thd, Event_parse_data *parse_data, my_bool create_if_not,
                uint *rows_affected);
 
@@ -70,7 +67,7 @@ public:
   drop_event(THD *thd, LEX_STRING db, LEX_STRING name, bool drop_if_exists,
              uint *rows_affected);
 
-  int
+  void
   drop_schema_events(THD *thd, LEX_STRING schema);
 
   bool
@@ -79,7 +76,6 @@ public:
   bool
   load_named_event(THD *thd, LEX_STRING dbname, LEX_STRING name, Event_basic *et);
 
-
   int
   open_event_table(THD *thd, enum thr_lock_type lock_type, TABLE **table);
 
@@ -87,14 +83,14 @@ public:
   fill_schema_events(THD *thd, TABLE_LIST *tables, char *db);
 
 private:
-  int
+  void
   drop_events_by_field(THD *thd, enum enum_events_table_field field,
                        LEX_STRING field_value);
-  int
+  bool
   index_read_for_db_for_i_s(THD *thd, TABLE *schema_table, TABLE *event_table,
                             char *db);
 
-  int
+  bool
   table_scan_all_for_i_s(THD *thd, TABLE *schema_table, TABLE *event_table);
 
   static bool

sql/event_queue.h

@@ -36,7 +36,7 @@ public:
   deinit_mutexes();
 
   bool
-  init_queue(Event_db_repository *db_repo, Event_scheduler *sched);
+  init_queue(THD *thd, Event_db_repository *db_repo, Event_scheduler *sched);
   
   void
   deinit_queue();
@@ -56,9 +56,6 @@ public:
   void
   drop_schema_events(THD *thd, LEX_STRING schema);
 
-  void
-  check_system_tables(THD *thd);
-
   void
   recalculate_activation_times(THD *thd);
 
@@ -72,7 +69,7 @@ public:
   load_events_from_db(THD *thd);
 
 protected:
-  Event_queue_element *
+  void
   find_n_remove_event(LEX_STRING db, LEX_STRING name);
 
 

sql/event_scheduler.cc

@@ -384,7 +384,7 @@ Event_scheduler::start()
 
   if (!(new_thd= new THD))
   {
-    sql_print_error("SCHEDULER: Cannot init manager event thread.");
+    sql_print_error("SCHEDULER: Cannot init manager event thread");
     ret= TRUE;
     goto end;
   }
@@ -441,7 +441,7 @@ end:
 bool
 Event_scheduler::run(THD *thd)
 {
-  int res;
+  int res= FALSE;
   struct timespec abstime;
   Event_job_data *job_data;
   DBUG_ENTER("Event_scheduler::run");
@@ -464,7 +464,7 @@ Event_scheduler::run(THD *thd)
                                              &job_data, &abstime))
     {
       sql_print_information("SCHEDULER: Serious error during getting next"
-                            " event to execute. Stopping.");
+                            " event to execute. Stopping");
       break;
     }
 
@@ -532,7 +532,7 @@ Event_scheduler::execute_top(THD *thd, Event_job_data *job_data)
   pthread_t th;
   int res= 0;
   DBUG_ENTER("Event_scheduler::execute_top");
-  if (!(new_thd= new THD))
+  if (!(new_thd= new THD()))
     goto error;
 
   pre_init_event_thread(new_thd);

sql/event_scheduler.h

@@ -31,12 +31,13 @@ deinit_event_thread(THD *thd);
 class Event_scheduler
 {
 public:
-  Event_scheduler(){}
+  Event_scheduler():state(UNINITIALIZED){}
   ~Event_scheduler(){}
 
   enum enum_state
   {
-    INITIALIZED = 0,
+    UNINITIALIZED = 0,
+    INITIALIZED,
     RUNNING,
     STOPPING
   };

sql/events.h

@@ -42,12 +42,12 @@ sortcmp_lex_string(LEX_STRING s, LEX_STRING t, CHARSET_INFO *cs);
 class Events
 {
 public:
-  friend class Event_queue_element;
   /*
     Quite NOT the best practice and will be removed once
     Event_timed::drop() and Event_timed is fixed not do drop directly
     or other scheme will be found.
   */
+  friend class Event_queue_element;
 
   static ulong opt_event_scheduler;
   static TYPELIB opt_typelib;
@@ -71,30 +71,30 @@ public:
   stop_execution_of_events();
 
   bool
-  is_started();
+  is_execution_of_events_started();
 
-  static Events*
+  static Events *
   get_instance();
 
-  int
+  bool
   create_event(THD *thd, Event_parse_data *parse_data, bool if_exists,
                uint *rows_affected);
 
-  int
+  bool
   update_event(THD *thd, Event_parse_data *parse_data, sp_name *rename_to,
                uint *rows_affected);
 
-  int
+  bool
   drop_event(THD *thd, LEX_STRING dbname, LEX_STRING name, bool if_exists,
              uint *rows_affected, bool only_from_disk);
 
-  int
+  void
   drop_schema_events(THD *thd, char *db);
 
   int
   open_event_table(THD *thd, enum thr_lock_type lock_type, TABLE **table);
 
-  int
+  bool
   show_create_event(THD *thd, LEX_STRING dbname, LEX_STRING name);
 
   /* Needed for both SHOW CREATE EVENT and INFORMATION_SCHEMA */
@@ -109,8 +109,11 @@ public:
   dump_internal_status(THD *thd);
 
 private:
+  bool
+  check_system_tables(THD *thd);
+
   /* Singleton DP is used */
-  Events(){}
+  Events();
   ~Events(){}
 
   /* Singleton instance */
@@ -122,6 +125,8 @@ private:
 
   pthread_mutex_t LOCK_event_metadata;
 
+  bool check_system_tables_error;
+
   /* Prevent use of these */
   Events(const Events &);
   void operator=(Events &);

sql/set_var.cc

@@ -3978,7 +3978,7 @@ byte *sys_var_event_scheduler::value_ptr(THD *thd, enum_var_type type,
 {
   if (Events::opt_event_scheduler == 0)
     thd->sys_var_tmp.long_value= 0;
-  else if (Events::get_instance()->is_started())
+  else if (Events::get_instance()->is_execution_of_events_started())
     thd->sys_var_tmp.long_value= 1;
   else
     thd->sys_var_tmp.long_value= 2;

sql/share/errmsg.txt

@@ -5851,3 +5851,5 @@ ER_CANT_DROP_LOG_TABLE
         eng "Cannot drop log table if log is enabled"
 ER_EVENT_RECURSIVITY_FORBIDDEN
         eng "Recursivity of EVENT DDL statements is forbidden when body is present"
+ER_EVENTS_DB_ERROR
+        eng "Cannot proceed because the tables used by events were found damaged at server start"

sql/sql_db.cc

@@ -949,7 +949,7 @@ bool mysql_rm_db(THD *thd,char *db,bool if_exists, bool silent)
 
 exit:
   (void)sp_drop_db_routines(thd, db); /* QQ Ignore errors for now  */
-  error= Events::get_instance()->drop_schema_events(thd, db);
+  Events::get_instance()->drop_schema_events(thd, db);
   /*
     If this database was the client's selected database, we silently
     change the client's selected database to nothing (to have an empty

sql/sql_yacc.yy

@@ -1445,11 +1445,11 @@ ev_sql_stmt:
           {
             LEX *lex=Lex;
 
-            // return back to the original memory root ASAP
+            /* return back to the original memory root ASAP */
             lex->sphead->init_strings(YYTHD, lex);
             lex->sphead->restore_thd_mem_root(YYTHD);
 
-            lex->sp_chistics.suid= SP_IS_SUID;//always the definer!
+            lex->sp_chistics.suid= SP_IS_SUID;  //always the definer!
 
             Lex->event_parse_data->init_body(YYTHD);
           }
@@ -1568,9 +1568,9 @@ create_function_tail:
 	    sp->m_type= TYPE_ENUM_FUNCTION;
 	    lex->sphead= sp;
 	    /*
-	     * We have to turn of CLIENT_MULTI_QUERIES while parsing a
-	     * stored procedure, otherwise yylex will chop it into pieces
-	     * at each ';'.
+	      We have to turn off CLIENT_MULTI_QUERIES while parsing a
+	      stored procedure, otherwise yylex will chop it into pieces
+	      at each ';'.
 	    */
             $<ulong_num>$= YYTHD->client_capabilities & CLIENT_MULTI_QUERIES;
 	    YYTHD->client_capabilities &= ~CLIENT_MULTI_QUERIES;
@@ -4684,14 +4684,13 @@ alter:
               If it had to be supported spname had to be added to
               Event_parse_data.
             */
-            Lex->spname= NULL;
 
             if (!(Lex->event_parse_data= Event_parse_data::new_instance(YYTHD)))
               YYABORT;
             Lex->event_parse_data->identifier= $3;
 
             /*
-              We have to turn of CLIENT_MULTI_QUERIES while parsing a
+              We have to turn off CLIENT_MULTI_QUERIES while parsing a
               stored procedure, otherwise yylex will chop it into pieces
               at each ';'.
             */
@@ -4757,9 +4756,11 @@ ev_alter_on_schedule_completion: /* empty */ { $$= 0;}
 opt_ev_rename_to: /* empty */ { $$= 0;}
         | RENAME TO_SYM sp_name
           {
-            LEX *lex=Lex;
-            lex->spname= $3; //use lex's spname to hold the new name
-	                     //the original name is in the Event_parse_data object
+            /*
+              Use lex's spname to hold the new name.
+              The original name is in the Event_parse_data object
+            */
+            Lex->spname= $3; 
             $$= 1;
           }
       ;
@@ -4783,7 +4784,7 @@ alter_commands:
         | remove_partitioning
         | partitioning
 /*
-  This part was added for release 5.1 by Mikael Ronström.
+  This part was added for release 5.1 by Mikael Ronstrm.
   From here we insert a number of commands to manage the partitions of a
   partitioned table such as adding partitions, dropping partitions,
   reorganising partitions in various manners. In future releases the list

sql/table.cc

@@ -2352,28 +2352,28 @@ bool check_column_name(const char *name)
   Checks whether a table is intact. Should be done *just* after the table has
   been opened.
   
-  Synopsis
+  SYNOPSIS
     table_check_intact()
-      table         - the table to check
-      table_f_count - expected number of columns in the table
-      table_def     - expected structure of the table (column name and type)
-    last_create_time- the table->file->create_time of the table in memory
+      table             The table to check
+      table_f_count     Expected number of columns in the table
+      table_def         Expected structure of the table (column name and type)
+      last_create_time  The table->file->create_time of the table in memory
                         we have checked last time
-      error_num     - ER_XXXX from the error messages file. When 0 no error
+      error_num         ER_XXXX from the error messages file. When 0 no error
                         is sent to the client in case types does not match.
                         If different col number either 
                         ER_COL_COUNT_DOESNT_MATCH_PLEASE_UPDATE or 
                         ER_COL_COUNT_DOESNT_MATCH_CORRUPTED is used
 
   RETURNS
-    0   - OK
-    1   - There was an error
+    FALSE  OK
+    TRUE   There was an error
 */
 
 my_bool
-table_check_intact(TABLE *table, uint table_f_count,
-                   TABLE_FIELD_W_TYPE *table_def, time_t *last_create_time,
-                   int error_num)
+table_check_intact(TABLE *table, const uint table_f_count,
+                   const TABLE_FIELD_W_TYPE *table_def,
+                   time_t *last_create_time, int error_num)
 {
   uint i;
   my_bool error= FALSE;
@@ -2388,7 +2388,7 @@ table_check_intact(TABLE *table, uint table_f_count,
     DBUG_PRINT("info", ("I am suspecting, checking table"));
     if (fields_diff_count)
     {
-      // previous MySQL version
+      /* previous MySQL version */
       error= TRUE;
       if (MYSQL_VERSION_ID > table->s->mysql_version)
       {
@@ -2411,22 +2411,22 @@ table_check_intact(TABLE *table, uint table_f_count,
       else
       {
         /*
-          moving from newer mysql to older one -> let's say not an error but
+          Moving from newer mysql to older one -> let's say not an error but
           will check the definition afterwards. If a column was added at the
           end then we don't care much since it's not in the middle.
         */
         error= FALSE;
       }
     }
-    //definitely something has changed
+    /* definitely something has changed */
     char buffer[255];
     for (i=0 ; i < table_f_count; i++, table_def++)
     {      
       String sql_type(buffer, sizeof(buffer), system_charset_info);
       sql_type.length(0);
       /*
-        name changes are not fatal, we use sequence numbers => no prob for us
-        but this can show tampered table or broken table.
+        Name changes are not fatal, we use sequence numbers => no problem
+        for us but this can show tampered table or broken table.
       */
       if (i < table->s->fields)
       {
@@ -2440,7 +2440,7 @@ table_check_intact(TABLE *table, uint table_f_count,
         }
                         
         /*
-          IF the type does not match than something is really wrong
+          If the type does not match than something is really wrong
           Check up to length - 1. Why?
           1. datetime -> datetim -> the same
           2. int(11) -> int(11  -> the same

sql/table.h

@@ -965,9 +965,9 @@ typedef struct st_table_field_w_type
 
 
 my_bool
-table_check_intact(TABLE *table, uint table_f_count,
-                   TABLE_FIELD_W_TYPE *table_def, time_t *last_create_time,
-                   int error_num);
+table_check_intact(TABLE *table, const uint table_f_count,
+                   const TABLE_FIELD_W_TYPE * const table_def,
+                   time_t *last_create_time, int error_num);
 
 static inline my_bitmap_map *tmp_use_all_columns(TABLE *table,
                                                  MY_BITMAP *bitmap)