Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit2657283

Browse files
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'snumber of inputs, rather than the combine function's number ofinputs (always 1).That lead to partial aggregates with strict combine functions towrongly check for NOT NULL input as required by strictness. When theaggregate wasn't exactly passed one argument, the strictness check waseither omitted (in the 0 args case) or too many arguments werechecked. In the latter case we'd read beyond the end ofFunctionCallInfoData->args (only in master).AggStatePerTrans->numTransInputs actually has been wrong since since9.6, where partial aggregates were added. But it turns out to not bean active problem in 9.6 and 10, because numTransInputs wasn't used atall for combine functions: Beforec253b72 there simply was no NULLcheck for the input to strict trans functions, and after that thecheck was simply hardcoded for the right offset in fcinfo, as it'sdone by code specific to combine functions.Inbf6c614 (11) the strictness check was generalized, with commoncode doing the strictness checks for both plain and combine transitionfunctions, based on numTransInputs. For combine functions this lead tonot emitting an expression step to check for strict input in the 0arguments case, and in the > 1 arguments case, we'd check too manyarguments.Due to the fact that the relevant fcinfo->isnull[2..] wasalways zero-initialized (more or less by accident, by being part ofthe AggStatePerTrans struct, which is palloc0'ed), there was noobservable damage in the latter case beforea9c35cf, we justchecked too many array elements.Due to the changes ina9c35cf, > 1 argument bug became visible,because these days fcinfo is a) dynamically allocated without beingzeroed b) exactly the length required for the number of specifiedarguments (hardcoded to 2 in this case).This commit only contains a fairly minimal fix, setting numTransInputsto a hardcoded 1 when building a pertrans for a combine function. Itseems likely that we'll want to clean this up further (e.g. thearguments build_pertrans_for_aggref() aren't particularly meaningfulfor combine functions). But the wrap date for 12 beta1 is coming upfast, so it seems good to have a minimal fix in place.Backpatch to 11. While AggStatePerTrans->numTransInputs was setwrongly before that, the value was not used for combine functions.Reported-By: Rajkumar RaghuwanshiDiagnosed-By: Kyotaro Horiguchi, Jeevan Chalke, Andres Freund, David RowleyAuthor: David Rowley, Kyotaro Horiguchi, Andres FreundDiscussion:https://postgr.es/m/CAKcux6=uZEyWyLw0N7HtR9OBc-sWEFeByEZC7t-KDf15FKxVew@mail.gmail.com
1 parent03310db commit2657283

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_tnumTransArgs;
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_tnumInputs=pertrans->numTransInputs+1;
2964+
size_tnumTransArgs;
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)
[8]ページ先頭
©2009-2025 Movatter.jp