Skip to content

MDEV-40821 SIGSEGV in Window_funcs_sort::setup (11.8) - #5728

Open
midenok wants to merge 3 commits into
11.8from
11.8-midenok-MDEV-40821
Open

midenok wants to merge 3 commits into
11.8from
11.8-midenok-MDEV-40821

Conversation

@midenok

@midenok midenok commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

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.

Copilot AI lite review requested due to automatic review settings September 21, 2026 16:35

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_table for 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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The fix and regression coverage are acceptable; the remaining formatting nit is non-blocking.

Review effort: Lite
Findings: 1 Low severity

Open (1)

Comment thread sql/sql_yacc.yy Outdated
@CLAassistant

CLAassistant commented Sep 21, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@sanja-byelkin sanja-byelkin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Target branch. The same last_table()->vers_conditions / ->period_conditions assignments 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.

  2. The DELETE target is spelled three ways (sql_yacc.yy:13963, 14011, sql_base.cc:1876): auxiliary_table_list.first->correspondent_table twice in the grammar and query_tables in the assertion. The sibling update_table_list just uses $$. Either make delete_single_table return the TABLE_LIST* and use $2 / $1, or use Lex->query_tables in both actions. The latter also backports to 10.11, where correspondent_table isn't set here.

  3. 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 of t1, 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, only DELETE_ACL is checked there, not DELETE_HISTORY_ACL.

    Please mention this in the commit message, because it affects how severe the bug is and where to backport.

  4. 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 t1 to show the rows were not wholly deleted;
    • a successful DELETE HISTORY with a subquery in BEFORE SYSTEM_TIME, followed by select count(*) from t1 and ... 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.
  5. 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 the ER_NOT_CONSTANT_EXPRESSION check. Confirmed on unpatched 11.8 with a constant range; the patch doesn't touch it. Options:

    • add has_period() to processing_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.

Nits

  • sql_yacc.yy:13963 and 14011 are over 80 columns; break after =.
  • The tests hard-code engine innodb / engine myisam, which overrides the suites' engine combinations; please drop them. Keep ignore 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.

@midenok
midenok force-pushed the 11.8-midenok-MDEV-40821 branch 2 times, most recently from 3fea08c to cc1a225 Compare October 20, 2026 12:50
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.
@midenok
midenok force-pushed the 11.8-midenok-MDEV-40821 branch from cc1a225 to 9abcba2 Compare October 20, 2026 16:11
@midenok midenok changed the title MDEV-40821 SIGSEGV in Window_funcs_sort::setup MDEV-40821 SIGSEGV in Window_funcs_sort::setup (11.8) Oct 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

5 participants