MDEV-41020 A MEMORY table refuses a row it just held - #5633
Open
arcivanov wants to merge 6 commits into
Open
Conversation
`Item_func_group_concat::add()` decided whether a row was a duplicate
by checking whether `Unique::elements_in_tree()` had grown after
`unique_add()`:
uint count= unique_filter->elements_in_tree();
unique_filter->unique_add(get_record_pointer());
if (count == unique_filter->elements_in_tree())
row_eligible= FALSE;
`Unique` flushes its whole in-memory tree to disk when it runs out of
memory, and `elements_in_tree()` only counts what is still in memory.
After the first flush the test says nothing about the rows that were
already spilled.
**MDEV-11563** made this harmless for `GROUP_CONCAT(DISTINCT x)` by
building the result in `val_str()` from `unique_filter->walk()`, which
merges the spilled parts back in. It left the `ORDER BY` case alone.
There the result comes from the sort tree, which `add()` fills gated by
`row_eligible`, so the defect is still fully live.
Both directions of the failure are reachable, depending on how often
the filter flushes relative to the insert:
1. Duplicates reach the result. 100 rows holding 50 distinct values
give all 100 values back.
2. Rows are lost. 30 distinct rows of 2000 bytes give one value back.
`JSON_ARRAYAGG(DISTINCT x ORDER BY y)` fails in the same way.
Fixed by not filling the sort tree from `add()` when `DISTINCT` is
used. `val_str()` now walks the merged `unique_filter` into the sort
tree and then walks the sort tree, so the rows are sorted after the
duplicate filtering is complete instead of during it.
`Unique::walk()` merges everything it flushed, so the sort tree can be
handed more rows than fit in memory. `insert_to_order_tree()` repacks
it on the same memory budget `add()` used, and a walk that runs out of
memory sets `result_cut`, so the user gets a cut value warning rather
than a silently short result.
**Behaviour change.** `ORDER BY` does not order rows that tie on the
ordering expression, and which of them comes first changes here. It
used to follow the order the rows were read in; it now follows the
order the duplicate filter keeps them in. Unlike the old order, the new
one depends on neither the memory available nor the physical row order.
`main.gconcat_distinct_spill` checks that, and `main.func_gconcat`
records one such tie.
Give a warning when `GROUP_CONCAT(DISTINCT x)` or `JSON_ARRAYAGG(DISTINCT x)` returns only part of a group, or nothing at all, because the walk of the duplicate filter failed. The result is wrong rather than deliberately cut, and nothing was said about it. Both build their result in `val_str()` by walking `unique_filter`, and threw the walk's return value away. `Unique::walk()` reports its own failures through it, from allocating the merge buffer to reading back the chunks it merged, so a failure gave a short result, or an empty one, in silence. The return value cannot be used on its own. `dump_leaf_key()` also stops the walk, for two reasons that are not failures: it cuts the result at `group_concat_max_len`, which it already reports by setting `result_cut`, and it stops without losing anything once the `LIMIT` is used up. Reporting every non-zero return as a cut warns about `GROUP_CONCAT(DISTINCT a LIMIT 5)` returning exactly the five rows that were asked for. `dump_leaf_key()` now records that it was the one that stopped the walk, so `val_str()` asks for the cut value warning only when the walk itself failed. Not every failure is silent either. The merge buffer is allocated with `MY_WME` and the spill file is opened with `MY_WME`, so running out of memory or failing to read raises an error of its own. Only the guard at the top of `merge_walk()`, which refuses a merge buffer too small to hold one key per chunk, returns without saying anything. Warn only when no error was raised: where one was, the user has been told and the statement is failing, so describing the length of a result nobody will see adds nothing. The debug keyword `unique_walk_merge_fail` fails the merging walk quietly and `unique_walk_merge_error` fails it with an error raised. `main.gconcat_distinct_walk_fail` uses both. The `LIMIT` case needs no debug build and is checked in `main.gconcat_distinct_spill`.
A TEXT value longer than `group_concat_max_len` is cut on its way into `blob_storage`, in `Field_blob::handle_group_concat()`. That happens while the group is being built, not when the answer is put together, so it need not have changed the answer at all: the result may well have been cut in the same place anyway. It was reported as a cut value warning, which says that the answer lost something the user asked for. `ER_CUT_VALUES_WHILE_PROCESSING` says instead that a value was cut while the query was processed and names `group_concat_max_len`, which is the limit `handle_group_concat()` uses. It is a note, and one note per aggregate is enough for a statement however many groups had a value cut. `cleanup()` clears the mark, so a statement that is run again gets its own note. What the result lost is still a warning, and is untouched: the result cut at `gconcat_max_len()`, the rows a repack cannot keep, and the failures MDEV-41007 reports. A group can hit both, and then both are given, the warning first. Keeping the warning also keeps a strict `sql_mode` aborting on a cut result, which it does because `THD::raise_condition()` promotes a warning and never promotes a note. `ST_COLLECT` is not affected. It reports `ER_CUT_VALUE_GROUP_CONCAT` itself, against `group_collect_max_len`. `main.gconcat_cut_note` covers the granularity: one note for three cut values in three groups, one note per aggregate when a statement has two of them, a fresh note when the statement runs again, and silence when nothing is cut. It also shows the two diagnostics together, as `JSON_ARRAYAGG()` loses data on the same rows where `GROUP_CONCAT()` does not: its brackets take the result past the limit. Note that `blob_storage` only exists when the aggregate has an `ORDER BY` or a `DISTINCT` and a blob field, so this is the only shape in which a value is cut this way.
Nothing says how many times a statement asks for the result of a group,
and the answer must not depend on it. A `HAVING` clause on the alias is
the shortest statement that asks twice, and it returns a different value
than the same aggregate asked once:
SELECT GROUP_CONCAT(a ORDER BY a LIMIT 2 OFFSET 4) v FROM t1;
-> (empty)
SELECT GROUP_CONCAT(a ORDER BY a LIMIT 2 OFFSET 4) v FROM t1
HAVING v LIKE '%';
-> a,b
`val_str()` walks only while `result_finalized` is false, and
`dump_leaf_key()` raises that flag for the first row it writes. A row
that falls inside the offset is skipped by an earlier return, which
decrements the offset counter and leaves the flag alone. The row-limit
arm immediately above it does raise the flag before its own early
return, so two adjacent early returns behave differently.
A walk in which every row was skipped therefore writes nothing and
records nothing. The next caller walks again with the offset already
spent, and the rows skipped the first time are appended to a result
buffer that was handed over once already.
Once the duplicate filter has spilled to disk the second walk is worse
than wrong. `Unique::reset()` documents the contract:
Clear the tree and the file.
You must call reset() if you want to reuse Unique after walk().
The first walk flushed the tree and emptied it, so the second flushes an
empty tree, appending a chunk that holds no rows. `merge_walk()` reads
nothing back from it and fails `DBUG_ASSERT(bytes_read)`. A build
without assertions goes on to take keys from that chunk.
Set `result_finalized` where the walk block ends, so that it records
every path that has consumed the filter rather than only the paths that
wrote a row.
On the release branches only the form without `ORDER BY` reaches the
duplicate filter. Since MDEV-21879 the `DISTINCT ... ORDER BY`
combination builds its result the same way, so both forms can reach the
assertion here.
- Remove some if - Reorder code - More code comments
`hp_alloc_from_tail()` tests the table memory ceiling before it decides whether the leaf it is about to use has to be allocated or is merely being reclaimed. Reclaiming adds no memory, so on that branch the test answers a question nobody asked. The state it misjudges is routine. `hp_find_free_hash()` allocates an index leaf without consulting `max_table_size`, and no leaf is smaller than `heap_min_allocation_block`, so a table with two hash indexes is already over a 32K ceiling once it holds its first row. That is legal: the ceiling is only tested when the record cursor lands on a leaf boundary, and the first row tests it while both counters are still zero. `hp_shrink_tail()` puts the cursor back on a leaf boundary whenever it empties the tail, and `data_length` goes on counting the leaf, which is still allocated. The next write re-reads that sum and reports `HA_ERR_RECORD_FILE_FULL` for a row the table held a moment earlier. Move the ceiling test to the arm that calls `hp_get_new_block()`. The `max_records` row-count test stays where it is, because it caps rows however the slot is obtained. **Why a master and a slave disagreed about the same statement.** `REPLACE INTO t SELECT * FROM t` feeds the row back out of the table, so the record buffer still points into the chain that the delete parked; the chain is adopted rather than freed and the cursor never moves. The slave builds the row from the replication event buffer, nothing points into the parked chain, it is freed, the tail empties and the write is refused. Replication is one way to reach the state, not the cause: a targeted `DELETE` and a re-`INSERT` on a single server reach it too. Tests: `heap.blob_delete_reinsert_ceiling` covers the single-server route and checks that a table that genuinely needs more memory is still refused; `heap.blob_replace_repl_ceiling` covers the reported master/slave divergence; `hp_test_freelist` test 22 pins the branch itself. Each fails without the change.
gkodinov
added
the
External Contribution
gkodinov
approved these changes
Sep 4, 2026
gkodinov
left a comment
gkodinov
left a comment
Member
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
LGTM. Please stand by for the final review.
Member
|
FYI: According to our development cycle we work on bugs In the following periods 15 Mar-30 Apr, 15 Jun-30 Jul, 15 Sep-30 Oct and 15 Dec-31 Jan. So, please, expect to get a review somewhere between these two dates and the goal is to have your PR merged before the second date |
montywi
force-pushed
the
bb-blob-main-monty
branch
from
September 5, 2026 11:07
dc6a897 to
9357393
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
3 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.
https://jira.mariadb.org/browse/MDEV-41020
hp_alloc_from_tail()tests the table memory ceiling before it decides whether the leaf it is about to use has to be allocated or is merely being reclaimed. Reclaiming adds no memory, so on that branch the test answers a question nobody asked.The state it misjudges is routine.
hp_find_free_hash()allocates an index leaf without consultingmax_table_size, and no leaf is smaller thanheap_min_allocation_block, so a table with two hash indexes is already over a 32K ceiling once it holds its first row — measuredDATA_LENGTH16352 plusINDEX_LENGTH32704 against a 32768 ceiling. That is legal: the ceiling is only tested when the record cursor lands on a leaf boundary, and the first row tests it while both counters are still zero.hp_shrink_tail()then puts the cursor back on a leaf boundary whenever it empties the tail, anddata_lengthgoes on counting the leaf, which is still allocated. The next write re-reads that sum and reportsHA_ERR_RECORD_FILE_FULLfor a row the table held a moment earlier.The fix moves the ceiling test to the arm that calls
hp_get_new_block(). Themax_recordsrow-count test stays where it is, because it caps rows however the slot is obtained.Why a master and a slave disagreed about the same statement
The reported symptom is a slave dying on a
REPLACEthe master accepted.REPLACE INTO t SELECT * FROM tfeeds the row back out of the table, so the record buffer still points into the chain that the delete parked; the chain is adopted rather than freed and the cursor never moves. The slave builds the row from the replication event buffer, nothing points into the parked chain, it is freed, the tail empties and the write is refused.Replication is one way to reach the state, not the cause. A targeted
DELETEand a re-INSERTon a single server reach it too, which is what the non-replication test covers.Scope
The defect arrives with the HEAP blob work:
hp_shrink_tail()is the only thing that moveslast_allocatedbackwards, and before it existed, reachingblock_pos == 0always did mean a new leaf was needed. It reproduces only when the ceiling is below what the fixed 16K-per-leaf minimum forces the table to allocate, so it does not scale to largermax_heap_table_sizevalues.The unbudgeted index leaves are a separate question and are deliberately left alone. Budgeting them would need
CREATE TABLE-time knowledge of key definitions timesheap_min_allocation_block, and would turn currently-working small-ceiling multi-index tables intoCREATE TABLEor first-INSERTfailures.Tests
Written before the fix and each confirmed failing without it:
heap.blob_delete_reinsert_ceiling— the single-server route. PrintsDATA_LENGTH/INDEX_LENGTHso the arithmetic is recorded rather than implied, and closes with a case proving a table that genuinely needs more memory is still refused.heap.blob_replace_repl_ceiling— the reported master/slave divergence. The ceiling is set in the.optso both servers hold the same value throughout, and the table holds a single row on purpose:REPLACEfrees and rewrites one row at a time, so with a second row present the freed records are never the ones at the tail and the defect is invisible.hp_test_freelisttest 22 — pins the allocator branch directly.Verification
main,heap,rpl2717/2717.heap,funcs_1,sys_vars,parts,innodb1726/1726 — internal temporary tables reach this allocator throughMIN(tmp_memory_table_size, max_heap_table_size), so those suites are in scope. Fullctest109/109. Build clean, no new warnings.