MDEV-40800 Assert in ha_close() when a temporary table converts

[arcivanov]

Two independent defects in create_internal_tmp_table_from_heap(), one
commit 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 mode
and 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 and
deletes the very handler the batch was started on:

handler::ha_close ... Assertion `m_psi_batch_mode == PSI_BATCH_MODE_NONE' failed

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 as well. Ending the batch and leaving it
ended 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 is
started 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 in
join_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_read covers five query shapes that
each reach the conversion while a reader is in progress: a derived table with
DISTINCT, one with GROUP BY, an unmerged derived table, a derived table
filled by UNION, and an ALGORITHM=TEMPTABLE view. Each was confirmed
individually to hit the assert without the fix. Created_tmp_disk_tables is
checked 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:

  if (table->file->ha_rnd_init_with_error(1))
    DBUG_RETURN(1);

The replacement was left open with its files on disk, its handler was never
freed, save_proc_info was not restored, and everything allocated on
new_table.mem_root was orphaned, since new_table holds a copy of the
MEM_ROOT taken before those allocations. On a debug build this is not a slow
leak 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_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 a failed rnd_init leaves none. 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 new case in main.error_simulation drives
it with a debug injection, placed next to the raise_error injection that
already covers the copy loop of the same function.

Testing

--suite=main,perfschema passes 1853/1853 on a debug build. Both new tests
also 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 an
earlier branch is preferred as the target, I am happy to rebase.

[gkodinov]

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).

[arcivanov]

@gkodinov if you'd like I can merge them but I left them here separately for review because they are technically independent it's just the second commit is so small it doesn't warrant its own MDEV.

[gkodinov]

@gkodinov if you'd like I can merge them but I left them here separately for review because they are technically independent it's just the second commit is so small it doesn't warrant its own MDEV.

If you think this will speed up reviews, let's leave them as is. But I believe that multiple related commits for the same "feature" make the git history harder to read. Thus, I'd squash post- review. But, this will be up to you and the final reviewer.

I still stand by my request to rebase though. Bugs need to be fixed in the lowest affected version IMHO.