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

Commit305cf1f

Browse files
committed
Fix AggGetAggref() so it won't lie to aggregate final functions.
If we merge the transition calculations for two different aggregates,it's reasonable to assume that the transition function should not carewhich of those Aggref structs it gets from AggGetAggref(). It is notreasonable to make the same assumption about an aggregate final function,however. Commit804163b broke this, as it will pass whichever Aggrefwas first associated with the transition state in both cases.This doesn't create an observable bug so far as the core system isconcerned, because the only existing uses of AggGetAggref() are inordered-set aggregates that happen to not pay attention to anythingbut the input properties of the Aggref; and besides that, we disabledsharing of transition calculations for OSAs yesterday. Nonetheless,if some third-party code were using AggGetAggref() in a normal aggregate,they would be entitled to call this a bug. Hence, back-patch the fixto 9.6 where the problem was introduced.In passing, improve some of the comments about transition state sharing.Discussion:https://postgr.es/m/CAB4ELO5RZhOamuT9Xsf72ozbenDLLXZKSk07FiSVsuJNZB861A@mail.gmail.com
1 parentad4a7ed commit305cf1f

File tree

2 files changed

+44
-24
lines changed

2 files changed

+44
-24
lines changed

‎src/backend/executor/nodeAgg.c‎

Lines changed: 42 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1528,8 +1528,8 @@ finalize_aggregate(AggState *aggstate,
15281528
{
15291529
intnumFinalArgs=peragg->numFinalArgs;
15301530

1531-
/* set up aggstate->curpertrans for AggGetAggref() */
1532-
aggstate->curpertrans=pertrans;
1531+
/* set up aggstate->curperagg for AggGetAggref() */
1532+
aggstate->curperagg=peragg;
15331533

15341534
InitFunctionCallInfoData(fcinfo,&peragg->finalfn,
15351535
numFinalArgs,
@@ -1562,7 +1562,7 @@ finalize_aggregate(AggState *aggstate,
15621562
*resultVal=FunctionCallInvoke(&fcinfo);
15631563
*resultIsNull=fcinfo.isnull;
15641564
}
1565-
aggstate->curpertrans=NULL;
1565+
aggstate->curperagg=NULL;
15661566
}
15671567
else
15681568
{
@@ -2712,6 +2712,7 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
27122712
aggstate->current_set=0;
27132713
aggstate->peragg=NULL;
27142714
aggstate->pertrans=NULL;
2715+
aggstate->curperagg=NULL;
27152716
aggstate->curpertrans=NULL;
27162717
aggstate->input_done= false;
27172718
aggstate->agg_done= false;
@@ -3060,27 +3061,29 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
30603061
*
30613062
* Scenarios:
30623063
*
3063-
* 1.An aggregate functionappears more than once in query:
3064+
* 1.Identical aggregate functioncalls appear in the query:
30643065
*
30653066
* SELECT SUM(x) FROM ... HAVING SUM(x) > 0
30663067
*
3067-
* Since the aggregates are the identical, we only need to calculate
3068-
* the calculate it once. Both aggregates will share the same 'aggno'
3069-
* value.
3068+
* Since these aggregates are identical, we only need to calculate
3069+
* the value once. Both aggregates will share the same 'aggno' value.
30703070
*
30713071
* 2. Two different aggregate functions appear in the query, but the
3072-
* aggregates have the same transitionfunction and initial value, but
3073-
* different finalfunction:
3072+
* aggregates have the samearguments,transitionfunctions and
3073+
*initial values (and, presumably,different finalfunctions):
30743074
*
3075-
* SELECTSUM(x),AVG(x) FROM ...
3075+
* SELECTAVG(x),STDDEV(x) FROM ...
30763076
*
30773077
* In this case we must create a new peragg for the varying aggregate,
3078-
* and need to call the final functions separately, but can share the
3079-
* same transition state.
3078+
* and we need to call the final functions separately, but we need
3079+
* only run the transition function once. (This requires that the
3080+
* final functions be nondestructive of the transition state, but
3081+
* that's required anyway for other reasons.)
30803082
*
3081-
* For either of these optimizations to be valid, the aggregate's
3082-
* arguments must be the same, including any modifiers such as ORDER BY,
3083-
* DISTINCT and FILTER, and they mustn't contain any volatile functions.
3083+
* For either of these optimizations to be valid, all aggregate properties
3084+
* used in the transition phase must be the same, including any modifiers
3085+
* such as ORDER BY, DISTINCT and FILTER, and the arguments mustn't
3086+
* contain any volatile functions.
30843087
* -----------------
30853088
*/
30863089
aggno=-1;
@@ -3723,7 +3726,7 @@ GetAggInitVal(Datum textInitVal, Oid transtype)
37233726
*
37243727
* As a side-effect, this also collects a list of existing per-Trans structs
37253728
* with matching inputs. If no identical Aggref is found, the list is passed
3726-
* later tofind_compatible_perstate, to see if we can at least reuse the
3729+
* later tofind_compatible_pertrans, to see if we can at least reuse the
37273730
* state value of another aggregate.
37283731
*/
37293732
staticint
@@ -3743,11 +3746,12 @@ find_compatible_peragg(Aggref *newagg, AggState *aggstate,
37433746

37443747
/*
37453748
* Search through the list of already seen aggregates. If we find an
3746-
* existing aggregate with the same aggregate function and input
3747-
* parameters as an existing one, then we can re-use that one. While
3749+
* existing identical aggregate call, then we can re-use that one. While
37483750
* searching, we'll also collect a list of Aggrefs with the same input
37493751
* parameters. If no matching Aggref is found, the caller can potentially
3750-
* still re-use the transition state of one of them.
3752+
* still re-use the transition state of one of them. (At this stage we
3753+
* just compare the parsetrees; whether different aggregates share the
3754+
* same transition function will be checked later.)
37513755
*/
37523756
for (aggno=0;aggno <=lastaggno;aggno++)
37533757
{
@@ -3796,7 +3800,7 @@ find_compatible_peragg(Aggref *newagg, AggState *aggstate,
37963800
* struct
37973801
*
37983802
* Searches the list of transnos for a per-Trans struct with the same
3799-
* transitionstate and initial condition. (The inputs have already been
3803+
* transitionfunction and initial condition. (The inputs have already been
38003804
* verified to match.)
38013805
*/
38023806
staticint
@@ -3842,16 +3846,16 @@ find_compatible_pertrans(AggState *aggstate, Aggref *newagg,
38423846
aggdeserialfn!=pertrans->deserialfn_oid)
38433847
continue;
38443848

3845-
/* Check that the initial condition matches, too. */
3849+
/*
3850+
* Check that the initial condition matches, too.
3851+
*/
38463852
if (initValueIsNull&&pertrans->initValueIsNull)
38473853
returntransno;
38483854

38493855
if (!initValueIsNull&& !pertrans->initValueIsNull&&
38503856
datumIsEqual(initValue,pertrans->initValue,
38513857
pertrans->transtypeByVal,pertrans->transtypeLen))
3852-
{
38533858
returntransno;
3854-
}
38553859
}
38563860
return-1;
38573861
}
@@ -4070,6 +4074,13 @@ AggCheckCallContext(FunctionCallInfo fcinfo, MemoryContext *aggcontext)
40704074
* If the function is being called as an aggregate support function,
40714075
* return the Aggref node for the aggregate call. Otherwise, return NULL.
40724076
*
4077+
* Aggregates sharing the same inputs and transition functions can get
4078+
* merged into a single transition calculation. If the transition function
4079+
* calls AggGetAggref, it will get some one of the Aggrefs for which it is
4080+
* executing. It must therefore not pay attention to the Aggref fields that
4081+
* relate to the final function, as those are indeterminate. But if a final
4082+
* function calls AggGetAggref, it will get a precise result.
4083+
*
40734084
* Note that if an aggregate is being used as a window function, this will
40744085
* return NULL. We could provide a similar function to return the relevant
40754086
* WindowFunc node in such cases, but it's not needed yet.
@@ -4079,8 +4090,16 @@ AggGetAggref(FunctionCallInfo fcinfo)
40794090
{
40804091
if (fcinfo->context&&IsA(fcinfo->context,AggState))
40814092
{
4093+
AggStatePerAggcurperagg;
40824094
AggStatePerTranscurpertrans;
40834095

4096+
/* check curperagg (valid when in a final function) */
4097+
curperagg= ((AggState*)fcinfo->context)->curperagg;
4098+
4099+
if (curperagg)
4100+
returncurperagg->aggref;
4101+
4102+
/* check curpertrans (valid when in a transition function) */
40844103
curpertrans= ((AggState*)fcinfo->context)->curpertrans;
40854104

40864105
if (curpertrans)

‎src/include/nodes/execnodes.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1808,7 +1808,8 @@ typedef struct AggState
18081808
ExprContext**aggcontexts;/* econtexts for long-lived data (per GS) */
18091809
ExprContext*tmpcontext;/* econtext for input expressions */
18101810
ExprContext*curaggcontext;/* currently active aggcontext */
1811-
AggStatePerTranscurpertrans;/* currently active trans state */
1811+
AggStatePerAggcurperagg;/* currently active aggregate, if any */
1812+
AggStatePerTranscurpertrans;/* currently active trans state, if any */
18121813
boolinput_done;/* indicates end of input */
18131814
boolagg_done;/* indicates completion of Agg scan */
18141815
intprojected_set;/* The last projected grouping set */

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp