Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Add coverage for the analogous FOR PORTION OF path and address the two style nits.
Review effort: Lite
Findings: None
What changed in this PR
Fixes MDEV-40821 by attaching versioning conditions to the intended DELETE target, preventing a SIGSEGV when expressions introduce another table.
Changes:
- Uses
correspondent_tablefor condition assignment. - Adds regression coverage for sequence and window-function expressions.
- Updates expected results.
| File | Summary |
|---|---|
sql/sql_yacc.yy |
Corrects DELETE condition association. A moderate regression case remains missing for FOR PORTION OF with setval(), along with two line-length nits. |
mysql-test/suite/versioning/t/delete_history.test |
Adds MDEV-40821 regression cases. |
mysql-test/suite/versioning/r/delete_history.result |
Records expected test results. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
6c91d89 to
f0f8aef
Compare
f0f8aef to
aaea94b
Compare
sanja-byelkin
left a comment
There was a problem hiding this comment.
The fix is in the right place: the parser attached vers_conditions / period_conditions to last_table(), which becomes a table referenced from the BEFORE SYSTEM_TIME / FOR PORTION OF expression (here cs1 via setval()) instead of the DELETE target. Fixing that lets the existing fix_fields() / vers_setup_conds() checks do their job, with no guard needed in Window_funcs_sort::setup(). The vers_switch_partition assertion fix is also right. A few things should change before merge, though.
Should change
-
Target branch. The same
last_table()->vers_conditions/->period_conditionsassignments and the assertion exist unchanged in 10.11 (sql_yacc.yy:13428/13454,sql_base.cc:1845) and 11.4 (sql_yacc.yy:13622/13668,sql_base.cc:1882). Please target 10.11 and merge up. -
The DELETE target is spelled three ways (
sql_yacc.yy:13963,14011,sql_base.cc:1876):auxiliary_table_list.first->correspondent_tabletwice in the grammar andquery_tablesin the assertion. The siblingupdate_table_listjust uses$$. Either makedelete_single_tablereturn theTABLE_LIST*and use$2/$1, or useLex->query_tablesin both actions. The latter also backports to 10.11, wherecorrespondent_tableisn't set here. -
The bug is also silent data loss, not only a crash. Without a window function there is no crash, but the conditions still go to the wrong table. Checked on an unpatched 11.8 debug build:
DELETE FROM t1 FOR PORTION OF app FROM (SELECT max(d) FROM t2) TO ...deleted every row oft1, with no error.DELETE HISTORY FROM t1 BEFORE SYSTEM_TIME (SELECT ... FROM t2 LIMIT 1)deleted the current row and kept the history row. From reading the code, onlyDELETE_ACLis checked there, notDELETE_HISTORY_ACL.
Please mention this in the commit message, because it affects how severe the bug is and where to backport.
-
Tests only check error codes, so they don't cover the case above. Please add:
- a FOR PORTION OF case whose bound is a subquery on another table, with no window function, followed by
select * from t1to show the rows were not wholly deleted; - a successful DELETE HISTORY with a subquery in BEFORE SYSTEM_TIME, followed by
select count(*) from t1and... for system_time all, to show current rows survive and only history is purged; - optionally, a user who has DELETE but not DELETE HISTORY, to cover the privilege check.
- a FOR PORTION OF case whose bound is a subquery on another table, with no window function, followed by
-
FOR PORTION OF via the multi-table path (
sql_delete.cc:1961,2090). A single-table DELETE is converted to multi-table when it has index hints (the shape used in this PR's test) or a subquery that references the target table. On that path FOR PORTION OF still deletes whole overlapping rows instead of splitting them, and skips theER_NOT_CONSTANT_EXPRESSIONcheck. Confirmed on unpatched 11.8 with a constant range; the patch doesn't touch it. Options:- add
has_period()toprocessing_as_multitable_delete_prohibited(); - reject it with
ER_NOT_SUPPORTED_YET, as UPDATE already does; - at least file a follow-up MDEV and link it here.
- add
Nits
sql_yacc.yy:13963and14011are over 80 columns; break after=.- The tests hard-code
engine innodb/engine myisam, which overrides the suites' engine combinations; please drop them. Keepignore index (primary)only if it's needed to reach the crash, which ties in with point 5. sql_base.cc:1876: if you keep two spellings of the target, add a short comment saying they're the same table for single-table DELETE.
Note: the 10.11/11.4 and multi-table points come from reading the code. The patched build was not run; the runtime results above are from the unpatched 11.8 build.
3fea08c to
cc1a225
Compare
The parser assigned versioning conditions of DELETE HISTORY ... BEFORE SYSTEM_TIME and period conditions of DELETE ... FOR PORTION OF to the table pointed by last_table(). When the BEFORE SYSTEM_TIME or FOR PORTION OF expression references another table (e.g. via a subquery or a sequence function), last_table() is already switched to that table, and the DELETE target gets no conditions. DELETE HISTORY then deletes current rows permanently instead of history, keeps the history it had to purge, and is checked for DELETE privilege instead of DELETE HISTORY. DELETE HISTORY from a non-versioned table is not rejected with ER_VERS_NOT_VERSIONED. DELETE ... FOR PORTION OF deletes all rows of the table, ignoring the period. The fix assigns the conditions to the TABLE_LIST returned by delete_single_table, i.e. the DELETE target itself, regardless of tables added later in the statement. DBUG_ASSERT in TABLE::vers_switch_partition() is removed. It checked delete_history on last_table() and assumed no other table is opened for write by DELETE HISTORY, which is false for the underlying table of a view or a table locked by SELECT ... FOR UPDATE in the history point. The DELETE HISTORY target is already skipped there by its own delete_history check.
Single-table DELETE converted to multi-table (when a subquery references the target table) skipped FOR PORTION OF row splitting and the constant expression check, so it deleted whole rows instead of the portion. The fix prohibits processing DELETE ... FOR PORTION OF as multi-table, like DELETE ... RETURNING.
Window_funcs_sort failed accessing win_func->window_spec as window spec was not defined in the query. Usually this is checked by fixing item win_func, but it was not done by vers_setup_conds(). Actually, earlier check by vers_setup_conds() must fail on DELETE HISTORY from non-versioned table, but TABLE_LIST for t1 has no versioning conditions. The cause was the parser assigning versioning conditions to wrong table pointed by last_table() which was already switched to another table from SYSTEM_TIME expression (cs1). It is fixed by the 10.11 commit of MDEV-40821, this commit adds the tests. DELETE ... FOR PORTION OF with index hints is converted to multi-table, same as with a subquery on the target table, which is prohibited by the 11.4 commit of MDEV-40821. Tests added for index hints. Warning WARN_INDEX_HINTS_IGNORED now names the clause the index hints are incompatible with: RETURNING or FOR PORTION OF.
cc1a225 to
9abcba2
Compare

This is 11.8 part of the fix, on top of 10.11 (#5881) and 11.4 (#5930).
The merge of 11.4 branch after #5930 pushed can be resolved by 10.11 and 11.4 commits from this PR.
Window_funcs_sort failed accessing win_func->window_spec as window spec was not defined in the query. Usually this is checked by fixing item win_func, but it was not done by setup_conds.
Actually, earlier check by vers_setup_conds() must fail on DELETE HISTORY from non-versioned table, but TABLE_LIST for t1 has no versioning conditions.
The cause was the parser assigning versioning conditions to wrong table pointed by last_table() which was already switched to another table from SYSTEM_TIME expression (cs1).
The fix assigns versioning conditions to the correct table stored to correspondent_table by delete_single_table branch of the parser.