MDEV-21879 GROUP_CONCAT(DISTINCT ORDER BY) is wrong when Unique spills by arcivanov · Pull Request #5574 · MariaDB/server

arcivanov · 2026-08-20T03:40:14Z

GROUP_CONCAT(DISTINCT x ORDER BY y) and JSON_ARRAYAGG(DISTINCT x ORDER BY y)
return a wrong answer once the duplicate filter runs out of memory.

The defect

Item_func_group_concat::add() decides whether a row is a duplicate by checking
whether Unique::elements_in_tree() grew after unique_add(). 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, so after the first
flush that test says nothing about the rows 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, where the result comes from the
sort tree that add() fills gated by the broken test.

Both directions of the failure are reachable, depending on how often the filter
flushes relative to the insert:

Duplicates reach the result - 100 rows holding 50 distinct values give all
100 values back.
Rows are lost - 30 distinct rows of 2000 bytes give one value back.

The fix

add() no longer fills the sort tree when DISTINCT is used. val_str() walks
the merged unique_filter into the sort tree and then walks the sort tree, so the
rows are sorted after 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.

This targets bb-blob-main-monty rather than main because it relies on
37077ccef15 "Limit the memory used by GROUP_CONCAT() with ORDER BY". Without
that commit's tree->allocated bound and its st.oom correction to
repack_tree(), pouring a spilled Unique into the sort tree trades a wrong
answer for an out-of-memory error.

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.func_gconcat records one such tie and is re-recorded accordingly.

Testing

New main.gconcat_distinct_spill covers three things, and each assertion was
confirmed to fail before the fix and pass after, with the test unchanged between
the two runs:

the answer with the filter starved must equal the answer with memory to spare,
for GROUP_CONCAT and JSON_ARRAYAGG, with and without ORDER BY
with the sort tree also starved so repack_tree() cuts the group, whatever
comes back must still be deduplicated, still ordered, and still valid JSON
the tie order must not depend on the memory available or on the physical row
order

Full main+heap suite: 1436/1436 pass.

GROUP_CONCAT() with ORDER BY collects all rows of the group in a TREE and only cuts it down in repack_tree(). The repack was triggered by (tree_len >> GCONCAT_REPACK_FACTOR) > thd->gconcat_max_len() with GCONCAT_REPACK_FACTOR 10, that is when the rows in the tree had produced 1024 * group_concat_max_len bytes, or 1G with the default settings. On top of that tree_len only counted the length of the strings, while the tree costs sizeof(TREE_ELEMENT) + reclength per row. For GROUP_CONCAT(int_col ORDER BY int_col) that is about 40 bytes per row against 6 bytes of result, so the tree had grown to several GB before the first repack. In practice the server ran out of memory first and the repack code was close to never used. The tree is now limited by the memory it has really allocated, tree->allocated, instead of by the length of the strings it holds. The limit is MY_MAX(thd->ram_limitation(), thd->gconcat_max_len()) and is never set so low that the tree can not hold a few rows. repack_tree() builds a new tree while the old one is still in memory, so the peak usage is the size we start the repack at plus the size we copy to. To keep the sum within the limit it is split into GCONCAT_TREE_PARTS parts; the repack starts when GCONCAT_TREE_REPACK_PARTS of them are used and copies to the remaining part. The part we do not copy to is also the room the tree has to grow before the next repack, which keeps the repacks amortized. Other changes: - tree_len is removed. It was only read by the old trigger. - repack_tree() decided that it had run out of memory by testing st.len <= st.maxlen after the walk. That test was only valid because the old trigger guaranteed that a complete copy had to overshoot st.maxlen. A repack triggered by memory can complete the walk with st.len far below st.maxlen, which would have failed the query with a wrong out of memory error. There is now an explicit flag for it. - The length that decides which rows to keep now also counts the separator that is put between two rows, so that it matches what val_str() will produce. - When the memory limit stops the copy, the result becomes shorter than group_concat_max_len. dump_leaf_key() can not detect this, as the result never reaches the maximum length. This is now remembered in result_cut and reported to the user. - All cut value reporting is moved to val_str(); dump_leaf_key() only marks that the result was cut. This removes the need to clear the truncated flag of table->blob_storage to avoid a duplicated warning, and gives one warning per group also when val_str() is called more than once for the same group, which repeated the warning before. - Added a function comment for repack_tree() that describes where the rows are cut away and why building a copy frees memory.

`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.

gkodinov

Thank you for your contribution! This is a preliminary review.

LGTM. One small cleanup proposed.

Please stand by for the final review.

gkodinov · 2026-08-20T07:56:38Z

 00,01,10,11,31
 select group_concat(distinct a, c order by a) from t1;
 group_concat(distinct a, c order by a)
-00,01,11,10,31


I'd try to stabilize this test instead of re-recording the new undeterministic order.

it is deterministic, it's just that determinism has changed.

See Behavior Change section please

gkodinov · 2026-08-20T08:02:19Z

BTW, any specific reason this is not based on 12.3 (the lowest affected version according to Jira)?

arcivanov · 2026-08-20T08:06:44Z

BTW, any specific reason this is not based on 12.3 (the lowest affected version according to Jira)?

Yes, because it depends on other commits that are specifically in bb-blob-main-monty. I'll discuss this with @montywi if/when he is available. There may be further work around GROUP_CONCAT as well (e.g. mem -> HEAP -> Aria spillover mechanisms to eliminate the avoidable truncation of the results).

gkodinov · 2026-08-24T08:07:13Z

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 and others added 2 commits August 20, 2026 00:21

gkodinov added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Aug 20, 2026

gkodinov self-assigned this Aug 20, 2026

gkodinov approved these changes Aug 20, 2026

View reviewed changes

gkodinov requested a review from montywi August 20, 2026 08:01

gkodinov assigned montywi and unassigned gkodinov Aug 20, 2026

montywi force-pushed the bb-blob-main-monty branch from 37077cc to 5847c1a Compare August 23, 2026 16:47

Navigation Menu

Uh oh!

Uh oh!

MDEV-21879 GROUP_CONCAT(DISTINCT ORDER BY) is wrong when Unique spills - #5574

MDEV-21879 GROUP_CONCAT(DISTINCT ORDER BY) is wrong when Unique spills#5574
arcivanov wants to merge 2 commits into
MariaDB:bb-blob-main-montyfrom
arcivanov:MDEV-21879

arcivanov commented Aug 20, 2026

Uh oh!

gkodinov left a comment

Uh oh!

gkodinov Aug 20, 2026

Uh oh!

arcivanov Aug 20, 2026

Uh oh!

arcivanov Aug 20, 2026

Uh oh!

gkodinov commented Aug 20, 2026

Uh oh!

arcivanov commented Aug 20, 2026

Uh oh!

gkodinov commented Aug 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

arcivanov commented Aug 20, 2026

The defect

The fix

Behaviour change

Testing

Uh oh!

gkodinov left a comment

Choose a reason for hiding this comment

Uh oh!

gkodinov Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

arcivanov Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

arcivanov Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

gkodinov commented Aug 20, 2026

Uh oh!

arcivanov commented Aug 20, 2026

Uh oh!

gkodinov commented Aug 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants