Skip to content

Commit 8a041de

Browse files
arcivanovarcivanov
committed
MDEV-21879 GROUP_CONCAT(DISTINCT ORDER BY) is wrong when Unique spills
`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.
1 parent 37077cc commit 8a041de

5 files changed

Lines changed: 294 additions & 23 deletions

File tree

‎mysql-test/main/func_gconcat.result‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -863,7 +863,7 @@ group_concat(distinct a, c)
863863
00,01,10,11,31
864864
select group_concat(distinct a, c order by a) from t1;
865865
group_concat(distinct a, c order by a)
866-
00,01,11,10,31
866+
01,00,11,10,31
867867
select group_concat(distinct a, c) from t1;
868868
group_concat(distinct a, c)
869869
00,01,10,11,31

‎mysql-test/main/gconcat_distinct_spill.result‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
#
2+
# Each block records the answer computed with memory to spare, then
3+
# recomputes it with the duplicate filter starved, and compares.
4+
#
5+
CREATE TABLE t1 (pk INT AUTO_INCREMENT PRIMARY KEY, a VARCHAR(100) NOT NULL);
6+
INSERT INTO t1 (a) SELECT LPAD(seq, 4, '0') FROM seq_1_to_50;
7+
INSERT INTO t1 (a) SELECT a FROM t1 ORDER BY pk;
8+
SELECT COUNT(*) AS rows_in_table, COUNT(DISTINCT a) AS distinct_values FROM t1;
9+
rows_in_table distinct_values
10+
100 50
11+
SELECT GROUP_CONCAT(DISTINCT a) INTO @gc FROM t1;
12+
SELECT GROUP_CONCAT(DISTINCT a ORDER BY a) INTO @gc_order FROM t1;
13+
SELECT JSON_ARRAYAGG(DISTINCT a) INTO @ja FROM t1;
14+
SELECT JSON_ARRAYAGG(DISTINCT a ORDER BY a) INTO @ja_order FROM t1;
15+
SET @@tmp_memory_table_size=0;
16+
SELECT GROUP_CONCAT(DISTINCT a) = @gc AS gc_unchanged FROM t1;
17+
gc_unchanged
18+
1
19+
SELECT GROUP_CONCAT(DISTINCT a ORDER BY a) = @gc_order AS gc_order_unchanged FROM t1;
20+
gc_order_unchanged
21+
1
22+
SELECT JSON_ARRAYAGG(DISTINCT a) = @ja AS ja_unchanged FROM t1;
23+
ja_unchanged
24+
1
25+
SELECT JSON_ARRAYAGG(DISTINCT a ORDER BY a) = @ja_order AS ja_order_unchanged FROM t1;
26+
ja_order_unchanged
27+
1
28+
SET @@tmp_memory_table_size=DEFAULT;
29+
DROP TABLE t1;
30+
#
31+
# Values wide enough that the filter flushes on nearly every row.
32+
# Here the ORDER BY case used to return a single value out of 30.
33+
#
34+
CREATE TABLE t2 (a VARCHAR(2000)) AS
35+
SELECT CONCAT(seq, REPEAT('.', 1990)) AS a FROM seq_1_to_30;
36+
SELECT COUNT(*) AS rows_in_table, COUNT(DISTINCT a) AS distinct_values FROM t2;
37+
rows_in_table distinct_values
38+
30 30
39+
SELECT GROUP_CONCAT(DISTINCT a) INTO @gc FROM t2;
40+
SELECT GROUP_CONCAT(DISTINCT a ORDER BY a) INTO @gc_order FROM t2;
41+
SET @@tmp_memory_table_size=1000, @@max_heap_table_size=1000;
42+
Warnings:
43+
Warning 1292 Truncated incorrect tmp_memory_table_size value: '1000'
44+
Warning 1292 Truncated incorrect max_heap_table_size value: '1000'
45+
SELECT GROUP_CONCAT(DISTINCT a) = @gc AS gc_unchanged FROM t2;
46+
gc_unchanged
47+
1
48+
SELECT GROUP_CONCAT(DISTINCT a ORDER BY a) = @gc_order AS gc_order_unchanged FROM t2;
49+
gc_order_unchanged
50+
1
51+
SET @@tmp_memory_table_size=DEFAULT, @@max_heap_table_size=DEFAULT;
52+
DROP TABLE t2;
53+
#
54+
# ORDER BY does not order the rows that tie on the ordering
55+
# expression. Which of them comes first is not specified, but it
56+
# must not depend on the memory available or on the order the rows
57+
# are read in.
58+
#
59+
CREATE TABLE t3 (a BIT(2), b VARCHAR(10), c BIT);
60+
INSERT INTO t3 VALUES (1, 'a', 0), (0, 'b', 1), (0, 'c', 0), (3, 'd', 1),
61+
(1, 'e', 1), (3, 'f', 1), (0, 'g', 1);
62+
SELECT GROUP_CONCAT(DISTINCT a, c ORDER BY a) AS at_default FROM t3;
63+
at_default
64+
01,00,11,10,31
65+
SET @@tmp_memory_table_size=0;
66+
SELECT GROUP_CONCAT(DISTINCT a, c ORDER BY a) AS at_zero FROM t3;
67+
at_zero
68+
01,00,11,10,31
69+
SET @@tmp_memory_table_size=DEFAULT;
70+
DELETE FROM t3;
71+
INSERT INTO t3 VALUES (0, 'c', 0), (0, 'b', 1), (1, 'a', 0), (3, 'd', 1),
72+
(1, 'e', 1), (3, 'f', 1), (0, 'g', 1);
73+
SELECT GROUP_CONCAT(DISTINCT a, c ORDER BY a) AS reversed_scan_order FROM t3;
74+
reversed_scan_order
75+
01,00,11,10,31
76+
DROP TABLE t3;
77+
#
78+
# The sort tree can overflow too. Starving it makes repack_tree()
79+
# cut rows out of the group, which is expected, but the rows that
80+
# do come back must still be deduplicated and still be in order.
81+
#
82+
CREATE TABLE t4 (pk INT AUTO_INCREMENT PRIMARY KEY, a VARCHAR(20));
83+
INSERT INTO t4 (a) SELECT LPAD(seq, 6, '0') FROM seq_1_to_200;
84+
INSERT INTO t4 (a) SELECT a FROM t4 ORDER BY pk;
85+
SET @@tmp_memory_table_size=0, @@group_concat_max_len=4000;
86+
SELECT GROUP_CONCAT(DISTINCT a ORDER BY a) INTO @gc FROM t4;
87+
Warnings:
88+
Warning 1260 Row 32 was cut by group_concat()
89+
SELECT JSON_ARRAYAGG(DISTINCT a ORDER BY a) INTO @ja FROM t4;
90+
SET @@tmp_memory_table_size=DEFAULT, @@group_concat_max_len=DEFAULT;
91+
SELECT COUNT(*) = COUNT(DISTINCT val) AS gc_no_duplicates,
92+
GROUP_CONCAT(val ORDER BY val) = @gc AS gc_ascending
93+
FROM (SELECT SUBSTRING_INDEX(SUBSTRING_INDEX(@gc, ',', seq), ',', -1) AS val
94+
FROM seq_1_to_500
95+
WHERE seq <= 1 + LENGTH(@gc) - LENGTH(REPLACE(@gc, ',', ''))) split;
96+
gc_no_duplicates gc_ascending
97+
1 1
98+
SELECT JSON_VALID(@ja) AS ja_valid,
99+
COUNT(*) = COUNT(DISTINCT val) AS ja_no_duplicates,
100+
JSON_ARRAYAGG(val ORDER BY val) = @ja AS ja_ascending
101+
FROM (SELECT JSON_UNQUOTE(JSON_EXTRACT(@ja, CONCAT('$[', seq - 1, ']'))) AS val
102+
FROM seq_1_to_500 WHERE seq <= JSON_LENGTH(@ja)) split;
103+
ja_valid ja_no_duplicates ja_ascending
104+
1 1 1
105+
DROP TABLE t4;

‎mysql-test/main/gconcat_distinct_spill.test‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
#
2+
# GROUP_CONCAT(DISTINCT ...) and JSON_ARRAYAGG(DISTINCT ...) filter
3+
# duplicates with a Unique object, which flushes to disk when it runs out
4+
# of memory. The answer must not depend on whether that flush happened.
5+
#
6+
--source include/have_sequence.inc
7+
8+
--echo #
9+
--echo # Each block records the answer computed with memory to spare, then
10+
--echo # recomputes it with the duplicate filter starved, and compares.
11+
--echo #
12+
13+
CREATE TABLE t1 (pk INT AUTO_INCREMENT PRIMARY KEY, a VARCHAR(100) NOT NULL);
14+
INSERT INTO t1 (a) SELECT LPAD(seq, 4, '0') FROM seq_1_to_50;
15+
INSERT INTO t1 (a) SELECT a FROM t1 ORDER BY pk;
16+
SELECT COUNT(*) AS rows_in_table, COUNT(DISTINCT a) AS distinct_values FROM t1;
17+
18+
SELECT GROUP_CONCAT(DISTINCT a) INTO @gc FROM t1;
19+
SELECT GROUP_CONCAT(DISTINCT a ORDER BY a) INTO @gc_order FROM t1;
20+
SELECT JSON_ARRAYAGG(DISTINCT a) INTO @ja FROM t1;
21+
SELECT JSON_ARRAYAGG(DISTINCT a ORDER BY a) INTO @ja_order FROM t1;
22+
23+
SET @@tmp_memory_table_size=0;
24+
SELECT GROUP_CONCAT(DISTINCT a) = @gc AS gc_unchanged FROM t1;
25+
SELECT GROUP_CONCAT(DISTINCT a ORDER BY a) = @gc_order AS gc_order_unchanged FROM t1;
26+
SELECT JSON_ARRAYAGG(DISTINCT a) = @ja AS ja_unchanged FROM t1;
27+
SELECT JSON_ARRAYAGG(DISTINCT a ORDER BY a) = @ja_order AS ja_order_unchanged FROM t1;
28+
SET @@tmp_memory_table_size=DEFAULT;
29+
DROP TABLE t1;
30+
31+
--echo #
32+
--echo # Values wide enough that the filter flushes on nearly every row.
33+
--echo # Here the ORDER BY case used to return a single value out of 30.
34+
--echo #
35+
CREATE TABLE t2 (a VARCHAR(2000)) AS
36+
SELECT CONCAT(seq, REPEAT('.', 1990)) AS a FROM seq_1_to_30;
37+
SELECT COUNT(*) AS rows_in_table, COUNT(DISTINCT a) AS distinct_values FROM t2;
38+
39+
SELECT GROUP_CONCAT(DISTINCT a) INTO @gc FROM t2;
40+
SELECT GROUP_CONCAT(DISTINCT a ORDER BY a) INTO @gc_order FROM t2;
41+
42+
SET @@tmp_memory_table_size=1000, @@max_heap_table_size=1000;
43+
SELECT GROUP_CONCAT(DISTINCT a) = @gc AS gc_unchanged FROM t2;
44+
SELECT GROUP_CONCAT(DISTINCT a ORDER BY a) = @gc_order AS gc_order_unchanged FROM t2;
45+
SET @@tmp_memory_table_size=DEFAULT, @@max_heap_table_size=DEFAULT;
46+
DROP TABLE t2;
47+
48+
--echo #
49+
--echo # ORDER BY does not order the rows that tie on the ordering
50+
--echo # expression. Which of them comes first is not specified, but it
51+
--echo # must not depend on the memory available or on the order the rows
52+
--echo # are read in.
53+
--echo #
54+
CREATE TABLE t3 (a BIT(2), b VARCHAR(10), c BIT);
55+
INSERT INTO t3 VALUES (1, 'a', 0), (0, 'b', 1), (0, 'c', 0), (3, 'd', 1),
56+
(1, 'e', 1), (3, 'f', 1), (0, 'g', 1);
57+
SELECT GROUP_CONCAT(DISTINCT a, c ORDER BY a) AS at_default FROM t3;
58+
SET @@tmp_memory_table_size=0;
59+
SELECT GROUP_CONCAT(DISTINCT a, c ORDER BY a) AS at_zero FROM t3;
60+
SET @@tmp_memory_table_size=DEFAULT;
61+
62+
DELETE FROM t3;
63+
INSERT INTO t3 VALUES (0, 'c', 0), (0, 'b', 1), (1, 'a', 0), (3, 'd', 1),
64+
(1, 'e', 1), (3, 'f', 1), (0, 'g', 1);
65+
SELECT GROUP_CONCAT(DISTINCT a, c ORDER BY a) AS reversed_scan_order FROM t3;
66+
DROP TABLE t3;
67+
68+
--echo #
69+
--echo # The sort tree can overflow too. Starving it makes repack_tree()
70+
--echo # cut rows out of the group, which is expected, but the rows that
71+
--echo # do come back must still be deduplicated and still be in order.
72+
--echo #
73+
CREATE TABLE t4 (pk INT AUTO_INCREMENT PRIMARY KEY, a VARCHAR(20));
74+
INSERT INTO t4 (a) SELECT LPAD(seq, 6, '0') FROM seq_1_to_200;
75+
INSERT INTO t4 (a) SELECT a FROM t4 ORDER BY pk;
76+
77+
SET @@tmp_memory_table_size=0, @@group_concat_max_len=4000;
78+
SELECT GROUP_CONCAT(DISTINCT a ORDER BY a) INTO @gc FROM t4;
79+
SELECT JSON_ARRAYAGG(DISTINCT a ORDER BY a) INTO @ja FROM t4;
80+
SET @@tmp_memory_table_size=DEFAULT, @@group_concat_max_len=DEFAULT;
81+
82+
SELECT COUNT(*) = COUNT(DISTINCT val) AS gc_no_duplicates,
83+
GROUP_CONCAT(val ORDER BY val) = @gc AS gc_ascending
84+
FROM (SELECT SUBSTRING_INDEX(SUBSTRING_INDEX(@gc, ',', seq), ',', -1) AS val
85+
FROM seq_1_to_500
86+
WHERE seq <= 1 + LENGTH(@gc) - LENGTH(REPLACE(@gc, ',', ''))) split;
87+
88+
SELECT JSON_VALID(@ja) AS ja_valid,
89+
COUNT(*) = COUNT(DISTINCT val) AS ja_no_duplicates,
90+
JSON_ARRAYAGG(val ORDER BY val) = @ja AS ja_ascending
91+
FROM (SELECT JSON_UNQUOTE(JSON_EXTRACT(@ja, CONCAT('$[', seq - 1, ']'))) AS val
92+
FROM seq_1_to_500 WHERE seq <= JSON_LENGTH(@ja)) split;
93+
DROP TABLE t4;

‎sql/item_sum.cc‎

Lines changed: 92 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -4296,6 +4296,60 @@ bool Item_func_group_concat::repack_tree(THD *thd)
42964296
}
42974297

42984298

4299+
/*
4300+
Insert one row into the ORDER BY tree, repacking it first if it has
4301+
grown past the memory we are allowed to use.
4302+
4303+
'key' is a temporary table record in the format produced by
4304+
get_record_pointer(). table->field[0] of that record holds the length
4305+
the row adds to the result, which is what repack_tree() adds up to
4306+
decide where to cut.
4307+
4308+
@return FALSE row inserted
4309+
@return TRUE out of memory
4310+
*/
4311+
4312+
bool Item_func_group_concat::insert_to_order_tree(uchar *key)
4313+
{
4314+
DBUG_ASSERT(tree);
4315+
THD *thd= table->in_use;
4316+
/*
4317+
Repack when GCONCAT_TREE_REPACK_PARTS of the memory we may use is
4318+
gone. The rest is needed for the new tree that repack_tree()
4319+
allocates while the old one is still around.
4320+
*/
4321+
if (tree->allocated >
4322+
max_tree_size / GCONCAT_TREE_PARTS * GCONCAT_TREE_REPACK_PARTS &&
4323+
tree->elements_in_tree > 1)
4324+
if (repack_tree(thd))
4325+
return TRUE;
4326+
/* check if there was enough memory to insert the row */
4327+
return !tree_insert(tree, key, 0, tree->custom_arg);
4328+
}
4329+
4330+
4331+
/*
4332+
Insert one deduplicated row into the ORDER BY tree.
4333+
4334+
Callback for Unique::walk(). The walk merges what unique_filter spilled
4335+
to disk back with what it still holds in memory, so it is the first
4336+
point at which the whole set of distinct rows is known. Every row it
4337+
visits belongs in the result.
4338+
4339+
@return 0 row inserted
4340+
@return 1 out of memory, which stops the walk
4341+
*/
4342+
4343+
int Item_func_group_concat::dump_leaf_key_to_tree(void *key_arg,
4344+
element_count count
4345+
__attribute__((unused)),
4346+
void *item_arg)
4347+
{
4348+
auto item= static_cast<Item_func_group_concat *>(item_arg);
4349+
return item->insert_to_order_tree(static_cast<uchar *>(key_arg));
4350+
}
4351+
4352+
42994353
bool Item_func_group_concat::add(bool exclude_nulls)
43004354
{
43014355
if (always_null && exclude_nulls)
@@ -4337,7 +4391,15 @@ bool Item_func_group_concat::add(bool exclude_nulls)
43374391
null_value= FALSE;
43384392
bool row_eligible= TRUE;
43394393

4340-
if (distinct)
4394+
/*
4395+
Store how much this row adds to the result in the record itself. With
4396+
DISTINCT the row does not reach the ORDER BY tree until val_str(), and
4397+
the record kept by unique_filter is all that is left of it by then.
4398+
*/
4399+
if (tree)
4400+
table->field[0]->store(row_str_len, FALSE);
4401+
4402+
if (distinct)
43414403
{
43424404
/* Filter out duplicate rows. */
43434405
uint count= unique_filter->elements_in_tree();
@@ -4346,26 +4408,16 @@ bool Item_func_group_concat::add(bool exclude_nulls)
43464408
row_eligible= FALSE;
43474409
}
43484410

4349-
TREE_ELEMENT *el= 0; // Only for safety
4350-
if (row_eligible && tree)
4351-
{
4352-
THD *thd= table->in_use;
4353-
table->field[0]->store(row_str_len, FALSE);
4354-
/*
4355-
Repack when GCONCAT_TREE_REPACK_PARTS of the memory we may use is
4356-
gone. The rest is needed for the new tree that repack_tree()
4357-
allocates while the old one is still around.
4358-
*/
4359-
if (tree->allocated >
4360-
max_tree_size / GCONCAT_TREE_PARTS * GCONCAT_TREE_REPACK_PARTS &&
4361-
tree->elements_in_tree > 1)
4362-
if (repack_tree(thd))
4363-
return 1;
4364-
el= tree_insert(tree, get_record_pointer(), 0, tree->custom_arg);
4365-
/* check if there was enough memory to insert the row */
4366-
if (!el)
4367-
return 1;
4368-
}
4411+
/*
4412+
With DISTINCT the ORDER BY tree is not filled here. row_eligible only
4413+
reflects the part of unique_filter that is currently in memory, so as
4414+
soon as unique_filter starts flushing to disk it stops telling us
4415+
whether a row is a duplicate. val_str() fills the tree instead, from
4416+
the unique_filter walk that merges everything back together.
4417+
*/
4418+
if (!distinct && row_eligible && tree &&
4419+
insert_to_order_tree(get_record_pointer()))
4420+
return 1;
43694421

43704422
/*
43714423
In case of GROUP_CONCAT with DISTINCT or ORDER BY (or both) don't dump the
@@ -4595,7 +4647,25 @@ String* Item_func_group_concat::val_str(String* str)
45954647

45964648
if (!result_finalized) // Result yet to be written.
45974649
{
4598-
if (tree != NULL) // order by
4650+
if (tree && distinct) // distinct and order by
4651+
{
4652+
/*
4653+
Sort the distinct rows now. add() could not do it, as a row is
4654+
only known not to be a duplicate once unique_filter has merged
4655+
everything it flushed to disk, which the walk below does.
4656+
*/
4657+
if (unique_filter->walk(table, &dump_leaf_key_to_tree, this))
4658+
{
4659+
/*
4660+
Out of memory; mysys has reported it. The tree holds only part
4661+
of the group, so tell the user that the result was cut instead
4662+
of returning a short one silently.
4663+
*/
4664+
result_cut= TRUE;
4665+
}
4666+
tree_walk(tree, &dump_leaf_key, this, left_root_right);
4667+
}
4668+
else if (tree != NULL) // order by
45994669
tree_walk(tree, &dump_leaf_key, this, left_root_right);
46004670
else if (distinct) // distinct (and no order by).
46014671
unique_filter->walk(table, &dump_leaf_key, this);

‎sql/item_sum.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2199,6 +2199,9 @@ class Item_func_group_concat : public Item_sum_str
21992199
qsort_cmp2 get_comparator_function_for_order_by();
22002200
uchar* get_record_pointer();
22012201
uint get_null_bytes();
2202+
bool insert_to_order_tree(uchar *key);
2203+
static int dump_leaf_key_to_tree(void *key_arg, element_count count,
2204+
void *item_arg);
22022205

22032206
protected:
22042207
Item *shallow_copy(THD *thd) const override

0 commit comments

Comments
 (0)