Skip to content

Commit 2657283

Browse files
anarazelanarazel
committed
Minimally fix partial aggregation for aggregates that don't have one argument.
For partial aggregation combine steps, AggStatePerTrans->numTransInputs was set to the transition function's number of inputs, rather than the combine function's number of inputs (always 1). That lead to partial aggregates with strict combine functions to wrongly check for NOT NULL input as required by strictness. When the aggregate wasn't exactly passed one argument, the strictness check was either omitted (in the 0 args case) or too many arguments were checked. In the latter case we'd read beyond the end of FunctionCallInfoData->args (only in master). AggStatePerTrans->numTransInputs actually has been wrong since since 9.6, where partial aggregates were added. But it turns out to not be an active problem in 9.6 and 10, because numTransInputs wasn't used at all for combine functions: Before c253b72 there simply was no NULL check for the input to strict trans functions, and after that the check was simply hardcoded for the right offset in fcinfo, as it's done by code specific to combine functions. In bf6c614 (11) the strictness check was generalized, with common code doing the strictness checks for both plain and combine transition functions, based on numTransInputs. For combine functions this lead to not emitting an expression step to check for strict input in the 0 arguments case, and in the > 1 arguments case, we'd check too many arguments.Due to the fact that the relevant fcinfo->isnull[2..] was always zero-initialized (more or less by accident, by being part of the AggStatePerTrans struct, which is palloc0'ed), there was no observable damage in the latter case before a9c35cf, we just checked too many array elements. Due to the changes in a9c35cf, > 1 argument bug became visible, because these days fcinfo is a) dynamically allocated without being zeroed b) exactly the length required for the number of specified arguments (hardcoded to 2 in this case). This commit only contains a fairly minimal fix, setting numTransInputs to a hardcoded 1 when building a pertrans for a combine function. It seems likely that we'll want to clean this up further (e.g. the arguments build_pertrans_for_aggref() aren't particularly meaningful for combine functions). But the wrap date for 12 beta1 is coming up fast, so it seems good to have a minimal fix in place. Backpatch to 11. While AggStatePerTrans->numTransInputs was set wrongly before that, the value was not used for combine functions. Reported-By: Rajkumar Raghuwanshi Diagnosed-By: Kyotaro Horiguchi, Jeevan Chalke, Andres Freund, David Rowley Author: David Rowley, Kyotaro Horiguchi, Andres Freund Discussion: https://postgr.es/m/CAKcux6=uZEyWyLw0N7HtR9OBc-sWEFeByEZC7t-KDf15FKxVew@mail.gmail.com
1 parent 03310db commit 2657283

File tree

3 files changed

+43
-23
lines changed

3 files changed

+43
-23
lines changed

‎src/backend/executor/nodeAgg.c

Lines changed: 24 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2911,12 +2911,6 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
29112911

29122912
pertrans->aggtranstype = aggtranstype;
29132913

2914-
/* Detect how many arguments to pass to the transfn */
2915-
if (AGGKIND_IS_ORDERED_SET(aggref->aggkind))
2916-
pertrans->numTransInputs = numInputs;
2917-
else
2918-
pertrans->numTransInputs = numArguments;
2919-
29202914
/*
29212915
* When combining states, we have no use at all for the aggregate
29222916
* function's transfn. Instead we use the combinefn. In this case, the
@@ -2926,6 +2920,17 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
29262920
if (DO_AGGSPLIT_COMBINE(aggstate->aggsplit))
29272921
{
29282922
Expr *combinefnexpr;
2923+
size_t numTransArgs;
2924+
2925+
/*
2926+
* When combining there's only one input, the to-be-combined added
2927+
* transition value from below (this node's transition value is
2928+
* counted separately).
2929+
*/
2930+
pertrans->numTransInputs = 1;
2931+
2932+
/* account for the current transition state */
2933+
numTransArgs = pertrans->numTransInputs + 1;
29292934

29302935
build_aggregate_combinefn_expr(aggtranstype,
29312936
aggref->inputcollid,
@@ -2938,7 +2943,7 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
29382943
(FunctionCallInfo) palloc(SizeForFunctionCallInfo(2));
29392944
InitFunctionCallInfoData(*pertrans->transfn_fcinfo,
29402945
&pertrans->transfn,
2941-
2,
2946+
numTransArgs,
29422947
pertrans->aggCollation,
29432948
(void *) aggstate, NULL);
29442949

@@ -2956,7 +2961,16 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
29562961
else
29572962
{
29582963
Expr *transfnexpr;
2959-
size_t numInputs = pertrans->numTransInputs + 1;
2964+
size_t numTransArgs;
2965+
2966+
/* Detect how many arguments to pass to the transfn */
2967+
if (AGGKIND_IS_ORDERED_SET(aggref->aggkind))
2968+
pertrans->numTransInputs = numInputs;
2969+
else
2970+
pertrans->numTransInputs = numArguments;
2971+
2972+
/* account for the current transition state */
2973+
numTransArgs = pertrans->numTransInputs + 1;
29602974

29612975
/*
29622976
* Set up infrastructure for calling the transfn. Note that invtrans
@@ -2976,10 +2990,10 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
29762990
fmgr_info_set_expr((Node *) transfnexpr, &pertrans->transfn);
29772991

29782992
pertrans->transfn_fcinfo =
2979-
(FunctionCallInfo) palloc(SizeForFunctionCallInfo(numInputs));
2993+
(FunctionCallInfo) palloc(SizeForFunctionCallInfo(numTransArgs));
29802994
InitFunctionCallInfoData(*pertrans->transfn_fcinfo,
29812995
&pertrans->transfn,
2982-
numInputs,
2996+
numTransArgs,
29832997
pertrans->aggCollation,
29842998
(void *) aggstate, NULL);
29852999

‎src/test/regress/expected/aggregates.out

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2204,21 +2204,26 @@ SET max_parallel_workers_per_gather = 4;
22042204
SET enable_indexonlyscan = off;
22052205
-- variance(int4) covers numeric_poly_combine
22062206
-- sum(int8) covers int8_avg_combine
2207-
EXPLAIN (COSTS OFF)
2208-
SELECT variance(unique1::int4), sum(unique1::int8) FROM tenk1;
2209-
QUERY PLAN
2210-
----------------------------------------------
2207+
-- regr_count(float8, float8) covers int8inc_float8_float8 and aggregates with > 1 arg
2208+
EXPLAIN (COSTS OFF, VERBOSE)
2209+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8) FROM tenk1;
2210+
QUERY PLAN
2211+
-------------------------------------------------------------------------------------------------------------------------------------------------------------------
22112212
Finalize Aggregate
2213+
Output: variance(unique1), sum((unique1)::bigint), regr_count((unique1)::double precision, (unique1)::double precision)
22122214
-> Gather
2215+
Output: (PARTIAL variance(unique1)), (PARTIAL sum((unique1)::bigint)), (PARTIAL regr_count((unique1)::double precision, (unique1)::double precision))
22132216
Workers Planned: 4
22142217
-> Partial Aggregate
2215-
-> Parallel Seq Scan on tenk1
2216-
(5 rows)
2218+
Output: PARTIAL variance(unique1), PARTIAL sum((unique1)::bigint), PARTIAL regr_count((unique1)::double precision, (unique1)::double precision)
2219+
-> Parallel Seq Scan on public.tenk1
2220+
Output: unique1, unique2, two, four, ten, twenty, hundred, thousand, twothousand, fivethous, tenthous, odd, even, stringu1, stringu2, string4
2221+
(9 rows)
22172222

2218-
SELECT variance(unique1::int4), sum(unique1::int8) FROM tenk1;
2219-
variance | sum
2220-
----------------------+----------
2221-
8334166.666666666667 | 49995000
2223+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8) FROM tenk1;
2224+
variance | sum | regr_count
2225+
----------------------+----------+------------
2226+
8334166.666666666667 | 49995000 | 10000
22222227
(1 row)
22232228

22242229
ROLLBACK;

‎src/test/regress/sql/aggregates.sql

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -963,10 +963,11 @@ SET enable_indexonlyscan = off;
963963

964964
-- variance(int4) covers numeric_poly_combine
965965
-- sum(int8) covers int8_avg_combine
966-
EXPLAIN (COSTS OFF)
967-
SELECT variance(unique1::int4), sum(unique1::int8) FROM tenk1;
966+
-- regr_count(float8, float8) covers int8inc_float8_float8 and aggregates with > 1 arg
967+
EXPLAIN (COSTS OFF, VERBOSE)
968+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8) FROM tenk1;
968969

969-
SELECT variance(unique1::int4), sum(unique1::int8) FROM tenk1;
970+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8) FROM tenk1;
970971

971972
ROLLBACK;
972973

0 commit comments

Comments
 (0)