MDEV-40032: Promote wide VARCHAR to BLOB for HEAP internal temp tables - #5230
Open
arcivanov wants to merge 1 commit into
Open
MDEV-40032: Promote wide VARCHAR to BLOB for HEAP internal temp tables#5230arcivanov wants to merge 1 commit into
arcivanov wants to merge 1 commit into
Conversation
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements support for BLOB, TEXT, JSON, and GEOMETRY columns in the HEAP (MEMORY) storage engine (MDEV-38975), allowing operations like GROUP BY, DISTINCT, and joins on blob columns to remain in memory without immediately forcing a conversion to Aria on disk. The code review identified several critical and high-severity issues, including a potential server crash due to a missing NULL check on ftfunc_list in sql_derived.cc, a compilation error from the missing definition of Item_type_holder::create_tmp_field_ex, and a potential infinite loop in heap_check_heap() under corrupted data. Additionally, the reviewer provided actionable suggestions to clean up redundant assignments, use typed pointer arithmetic instead of raw byte manipulation, and parenthesize macro operands to prevent precedence issues.
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.
Closed
9 tasks
gkodinov
added
the
External Contribution
gkodinov
approved these changes
Jun 15, 2026
gkodinov
left a comment
gkodinov
left a comment
Member
There was a problem hiding this comment.
LGTM after reverting the space only changes. Please stand by for the final review.
arcivanov
force-pushed
the
MDEV-40032-main
branch
3 times, most recently
from
July 19, 2026 23:55
62b84f6 to
45a781a
Compare
arcivanov
force-pushed
the
MDEV-40032-main
branch
2 times, most recently
from
July 30, 2026 22:58
c37ca6d to
dbca600
Compare
arcivanov
force-pushed
the
MDEV-40032-main
branch
2 times, most recently
from
August 16, 2026 06:11
b841790 to
bb1f256
Compare
arcivanov
force-pushed
the
MDEV-40032-main
branch
from
August 19, 2026 21:07
bb1f256 to
142e8e0
Compare
montywi
force-pushed
the
bb-blob-main-monty
branch
from
August 19, 2026 21:21
5301674 to
37077cc
Compare
arcivanov
force-pushed
the
MDEV-40032-main
branch
from
August 19, 2026 21:31
142e8e0 to
2b40664
Compare
montywi
force-pushed
the
bb-blob-main-monty
branch
from
August 23, 2026 16:47
37077cc to
5847c1a
Compare
arcivanov
force-pushed
the
MDEV-40032-main
branch
2 times, most recently
from
August 28, 2026 18:22
6890110 to
6598f1e
Compare
montywi
force-pushed
the
bb-blob-main-monty
branch
from
September 5, 2026 11:07
dc6a897 to
9357393
Compare
arcivanov
force-pushed
the
MDEV-40032-main
branch
4 times, most recently
from
September 7, 2026 18:53
49afd0a to
23c4f94
Compare
Use `Field_blob_key` as the single unified mechanism for ALL blob columns in HEAP temp tables, replacing Phase 1's dual approach of `Field_blob_key` for GROUP BY/DISTINCT + `rebuild_blob_key_from_segments` for derived table ref access. Key changes: - `Tmp_field_param::is_heap_engine()` gates `Field_blob_key` creation for all blob fields in HEAP temp tables (not just `part_of_unique_key`) - `varstring_type_handler()` promotes VARCHAR > `HEAP_CONVERT_IF_BIGGER_TO_BLOB` to blob via `blob_type_handler()` -> `type_handler_blob_key` - `Type_handler_blob_common::type_handler_for_tmp_table()` returns `blob_key_type_handler()` when `is_heap_engine()` - `Item_field::create_tmp_field_from_item_field()` redirects HEAP blob fields through the type handler system - `Item_type_holder::create_tmp_field_ex()` extended for UNION/CTE blobs Implement `Field_blob_key::key_cmp(const uchar*, uint)`, which ref access is the first caller to reach, and fix two latent bugs that reaching it exposes. Both `key_cmp()` overloads were placeholders calling `abort()`. The bodies beside them sat under `#ifdef NOT_YET_USED`, which is defined nowhere, and would not have compiled - the first references an undeclared `blob1`. They are a sketch rather than disabled code, and the sketch reads the key wrong: it takes the bytes at offset 4 as inline data where the format is `[4B length][8B pointer_to_data]`. Only the reached overload gets a body. `key_cmp(a, b)` is called from rowid comparison, DS-MRR key sorting, SEL_ARG trees and histogram lookup, and each of those builds a blob key part as `[HA_KEY_BLOB_LENGTH length][data]` - the format the key tuple description at the top of `opt_range.cc` documents, and the one `ha_innobase::cmp_ref()` handles blobs separately in order to honour. Writing a `[4B length][8B pointer]` reader for those callers would take inline data as a pointer and dereference it, and none of them reads an internal temporary table, the only place a `Field_blob_key` exists. So that overload becomes `DBUG_ASSERT(0)` rather than `abort()`: the same treatment `get_key_image()` has, and one that reports a future caller instead of following a fabricated pointer. 1. `cmp_buffer_with_ref()` eq_ref cache compared raw key buffer bytes. When `Field_blob_key::value` buffer is reused across lookups, the `[4B length][8B pointer]` bytes don't change even when the pointed-to data differs, causing stale result reuse. Disable the cache for all HEAP blob key parts (remove the `length == 0` guard). 2. `Field_blob_key::new_key_field()` returns `Field_blob_key` (unlike `Field_blob::new_key_field()` which returns `Field_varstring`). The `store_key` mechanism stores into `to_field->value` String, which leaks because `store_key` is `Sql_alloc` with no destructor. Add `store_key::cleanup()` called from `JOIN_TAB::cleanup()` and `subselect_uniquesubquery_engine::cleanup()`. The same pointer indirection decides how a **constant** reaches a blob key part. `create_ref_for_key()` normally compares against a constant by building a `store_key` on the stack, copying once and recording the key part in `const_ref_part_map`. A blob key part cannot be handled that way: the key holds a pointer to the value, so freeing the `store_key` leaves the key pointing at released memory, and not freeing it leaks the value for the lifetime of the connection. Route every `HA_BLOB_PART` key part to `ref.key_copy` instead, where `JOIN_TAB::cleanup()` frees it once execution is over, and leave `const_ref_part_map` clear so that `cp_buffer_from_ref()` re-copies the value. `store_key_const_item` still evaluates the constant only once, and prints as `const` in `EXPLAIN` exactly as the const map did. `Field_blob_key::key_part_length_bytes()` changed from 4 to 0: the 4 byte length is part of a blob key part, not a prefix in front of one. `Create_tmp_table::finalize()` computed `store_length` before the blob branch overwrote `length`, and that branch then reset `store_length` to the bare length, discarding the null flag byte which `keyinfo->key_length` had already counted. `store_length` is the distance to the next key part, so a blob key part reporting one byte too few made `TABLE_REF::tmp_table_index_lookup_init()` place the following key part over the last byte of the blob data pointer, which HEAP then dereferenced. Derive `store_length` once, from the final length plus the null byte, and accumulate `keyinfo->key_length` from that same value: `12 + null` matches HEAP's `seg->length (12) + null`. `TABLE::check_tmp_key()` rejected every blob key part outright. That has to be relaxed for a HEAP temporary table, which keys on a blob through `Field_blob_key`, but only that far. A native `Field_blob` has no key image of its own, reports `key_length() == 0`, and would produce a zero-length key segment that the on-disk engines reject. Reject on the absent key image rather than on the column type. `Item_field::create_tmp_field_from_item_field()`'s HEAP blob branch has to do what `Field::create_tmp_field()` does for the branch beside it, above all restoring `orig_table`. Without it the result set metadata reports a blob column that passed through a temporary table as belonging to that temporary table rather than to the table it came from. `hp_key_cmp()` blob packlength changed from hardcoded 4 to `seg->bit_start` (actual field packlength) for TEXT (packlength=2). New tests. `main.sj_mat_blob_multipart` puts a blob key part in every position of a multi-part subquery materialization key, including last, first, between two other parts, and beside a second blob part; every shape asserts its plan, because the materialization these depend on degrades silently to a dependent subquery once the value stops being promoted. `main.tmp_table_blob_key` covers a constant compared against a blob key part, a blob column of a derived table materialized on disk, and the result set metadata of a blob column that passes through a temporary table. Re-record GROUP_CONCAT-related results: the `Tmp_field_param` threading through `tmp_table_field_from_field_type()` (base branch) closed a plumbing gap where `Item_sum` and literal items dropped the param, so they now reach the HEAP promotion gates like all other expression items. `GROUP_CONCAT` results in HEAP temp tables become `Field_blob_key` (longtext metadata, 12-byte blob key parts), consistent with the `Item_func` path that was already recorded (e.g. `substring()` sj-materialization keys).
arcivanov
force-pushed
the
MDEV-40032-main
branch
from
September 7, 2026 19:20
23c4f94 to
ba50d91
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
octet_length > HEAP_CONVERT_IF_BIGGER_TO_BLOB(32 bytes) to BLOB when the temporary table uses the HEAP engine, storing only actual data in continuation chains (from MDEV-38975) instead of reserving the full declared width in every rowField_blob_keyas the single unified mechanism for ALL blob columns in HEAP temp tables -- both GROUP BY/DISTINCT keys and derived table ref accessField_blob_keybugs exposed by ref access:key_cmp()pointer-vs-data comparison,cmp_buffer_with_ref()stale cache, andnew_key_field()memory leakheap_store_key_blob_refstore_key subclass bypasses SQL-layer key buffer for BLOB key parts, enabling ref access for all BLOB sizes including LONGBLOB/JSON on HEAP derived tablesBased on
bb-blob-main-monty, which already carries all prerequisites (MDEV-38975 #5222, MDEV-40030, MDEV-40029, MDEV-40033) and the HEAP fix stack (MDEV-40523, MDEV-40376, MDEV-40378, MDEV-40431, MDEV-40448, MDEV-40447); this PR is the single promotion commit on top of that branch.Note on
heap.blob_window_overflowresult changes:CONCAT(@c:=@c+1, ':', PERCENTILE_DISC(...))declares a width of only 39 chars (Item_sum_percentile_disc::fix_length_and_dec()never setsmax_lengthfrom the ORDER BY argument), so before promotion the 2908-char percentile value was silently truncated to 39 when materialized through theVARCHAR(39)temp column. With promotion the column becomes a BLOB and the full value survives (min_len39 -> 2910); theCOUNT(DISTINCT e)dedup temp table then also converts to disk under the test's 4MBmax_heap_table_size(Created_tmp_disk_tables1 -> 2).Testing
heapsuite 38/38,hp_*unit tests 9/9,mainsuite 1392/1392 on the rebased branchJIRA: https://jira.mariadb.org/browse/MDEV-40032