Re: [Maria-developers] [Commits] 8bf23ef: MDEV-8208: Sporadic SEGFAULT on startup
Hi, Nirbhay! On Aug 19, Nirbhay Choubey wrote:
revision-id: 8bf23efc914f18c687067e9314cadf4f36a85eec parent(s): 71d1f35847a575239deff856590bf6f13afd74ed committer: Nirbhay Choubey branch nick: 5.5-galera.b8208 timestamp: 2015-08-19 17:17:56 -0400 message:
MDEV-8208: Sporadic SEGFAULT on startup
A couple of questions:
diff --git a/sql/mysqld.cc b/sql/mysqld.cc index 2008fc5..4c05999 100644 --- a/sql/mysqld.cc +++ b/sql/mysqld.cc @@ -4584,6 +4579,29 @@ static int init_server_components() } plugins_are_initialized= TRUE; /* Don't separate from init function */
+#ifdef WITH_WSREP + /* Wait for wsrep threads to get created. */ + if (wsrep_creating_startup_threads == 1) { + mysql_mutex_lock(&LOCK_thread_count); + while (wsrep_running_threads < 2) + { + mysql_cond_wait(&COND_thread_count, &LOCK_thread_count); + } + + /* Now is the time to initialize threads for queries. */ + THD *tmp; + I_List_iterator<THD> it(threads); + while ((tmp= it++)) + if (tmp->wsrep_applier == true) + tmp->init_for_queries();
Do you really need to iterate over all THD's? There is only one applier thread, right? And it's THD is not stored anywhere, only in the global list?
+ mysql_mutex_unlock(&LOCK_thread_count); + } +#endif + have_csv= plugin_status(STRING_WITH_LEN("csv"), MYSQL_STORAGE_ENGINE_PLUGIN); have_ndbcluster= plugin_status(STRING_WITH_LEN("ndbcluster"), @@ -4890,12 +4902,21 @@ pthread_handler_t start_wsrep_THD(void *arg) //thd->version= refresh_version; thd->proc_info= 0; thd->command= COM_SLEEP; - thd->set_time(); - thd->init_for_queries(); + + if (plugins_are_initialized) + { + thd->init_for_queries(); + }
What happens if plugins are not initialized yet? thd->init_for_queries() will be done in that THD loop above? In that case why do you need to do it here? Regards, Sergei
Hi Serg,
Thanks for the review. My comments inline.
On Sat, Sep 12, 2015 at 7:12 AM, Sergei Golubchik
Hi, Nirbhay!
On Aug 19, Nirbhay Choubey wrote:
revision-id: 8bf23efc914f18c687067e9314cadf4f36a85eec parent(s): 71d1f35847a575239deff856590bf6f13afd74ed committer: Nirbhay Choubey branch nick: 5.5-galera.b8208 timestamp: 2015-08-19 17:17:56 -0400 message:
MDEV-8208: Sporadic SEGFAULT on startup
A couple of questions:
diff --git a/sql/mysqld.cc b/sql/mysqld.cc index 2008fc5..4c05999 100644 --- a/sql/mysqld.cc +++ b/sql/mysqld.cc @@ -4584,6 +4579,29 @@ static int init_server_components() } plugins_are_initialized= TRUE; /* Don't separate from init function */
+#ifdef WITH_WSREP + /* Wait for wsrep threads to get created. */ + if (wsrep_creating_startup_threads == 1) { + mysql_mutex_lock(&LOCK_thread_count); + while (wsrep_running_threads < 2) + { + mysql_cond_wait(&COND_thread_count, &LOCK_thread_count); + } + + /* Now is the time to initialize threads for queries. */ + THD *tmp; + I_List_iterator<THD> it(threads); + while ((tmp= it++)) + if (tmp->wsrep_applier == true) + tmp->init_for_queries();
Do you really need to iterate over all THD's? There is only one applier thread, right? And it's THD is not stored anywhere, only in the global list?
2 actually : applier and replayer.
+ mysql_mutex_unlock(&LOCK_thread_count); + } +#endif + have_csv= plugin_status(STRING_WITH_LEN("csv"), MYSQL_STORAGE_ENGINE_PLUGIN); have_ndbcluster= plugin_status(STRING_WITH_LEN("ndbcluster"), @@ -4890,12 +4902,21 @@ pthread_handler_t start_wsrep_THD(void *arg) //thd->version= refresh_version; thd->proc_info= 0; thd->command= COM_SLEEP; - thd->set_time(); - thd->init_for_queries(); + + if (plugins_are_initialized) + { + thd->init_for_queries(); + }
What happens if plugins are not initialized yet?
In that case we do init_for_queries() only after plugins gets initialized. BTW, this can only happen for the initial 2 wsrep thds. As they are created before the initialization of plugins. For other wsrep threads, requested using wsrep_slave_threads, they get created only after plugins are initialized.
thd->init_for_queries() will be done in that THD loop above? In that case why do you need to do it here?
For wsrep threads requested via wsrep_slave_threads. The clarify the issue further, here is roughly what happens on the server start : ( http://pastie.org/10362496 -> In case the below diagram gets mangled by the mail client) main() | |----- wsrep_init_startup() \ | \ | | | | |- pthread_create(start_wsrep_THD()) <-- applier | |- pthread_create(start_wsrep_THD()) <-- rollbacker | | <-- main thread : wait (COND_wsrep_sst) / / <-- applier thread: signal(COND_wsrep_sst) / | (while the applier/rollbacker threads get initialized, applier later signals COND_wsrep_sst | post SST and main thread of execution continues. Now, the segfault can occur when | the rollbacker thread is still under thd->init_for_queries trying to access maria_thon | while its only partially initialized (e.g. maria_hton != 0 but its other members like slot | are not initialized) under plugin_init(). | |- main thread: plugin_init() || rollbacker thd : THD::init_for_queries() -> ha_maria::implicit_commit Best, Nirbhay
Regards, Sergei
Hi, Nirbhay! On Sep 12, Nirbhay Choubey wrote:
diff --git a/sql/mysqld.cc b/sql/mysqld.cc index 2008fc5..4c05999 100644 --- a/sql/mysqld.cc +++ b/sql/mysqld.cc @@ -4584,6 +4579,29 @@ static int init_server_components()
+ /* Now is the time to initialize threads for queries. */ + THD *tmp; + I_List_iterator<THD> it(threads); + while ((tmp= it++)) + if (tmp->wsrep_applier == true) + tmp->init_for_queries();
Do you really need to iterate over all THD's? There is only one applier thread, right? And it's THD is not stored anywhere, only in the global list?
2 actually : applier and replayer.
I meant that there might be, perhaps, a global variable, like THD *wsrep_applier_thd, and then you won't need to iterate. But now I realize that there can be no connection threads at this point, only wsrep threads. So practically you need to initialize *all* THD's in the list. Ok, then, I retract my comment, let's keep this code as you have it. ok to push, thanks
+ mysql_mutex_unlock(&LOCK_thread_count); + } +#endif +
Regards, Sergei
participants (2)
-
Nirbhay Choubey -
Sergei Golubchik