MDEV-39585/MDEV-39541 - mariadbd --bootstrap crashing in ~mem_pressure - #5070
Conversation
grooverdan
added
the
MariaDB Foundation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a crash during bootstrap (MDEV-39541) by refining the server shutdown sequence and InnoDB's memory pressure thread handling. Key changes include moving the signal thread termination to a common exit path in mysqld_main, replacing direct exits with a jump to a cleanup label, and ensuring the memory pressure thread is shut down even if InnoDB initialization is incomplete. Feedback suggests using exchange(nullptr) when retrieving the shutdown_user pointer to enhance thread safety and prevent potential double-free issues during the shutdown sequence.
aab6982 to
9e731da
Compare
dr-m
left a comment
dr-m
left a comment
There was a problem hiding this comment.
This looks OK to me. However, I am not that familiar with this code. I think that the changes of sql/mysqld.cc which I did not comment on have to be reviewed by @vaintroub or someone in @sanja-byelkin’s team.
vaintroub
left a comment
vaintroub
left a comment
There was a problem hiding this comment.
I wish we did not support SHUTDOWN in bootstrap scripts, as in "explicitly forbid and throw SQL error if it is used in bootstrap". This statement is to shutdown a server that is started and can accept requests, otherwise makes no real sense, except putting it into this single test script. It is kinda ugly. But ok, I do not have a very strong opinion, but if you can think on how to test it differently, I'd appreciate it very much.
There was a problem hiding this comment.
Pull request overview
This PR addresses crashes during mariadbd --bootstrap shutdown related to the InnoDB mem_pressure thread lifecycle by ensuring shutdown proceeds through the normal cleanup path (including plugin shutdown) and that the mem-pressure thread is joined before teardown.
Changes:
- Route
--bootstrap“shutdown requested” flow to a unified termination path so plugin shutdown / cleanup runs (instead of sleeping +exit(0)), and ensure the signal thread is waited beforeclean_up(). - Ensure the InnoDB mem-pressure thread is explicitly shut down even when InnoDB wasn’t fully started (e.g., bootstrap/early-exit cases).
- Add an MTR regression test covering
--bootstrapexecutingSHUTDOWN;with InnoDB enabled.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| storage/innobase/log/log0log.cc | Removes local buf_mem_pressure_shutdown() declaration now provided from a shared header. |
| storage/innobase/include/srv0start.h | Exposes buf_mem_pressure_shutdown() declaration (Linux/no-op elsewhere) for broader shutdown use. |
| storage/innobase/handler/ha_innodb.cc | Calls buf_mem_pressure_shutdown() when InnoDB wasn’t started but shutdown hooks run. |
| sql/mysqld.cc | Adjusts bootstrap termination and shutdown sequencing; moves signal-thread wait to a common termination path. |
| mysql-test/main/bootstrap_innodb.test / .result | Adds bootstrap + SHUTDOWN; regression coverage for the mem_pressure crash. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mariadbd under --bootstrap failed to preform plugin deinitialization. The sleep(2);exit is removed and replaced to a goto termination label to perform the same shutdown procedure of the server after all the connection closing. To prevent a compile error about char *user being uninitialized this sql_print_information(ER_DEFAULT(ER_NORMAL_SHUTDOWN)) is moved to its own block. The memory free did need to occur in the bootstrap mode too to avoid memory leak errors. wait_for_signal_thread_to_end(), was previously in close_connections() however its required too for --bootstrap.
Don't perform mysqld_win_initiate_shutdown under --bootstrap when triggered by SHUTDOWN. With this we don't perform any service interactions. Then the shutdown can proceeded without then hard process termination in mysqld_win_initiate_shutdown. This previously occurred because the handle_connections_win() was never called in --bootstrap and therefore startup_complete() was false. Thanks Vladislav Vaintroub for investigation and providing implementation guidance.
Fill in the anomaly shutdown paths of InnoDB to include call to buf_mem_pressure_shutdown now that MDEV-39585 provides some proper calls shutdown InnoDB and other plugins during the shutdown under --bootstrap. Alternate: destructor attribute on buf_mem_pressure_shutdown would acheive the same thing and given Linux compilers are capabile of this. This is possible as mem_pressure is currently implemented in Linux only.
MDEV-39585 on the first commit corrects the server operation to call a shutdown of plugins under bootstrap.
Second commit was a @Thirunarayanan suggestion of enforcing a fast shutdown (=2 was suggested by failed badly).
Finally we add the mem_pressure such that it joins the thread before attempting to kill it off.