Code review for Bug#43587 Putting event_scheduler=1 in init SQL file crashes

mysqld

mysql-test/std_data/init_file.dat

@@ -35,4 +35,10 @@ CREATE DATABASE IF NOT EXISTS init_file;
 CREATE TABLE IF NOT EXISTS init_file.startup ( startdate DATETIME );
 INSERT INTO init_file.startup VALUES ( NOW() );
 
+#
+# Bug#43587 "Putting event_scheduler=1 in init SQL file crashes mysqld"
+# Check that putting event_scheduler=1 to init file doesn't lead to 
+# crashes.
+#
 
+SET GLOBAL event_scheduler=1;

sql/event_scheduler.cc

@@ -158,6 +158,7 @@ deinit_event_thread(THD *thd)
   thread_count--;
   thread_running--;
   delete thd;
+  pthread_cond_broadcast(&COND_thread_count);
   pthread_mutex_unlock(&LOCK_thread_count);
 }
 
@@ -418,6 +419,7 @@ Event_scheduler::start()
     thread_count--;
     thread_running--;
     delete new_thd;
+    pthread_cond_broadcast(&COND_thread_count);
     pthread_mutex_unlock(&LOCK_thread_count);
   }
 end:
@@ -550,6 +552,7 @@ Event_scheduler::execute_top(Event_queue_element_for_exec *event_name)
     thread_count--;
     thread_running--;
     delete new_thd;
+    pthread_cond_broadcast(&COND_thread_count);
     pthread_mutex_unlock(&LOCK_thread_count);
   }
   delete event_name;

sql/events.cc

@@ -852,22 +852,23 @@ Events::fill_schema_events(THD *thd, TABLE_LIST *tables, COND * /* cond */)
 }
 
 
-/*
-  Inits the scheduler's structures.
+/**
+  Initializes the scheduler's structures.
 
-  SYNOPSIS
-    Events::init()
+  @param  opt_noacl_or_bootstrap
+                     TRUE if there is --skip-grant-tables or --bootstrap
+                     option. In that case we disable the event scheduler.
 
-  NOTES
-    This function is not synchronized.
+  @note   This function is not synchronized.
 
-  RETURN VALUE
-    FALSE  OK
-    TRUE   Error in case the scheduler can't start
+  @retval  FALSE   Perhaps there was an error, and the event scheduler
+                   is disabled. But the error is not fatal and the 
+                   server start up can continue.
+  @retval  TRUE    Fatal error. Startup must terminate (call unireg_abort()).
 */
 
 bool
-Events::init(my_bool opt_noacl)
+Events::init(my_bool opt_noacl_or_bootstrap)
 {
 
   THD *thd;
@@ -875,11 +876,6 @@ Events::init(my_bool opt_noacl)
 
   DBUG_ENTER("Events::init");
 
-  /* Disable the scheduler if running with --skip-grant-tables */
-  if (opt_noacl)
-    opt_event_scheduler= EVENTS_DISABLED;
-
-
   /* We need a temporary THD during boot */
   if (!(thd= new THD()))
   {
@@ -908,13 +904,21 @@ Events::init(my_bool opt_noacl)
   /*
     Since we allow event DDL even if the scheduler is disabled,
     check the system tables, as we might need them.
+
+    If run with --skip-grant-tables or --bootstrap, don't try to do the
+    check of system tables and don't complain: in these modes the tables
+    are most likely not there and we're going to disable the event
+    scheduler anyway.
   */
-  if (Event_db_repository::check_system_tables(thd))
+  if (opt_noacl_or_bootstrap || Event_db_repository::check_system_tables(thd))
   {
-    sql_print_error("Event Scheduler: An error occurred when initializing "
-                    "system tables.%s",
-                    opt_event_scheduler == EVENTS_DISABLED ?
-                    "" : " Disabling the Event Scheduler.");
+    if (! opt_noacl_or_bootstrap)
+    {
+      sql_print_error("Event Scheduler: An error occurred when initializing "
+                      "system tables.%s",
+                      opt_event_scheduler == EVENTS_DISABLED ?
+                      "" : " Disabling the Event Scheduler.");
+    }
 
     /* Disable the scheduler since the system tables are not up to date */
     opt_event_scheduler= EVENTS_DISABLED;
@@ -924,7 +928,8 @@ Events::init(my_bool opt_noacl)
 
   /*
     Was disabled explicitly from the command line, or because we're running
-    with --skip-grant-tables, or because we have no system tables.
+    with --skip-grant-tables, or --bootstrap, or because we have no system
+    tables.
   */
   if (opt_event_scheduler == Events::EVENTS_DISABLED)
     goto end;

sql/mysql_priv.h

@@ -1976,6 +1976,7 @@ extern bool opt_disable_networking, opt_skip_show_db;
 extern bool opt_ignore_builtin_innodb;
 extern my_bool opt_character_set_client_handshake;
 extern bool volatile abort_loop, shutdown_in_progress;
+extern bool in_bootstrap;
 extern uint volatile thread_count, thread_running, global_read_lock;
 extern uint connection_count;
 extern my_bool opt_sql_bin_update, opt_safe_user_create, opt_no_mix_types;

sql/mysqld.cc

@@ -416,6 +416,21 @@ my_bool locked_in_memory;
 bool opt_using_transactions;
 bool volatile abort_loop;
 bool volatile shutdown_in_progress;
+/*
+  True if the bootstrap thread is running. Protected by LOCK_thread_count,
+  just like thread_count.
+  Used in bootstrap() function to determine if the bootstrap thread
+  has completed. Note, that we can't use 'thread_count' instead,
+  since in 5.1, in presence of the Event Scheduler, there may be
+  event threads running in parallel, so it's impossible to know
+  what value of 'thread_count' is a sign of completion of the
+  bootstrap thread.
+
+  At the same time, we can't start the event scheduler after
+  bootstrap either, since we want to be able to process event-related
+  SQL commands in the init file and in --bootstrap mode.
+*/
+bool in_bootstrap= FALSE;
 /**
    @brief 'grant_option' is used to indicate if privileges needs
    to be checked, in which case the lock, LOCK_grant, is used
@@ -4426,6 +4441,11 @@ we force server id to 2, but this MySQL server will not act as a slave.");
     unireg_abort(1);
   }
 
+  execute_ddl_log_recovery();
+
+  if (Events::init(opt_noacl || opt_bootstrap))
+    unireg_abort(1);
+
   if (opt_bootstrap)
   {
     select_thread_in_use= 0;                    // Allow 'kill' to work
@@ -4437,14 +4457,10 @@ we force server id to 2, but this MySQL server will not act as a slave.");
     if (read_init_file(opt_init_file))
       unireg_abort(1);
   }
-  execute_ddl_log_recovery();
 
   create_shutdown_thread();
   start_handle_manager();
 
-  if (Events::init(opt_noacl))
-    unireg_abort(1);
-
   sql_print_information(ER(ER_STARTUP),my_progname,server_version,
                         ((unix_sock == INVALID_SOCKET) ? (char*) ""
                                                        : mysqld_unix_port),
@@ -4726,6 +4742,7 @@ static void bootstrap(FILE *file)
   thd->security_ctx->master_access= ~(ulong)0;
   thd->thread_id= thd->variables.pseudo_thread_id= thread_id++;
   thread_count++;
+  in_bootstrap= TRUE;
 
   bootstrap_file=file;
 #ifndef EMBEDDED_LIBRARY			// TODO:  Enable this
@@ -4738,7 +4755,7 @@ static void bootstrap(FILE *file)
   }
   /* Wait for thread to die */
   (void) pthread_mutex_lock(&LOCK_thread_count);
-  while (thread_count)
+  while (in_bootstrap)
   {
     (void) pthread_cond_wait(&COND_thread_count,&LOCK_thread_count);
     DBUG_PRINT("quit",("One thread died (count=%u)",thread_count));

sql/sql_parse.cc

@@ -533,8 +533,9 @@ pthread_handler_t handle_bootstrap(void *arg)
 #ifndef EMBEDDED_LIBRARY
   (void) pthread_mutex_lock(&LOCK_thread_count);
   thread_count--;
-  (void) pthread_mutex_unlock(&LOCK_thread_count);
+  in_bootstrap= FALSE;
   (void) pthread_cond_broadcast(&COND_thread_count);
+  (void) pthread_mutex_unlock(&LOCK_thread_count);
   my_thread_end();
   pthread_exit(0);
 #endif