Skip to content

MDEV-35673 Item_subselect::used_tables_cache: outer-reference name resolution and query-merge maintenance#5391

Draft
mariadb-RexJohnston wants to merge 3 commits into
10.11from
10.11-MDEV-35673-R1
Draft

MDEV-35673 Item_subselect::used_tables_cache: outer-reference name resolution and query-merge maintenance#5391
mariadb-RexJohnston wants to merge 3 commits into
10.11from
10.11-MDEV-35673-R1

Conversation

@mariadb-RexJohnston

Copy link
Copy Markdown
Member
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.

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.

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),
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.

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

Tests: introduce main.outer_reference with ~90 labelled cases and the
execute_various_ways.inc harness, which runs each query six ways (direct,
prepare+execute twice, derived table, view, CTE, stored procedure) and,
via include/evw_capture.inc, cross-checks that all six return identical
rows.

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.
resolution and query-merge maintenance (part 1/2)

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.

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.

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),
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.

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

Tests: introduce main.outer_reference with ~90 labelled cases and the
execute_various_ways.inc harness, which runs each query six ways (direct,
prepare+execute twice, derived table, view, CTE, stored procedure) and,
via include/evw_capture.inc, cross-checks that all six return identical
rows.

Split and documentation by Claude Opus 4.8
recalculation and the guards it requires (part 2/2)

setup_fields(): for SQLCOM_SELECT, call item->update_used_tables() after
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 in main.subselect_nulls under --ps-protocol).

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_direct_view_ref::used_tables(): return 0 when the item is not
   fixed yet, instead of asserting.
 - 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.

Remove select,ps.rdiff / select_jcl6,ps.rdiff / select_pkeycache,ps.rdiff:
the extra "resolved in SELECT #1" notes they recorded no longer appear
under --ps-protocol, so the select tests now match without an rdiff.

Split and documentation by Claude Opus 4.8.
@mariadb-RexJohnston mariadb-RexJohnston marked this pull request as draft July 16, 2026 03:15

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors how outer references are tracked and resolved across subquery merges and prepared statement executions, introducing a list of resolved outer references on SELECT_LEX and updating the recalc_used_tables logic. The review feedback highlights several safety and robustness improvements, including adding critical NULL pointer checks in Field_fixer::visit_field and SELECT_LEX::merge_subquery to prevent server crashes, avoiding unsafe downcasting of real_item() in Item_subselect::recalc_used_tables, addressing style guide violations regarding spacing after if statements, and ensuring consistent use of nullptr for pointer initializations.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread sql/item.cc
Comment thread sql/item_subselect.cc
Comment thread sql/sql_lex.cc
Comment on lines +5420 to +5427
for (order= subq_select->order_list.first ; order; order= order->next)
(*order->item)->walk(&Item::select_update_base_processor, TRUE,
(void*) &update);

for (order= subq_select->group_list.first ; order; order= order->next)
(*order->item)->walk(&Item::select_update_base_processor, TRUE,
(void*) &update);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

In SELECT_LEX::merge_subquery, the order_list and group_list are walked. However, order->item or *order->item can be NULL in some edge cases (e.g., if resolution failed or was cleared). Dereferencing them without a NULL check can lead to a server crash. We should add defensive NULL checks, similar to how where and having are checked.

  for (order= subq_select->order_list.first ; order; order= order->next)
  {
    if (order->item && *order->item)
      (*order->item)->walk(&Item::select_update_base_processor, TRUE,
                           (void*) &update);
  }

  for (order= subq_select->group_list.first ; order; order= order->next)
  {
    if (order->item && *order->item)
      (*order->item)->walk(&Item::select_update_base_processor, TRUE,
                           (void*) &update);
  }

Comment thread sql/item_subselect.cc
&substitution, TRUE);
select_lex->nest_level_base= select_lex->outer_select()->nest_level_base;
select_lex->merged_into= select_lex->outer_select();
if(parent_select) // can happen with SP

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Missing space after if keyword. According to the project's coding standards, there should be a space between if and the opening parenthesis.

    if (parent_select)                                    // can happen with SP
References
  1. Follow the project's specific coding standards (e.g., CODING_STANDARDS.md) for code formatting and spacing, such as spacing around assignment operators, rather than generic style guides like the Google C++ Style Guide. (link)

Comment thread sql/item.cc
table_map Item_direct_view_ref::used_tables() const
{
DBUG_ASSERT(fixed());
if(!fixed())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Missing space after if keyword. According to the project's coding standards, there should be a space between if and the opening parenthesis.

  if (!fixed())
References
  1. Follow the project's specific coding standards (e.g., CODING_STANDARDS.md) for code formatting and spacing, such as spacing around assignment operators, rather than generic style guides like the Google C++ Style Guide. (link)

Comment thread sql/sql_parse.cc
Comment on lines +8331 to +8334
order->view_ref= 0; // may be read in find_order_in_list() before being set
order->resolution= NOT_RESOLVED;
order->select_item= nullptr;
order->from_field= nullptr;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Inconsistent pointer initialization. order->view_ref is initialized to 0 while other pointers in the same block are initialized to nullptr. We should use nullptr consistently for all pointer initializations in modern C++ code.

  order->view_ref= nullptr;   // may be read in find_order_in_list() before being set
  order->resolution= NOT_RESOLVED;
  order->select_item= nullptr;
  order->from_field= nullptr;

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