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

Commitaa1e9b3

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 parent96cfc7e commitaa1e9b3

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
@@ -1372,8 +1372,8 @@ finalize_aggregate(AggState *aggstate,
13721372
{
13731373
intnumFinalArgs=peragg->numFinalArgs;
13741374

1375-
/* set up aggstate->curpertrans for AggGetAggref() */
1376-
aggstate->curpertrans=pertrans;
1375+
/* set up aggstate->curperagg for AggGetAggref() */
1376+
aggstate->curperagg=peragg;
13771377

13781378
InitFunctionCallInfoData(fcinfo,&peragg->finalfn,
13791379
numFinalArgs,
@@ -1406,7 +1406,7 @@ finalize_aggregate(AggState *aggstate,
14061406
*resultVal=FunctionCallInvoke(&fcinfo);
14071407
*resultIsNull=fcinfo.isnull;
14081408
}
1409-
aggstate->curpertrans=NULL;
1409+
aggstate->curperagg=NULL;
14101410
}
14111411
else
14121412
{
@@ -2391,6 +2391,7 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
23912391
aggstate->current_set=0;
23922392
aggstate->peragg=NULL;
23932393
aggstate->pertrans=NULL;
2394+
aggstate->curperagg=NULL;
23942395
aggstate->curpertrans=NULL;
23952396
aggstate->input_done= false;
23962397
aggstate->agg_done= false;
@@ -2659,27 +2660,29 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
26592660
*
26602661
* Scenarios:
26612662
*
2662-
* 1.An aggregate functionappears more than once in query:
2663+
* 1.Identical aggregate functioncalls appear in the query:
26632664
*
26642665
* SELECT SUM(x) FROM ... HAVING SUM(x) > 0
26652666
*
2666-
* Since the aggregates are the identical, we only need to calculate
2667-
* the calculate it once. Both aggregates will share the same 'aggno'
2668-
* value.
2667+
* Since these aggregates are identical, we only need to calculate
2668+
* the value once. Both aggregates will share the same 'aggno' value.
26692669
*
26702670
* 2. Two different aggregate functions appear in the query, but the
2671-
* aggregates have the same transitionfunction and initial value, but
2672-
* different finalfunction:
2671+
* aggregates have the samearguments,transitionfunctions and
2672+
*initial values (and, presumably,different finalfunctions):
26732673
*
2674-
* SELECTSUM(x),AVG(x) FROM ...
2674+
* SELECTAVG(x),STDDEV(x) FROM ...
26752675
*
26762676
* In this case we must create a new peragg for the varying aggregate,
2677-
* and need to call the final functions separately, but can share the
2678-
* same transition state.
2677+
* and we need to call the final functions separately, but we need
2678+
* only run the transition function once. (This requires that the
2679+
* final functions be nondestructive of the transition state, but
2680+
* that's required anyway for other reasons.)
26792681
*
2680-
* For either of these optimizations to be valid, the aggregate's
2681-
* arguments must be the same, including any modifiers such as ORDER BY,
2682-
* DISTINCT and FILTER, and they mustn't contain any volatile functions.
2682+
* For either of these optimizations to be valid, all aggregate properties
2683+
* used in the transition phase must be the same, including any modifiers
2684+
* such as ORDER BY, DISTINCT and FILTER, and the arguments mustn't
2685+
* contain any volatile functions.
26832686
* -----------------
26842687
*/
26852688
aggno=-1;
@@ -3287,7 +3290,7 @@ GetAggInitVal(Datum textInitVal, Oid transtype)
32873290
*
32883291
* As a side-effect, this also collects a list of existing per-Trans structs
32893292
* with matching inputs. If no identical Aggref is found, the list is passed
3290-
* later tofind_compatible_perstate, to see if we can at least reuse the
3293+
* later tofind_compatible_pertrans, to see if we can at least reuse the
32913294
* state value of another aggregate.
32923295
*/
32933296
staticint
@@ -3307,11 +3310,12 @@ find_compatible_peragg(Aggref *newagg, AggState *aggstate,
33073310

33083311
/*
33093312
* Search through the list of already seen aggregates. If we find an
3310-
* existing aggregate with the same aggregate function and input
3311-
* parameters as an existing one, then we can re-use that one. While
3313+
* existing identical aggregate call, then we can re-use that one. While
33123314
* searching, we'll also collect a list of Aggrefs with the same input
33133315
* parameters. If no matching Aggref is found, the caller can potentially
3314-
* still re-use the transition state of one of them.
3316+
* still re-use the transition state of one of them. (At this stage we
3317+
* just compare the parsetrees; whether different aggregates share the
3318+
* same transition function will be checked later.)
33153319
*/
33163320
for (aggno=0;aggno <=lastaggno;aggno++)
33173321
{
@@ -3360,7 +3364,7 @@ find_compatible_peragg(Aggref *newagg, AggState *aggstate,
33603364
* struct
33613365
*
33623366
* Searches the list of transnos for a per-Trans struct with the same
3363-
* transitionstate and initial condition. (The inputs have already been
3367+
* transitionfunction and initial condition. (The inputs have already been
33643368
* verified to match.)
33653369
*/
33663370
staticint
@@ -3406,16 +3410,16 @@ find_compatible_pertrans(AggState *aggstate, Aggref *newagg,
34063410
aggdeserialfn!=pertrans->deserialfn_oid)
34073411
continue;
34083412

3409-
/* Check that the initial condition matches, too. */
3413+
/*
3414+
* Check that the initial condition matches, too.
3415+
*/
34103416
if (initValueIsNull&&pertrans->initValueIsNull)
34113417
returntransno;
34123418

34133419
if (!initValueIsNull&& !pertrans->initValueIsNull&&
34143420
datumIsEqual(initValue,pertrans->initValue,
34153421
pertrans->transtypeByVal,pertrans->transtypeLen))
3416-
{
34173422
returntransno;
3418-
}
34193423
}
34203424
return-1;
34213425
}
@@ -3628,6 +3632,13 @@ AggCheckCallContext(FunctionCallInfo fcinfo, MemoryContext *aggcontext)
36283632
* If the function is being called as an aggregate support function,
36293633
* return the Aggref node for the aggregate call. Otherwise, return NULL.
36303634
*
3635+
* Aggregates sharing the same inputs and transition functions can get
3636+
* merged into a single transition calculation. If the transition function
3637+
* calls AggGetAggref, it will get some one of the Aggrefs for which it is
3638+
* executing. It must therefore not pay attention to the Aggref fields that
3639+
* relate to the final function, as those are indeterminate. But if a final
3640+
* function calls AggGetAggref, it will get a precise result.
3641+
*
36313642
* Note that if an aggregate is being used as a window function, this will
36323643
* return NULL. We could provide a similar function to return the relevant
36333644
* WindowFunc node in such cases, but it's not needed yet.
@@ -3637,8 +3648,16 @@ AggGetAggref(FunctionCallInfo fcinfo)
36373648
{
36383649
if (fcinfo->context&&IsA(fcinfo->context,AggState))
36393650
{
3651+
AggStatePerAggcurperagg;
36403652
AggStatePerTranscurpertrans;
36413653

3654+
/* check curperagg (valid when in a final function) */
3655+
curperagg= ((AggState*)fcinfo->context)->curperagg;
3656+
3657+
if (curperagg)
3658+
returncurperagg->aggref;
3659+
3660+
/* check curpertrans (valid when in a transition function) */
36423661
curpertrans= ((AggState*)fcinfo->context)->curpertrans;
36433662

36443663
if (curpertrans)

‎src/include/nodes/execnodes.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1834,7 +1834,7 @@ typedef struct AggState
18341834
AggStatePerTranspertrans;/* per-Trans state information */
18351835
ExprContext**aggcontexts;/* econtexts for long-lived data (per GS) */
18361836
ExprContext*tmpcontext;/* econtext for input expressions */
1837-
AggStatePerTranscurpertrans;/* currently active trans state */
1837+
AggStatePerTranscurpertrans;/* currently active trans state, if any */
18381838
boolinput_done;/* indicates end of input */
18391839
boolagg_done;/* indicates completion of Agg scan */
18401840
intprojected_set;/* The last projected grouping set */
@@ -1857,6 +1857,7 @@ typedef struct AggState
18571857
List*hash_needed;/* list of columns needed in hash table */
18581858
booltable_filled;/* hash table filled yet? */
18591859
TupleHashIteratorhashiter;/* for iterating through hash table */
1860+
AggStatePerAggcurperagg;/* currently active aggregate, if any */
18601861
}AggState;
18611862

18621863
/* ----------------

0 commit comments

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