MDEV-31808 crash upon altering table with NEXTVAL for default under exclusive lock#5125
MDEV-31808 crash upon altering table with NEXTVAL for default under exclusive lock#5125grooverdan wants to merge 1 commit into
Conversation
…xclusive lock A segfault in mysql_lock_merge waa the result of a null argument when called from open_and_lock_internal_tables. As the argument was null there was no lock to merge and thd->lock was already in place, so no futher action is required in open_and_lock_internal_tables. After passing the previous correctly on open_table of the sequence used in the table, we get to the checking the thd->open_table list where tdc is flushed. This puts the open_table into an error condition to reopen all tables. When we are just opening a sequence, this condition isn't one we want to fail on.
There was a problem hiding this comment.
Code Review
This pull request addresses a server crash (MDEV-31808) that occurs when altering a table with a NEXTVAL default value under an exclusive lock. It includes test cases to reproduce the issue and fixes in sql/sql_base.cc to prevent invalid lock merging and table flushing checks. The reviewer suggested a safer alternative to the early return when save_lock is null, recommending a ternary operator to avoid bypassing any cleanup or monitoring logic at the end of the function.
| /* no existing lock to merge with */ | ||
| if (save_lock == nullptr) | ||
| DBUG_RETURN(0); | ||
|
|
||
| if (!(new_lock= mysql_lock_merge(save_lock, thd->lock))) |
There was a problem hiding this comment.
Instead of returning early with DBUG_RETURN(0) when save_lock is nullptr, we can conditionally merge the locks using a ternary operator. Returning early from open_and_lock_internal_tables is risky because it bypasses any potential cleanup, monitoring, or other logic that might exist or be added to the end of the function.
By using a ternary operator, we can safely handle the case where there is no existing lock to merge, while allowing the function to proceed to its natural exit point.
/* Merge with the existing lock if there is one, otherwise keep the new lock. */
new_lock= save_lock ? mysql_lock_merge(save_lock, thd->lock) : thd->lock;
if (!new_lock)There was a problem hiding this comment.
There's no cleanup required, and an assignment of thd->lock back to itself a bit of an odd logic flow.
A segfault in mysql_lock_merge waa the result of a null argument when called from open_and_lock_internal_tables. As the argument was null there was no lock to merge and thd->lock was already in place, so no futher action is required in open_and_lock_internal_tables.
After passing the previous correctly on open_table of the sequence used in the table, we get to the checking the thd->open_table list where tdc is flushed. This puts the open_table into an error condition to reopen all tables. When we are just opening a sequence, this condition isn't one we want to fail on.