Skip to content

MDEV-30073 MDEV-35673 MDEV-40465 outer reference and subquery merge fixes for prepared statements - #5441

Open
mariadb-RexJohnston wants to merge 5 commits into
10.11from
10.11-MDEV-40465
Open

MDEV-30073 MDEV-35673 MDEV-40465 outer reference and subquery merge fixes for prepared statements#5441
mariadb-RexJohnston wants to merge 5 commits into
10.11from
10.11-MDEV-40465

Conversation

@mariadb-RexJohnston

Copy link
Copy Markdown
Member

MDEV-30073 Wrong result on 2nd execution of PS for query with NOT EXISTS

Summary: Items of type Item_direct_view_ref which are reverted with the
change_item_tree mechanism are involved in permanent optimizer transformations.
This commit ensures that items involved in these permanent transformations are
created during the first execution and re-used for subsequent executions.

MDEV-35673 Correlated subquery problems causing wrong results and server crash

Core name-resolution / used_tables rework:

  • Maintain SELECT_LEX::outer_references_resolved_here, a statement-memory
    list of the outer references resolved in each select_lex (relies on
    MDEV-30073 so these are not freed at end of PS execution), and rewrite
    Item_subselect::recalc_used_tables() to compute used_tables_cache from
    it (Item_belongs_to + Field_fixer).
  • Preserve Item_field::depended_from across executions and use it in
    fix_fields/fix_outer_field instead of re-running fix_outer_field, so
    2nd-execution resolution is stable.
  • Maintain nest_level/nest_level_base and merged_into during derived and
    semi-join merges, and update outer_references_resolved_here when a
    subquery is merged into its parent.

MDEV-40465 table map consulted during setup fields on unfixed items

In setup_fields() we call item->update_used_tables() before split_sum_func
so that used_tables() is not consulted before the caches are set up on the
2nd execution of a prepared statement (fixes a 1st/2nd execution result
mismatch under --ps-protocol).

Summary: Items of type Item_direct_view_ref which are reverted with the
change_item_tree mechanism are involved in permanent optimizer
transformations.  This commit ensures that items involved in these
permanent transformations are created during the first execution
and re-used for subsequent executions.

Queries affected by this bug are numerous, but will always involve
1) 2nd execution of a prepared statement or procedure
2) a permanent transformation, such as a semi-join optimization

Detail:

Consider this run as a prepared statement
SELECT * FROM t1
  WHERE EXISTS
  (
    SELECT dt.a FROM
      (
        SELECT t2a as a, t2b as b FROM t2
      ) dt
      WHERE dt.b = t1a
  )

During name resolution of field dt.b (in the where clause) we end
up calling find_field_in_view()/.../create_view_field().
This is responsible for creating a wrapper around the found Item
(Item_field*)`test`.`t2`.`t2b`
While this Item_direct_view_ref representing 'dt.b' is allocated on
Statement (permanent) memory the change is registered to be reversed
at the end of statement execution.  This is odd and contrary to the
permanent nature of this transformation.

Item::exists2in_processor() is called during the preparation in the
first execution.
We transform the query from
select * from t1 where
exists
(
  select `test`.`t2`.`t2b` from
    (
      select `test`.`t2`.`t2a` AS `a`,`test`.`t2`.`t2b` AS `b` from `test`.`t2`
    ) `dt`
    where `test`.`t2`.`t2b` = `test`.`t1`.`t1a`
    limit 1
)

select * from t1 where
`test`.`t1`.`t1a` in
(
  select `test`.`t2`.`t2b` from
    (
      select `test`.`t2`.`t2a` AS `a`,`test`.`t2`.`t2b` AS `b` from `test`.`t2`
    ) `dt`
    where 1
)

later, the optimizer merges the derived table dt into it's parent

select * from t1 where
`test`.`t1`.`t1a` in
(
  select `test`.`t2`.`t2b` from t2 where 1
)

then this is transformed into a semi-join

select t1.* from t1 semi join t2 on t1a = t2b

At the end of the first execution, the item t2b above is reverted to
dt.b.  During the subsequent name resolution of dt.b, it is resolved
t2a, and the semi-join executed corresponds to

select t1.* from t1 semi join t2 on t1a = t2a

causing a different result set.

Initial Author: Igor Babaev
Reformatted and refactored by: Rex Johnston (rex.johnston@mariadb.com)

Add assert to ensure Item_direct_view_refs are not allocated on
the 2nd execution.

Notes:
Item_exists_subselect implicit LIMIT 1 no longer present during prepare
phase

The find_order_in_list save/reuse is safe: every ORDER reaching it comes
from add_to_list (initialized, sql/sql_parse.cc) or copy-constructed
from those (group_concat, window copies). add_proc_to_list ORDERs are
uninitialized but only reach setup_new_fields, which uses find_item_in_list
directly (~sql/sql_select.cc:27694). Saved select_item points into
select_lex->item_list nodes in statement memory so stable across executions.
…ious_ways harness (1/3)

Introduce main.outer_reference: labelled test cases exercising outer
references resolved through view merges, derived-table merges, CTEs,
semi-join conversions, unions, and GROUP BY/ORDER BY subqueries.

Each case is run through main/execute_various_ways.inc, which executes
the query six ways: directly, as a prepared statement executed twice,
wrapped in a derived table, in a view, in a CTE, and in a stored
procedure called twice.  Beyond the visible (recorded) output, the
harness pipes every method through a fresh mysql client session
(include/evw_capture.inc), isolates the output of the *second*
execution for the PS/SP methods, and asserts row-for-row equality of
all six methods with --diff_files.

This commit contains only the cases that already return consistent,
correct results: it documents current behaviour.  The remaining cases
are added by the commits that fix them.

This commit was prepared with Claude Code: it classified each
execute_various_ways group of the original MDEV-35673 test file against
the base tree (one mtr test per group) and selected for this commit the
80 groups whose behaviour is already correct, and drafted this commit
message.
Split from MDEV-32294, discovered while inspecting how
Item_subselect::used_tables_cache is recalculated across the 1st and 2nd
executions of a prepared statement.

Name-resolution permanence:
 - Preserve Item_field::depended_from across executions and use it in
   fix_fields/fix_outer_field instead of re-running the outward search,
   so 2nd-execution resolution is stable.
 - find_field_in_tables: wrap a HAVING outer reference in an Item_ref
   during name resolution (previously done only in
   Item_field::fix_outer_field, now shared via
   fix_field_reference_to_having()), fixing a marked_for_read()
   assertion on queries such as
     SELECT 1 FROM (SELECT a FROM t1) b HAVING (SELECT b.a)=1
 - create_view_field: resolve view-field substitutions against
   current_select and allocate them on statement memory.

Bookkeeping consumed by the next commit:
 - Maintain SELECT_LEX::outer_references_resolved_here, a
   statement-memory list of the outer references resolved in each
   select_lex (relies on MDEV-30073 so these are not freed at end of PS
   execution), populated in st_select_lex::mark_as_dependent and kept
   correct across derived/semi-join merges (merged_into, nest_level /
   nest_level_base maintenance, select_update_base_processor).
   Item_subselect::recalc_used_tables() itself is rewritten in the next
   commit; until then the 2nd-execution guard from MDEV-30073 remains in
   Item_subselect::update_used_tables(), so re-executions run on the
   used_tables_cache computed by the first execution.

Ban EXPLAIN EXTENDED under the mtr --ps-protocol (warning output differs
because some select_transformers no longer run during PREPARE).

Remove select,ps.rdiff / select_jcl6,ps.rdiff / select_pkeycache,ps.rdiff:
the doubled "resolved in SELECT #1" notes they recorded no longer appear
under --ps-protocol.

main.union: the MDEV-29022 queries (UNION ordered by scalar subqueries
referencing the union columns by alias) are temporarily disabled: with
the interim used_tables handling they return misordered rows on the 2nd
execution under --ps-protocol.  Re-enabled by the next commit, which
fixes them.

main.subselect_mat_cost_bugs: the derived-table query whose correlated
WHERE subquery produces two 'Truncated incorrect DECIMAL value'
warnings loses those warnings on the 2nd execution under --ps-protocol
at this commit; its warnings are disabled here and re-enabled by the
next commit, which fixes the discrepancy.

Tests: extend main.outer_reference with the cases fixed by this commit
(view/derived/CTE-merged outer references, GROUP BY/ORDER BY subqueries,
MDEV-32297, MDEV-6054).

This commit was prepared with Claude Code: it partitioned the original
MDEV-35673 commit empirically (building and running the full SQL-layer
suites, both protocols, at each candidate boundary), verified that this
intermediate state is green apart from the two documented cases above,
moved the rdiff removals here from MDEV-40465 where they had gone stale,
and drafted this commit message.
Item_subselect::used_tables_cache was computed from upper_refs, a list
of Ref_to_outside snapshots taken at the first fix_fields().  Those
snapshots record the select the reference resolved in at that moment;
view merges, derived-table merges and semi-join conversions performed
later leave them pointing into merged-away selects, and they cannot be
rebuilt on re-execution.  The stale walk mis-classifies references, so
a correlated subquery can be treated as constant/uncorrelated.

Visible effects fixed here:
 - server crash (SIGSEGV in sub_select) on plain execution of
     ( SELECT a c28 FROM t0 LIMIT 5 )
       ORDER BY ( SELECT c28 FROM t0 LIMIT 1 );     (MDEV-32766)
 - UNION ... ORDER BY (scalar subquery referencing a union column by
   alias) returning misordered rows on the 2nd execution of a prepared
   statement (main.union, MDEV-29022 queries re-enabled here).

recalc_used_tables() now computes the map from
SELECT_LEX::outer_references_resolved_here (populated by the previous
commit and kept correct across merges), classifying each reference with
item_belongs_to()/Field_fixer against the merged_into chain.

With recalculation now valid on every execution:
 - Item_subselect::cleanup() clears used_tables_cache and the FIXED
   flag, so each execution recomputes rather than reusing the previous
   execution's map;
 - the temporary 2nd-execution guard in
   Item_subselect::update_used_tables() (from MDEV-30073) is removed;
 - Item_subselect::mark_as_dependent() no longer builds Ref_to_outside
   entries during PS context analysis (upper_refs is no longer the
   source of used_tables_cache);
 - Item_direct_view_ref::used_tables() returns 0 for a not-yet-fixed
   item instead of asserting, as recalculation can now run while items
   are being re-fixed.

main.win: the MDEV-18431 query (window function + correlated subquery
in one select-list expression) reads a stale used_tables_cache in
split_sum_func at re-execution and returns wrong results on the 2nd
execution under --ps-protocol; it is run with --disable_ps_protocol at
this commit.  The next commit (MDEV-40465) recomputes used_tables in
setup_fields before split_sum_func, fixes this, and removes the
wrapper to demonstrate it.

main.subselect_mat_cost_bugs: the warnings disabled by the previous
commit are re-enabled; the 2nd execution produces them again.

Tests: the remaining main.outer_reference case (MDEV-32766) is added.

This commit was prepared with Claude Code: it isolated the rewrite plus
its activation as the minimal unit that cannot be split further (any
intermediate state returns wrong results on re-execution), confirmed the
MDEV-32766 crash pre-exists on an unpatched 10.11 build, and drafted this
commit message.
In setup_fields() we call item->update_used_tables() before split_sum_func
so that used_tables() is not consulted before the caches are set up on the
2nd execution of a prepared statement.  This fixes a 1st/2nd execution
main.win --ps-protocol failure noted in the previous commit (the window
function's correlated argument looked constant to split_sum_func2, so no
Item_ref into the window temp table was created on re-execution).

That recalculation exposes items whose caches are read while still being
(re)built, so add the guards it now depends on:
 - Item_field::used_tables(): return 0 when field / field->table is not
   yet set, instead of dereferencing a null pointer.
 - Item_func::fix_fields(): reset used_tables_cache/const_item_cache at
   entry (assignment) instead of asserting they are already clear, so a
   re-fix is idempotent.

The --disable_ps_protocol wrapper placed around the main.win
MDEV-18431 query by the previous commit is removed: that query now
returns identical results on the 1st and 2nd execution.

Claude Code relocated this commit's rdiff removals to an earlier commit
of the series (where the recorded differences actually disappear) and
updated this message accordingly.
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.

2 participants