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 1 commit 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.
| @@ -0,0 +1,39 @@ | |||
| --echo # | |||
There was a problem hiding this comment.
The pull request description and title indicate that the core fix involves removing the save/restore of proc_info in free_tmp_table. However, the current diff only includes the test files (.test and .result). The actual implementation changes in the server source code (likely in sql/sql_select.cc) are missing from this PR. Please include the code changes to address the issue.
| ('zeta',6),('eta',7),('theta',8),('iota',9),('kappa',10); | ||
| SET profiling = 1; | ||
| INSERT INTO profile_test (name, value) | ||
| SELECT name, value FROM profile_test LIMIT 5; |
There was a problem hiding this comment.
The PR description mentions that this issue also affects GROUP BY queries with filesort, causing duplicate "Creating sort index" entries. It would be beneficial to add a test case covering this scenario as well to ensure the fix is comprehensive and prevents regressions for all identified cases.
| 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
| LAG(STATE) OVER (ORDER BY SEQ) AS prev_status, | ||
| LEAD(STATE) OVER (ORDER BY SEQ) AS next_status | ||
| FROM information_schema.profiling | ||
| WHERE QUERY_ID = 1 |
There was a problem hiding this comment.
Relying on a hardcoded QUERY_ID = 1 is fragile as it assumes the INSERT is the first profiled query in the session. A more robust approach would be to disable profiling after the target query and use a subquery like WHERE QUERY_ID = (SELECT MAX(QUERY_ID) FROM information_schema.profiling) to fetch the correct entry.
ARaveala
force-pushed
the
MDEV-39597-duplicate-sending-data-10.11
branch
from
c61b1f2 to
4e64d90
Compare
…he_info MDEV-39597: Remove save/restore of proc_info in free_tmp_table 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
4e64d90 to
6c3b644
Compare
2 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)