MDEV-35747: Wrong result from prepared TVC with parameter markers

[DaveGosselin-MariaDB]

The setup of column type information in table_value_constr::prepare() was wrapped in an "if (!holders)" guard so that it runs only once per prepared statement. However, the guard was too wide because it bound the allocation of item holders (which should happen only once) to the collection of type information (which should happen on each execution).

This leaves the TVC stuck with whatever placeholder type the parameter had at PREPARE time which likely won't match the type of the next substitution (because a type holder has no actual type at PREPARE time). Its type only becomes known when a value is bound at EXECUTE time. So both the TVC types and the corresponding Item_type_holder instance in the SELECT item list must be computed again on every EXECUTE.

This patch does just that, and separates the work done once per prepared statement from the work done on every execution as implied above.

[gemini-code-assist]

Code Review

This pull request fixes MDEV-35747, which caused prepared statements with Table Value Constructors (TVC) in CTEs to return incorrect results. The implementation reuses Type_holder buffers across executions and refreshes type metadata on each call. Review feedback identifies a critical bug where type_holders is assigned before full initialization, potentially leading to crashes on OOM. Additionally, the review suggests checking for other 'sticky' attributes that might cause metadata corruption and points out a redundant call to set_maybe_null.

[mariadb-YuchenPei]

claude review

---

Code Review: MDEV-35747 — Wrong result from prepared TVC with parameter markers

Author: Dave Gosselin · Files: sql/sql_tvc.cc (+80/−9), sql/sql_class.h (+7 comment), tests (+65)

Overview

A table value constructor used in a prepared statement (e.g. WITH t(id) AS (VALUES (?)) SELECT ...) returned stale results across EXECUTEs. Root cause: in table_value_constr::prepare(), the type collection (join_type_handlers_for_tvc + get_type_attributes_for_tvc) and the Item_type_holder list build were both wrapped in an if (!holders) guard that runs only once per statement. A parameter marker has no real type at PREPARE, so the TVC froze whatever placeholder type it had and never updated when a later EXECUTE bound a value of a different type.

The fix correctly splits the work:

• Once per statement (if (!holders)): allocate the Type_holder[] array and each holder's args[] buffer on the statement arena.
• Every EXECUTE: recompute type handlers/attributes, and either build the Item_type_holder list (first time) or mutate the existing instances in place (subsequent times). get_type_attributes_for_tvc now calls remove_arguments() (reset arg_count) instead of reallocating args[] each call.

Correctness — verified

• ✅ args[] reuse is safe. alloc_arguments(thd, lists_of_values.elements) sizes each buffer to the (SQL-fixed, constant) row count; per-execute remove_arguments() + one add_argument() per row refills it exactly. add_argument has no bounds check, but the fill count equals capacity every time, so no overflow. The count<=2 inline tmp_arg path also persists correctly since the Type_holder array lives on the stmt arena.
• ✅ Types are genuinely recomputed per execute. join_type_handlers_for_tvc does set_handler() on the first row each call (fresh, not accumulated), and Item_hybrid_func_fix_attributes recomputes max_length/decimals/collation into the holder. The test (1 → 'abc', 10/20 → 'aa'/'bb') confirms the observable fix.
• ✅ In-place mutation mirrors the constructor for the type-varying fields: set_handler ↔ Type_handler_hybrid_field_type(handler), Type_std_attributes::set ↔ ctor's Type_std_attributes::set, set_maybe_null ↔ ctor. name and the copied flags are structural and legitimately not re-set.
• ✅ Contract upheld. get_type_attributes_for_tvc has exactly one caller (prepare), and args[] is always allocated before it runs, in both first-prepare and re-execute paths.
• ✅ Good defensive change: building into a local new_holders and only publishing to holders/type_holders after all alloc_arguments succeed avoids leaving a half-initialized array behind on OOM — an improvement over the previous immediate assignment.

Issues / risks

• enum_set_typelib not refreshed in the mutate branch (minor completeness gap). The constructor sets enum_set_typelib(attr->get_typelib()), but the else branch updates only handler/attrs/maybe_null. For parameter markers the aggregated typelib is NULL/constant, so there's no impact on this fix; but if a TVC column's aggregated type could ever resolve to ENUM/SET and change across executes, the cached typelib would go stale. Consider mirroring it for parity (needs a setter on Item_type_holder, whose enum_set_typelib is currently protected).
• Sticky maybe_null (documented). The new sql_class.h comment honestly notes that aggregate_attributes OR-accumulates m_maybe_null and never resets it, so a column stays nullable for the statement's lifetime once any execute sees NULL. This is safe (only over-widens nullability — no wrong result) and, for parameter-based TVCs, arguably correct anyway (an unbound ? is nullable). It is, however, inconsistent with the handler, which is reset each execute. If you want tightness, resetting m_maybe_null= false at the top of aggregate_attributes (or right after remove_arguments) would make nullability reflect only the current execute. Documenting-instead-of-fixing is a reasonable call here.
• Unchecked static_cast<Item_type_holder*> in the mutate branch. Safe today because that list is built exclusively of Item_type_holders, but a DBUG_ASSERT((*it)->type() == Item::TYPE_HOLDER) would harden against future changes.

Style

• Clear, well-scoped comments explaining why each block is once-vs-per-execute — genuinely helpful.
• Minor: the "already built" discriminator sl->item_list.elements != cnt reads slightly obliquely; sl->item_list.is_empty() would express "not yet built" more directly (equivalent, since the build always produces exactly cnt).
• Two separate activate_stmt_arena_if_needed/restore_active_arena pairs in the same prepare — fine and necessary given the split, just slightly repetitive.

Test coverage

• ✅ Solid: single param, multi-row/multi-param with ORDER BY, type change across executes (int→string), and repeated identical executes (regression against the frozen-type bug).
• Suggestions: add (a) a NULL-then-non-NULL bind to lock in the documented sticky-nullable behavior; (b) a multi-column VALUES with parameter markers (VALUES (?, ?) as columns, not rows) to exercise per-column holders; both are cheap and cover paths the current tests don't.

Security

• No concerns. No new external input handling; buffer reuse is bounded by the fixed row count; allocations are arena-scoped.

Verdict

Approve. Correct, well-reasoned fix that properly separates per-statement from per-execute work and eliminates a real wrong-results bug, with a nice defensive improvement around partial initialization. Only minor follow-ups worth considering: refresh enum_set_typelib in the mutate path for parity, optionally reset m_maybe_null for tighter nullability, add the static_cast assert, and extend tests with NULL-binding and multi-column cases.

[mariadb-YuchenPei]

first PREPARE

I guess should be "PREPARE or first EXECUTE", or are there scenarios of multiple PREPAREs where it is not nullptr during the second and subsequent PREPAREs?

[DaveGosselin-MariaDB]

Thanks, right, I reworded the comment; I can't see how it would be otherwise.

[mariadb-YuchenPei]

It still says "first PREPARE". Are there cases of multiple PREPAREs where it is not nullptr during the second and subsequent PREPAREs?

[mariadb-YuchenPei]

I tried to narrow the "is_stmt_prepare_or_first_stmt_execute()" to "is_stmt_prepare()" and I get assertion failures in SP testcases as well as PS cases such as "SELECT * FROM t1 WHERE c IN ('10','20')" as the prepared statement. So for some reason the it could be the first EXECUTE too, not just PREPARE. Can you update the commit message to reflect that? Currently it refers to this branch as PREPARE only.

[DaveGosselin-MariaDB]

Added another paragraph to the commit message to clarify.

[mariadb-YuchenPei]

Thanks. The new paragraph says "For a prepared statement it happens at PREPARE". But there are (edge?) cases where it can happen at the first EXECUTE, such as when a TVC is constructed due to the "IN predicates into subqueries" optimisation e.g. this block in table_value_constr.test:

--echo #
--echo # MDEV-23182: Server crashes in
--echo # Item::fix_fields_if_needed / table_value_constr::prepare upon 2nd execution of PS
--echo #
SET @save_in_predicate_conversion_threshold=@@in_predicate_conversion_threshold;
SET in_predicate_conversion_threshold=2;

CREATE TABLE t1 (c VARCHAR(10)) DEFAULT CHARSET=utf8;
PREPARE stmt FROM "SELECT * FROM t1 WHERE c IN ('10','20')";
EXECUTE stmt;
--echo # Without the patch second execution of the prepared statement 'stmt'
--echo # results in crash.
EXECUTE stmt;
DEALLOCATE PREPARE stmt;

DROP TABLE t1;

[mariadb-YuchenPei]

claude review comment:

---

Code Review: MDEV-35747 — Wrong result from prepared TVC with parameter markers (revised)

Author: Dave Gosselin · Files: sql/sql_tvc.cc (+85/−9), sql/item.h (+11/−3), sql/sql_class.h (+7 comment), tests (+92)

Overview

Same core fix as the prior revision: table_value_constr::prepare() computed TVC column types only once (under an over-broad if (!holders) guard), freezing a parameter marker's placeholder type at build time so later EXECUTEs with a different type returned wrong results/metadata. The fix splits once-per-statement work (allocate the Type_holder[] array + each holder's args[] buffer on the statement arena) from per-execute work (recompute handlers/attributes, then build-or-refresh the Item_type_holder list).

What changed since the previous revision — and it's better

• ✅ refresh() helper extracted on Item_type_holder (item.h), called by both the constructor and the per-execute mutate path. This eliminates the constructor-vs-mutation drift risk I raised before: handler, std attributes, and maybe_null now provably stay in sync by construction. Verified it compiles — the default Type_handler_hybrid_field_type() ctor exists (sql_type.h:7569), set_handler() replaces the old init-list handler, and Type_std_attributes::set(const Type_std_attributes*) (pointer overload at sql_type.h:3225) binds attr/&holders[pos].
• ✅ Discriminator is now sl->item_list.is_empty() instead of elements != cnt — clearer "not yet built" intent, as suggested.
• ✅ DBUG_ASSERT(elem->type() == Item::TYPE_HOLDER) added before the static_cast in the mutate branch — the hardening I recommended.
• ✅ New bare-VALUES (?) test (no CTE) added, exercising a TVC as the top-level prepared statement — good extra coverage.
• ✅ Clearer commit message explaining that first allocation coincides with PREPARE for a PS but with first execution for a stored procedure, which is why the assertion permits both.

Correctness — verified (carried over + re-checked)

• ✅ args[] reuse is safe: one-time alloc_arguments(thd, lists_of_values.elements) sizes each buffer to the SQL-fixed row count; per-execute remove_arguments() + exactly-one add_argument() per row refills it to capacity (no bounds check in add_argument, but the fill count equals capacity every time).
• ✅ Types genuinely recomputed per execute: join_type_handlers_for_tvc re-set_handler()s on the first row (fresh, not accumulated), and aggregate_attributes→Item_hybrid_func_fix_attributes recomputes std attributes each call.
• ✅ holders/item_list stay coupled: both persist on the stmt arena and are built together on the first call; the only item_list.empty() is inside the guarded build block, so nothing re-empties it per execute (no per-execute allocation growth).
• ✅ Defensive local new_holders published to type_holders only after all args[] allocs succeed — avoids a half-initialized array being seen as "already built."
• ✅ is_empty() build branch keeps the arena assertion, consistent with first-time (PREPARE or first SP execution) build.

Remaining minor points (mostly unchanged from prior review)

• enum_set_typelib still not refreshed. The constructor sets enum_set_typelib(attr->get_typelib()), but refresh() — which now reads as "refresh all the type info" — omits it, so the mutate path never updates it. For parameter markers the aggregated typelib is NULL/constant, so no bug in the target scenario; but the omission is now easier to trip over given refresh()'s name. Recommend either folding typelib into refresh() (and having the ctor set it via refresh too) or adding a one-line comment on why it's deliberately excluded.
• Sticky maybe_null (documented). The sql_class.h comment honestly records that m_maybe_null OR-accumulates and never resets, so a column stays nullable for the statement's lifetime once any execute binds NULL. Safe direction (over-nullable only, no wrong result) and, for ?-based TVCs, effectively expected. Documenting rather than resetting is a reasonable call; a m_maybe_null= false reset at the top of aggregate_attributes would make it precise if desired.

Test coverage

• ✅ Good: bare VALUES (?), CTE-wrapped TVC, and multi-row/multi-param with ORDER BY; each repeats the same bind then switches type (int→string) to guard the frozen-type regression.
• Still worth adding: (a) a NULL-then-non-NULL bind to lock in the documented sticky-nullable behavior; (b) a multi-column VALUES (?, ?)-as-columns case (current multi-param test is multi-row, single-column) to exercise per-column holders independently.

Security / performance

• No concerns. Buffer reuse is bounded by the fixed row count; allocations are arena-scoped and now happen once instead of per-execute (a small perf improvement over pre-fix behavior); no new external-input handling.

Verdict

Approve. This revision keeps the correct once-vs-per-execute split and improves on the previous version: the shared refresh() removes the parity risk, is_empty() and the type assertion address earlier nits, and the bare-VALUES test broadens coverage. Only optional follow-ups remain — decide on enum_set_typelib in refresh() (fold in or comment), optionally reset m_maybe_null for tighter nullability, and add the NULL-bind and multi-column tests.

[mariadb-YuchenPei]

LGTM after addressing review comments. Sorry actually I'd like to take another look after this round.

Can you please also address claude's review comments - the "enum_set_typelib still not refreshed." one and the "Still worth adding:" one?

[mariadb-YuchenPei]

It still says "first PREPARE". Are there cases of multiple PREPAREs where it is not nullptr during the second and subsequent PREPAREs?

[mariadb-YuchenPei]

Thanks. The new paragraph says "For a prepared statement it happens at PREPARE". But there are (edge?) cases where it can happen at the first EXECUTE, such as when a TVC is constructed due to the "IN predicates into subqueries" optimisation e.g. this block in table_value_constr.test:

--echo #
--echo # MDEV-23182: Server crashes in
--echo # Item::fix_fields_if_needed / table_value_constr::prepare upon 2nd execution of PS
--echo #
SET @save_in_predicate_conversion_threshold=@@in_predicate_conversion_threshold;
SET in_predicate_conversion_threshold=2;

CREATE TABLE t1 (c VARCHAR(10)) DEFAULT CHARSET=utf8;
PREPARE stmt FROM "SELECT * FROM t1 WHERE c IN ('10','20')";
EXECUTE stmt;
--echo # Without the patch second execution of the prepared statement 'stmt'
--echo # results in crash.
EXECUTE stmt;
DEALLOCATE PREPARE stmt;

DROP TABLE t1;

[mariadb-YuchenPei]

The comment needs update too now that the condition has been simplified. Suggest something like "Populate the item_list with the Item_type_holders just constructed"

[mariadb-YuchenPei]

Given this assert also appears in the "if (!holder)" branch above, I wonder if this "if" branch is entered if and only if that "if" branch is. If that is the case, perhaps it would be a good idea to make this connection clear, with a comment or a boolean set to "!holder" above

[Copilot]

Pull request overview

Fixes MDEV-35747 where prepared statements using TVCs (VALUES (...) / WITH ... AS (VALUES ...)) could return wrong results because placeholder-derived column type metadata was incorrectly kept from PREPARE time instead of being recomputed on each EXECUTE.

Changes:

• Split TVC preparation work into “once per statement” (allocate Type_holder + args buffers) vs “each execution” (re-collect type handlers/attributes).
• Add Item_type_holder::refresh() and use it to update the cached SELECT-list type holders in place on every execution.
• Add regression tests covering type changes across EXECUTEs (including metadata output and NULL behavior).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
sql/sql_tvc.cc	Separates one-time holder allocation from per-execute type/attribute collection; refreshes cached Item_type_holder instances on later executions.
sql/sql_class.h	Documents current “sticky” maybe_null accumulation behavior in Type_holder::aggregate_attributes().
sql/item.h	Introduces Item_type_holder::refresh() and adjusts construction to support in-place refresh.
mysql-test/main/table_value_constr.test	Adds MDEV-35747 regression tests for prepared TVCs across multiple EXECUTEs and metadata checks.
mysql-test/main/table_value_constr.result	Expected output for the new tests.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.