MDEV-35673 Item_subselect::used_tables_cache: outer-reference name resolution and query-merge maintenance#5391
MDEV-35673 Item_subselect::used_tables_cache: outer-reference name resolution and query-merge maintenance#5391mariadb-RexJohnston wants to merge 3 commits into
Conversation
mariadb-RexJohnston
commented
Jul 16, 2026
mariadb-RexJohnston
commented
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.
![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 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.
| if (item->field->table->pos_in_table_list && | ||
| (item->field->table->pos_in_table_list->select_lex == cmp)) | ||
| used_tables|= item->field->table->map; |
There was a problem hiding this comment.
In Field_fixer::visit_field, item->field and item->field->table are dereferenced without checking if they are NULL. Since Item_field::used_tables() and other places in the PR explicitly guard against field or field->table being NULL, dereferencing them here directly can lead to a NULL pointer dereference and server crash.
if (item->field && item->field->table && item->field->table->pos_in_table_list &&
(item->field->table->pos_in_table_list->select_lex == cmp))
used_tables|= item->field->table->map;| item= (Item_ident *)item->real_item(); | ||
| // collect usage of Item_fields within this expression | ||
| Field_fixer collector; | ||
| collector.used_tables= 0; | ||
| collector.select= parent; | ||
| collector.not_ready= FALSE; | ||
| item->walk(&Item::enumerate_field_refs_processor, 0, &collector); | ||
| if (collector.not_ready) | ||
| { | ||
| // Now, iterate over fields and collect used_tables() attribute: | ||
| Field_fixer fixer; | ||
| fixer.used_tables= 0; | ||
| fixer.new_parent= new_parent; | ||
| upper->item->walk(&Item::enumerate_field_refs_processor, 0, &fixer); | ||
| used_tables_cache |= fixer.used_tables; | ||
| upper->item->walk(&Item::update_table_bitmaps_processor, FALSE, NULL); | ||
| /* | ||
| if (after_pullout) | ||
| upper->item->fix_after_pullout(new_parent, &(upper->item)); | ||
| upper->item->update_used_tables(); | ||
| */ | ||
| res= 0; | ||
| break; | ||
| } | ||
| res|= collector.used_tables; | ||
| item->walk(&Item::update_table_bitmaps_processor, FALSE, NULL); |
There was a problem hiding this comment.
In Item_subselect::recalc_used_tables, item->real_item() is cast to Item_ident * using a C-style cast. However, real_item() can return any Item subclass (such as Item_func for expressions), which does not inherit from Item_ident. Downcasting to Item_ident * when the object is not of that type is undefined behavior in C++. Since only walk() (a virtual method of Item) is called on the resulting pointer, we should store it as Item * instead to ensure type safety.
Item *real_item= item->real_item();
// collect usage of Item_fields within this expression
Field_fixer collector;
collector.used_tables= 0;
collector.select= parent;
collector.not_ready= FALSE;
real_item->walk(&Item::enumerate_field_refs_processor, 0, &collector);
if (collector.not_ready)
{
res= 0;
break;
}
res|= collector.used_tables;
real_item->walk(&Item::update_table_bitmaps_processor, FALSE, NULL);| 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); | ||
|
|
There was a problem hiding this comment.
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);
}| &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 |
There was a problem hiding this comment.
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 SPReferences
- 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)
| table_map Item_direct_view_ref::used_tables() const | ||
| { | ||
| DBUG_ASSERT(fixed()); | ||
| if(!fixed()) |
There was a problem hiding this comment.
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
- 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)
| 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; |
There was a problem hiding this comment.
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;
2 participants
