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

Commit5fc438f

Browse files
committed
Restore nodeAgg.c's ability to check for improperly-nested aggregates.
While poking around in the aggregate logic, I noticed that commit8ed3f11 broke the logic in nodeAgg.c that purports to detect nestedaggregates, by moving initialization of regular aggregate argumentexpressions out of the code segment that checks for that.You could argue that this check is unnecessary, but it's not much codeso I'm inclined to keep it as a backstop against parser and plannerbugs. However, there's certainly zero value in checking only some ofthe subexpressions.We can make the check complete again, and as a bonus make it a gooddeal more bulletproof against future mistakes of the same ilk, bymoving it out to the outermost level of ExecInitAgg. This means weneed to check only once per Agg node not once per aggregate, whichalso seems like a good thing --- if the check does find somethingwrong, it's not urgent that we report it before the plan nodeinitialization finishes.Since this requires remembering the original length of the aggs list,I deleted a long-obsolete stanza that changed numaggs from 0 to 1.That's so old it predates our decision that palloc(0) is a validoperation, in (digs...) 2004, see commit24a1e20.In passing improve a few comments.Back-patch to v10, just in case.
1 parentd8794fd commit5fc438f

File tree

1 file changed

+38
-36
lines changed

1 file changed

+38
-36
lines changed

‎src/backend/executor/nodeAgg.c‎

Lines changed: 38 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -268,7 +268,12 @@ typedef struct AggStatePerTransData
268268
*/
269269
intnumInputs;
270270

271-
/* offset of input columns in AggState->evalslot */
271+
/*
272+
* At each input row, we evaluate all argument expressions needed for all
273+
* the aggregates in this Agg node in a single ExecProject call. inputoff
274+
* is the starting index of this aggregate's argument expressions in the
275+
* resulting tuple (in AggState->evalslot).
276+
*/
272277
intinputoff;
273278

274279
/*
@@ -2809,10 +2814,11 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
28092814
/*
28102815
* initialize child expressions
28112816
*
2812-
* We rely on the parser to have checked that no aggs contain other agg
2813-
* calls in their arguments. This would make no sense under SQL semantics
2814-
* (and it's forbidden by the spec). Because it is true, we don't need to
2815-
* worry about evaluating the aggs in any particular order.
2817+
* We expect the parser to have checked that no aggs contain other agg
2818+
* calls in their arguments (and just to be sure, we verify it again while
2819+
* initializing the plan node). This would make no sense under SQL
2820+
* semantics, and it's forbidden by the spec. Because it is true, we
2821+
* don't need to worry about evaluating the aggs in any particular order.
28162822
*
28172823
* Note: execExpr.c finds Aggrefs for us, and adds their AggrefExprState
28182824
* nodes to aggstate->aggs. Aggrefs in the qual are found here; Aggrefs
@@ -2851,17 +2857,6 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
28512857
*/
28522858
numaggs=aggstate->numaggs;
28532859
Assert(numaggs==list_length(aggstate->aggs));
2854-
if (numaggs <=0)
2855-
{
2856-
/*
2857-
* This is not an error condition: we might be using the Agg node just
2858-
* to do hash-based grouping. Even in the regular case,
2859-
* constant-expression simplification could optimize away all of the
2860-
* Aggrefs in the targetlist and qual. So keep going, but force local
2861-
* copy of numaggs positive so that palloc()s below don't choke.
2862-
*/
2863-
numaggs=1;
2864-
}
28652860

28662861
/*
28672862
* For each phase, prepare grouping set data and fmgr lookup data for
@@ -3364,19 +3359,19 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
33643359
}
33653360

33663361
/*
3367-
* Update numaggs tomatch the number of unique aggregates found. Also set
3368-
* numstates to the number of uniqueaggregate states found.
3362+
* Updateaggstate->numaggs tobe the number of unique aggregates found.
3363+
*Also setnumstates to the number of uniquetransition states found.
33693364
*/
33703365
aggstate->numaggs=aggno+1;
33713366
aggstate->numtrans=transno+1;
33723367

33733368
/*
33743369
* Build a single projection computing the aggregate arguments for all
3375-
* aggregates at once, that'sconsiderably fasterthandoing it separately
3376-
* for each.
3370+
* aggregates at once; if there'smorethanone, that's considerably
3371+
*faster than doing it separatelyfor each.
33773372
*
3378-
* First create a targetlist combining thetargetlist of all the
3379-
*transitions.
3373+
* First create a targetlist combining thetargetlists of all the
3374+
*per-trans states.
33803375
*/
33813376
combined_inputeval=NIL;
33823377
column_offset=0;
@@ -3385,10 +3380,14 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
33853380
AggStatePerTranspertrans=&pertransstates[transno];
33863381
ListCell*arg;
33873382

3383+
/*
3384+
* Mark this per-trans state with its starting column in the combined
3385+
* slot.
3386+
*/
33883387
pertrans->inputoff=column_offset;
33893388

33903389
/*
3391-
* Adjustresno ina copied target entries, topoint into the combined
3390+
* Adjustresnos inthe copied target entries tomatch the combined
33923391
* slot.
33933392
*/
33943393
foreach(arg,pertrans->aggref->args)
@@ -3405,7 +3404,7 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
34053404
column_offset+=list_length(pertrans->aggref->args);
34063405
}
34073406

3408-
/*and thencreate a projection forthat targetlist */
3407+
/*Nowcreate a projection forthe combined targetlist */
34093408
aggstate->evaldesc=ExecTypeFromTL(combined_inputeval, false);
34103409
aggstate->evalslot=ExecInitExtraTupleSlot(estate);
34113410
aggstate->evalproj=ExecBuildProjectionInfo(combined_inputeval,
@@ -3415,6 +3414,21 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
34153414
NULL);
34163415
ExecSetSlotDescriptor(aggstate->evalslot,aggstate->evaldesc);
34173416

3417+
/*
3418+
* Last, check whether any more aggregates got added onto the node while
3419+
* we processed the expressions for the aggregate arguments (including not
3420+
* only the regular arguments handled immediately above, but any FILTER
3421+
* expressions and direct arguments we might've handled earlier). If so,
3422+
* we have nested aggregate functions, which is semantically nonsensical,
3423+
* so complain. (This should have been caught by the parser, so we don't
3424+
* need to work hard on a helpful error message; but we defend against it
3425+
* here anyway, just to be sure.)
3426+
*/
3427+
if (numaggs!=list_length(aggstate->aggs))
3428+
ereport(ERROR,
3429+
(errcode(ERRCODE_GROUPING_ERROR),
3430+
errmsg("aggregate function calls cannot be nested")));
3431+
34183432
returnaggstate;
34193433
}
34203434

@@ -3444,7 +3458,6 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
34443458
List*sortlist;
34453459
intnumSortCols;
34463460
intnumDistinctCols;
3447-
intnaggs;
34483461
inti;
34493462

34503463
/* Begin filling in the pertrans data */
@@ -3586,22 +3599,11 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
35863599
}
35873600

35883601
/* Initialize the input and FILTER expressions */
3589-
naggs=aggstate->numaggs;
35903602
pertrans->aggfilter=ExecInitExpr(aggref->aggfilter,
35913603
(PlanState*)aggstate);
35923604
pertrans->aggdirectargs=ExecInitExprList(aggref->aggdirectargs,
35933605
(PlanState*)aggstate);
35943606

3595-
/*
3596-
* Complain if the aggregate's arguments contain any aggregates; nested
3597-
* agg functions are semantically nonsensical. (This should have been
3598-
* caught earlier, but we defend against it here anyway.)
3599-
*/
3600-
if (naggs!=aggstate->numaggs)
3601-
ereport(ERROR,
3602-
(errcode(ERRCODE_GROUPING_ERROR),
3603-
errmsg("aggregate function calls cannot be nested")));
3604-
36053607
/*
36063608
* If we're doing either DISTINCT or ORDER BY for a plain agg, then we
36073609
* have a list of SortGroupClause nodes; fish out the data in them and

0 commit comments

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