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

Commit9fea0b0

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 parent0950d25 commit9fea0b0

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_tnumTransArgs;
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_tnumTransArgs;
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)
[8]ページ先頭
©2009-2025 Movatter.jp