Skip to content

Commit 217fe4c

Browse files
svenklemmsvenklemm
committed
Fix wrong grouping in VectorAgg and ColumnarIndexScan
VectorAgg and ColumnarIndexScan rewrite the plan and have to renumber the grouping columns to point at their new positions. The renumbering changed the list while still reading from it. A new position can reuse a number that an old position used, so an entry that was already given its new number could be mistaken for an old one and changed a second time. Two grouping columns then pointed at the same column: GROUP BY a, b became GROUP BY b, b. This returned wrong results and could crash ColumnarIndexScan. Fixes #9902
1 parent 14583bd commit 217fe4c

10 files changed

Lines changed: 154 additions & 2 deletions

File tree

‎.unreleased/fix_9902‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Fixes: #9902 Fix wrong results and crashes when grouping by columns that are not in the SELECT list with vectorized aggregation or columnar index scan

‎tsl/src/nodes/columnar_index_scan/columnar_index_scan.c‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -399,6 +399,8 @@ validate_entries_walker(Node *node, void *context)
399399
typedef struct RewriteContext
400400
{
401401
Agg *agg;
402+
/* Original grpColIdx, matched against so rewritten entries don't collide. */
403+
AttrNumber *old_grpColIdx;
402404
List *custom_scan_tlist;
403405
AttrNumber next_resno;
404406
} RewriteContext;
@@ -426,7 +428,7 @@ rewrite_agg_tlist_mutator(Node *node, void *context)
426428
/* Update grpColIdx for GROUP BY Vars */
427429
for (int k = 0; k < ctx->agg->numCols; k++)
428430
{
429-
if (ctx->agg->grpColIdx[k] == var->varattno)
431+
if (ctx->old_grpColIdx[k] == var->varattno)
430432
{
431433
ctx->agg->grpColIdx[k] = resno;
432434
}
@@ -593,8 +595,16 @@ columnar_index_scan_plan_create(Agg *agg, CustomScan *cscan, List *rtable)
593595
* Rewrite pass: walk the Agg's targetlist with a mutator that rewrites
594596
* Var and Aggref nodes to reference ColumnarIndexScan output columns.
595597
*/
598+
AttrNumber *old_grpColIdx = NULL;
599+
if (agg->numCols > 0)
600+
{
601+
old_grpColIdx = palloc(sizeof(AttrNumber) * agg->numCols);
602+
memcpy(old_grpColIdx, agg->grpColIdx, sizeof(AttrNumber) * agg->numCols);
603+
}
604+
596605
RewriteContext rewrite_ctx = {
597606
.agg = agg,
607+
.old_grpColIdx = old_grpColIdx,
598608
.custom_scan_tlist = custom_scan_tlist,
599609
.next_resno = 1,
600610
};

‎tsl/src/nodes/vector_agg/plan.c‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -557,6 +557,8 @@ mark_partial_aggref_mutator(Node *node, void *context)
557557
typedef struct MakeFinalizeAggContext
558558
{
559559
Agg *agg;
560+
/* Original grpColIdx, matched against so rewritten entries don't collide. */
561+
AttrNumber *old_grpColIdx;
560562
List *vector_agg_targetlist;
561563
} MakeFinalizeAggContext;
562564

@@ -581,7 +583,7 @@ make_finalize_agg_mutator(Node *node, void *context)
581583
var->varattno = tle->resno;
582584
for (int k = 0; k < ctx->agg->numCols; k++)
583585
{
584-
if (ctx->agg->grpColIdx[k] == old_attno)
586+
if (ctx->old_grpColIdx[k] == old_attno)
585587
{
586588
ctx->agg->grpColIdx[k] = tle->resno;
587589
}
@@ -813,8 +815,16 @@ insert_vector_agg(Plan *plan, void *context)
813815
agg->aggsplit = AGGSPLIT_FINAL_DESERIAL;
814816
agg->plan.lefttree = vector_agg_plan;
815817

818+
AttrNumber *old_grpColIdx = NULL;
819+
if (agg->numCols > 0)
820+
{
821+
old_grpColIdx = palloc(sizeof(AttrNumber) * agg->numCols);
822+
memcpy(old_grpColIdx, agg->grpColIdx, sizeof(AttrNumber) * agg->numCols);
823+
}
824+
816825
MakeFinalizeAggContext finalize_ctx = {
817826
.agg = agg,
827+
.old_grpColIdx = old_grpColIdx,
818828
.vector_agg_targetlist = vector_agg->scan.plan.targetlist,
819829
};
820830
agg->plan.targetlist = (List *) expression_tree_mutator((Node *) agg->plan.targetlist,

‎tsl/test/expected/columnar_index_scan-15.out‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3398,3 +3398,19 @@ NOTICE: using column "ts" as partitioning column
33983398
1
33993399
1
34003400

3401+
count
3402+
-------
3403+
6
3404+
3405+
count
3406+
-------
3407+
6
3408+
3409+
count
3410+
-------
3411+
6
3412+
3413+
count
3414+
-------
3415+
6
3416+

‎tsl/test/expected/columnar_index_scan-16.out‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3393,3 +3393,19 @@ NOTICE: using column "ts" as partitioning column
33933393
1
33943394
1
33953395

3396+
count
3397+
-------
3398+
6
3399+
3400+
count
3401+
-------
3402+
6
3403+
3404+
count
3405+
-------
3406+
6
3407+
3408+
count
3409+
-------
3410+
6
3411+

‎tsl/test/expected/columnar_index_scan-17.out‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3555,3 +3555,19 @@ NOTICE: using column "ts" as partitioning column
35553555
1
35563556
1
35573557

3558+
count
3559+
-------
3560+
6
3561+
3562+
count
3563+
-------
3564+
6
3565+
3566+
count
3567+
-------
3568+
6
3569+
3570+
count
3571+
-------
3572+
6
3573+

‎tsl/test/expected/columnar_index_scan-18.out‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3561,3 +3561,19 @@ NOTICE: using column "ts" as partitioning column
35613561
1
35623562
1
35633563

3564+
count
3565+
-------
3566+
6
3567+
3568+
count
3569+
-------
3570+
6
3571+
3572+
count
3573+
-------
3574+
6
3575+
3576+
count
3577+
-------
3578+
6
3579+

‎tsl/test/expected/vector_agg_grouping.out‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2005,3 +2005,40 @@ select sum(t), a, b from groupnull group by b, a order by 1;
20052005

20062006
reset timescaledb.debug_require_vector_agg;
20072007
reset timescaledb.enable_vectorized_aggregation;
2008+
-- Grouping columns not in the output targetlist must map to the right output
2009+
-- positions, even when one column's input position equals another's output.
2010+
create table groupnotinto(t int, a int, b int);
2011+
select create_hypertable('groupnotinto', 't', chunk_time_interval => 1000000);
2012+
create_hypertable
2013+
----------------------------
2014+
(11,public,groupnotinto,t)
2015+
2016+
insert into groupnotinto select g, g % 100, g % 5 from generate_series(1, 1000) g;
2017+
alter table groupnotinto set (timescaledb.compress, timescaledb.compress_segmentby = 'b');
2018+
select count(compress_chunk(x)) from show_chunks('groupnotinto') x;
2019+
count
2020+
-------
2021+
1
2022+
2023+
set timescaledb.debug_require_vector_agg = 'require';
2024+
-- Uncomment to generate reference.
2025+
--set timescaledb.enable_vectorized_aggregation to off; set timescaledb.debug_require_vector_agg = 'allow';
2026+
-- Expecting 100 distinct (a, b) groups.
2027+
select count(*) from (select count(*) from groupnotinto group by a, b) g;
2028+
count
2029+
-------
2030+
100
2031+
2032+
select count(*) from (select count(*) from groupnotinto group by b, a) g;
2033+
count
2034+
-------
2035+
100
2036+
2037+
-- A grouping column also used in the output is handled the same way.
2038+
select count(*) from (select b, count(*) from groupnotinto group by a, b) g;
2039+
count
2040+
-------
2041+
100
2042+
2043+
reset timescaledb.debug_require_vector_agg;
2044+
reset timescaledb.enable_vectorized_aggregation;

‎tsl/test/sql/columnar_index_scan.sql.in‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,3 +146,11 @@ SET timescaledb.enable_columnarindexscan = on;
146146
EXPLAIN (costs off) SELECT count(*) FROM :CAST_CHUNK GROUP BY (device_id)::text;
147147
SELECT count(*) FROM :CAST_CHUNK GROUP BY (device_id)::text ORDER BY 1;
148148

149+
-- Multiple GROUP BY columns not in SELECT must map to the right output columns.
150+
SET timescaledb.enable_columnarindexscan = off;
151+
SELECT count(*) FROM (SELECT count(*) FROM metrics GROUP BY device, sensor) g;
152+
SELECT count(*) FROM (SELECT count(*) FROM metrics GROUP BY sensor, device) g;
153+
SET timescaledb.enable_columnarindexscan = on;
154+
SELECT count(*) FROM (SELECT count(*) FROM metrics GROUP BY device, sensor) g;
155+
SELECT count(*) FROM (SELECT count(*) FROM metrics GROUP BY sensor, device) g;
156+

‎tsl/test/sql/vector_agg_grouping.sql‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -214,3 +214,25 @@ select sum(t), a, b from groupnull group by b, a order by 1;
214214
reset timescaledb.debug_require_vector_agg;
215215
reset timescaledb.enable_vectorized_aggregation;
216216

217+
218+
-- Grouping columns not in the output targetlist must map to the right output
219+
-- positions, even when one column's input position equals another's output.
220+
create table groupnotinto(t int, a int, b int);
221+
select create_hypertable('groupnotinto', 't', chunk_time_interval => 1000000);
222+
insert into groupnotinto select g, g % 100, g % 5 from generate_series(1, 1000) g;
223+
alter table groupnotinto set (timescaledb.compress, timescaledb.compress_segmentby = 'b');
224+
select count(compress_chunk(x)) from show_chunks('groupnotinto') x;
225+
226+
set timescaledb.debug_require_vector_agg = 'require';
227+
-- Uncomment to generate reference.
228+
--set timescaledb.enable_vectorized_aggregation to off; set timescaledb.debug_require_vector_agg = 'allow';
229+
230+
-- Expecting 100 distinct (a, b) groups.
231+
select count(*) from (select count(*) from groupnotinto group by a, b) g;
232+
select count(*) from (select count(*) from groupnotinto group by b, a) g;
233+
-- A grouping column also used in the output is handled the same way.
234+
select count(*) from (select b, count(*) from groupnotinto group by a, b) g;
235+
236+
reset timescaledb.debug_require_vector_agg;
237+
reset timescaledb.enable_vectorized_aggregation;
238+

0 commit comments

Comments
 (0)