MDEV-40800 Assert in ha_close() when a temporary table converts - #5565
Open
arcivanov wants to merge 2 commits into
Open
MDEV-40800 Assert in ha_close() when a temporary table converts#5565arcivanov wants to merge 2 commits into
arcivanov wants to merge 2 commits into
Conversation
`JOIN_CACHE::join_records()` puts the handler of its table into PFS batch mode and only then opens the scan, and that scan is what materializes a derived table for the first time. When the temporary table outgrows the in-memory engine at that point, `create_internal_tmp_table_from_heap()` closes and deletes the very handler the batch was started on, and `handler::ha_close()` asserts that no batch is in progress. The close is not the only call that breaks. After the swap `table->file` is the replacement handler, whose mode is `PSI_BATCH_MODE_NONE`, so the caller's `end_psi_batch_mode()` would assert too. Ending the batch and leaving it ended is therefore not a fix. Hand the batch over to the replacement: end it on the old handler right before `ha_close()` and start it again on the new one once the swap is complete. It is started at the very end so that the writes performed by the conversion itself are not counted into the reader's batch. `sub_select()` is not affected: it materializes in `join_tab_execution_startup()` before it starts its own batch.
`create_internal_tmp_table_from_heap()` creates and opens the on-disk replacement table before it starts the scan of the in-memory table that supplies the rows. When that scan could not be started the function returned immediately, skipping all of the cleanup: the replacement was left open with its files on disk, its handler was never freed, the saved `proc_info` was not restored, and the blocks allocated on `new_table.mem_root` were orphaned, since `new_table` holds a copy of the `MEM_ROOT` taken before those allocations. Route that exit through a new `err_drop` label placed between the two existing ones. `err_killed` cannot be used because its `ha_rnd_end()` asserts that a scan is in progress, and `err2` cannot be used because it never drops the replacement table. The failure is not reachable as the code stands, since the in-memory engine cannot fail to start a scan, so the test drives it with a new debug injection next to the one that already covers the copy loop of the same function.
gkodinov
requested changes
Aug 18, 2026
gkodinov
left a comment
Member
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
Please consider merging the two commits, unless there's a specific reason to keep them separate.
Please also rebase to the version Elena mentioned on jira (lowest affected).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two independent defects in
create_internal_tmp_table_from_heap(), onecommit each.
MDEV-40800: PFS batch mode is left on a handler that is about to be deleted
JOIN_CACHE::join_records()puts the handler of its table into PFS batch modeand only then opens the scan, and it is that scan which materializes a derived
table for the first time. When the temporary table outgrows the in-memory
engine at that point,
create_internal_tmp_table_from_heap()closes anddeletes the very handler the batch was started on:
The close is not the only call that breaks. After the swap
table->fileis thereplacement handler, whose mode is
PSI_BATCH_MODE_NONE, so the caller'send_psi_batch_mode()would assert as well. Ending the batch and leaving itended is therefore not sufficient.
The batch is handed over instead: ended on the old handler right before
ha_close(), started again on the replacement once the swap is complete. It isstarted at the very end so that the writes the conversion performs itself are
not counted into the reader's batch.
sub_select()is not affected, because it materializes injoin_tab_execution_startup()before it starts its own batch.This is the approach agreed in MDEV-22104, which is the same assert with the
same stacks. That ticket has an unmerged pull request, #3817, based on 10.5.
The new test
main.tmp_table_convert_while_readcovers five query shapes thateach reach the conversion while a reader is in progress: a derived table with
DISTINCT, one withGROUP BY, an unmerged derived table, a derived tablefilled by
UNION, and anALGORITHM=TEMPTABLEview. Each was confirmedindividually to hit the assert without the fix.
Created_tmp_disk_tablesischecked after every query, so that a later change to the size limits cannot
silently leave a query without a conversion.
The replacement table is leaked when the scan of the in-memory table cannot start
The same function creates and opens the on-disk replacement before it starts
the scan that supplies the rows, and the failure exit of that scan returned
immediately, skipping all of the cleanup:
The replacement was left open with its files on disk, its handler was never
freed,
save_proc_infowas not restored, and everything allocated onnew_table.mem_rootwas orphaned, sincenew_tableholds a copy of theMEM_ROOTtaken before those allocations. On a debug build this is not a slowleak but an abort: safemalloc reports the Aria allocations as lost and
~THD()fails
status_var.local_memory_used == 0.That exit now goes to a new
err_droplabel placed between the two existingones.
err_killedcannot be used, because itsha_rnd_end()asserts that ascan is in progress and a failed
rnd_initleaves none.err2cannot be used,because it never drops the replacement table.
The failure is not reachable as the code stands, since the in-memory engine
cannot fail to start a scan, so the new case in
main.error_simulationdrivesit with a debug injection, placed next to the
raise_errorinjection thatalready covers the copy loop of the same function.
Testing
--suite=main,perfschemapasses 1853/1853 on a debug build. Both new testsalso pass under
--ps-protocol --view-protocol --sp-protocol --cursor-protocol.This is based on
main. MDEV-22104 lists 10.11 and 11.4 as affected, so if anearlier branch is preferred as the target, I am happy to rebase.