MDEV-40032: Promote wide VARCHAR to BLOB for HEAP internal temp tables - #5230
MDEV-40032: Promote wide VARCHAR to BLOB for HEAP internal temp tables#5230arcivanov wants to merge 1 commit into
Conversation
|
|
![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 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.
gkodinov
added
the
External Contribution
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
LGTM after reverting the space only changes. Please stand by for the final review.
62b84f6 to
45a781a
Compare
45a781a to
c37ca6d
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 Fix three latent bugs in `Field_blob_key` exposed by ref access: 1. `Field_blob_key::key_cmp()` treated key bytes at offset 4 as inline data, but the key format is `[4B length][8B pointer_to_data]` -- comparison was against raw pointer bytes instead of actual data. 2. `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). 3. `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()`. `Field_blob_key::key_part_length_bytes()` changed from 4 to 0 so `store_length = key_length (12) + null_byte + 0`, matching HEAP's `seg->length (12) + null` for correct multi-part key alignment. `hp_key_cmp()` blob packlength changed from hardcoded 4 to `seg->bit_start` (actual field packlength) for TEXT (packlength=2). 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).
c37ca6d to
dbca600
Compare
4 participants
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