MDEV-39585/MDEV-39541 - mariadbd --bootstrap crashing in ~mem_pressure#5070
MDEV-39585/MDEV-39541 - mariadbd --bootstrap crashing in ~mem_pressure#5070grooverdan wants to merge 3 commits into
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.
| char *user= shutdown_user.load(std::memory_order_relaxed); | ||
| if (user) | ||
| my_free(user); |
There was a problem hiding this comment.
It is safer to use exchange(nullptr) when retrieving the pointer from the atomic shutdown_user for freeing. This ensures that the atomic is cleared and prevents any potential double-free or dangling pointer issues if other parts of the shutdown sequence (e.g., in clean_up) were to access the same pointer.
char *user= shutdown_user.exchange(nullptr, std::memory_order_relaxed);
if (user)
my_free(user);
grooverdan
force-pushed
the
MDEV-39541
branch
2 times, most recently
from
bdee36b to
ebb0d89
Compare
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.
…on (Windows) 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 occured 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 onny.
1 participant
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.