Skip to content

Commit 9fea0b0

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 c253b722f6 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 0950d25 commit 9fea0b0

File tree

3 files changed

+42
-21
lines changed

3 files changed

+42
-21
lines changed

‎src/backend/executor/nodeAgg.c

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2907,12 +2907,6 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
29072907

29082908
pertrans->aggtranstype = aggtranstype;
29092909

2910-
/* Detect how many arguments to pass to the transfn */
2911-
if (AGGKIND_IS_ORDERED_SET(aggref->aggkind))
2912-
pertrans->numTransInputs = numInputs;
2913-
else
2914-
pertrans->numTransInputs = numArguments;
2915-
29162910
/*
29172911
* When combining states, we have no use at all for the aggregate
29182912
* function's transfn. Instead we use the combinefn. In this case, the
@@ -2922,6 +2916,17 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
29222916
if (DO_AGGSPLIT_COMBINE(aggstate->aggsplit))
29232917
{
29242918
Expr *combinefnexpr;
2919+
size_t numTransArgs;
2920+
2921+
/*
2922+
* When combining there's only one input, the to-be-combined added
2923+
* transition value from below (this node's transition value is
2924+
* counted separately).
2925+
*/
2926+
pertrans->numTransInputs = 1;
2927+
2928+
/* account for the current transition state */
2929+
numTransArgs = pertrans->numTransInputs + 1;
29252930

29262931
build_aggregate_combinefn_expr(aggtranstype,
29272932
aggref->inputcollid,
@@ -2932,7 +2937,7 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
29322937

29332938
InitFunctionCallInfoData(pertrans->transfn_fcinfo,
29342939
&pertrans->transfn,
2935-
2,
2940+
numTransArgs,
29362941
pertrans->aggCollation,
29372942
(void *) aggstate, NULL);
29382943

@@ -2950,6 +2955,16 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
29502955
else
29512956
{
29522957
Expr *transfnexpr;
2958+
size_t numTransArgs;
2959+
2960+
/* Detect how many arguments to pass to the transfn */
2961+
if (AGGKIND_IS_ORDERED_SET(aggref->aggkind))
2962+
pertrans->numTransInputs = numInputs;
2963+
else
2964+
pertrans->numTransInputs = numArguments;
2965+
2966+
/* account for the current transition state */
2967+
numTransArgs = pertrans->numTransInputs + 1;
29532968

29542969
/*
29552970
* Set up infrastructure for calling the transfn. Note that invtrans
@@ -2970,7 +2985,7 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
29702985

29712986
InitFunctionCallInfoData(pertrans->transfn_fcinfo,
29722987
&pertrans->transfn,
2973-
pertrans->numTransInputs + 1,
2988+
numTransArgs,
29742989
pertrans->aggCollation,
29752990
(void *) aggstate, NULL);
29762991

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

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2074,21 +2074,26 @@ SET max_parallel_workers_per_gather = 4;
20742074
SET enable_indexonlyscan = off;
20752075
-- variance(int4) covers numeric_poly_combine
20762076
-- sum(int8) covers int8_avg_combine
2077-
EXPLAIN (COSTS OFF)
2078-
SELECT variance(unique1::int4), sum(unique1::int8) FROM tenk1;
2079-
QUERY PLAN
2080-
----------------------------------------------
2077+
-- regr_count(float8, float8) covers int8inc_float8_float8 and aggregates with > 1 arg
2078+
EXPLAIN (COSTS OFF, VERBOSE)
2079+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8) FROM tenk1;
2080+
QUERY PLAN
2081+
-------------------------------------------------------------------------------------------------------------------------------------------------------------------
20812082
Finalize Aggregate
2083+
Output: variance(unique1), sum((unique1)::bigint), regr_count((unique1)::double precision, (unique1)::double precision)
20822084
-> Gather
2085+
Output: (PARTIAL variance(unique1)), (PARTIAL sum((unique1)::bigint)), (PARTIAL regr_count((unique1)::double precision, (unique1)::double precision))
20832086
Workers Planned: 4
20842087
-> Partial Aggregate
2085-
-> Parallel Seq Scan on tenk1
2086-
(5 rows)
2088+
Output: PARTIAL variance(unique1), PARTIAL sum((unique1)::bigint), PARTIAL regr_count((unique1)::double precision, (unique1)::double precision)
2089+
-> Parallel Seq Scan on public.tenk1
2090+
Output: unique1, unique2, two, four, ten, twenty, hundred, thousand, twothousand, fivethous, tenthous, odd, even, stringu1, stringu2, string4
2091+
(9 rows)
20872092

2088-
SELECT variance(unique1::int4), sum(unique1::int8) FROM tenk1;
2089-
variance | sum
2090-
----------------------+----------
2091-
8334166.666666666667 | 49995000
2093+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8) FROM tenk1;
2094+
variance | sum | regr_count
2095+
----------------------+----------+------------
2096+
8334166.666666666667 | 49995000 | 10000
20922097
(1 row)
20932098

20942099
ROLLBACK;

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

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

920920
-- variance(int4) covers numeric_poly_combine
921921
-- sum(int8) covers int8_avg_combine
922-
EXPLAIN (COSTS OFF)
923-
SELECT variance(unique1::int4), sum(unique1::int8) FROM tenk1;
922+
-- regr_count(float8, float8) covers int8inc_float8_float8 and aggregates with > 1 arg
923+
EXPLAIN (COSTS OFF, VERBOSE)
924+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8) FROM tenk1;
924925

925-
SELECT variance(unique1::int4), sum(unique1::int8) FROM tenk1;
926+
SELECT variance(unique1::int4), sum(unique1::int8), regr_count(unique1::float8, unique1::float8) FROM tenk1;
926927

927928
ROLLBACK;
928929

0 commit comments

Comments
 (0)