MDEV-39597: Remove save/restore of proc_info in free_tmp_table#5102
MDEV-39597: Remove save/restore of proc_info in free_tmp_table#5102ARaveala wants to merge 2 commits into
Conversation
![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 introduces a new test case to verify that profiling stages are not duplicated during INSERT...SELECT operations, specifically targeting the 'Removing tmp table' stage. However, the actual server-side implementation to fix the underlying issue is missing from the current diff. Feedback also suggests improving the test's robustness by avoiding hardcoded query IDs, fixing a typo in the test output, and expanding the test suite to cover GROUP BY scenarios with filesort.
| INSERT INTO profile_test (name, value) | ||
| SELECT name, value FROM profile_test LIMIT 5; | ||
|
|
||
| --echo # Check that no stage appears twice consecutivley around removing tmp table |
There was a problem hiding this comment.
There is a typo in the echo message: "consecutivley" should be "consecutively".
--echo # Check that no stage appears twice consecutively around removing tmp table
ARaveala
force-pushed
the
MDEV-39597-duplicate-sending-data-10.11
branch
2 times, most recently
from
4e64d90 to
6c3b644
Compare
SHOW PROFILE reports duplicate stage entries for INSERT...SELECT caused by a save/restore pattern in free_tmp_table that blindly restores the previous stage name after tmp table removal. The fix removes the save/restore. The 'Removing tmp table' stage now remains active until the caller sets the next stage. Test uses --disable_ps2_protocol following the pattern in show_profile.test to ensure consistent QUERY_ID in PS protocol mode. fixup: add trailing newline to test file
ARaveala
force-pushed
the
MDEV-39597-duplicate-sending-data-10.11
branch
from
6c3b644 to
b317ede
Compare
|
rpl.rpl_row_foreign_key_mdl passes locally in isolation. Likely a flaky replication test that fails under parallel load on the buildbot. Unrelated to this fix. |
gkodinov
added
the
External Contribution
gkodinov
changed the title
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
LGTM. Thank you for your work so far!
Please stay tuned for the final review.
3 participants
This is a continuation of #5097, targeting 10.11 as requested by the reviewer.
MDEV-39597: Remove save/restore of proc_info in free_tmp_table
Problem
SHOW PROFILEreports duplicate stage entries forINSERT...SELECTqueries. The duplicate appears immediately after "Removing tmp table" and is caused by a save/restore pattern infree_tmp_tablethat blindly restores the previous stage name on exit, regardless of whether that stage is still accurate.The same pattern implicitly also caused a duplicate "Creating sort index" entry in
GROUP BYwith filesort queries.Fix
Removed the save and restore of proc_info in
free_tmp_table. The "Removing tmp table" stage now remains active until the caller sets the next stage.Trade-off
"Removing tmp table" will now appear slightly longer in profiles as it remains active briefly after the function returns. This is considered less misleading than restoring an incorrect stage name.
Scope
free_tmp_tableis called from multiple files. A broader audit of all call sites is out of scope for this fix:Further investigation
The same save/restore pattern appears in other locations in the codebase. I intend to investigate these separately, any guidance on what to prioritise or test would be welcome.
SHOW PROFILE results abbreviated:
Before (INSERT...SELECT)
| Sending data | 0.002404 |
| Removing tmp table | 0.000098 |
| Sending data | 0.000079 | ← duplicate
| End of update loop | 0.000074 |
After
| Sending data | 0.002482 |
| Removing tmp table | 0.000172 |
| End of update loop | 0.000077 |
Provided under the MariaDB Contributor Agreement (MCA)