Skip to content

Commit d90f288

Browse files
committed
Fix crash in VectorAgg plan code
Due to a refactor in commit 947f7f4 that silenced a coverity warning, a crash was introduced for certain aggregation query plans. However, no such plan was covered in our tests, so the crash was never triggered. Add a fix and a test case that triggers crash when the fix is not present.
1 parent daf7860 commit d90f288

File tree

9 files changed

+87
-6
lines changed

9 files changed

+87
-6
lines changed

.unreleased/pr_7902

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Fixes: #7902 Fix crash in VectorAgg plan code

tsl/src/nodes/columnar_scan/columnar_scan.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1038,6 +1038,7 @@ columnar_scan_plan_create(PlannerInfo *root, RelOptInfo *rel, CustomPath *best_p
10381038
VectorQualInfoHypercore vqih = {
10391039
.vqinfo = {
10401040
.rti = rel->relid,
1041+
.maxattno = hcinfo->num_columns,
10411042
.vector_attrs = columnar_scan_build_vector_attrs(hcinfo->columns, hcinfo->num_columns),
10421043
},
10431044
.hcinfo = hcinfo,

tsl/src/nodes/decompress_chunk/planner.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -884,6 +884,7 @@ find_vectorized_quals(DecompressionMapContext *context, DecompressChunkPath *pat
884884
List **vectorized, List **nonvectorized)
885885
{
886886
VectorQualInfo vqi = {
887+
.maxattno = path->info->chunk_rel->max_attr,
887888
.vector_attrs = build_vector_attrs_array(context->uncompressed_attno_info, path->info),
888889
.rti = path->info->chunk_rel->relid,
889890
};

tsl/src/nodes/decompress_chunk/vector_quals.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,18 @@ typedef struct VectorQualInfo
2525
Index rti;
2626

2727
bool reverse;
28+
2829
/*
2930
* Arrays indexed by uncompressed attno indicating whether an
3031
* attribute/column is a vectorizable type and/or a segmentby attribute.
32+
*
33+
* Note: array lengths are maxattno + 1.
3134
*/
3235
bool *vector_attrs;
3336
bool *segmentby_attrs;
37+
38+
/* Max attribute number found in arrays above */
39+
AttrNumber maxattno;
3440
} VectorQualInfo;
3541

3642
/*

tsl/src/nodes/vector_agg/plan.c

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -221,7 +221,9 @@ is_vector_var(const VectorQualInfo *vqinfo, Expr *expr)
221221
return false;
222222
}
223223

224-
return vqinfo->vector_attrs[var->varattno];
224+
Assert(var->varattno <= vqinfo->maxattno);
225+
226+
return vqinfo->vector_attrs && vqinfo->vector_attrs[var->varattno];
225227
}
226228

227229
/*
@@ -497,14 +499,26 @@ vectoragg_plan_possible(Plan *childplan, const List *rtable, VectorQualInfo *vqi
497499
}
498500

499501
CustomScan *customscan = castNode(CustomScan, childplan);
500-
RangeTblEntry *rte = rt_fetch(customscan->scan.scanrelid, rtable);
501502

502-
if (ts_is_hypercore_am(ts_get_rel_am(rte->relid)))
503-
vectoragg_plan_tam(childplan, rtable, vqi);
504-
else if (strcmp(customscan->methods->CustomName, "DecompressChunk") == 0)
503+
if (strcmp(customscan->methods->CustomName, "DecompressChunk") == 0)
504+
{
505505
vectoragg_plan_decompress_chunk(childplan, vqi);
506+
return true;
507+
}
508+
509+
/* We're looking for a baserel scan */
510+
if (customscan->scan.scanrelid > 0)
511+
{
512+
RangeTblEntry *rte = rt_fetch(customscan->scan.scanrelid, rtable);
506513

507-
return true;
514+
if (rte && ts_is_hypercore_am(ts_get_rel_am(rte->relid)))
515+
{
516+
vectoragg_plan_tam(childplan, rtable, vqi);
517+
return true;
518+
}
519+
}
520+
521+
return false;
508522
}
509523

510524
/*
@@ -597,6 +611,7 @@ try_insert_vector_agg_node(Plan *plan, List *rtable)
597611

598612
Plan *childplan = agg->plan.lefttree;
599613
VectorQualInfo vqi;
614+
MemSet(&vqi, 0, sizeof(VectorQualInfo));
600615

601616
/*
602617
* Build supplementary info to determine whether we can vectorize the

tsl/src/nodes/vector_agg/plan_decompress_chunk.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,7 @@ vectoragg_plan_decompress_chunk(Plan *childplan, VectorQualInfo *vqi)
111111
}
112112
}
113113

114+
vqi->maxattno = maxattno;
114115
vqi->vector_attrs = (bool *) palloc0(sizeof(bool) * (maxattno + 1));
115116
vqi->segmentby_attrs = (bool *) palloc0(sizeof(bool) * (maxattno + 1));
116117

tsl/src/nodes/vector_agg/plan_tam.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ vectoragg_plan_tam(Plan *childplan, const List *rtable, VectorQualInfo *vqi)
2222

2323
*vqi = (VectorQualInfo){
2424
.rti = customscan->scan.scanrelid,
25+
.maxattno = hinfo->num_columns,
2526
.vector_attrs = (bool *) palloc0(sizeof(bool) * (hinfo->num_columns + 1)),
2627
.segmentby_attrs = (bool *) palloc0(sizeof(bool) * (hinfo->num_columns + 1)),
2728
/*

tsl/test/expected/vectorized_aggregation.out

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3602,3 +3602,42 @@ SELECT sum(float_value), int_value FROM testtable2 WHERE int_value = 1 GROUP BY
36023602
3162 | 1
36033603
(1 row)
36043604

3605+
--
3606+
-- Test handling of Agg on top of ChunkAppend. This reproduces a crash
3607+
-- in the vector agg planning code introduced in commit 947f7f400.
3608+
--
3609+
CREATE TABLE testtable3 (time timestamptz, location_id int, device_id int, sensor_id int);
3610+
SELECT create_hypertable('testtable3', 'time');
3611+
NOTICE: adding not-null constraint to column "time"
3612+
create_hypertable
3613+
-------------------------
3614+
(5,public,testtable3,t)
3615+
(1 row)
3616+
3617+
ALTER TABLE testtable3 SET (timescaledb.compress_orderby='time', timescaledb.compress_segmentby='location_id');
3618+
INSERT INTO testtable3 SELECT t, ceil(random() * 20)::int, ceil(random() * 30)::int, ceil(random() * 20)::int FROM generate_series('2024-01-01'::timestamptz, '2024-01-10'::timestamptz, '1h') AS t;
3619+
SELECT count(compress_chunk(ch)) FROM show_chunks('testtable3') ch;
3620+
count
3621+
-------
3622+
2
3623+
(1 row)
3624+
3625+
EXPLAIN (costs off) SELECT (date_trunc('hour', '2024-01-09'::timestamptz) - interval '1 hour')::timestamp as time, TT.location_id as location_id, TT.device_id as device_id, 0 as sensor_id, date_trunc('day', current_timestamp) as discovered_date FROM testtable3 TT WHERE time >= date_trunc('hour', '2024-01-09'::timestamptz) - interval '1 hour' GROUP BY TT.location_id, TT.device_id;
3626+
QUERY PLAN
3627+
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
3628+
Group
3629+
Group Key: tt.location_id, tt.device_id
3630+
-> Gather Merge
3631+
Workers Planned: 2
3632+
-> Sort
3633+
Sort Key: tt.location_id, tt.device_id
3634+
-> Partial HashAggregate
3635+
Group Key: tt.location_id, tt.device_id
3636+
-> Parallel Custom Scan (ChunkAppend) on testtable3 tt
3637+
Chunks excluded during startup: 1
3638+
-> Custom Scan (DecompressChunk) on _hyper_5_120_chunk tt_1
3639+
Vectorized Filter: ("time" >= (date_trunc('hour'::text, 'Tue Jan 09 00:00:00 2024 PST'::timestamp with time zone) - '@ 1 hour'::interval))
3640+
-> Parallel Seq Scan on compress_hyper_6_122_chunk
3641+
(13 rows)
3642+
3643+
SELECT (date_trunc('hour', '2024-01-09'::timestamptz) - interval '1 hour')::timestamp as time, TT.location_id as location_id, TT.device_id as device_id, 0 as sensor_id, date_trunc('day', current_timestamp) as discovered_date FROM testtable3 TT WHERE time >= date_trunc('hour', '2024-01-09'::timestamptz) - interval '1 hour' GROUP BY TT.location_id, TT.device_id \g :TEST_OUTPUT_DIR/vectorized_aggregation_query_result.out

tsl/test/sql/vectorized_aggregation.sql

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -421,3 +421,19 @@ SELECT sum(float_value) FROM testtable2 GROUP BY tableoid ORDER BY 1 LIMIT 1;
421421
-- Postgres versions starting with 16 remove the grouping columns that are
422422
-- equated to a constant. Check that our planning code handles this well.
423423
SELECT sum(float_value), int_value FROM testtable2 WHERE int_value = 1 GROUP BY int_value;
424+
425+
--
426+
-- Test handling of Agg on top of ChunkAppend. This reproduces a crash
427+
-- in the vector agg planning code introduced in commit 947f7f400.
428+
--
429+
CREATE TABLE testtable3 (time timestamptz, location_id int, device_id int, sensor_id int);
430+
SELECT create_hypertable('testtable3', 'time');
431+
ALTER TABLE testtable3 SET (timescaledb.compress_orderby='time', timescaledb.compress_segmentby='location_id');
432+
433+
INSERT INTO testtable3 SELECT t, ceil(random() * 20)::int, ceil(random() * 30)::int, ceil(random() * 20)::int FROM generate_series('2024-01-01'::timestamptz, '2024-01-10'::timestamptz, '1h') AS t;
434+
435+
SELECT count(compress_chunk(ch)) FROM show_chunks('testtable3') ch;
436+
437+
EXPLAIN (costs off) SELECT (date_trunc('hour', '2024-01-09'::timestamptz) - interval '1 hour')::timestamp as time, TT.location_id as location_id, TT.device_id as device_id, 0 as sensor_id, date_trunc('day', current_timestamp) as discovered_date FROM testtable3 TT WHERE time >= date_trunc('hour', '2024-01-09'::timestamptz) - interval '1 hour' GROUP BY TT.location_id, TT.device_id;
438+
439+
SELECT (date_trunc('hour', '2024-01-09'::timestamptz) - interval '1 hour')::timestamp as time, TT.location_id as location_id, TT.device_id as device_id, 0 as sensor_id, date_trunc('day', current_timestamp) as discovered_date FROM testtable3 TT WHERE time >= date_trunc('hour', '2024-01-09'::timestamptz) - interval '1 hour' GROUP BY TT.location_id, TT.device_id \g :TEST_OUTPUT_DIR/vectorized_aggregation_query_result.out

0 commit comments

Comments
 (0)