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

Commitcb591fc

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 parenta3b1c22 commitcb591fc

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
/*
@@ -2799,10 +2804,11 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
27992804
/*
28002805
* initialize child expressions
28012806
*
2802-
* We rely on the parser to have checked that no aggs contain other agg
2803-
* calls in their arguments. This would make no sense under SQL semantics
2804-
* (and it's forbidden by the spec). Because it is true, we don't need to
2805-
* worry about evaluating the aggs in any particular order.
2807+
* We expect the parser to have checked that no aggs contain other agg
2808+
* calls in their arguments (and just to be sure, we verify it again while
2809+
* initializing the plan node). This would make no sense under SQL
2810+
* semantics, and it's forbidden by the spec. Because it is true, we
2811+
* don't need to worry about evaluating the aggs in any particular order.
28062812
*
28072813
* Note: execExpr.c finds Aggrefs for us, and adds their AggrefExprState
28082814
* nodes to aggstate->aggs. Aggrefs in the qual are found here; Aggrefs
@@ -2841,17 +2847,6 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
28412847
*/
28422848
numaggs=aggstate->numaggs;
28432849
Assert(numaggs==list_length(aggstate->aggs));
2844-
if (numaggs <=0)
2845-
{
2846-
/*
2847-
* This is not an error condition: we might be using the Agg node just
2848-
* to do hash-based grouping. Even in the regular case,
2849-
* constant-expression simplification could optimize away all of the
2850-
* Aggrefs in the targetlist and qual. So keep going, but force local
2851-
* copy of numaggs positive so that palloc()s below don't choke.
2852-
*/
2853-
numaggs=1;
2854-
}
28552850

28562851
/*
28572852
* For each phase, prepare grouping set data and fmgr lookup data for
@@ -3343,19 +3338,19 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
33433338
}
33443339

33453340
/*
3346-
* Update numaggs tomatch the number of unique aggregates found. Also set
3347-
* numstates to the number of uniqueaggregate states found.
3341+
* Updateaggstate->numaggs tobe the number of unique aggregates found.
3342+
*Also setnumstates to the number of uniquetransition states found.
33483343
*/
33493344
aggstate->numaggs=aggno+1;
33503345
aggstate->numtrans=transno+1;
33513346

33523347
/*
33533348
* Build a single projection computing the aggregate arguments for all
3354-
* aggregates at once, that'sconsiderably fasterthandoing it separately
3355-
* for each.
3349+
* aggregates at once; if there'smorethanone, that's considerably
3350+
*faster than doing it separatelyfor each.
33563351
*
3357-
* First create a targetlist combining thetargetlist of all the
3358-
*transitions.
3352+
* First create a targetlist combining thetargetlists of all the
3353+
*per-trans states.
33593354
*/
33603355
combined_inputeval=NIL;
33613356
column_offset=0;
@@ -3364,10 +3359,14 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
33643359
AggStatePerTranspertrans=&pertransstates[transno];
33653360
ListCell*arg;
33663361

3362+
/*
3363+
* Mark this per-trans state with its starting column in the combined
3364+
* slot.
3365+
*/
33673366
pertrans->inputoff=column_offset;
33683367

33693368
/*
3370-
* Adjustresno ina copied target entries, topoint into the combined
3369+
* Adjustresnos inthe copied target entries tomatch the combined
33713370
* slot.
33723371
*/
33733372
foreach(arg,pertrans->aggref->args)
@@ -3384,7 +3383,7 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
33843383
column_offset+=list_length(pertrans->aggref->args);
33853384
}
33863385

3387-
/*and thencreate a projection forthat targetlist */
3386+
/*Nowcreate a projection forthe combined targetlist */
33883387
aggstate->evaldesc=ExecTypeFromTL(combined_inputeval, false);
33893388
aggstate->evalslot=ExecInitExtraTupleSlot(estate);
33903389
aggstate->evalproj=ExecBuildProjectionInfo(combined_inputeval,
@@ -3394,6 +3393,21 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
33943393
NULL);
33953394
ExecSetSlotDescriptor(aggstate->evalslot,aggstate->evaldesc);
33963395

3396+
/*
3397+
* Last, check whether any more aggregates got added onto the node while
3398+
* we processed the expressions for the aggregate arguments (including not
3399+
* only the regular arguments handled immediately above, but any FILTER
3400+
* expressions and direct arguments we might've handled earlier). If so,
3401+
* we have nested aggregate functions, which is semantically nonsensical,
3402+
* so complain. (This should have been caught by the parser, so we don't
3403+
* need to work hard on a helpful error message; but we defend against it
3404+
* here anyway, just to be sure.)
3405+
*/
3406+
if (numaggs!=list_length(aggstate->aggs))
3407+
ereport(ERROR,
3408+
(errcode(ERRCODE_GROUPING_ERROR),
3409+
errmsg("aggregate function calls cannot be nested")));
3410+
33973411
returnaggstate;
33983412
}
33993413

@@ -3423,7 +3437,6 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
34233437
List*sortlist;
34243438
intnumSortCols;
34253439
intnumDistinctCols;
3426-
intnaggs;
34273440
inti;
34283441

34293442
/* Begin filling in the pertrans data */
@@ -3565,22 +3578,11 @@ build_pertrans_for_aggref(AggStatePerTrans pertrans,
35653578
}
35663579

35673580
/* Initialize the input and FILTER expressions */
3568-
naggs=aggstate->numaggs;
35693581
pertrans->aggfilter=ExecInitExpr(aggref->aggfilter,
35703582
(PlanState*)aggstate);
35713583
pertrans->aggdirectargs=ExecInitExprList(aggref->aggdirectargs,
35723584
(PlanState*)aggstate);
35733585

3574-
/*
3575-
* Complain if the aggregate's arguments contain any aggregates; nested
3576-
* agg functions are semantically nonsensical. (This should have been
3577-
* caught earlier, but we defend against it here anyway.)
3578-
*/
3579-
if (naggs!=aggstate->numaggs)
3580-
ereport(ERROR,
3581-
(errcode(ERRCODE_GROUPING_ERROR),
3582-
errmsg("aggregate function calls cannot be nested")));
3583-
35843586
/*
35853587
* If we're doing either DISTINCT or ORDER BY for a plain agg, then we
35863588
* have a list of SortGroupClause nodes; fish out the data in them and

0 commit comments

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