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

Commit915b703

Browse files
committed
Fix handling of argument and result datatypes for partial aggregation.
When doing partial aggregation, the args list of the upper (combining)Aggref node is replaced by a Var representing the output of the partialaggregation steps, which has either the aggregate's transition data typeor a serialized representation of that. However, nodeAgg.c blindlycontinued to use the args list as an indication of the user-level argumenttypes. This broke resolution of polymorphic transition datatypes atexecutor startup (though it accidentally failed to fail for the ANYARRAYcase, which is likely the only one anyone had tested). Moreover, theconstructed FuncExpr passed to the finalfunc contained completely wronginformation, which would have led to bogus answers or crashes for any casewhere the finalfunc examined that information (which is only likely to bewith polymorphic aggregates using a non-polymorphic transition type).As an independent bug, apply_partialaggref_adjustment neglected to resolvea polymorphic transition datatype before assigning it as the output typeof the lower-level Aggref node. This again accidentally failed to failfor ANYARRAY but would be unlikely to work in other cases.To fix the first problem, record the user-level argument types in aseparate OID-list field of Aggref, and look to that rather than the argslist when asking what the argument types were. (It turns out to beconvenient to include any "direct" arguments in this list too, althoughthose are not currently subject to being overwritten.)Rather than adding yet another resolve_aggregate_transtype() call to fixthe second problem, add an aggtranstype field to Aggref, and store theresolved transition type OID there when the planner first computes it.(By doing this in the planner and not the parser, we can allow theaggregate's transition type to change from time to time, although no DDLsupport yet exists for that.) This saves nothing of consequence forsimple non-polymorphic aggregates, but for polymorphic transition typeswe save a catalog lookup during executor startup as well as severalplanner lookups that are new in 9.6 due to parallel query planning.In passing, fix an error that was introduced into count_agg_clauses_walkersome time ago: it was applying exprTypmod() to something that wasn't anexpression node at all, but a TargetEntry. exprTypmod silently returned-1 so that there was not an obvious failure, but this broke the intendedsensitivity of aggregate space consumption estimates to the typmod ofvarchar and similar data types. This part needs to be back-patched.Catversion bump due to change of stored Aggref nodes.Discussion: <8229.1466109074@sss.pgh.pa.us>
1 parentd30d1ac commit915b703

File tree

13 files changed

+104
-53
lines changed

13 files changed

+104
-53
lines changed

‎src/backend/executor/nodeAgg.c

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2715,6 +2715,10 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
27152715
get_func_name(aggref->aggfnoid));
27162716
InvokeFunctionExecuteHook(aggref->aggfnoid);
27172717

2718+
/* planner recorded transition state type in the Aggref itself */
2719+
aggtranstype=aggref->aggtranstype;
2720+
Assert(OidIsValid(aggtranstype));
2721+
27182722
/*
27192723
* If this aggregation is performing state combines, then instead of
27202724
* using the transition function, we'll use the combine function
@@ -2745,7 +2749,7 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
27452749
* aggregate states. This is only required if the aggregate state is
27462750
* internal.
27472751
*/
2748-
if (aggstate->serialStates&&aggform->aggtranstype==INTERNALOID)
2752+
if (aggstate->serialStates&&aggtranstype==INTERNALOID)
27492753
{
27502754
/*
27512755
* The planner should only have generated an agg node with
@@ -2835,12 +2839,6 @@ ExecInitAgg(Agg *node, EState *estate, int eflags)
28352839
/* Count the "direct" arguments, if any */
28362840
numDirectArgs=list_length(aggref->aggdirectargs);
28372841

2838-
/* resolve actual type of transition state, if polymorphic */
2839-
aggtranstype=resolve_aggregate_transtype(aggref->aggfnoid,
2840-
aggform->aggtranstype,
2841-
inputTypes,
2842-
numArguments);
2843-
28442842
/* Detect how many arguments to pass to the finalfn */
28452843
if (aggform->aggfinalextra)
28462844
peragg->numFinalArgs=numArguments+1;
@@ -3304,6 +3302,7 @@ find_compatible_peragg(Aggref *newagg, AggState *aggstate,
33043302

33053303
/* all of the following must be the same or it's no match */
33063304
if (newagg->inputcollid!=existingRef->inputcollid||
3305+
newagg->aggtranstype!=existingRef->aggtranstype||
33073306
newagg->aggstar!=existingRef->aggstar||
33083307
newagg->aggvariadic!=existingRef->aggvariadic||
33093308
newagg->aggkind!=existingRef->aggkind||

‎src/backend/nodes/copyfuncs.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1237,6 +1237,8 @@ _copyAggref(const Aggref *from)
12371237
COPY_SCALAR_FIELD(aggoutputtype);
12381238
COPY_SCALAR_FIELD(aggcollid);
12391239
COPY_SCALAR_FIELD(inputcollid);
1240+
COPY_SCALAR_FIELD(aggtranstype);
1241+
COPY_NODE_FIELD(aggargtypes);
12401242
COPY_NODE_FIELD(aggdirectargs);
12411243
COPY_NODE_FIELD(args);
12421244
COPY_NODE_FIELD(aggorder);

‎src/backend/nodes/equalfuncs.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,8 @@ _equalAggref(const Aggref *a, const Aggref *b)
195195
COMPARE_SCALAR_FIELD(aggoutputtype);
196196
COMPARE_SCALAR_FIELD(aggcollid);
197197
COMPARE_SCALAR_FIELD(inputcollid);
198+
/* ignore aggtranstype since it might not be set yet */
199+
COMPARE_NODE_FIELD(aggargtypes);
198200
COMPARE_NODE_FIELD(aggdirectargs);
199201
COMPARE_NODE_FIELD(args);
200202
COMPARE_NODE_FIELD(aggorder);

‎src/backend/nodes/nodeFuncs.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2451,6 +2451,8 @@ expression_tree_mutator(Node *node,
24512451
Aggref*newnode;
24522452

24532453
FLATCOPY(newnode,aggref,Aggref);
2454+
/* assume mutation doesn't change types of arguments */
2455+
newnode->aggargtypes=list_copy(aggref->aggargtypes);
24542456
MUTATE(newnode->aggdirectargs,aggref->aggdirectargs,List*);
24552457
MUTATE(newnode->args,aggref->args,List*);
24562458
MUTATE(newnode->aggorder,aggref->aggorder,List*);

‎src/backend/nodes/outfuncs.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1033,6 +1033,8 @@ _outAggref(StringInfo str, const Aggref *node)
10331033
WRITE_OID_FIELD(aggoutputtype);
10341034
WRITE_OID_FIELD(aggcollid);
10351035
WRITE_OID_FIELD(inputcollid);
1036+
WRITE_OID_FIELD(aggtranstype);
1037+
WRITE_NODE_FIELD(aggargtypes);
10361038
WRITE_NODE_FIELD(aggdirectargs);
10371039
WRITE_NODE_FIELD(args);
10381040
WRITE_NODE_FIELD(aggorder);

‎src/backend/nodes/readfuncs.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -549,6 +549,8 @@ _readAggref(void)
549549
READ_OID_FIELD(aggoutputtype);
550550
READ_OID_FIELD(aggcollid);
551551
READ_OID_FIELD(inputcollid);
552+
READ_OID_FIELD(aggtranstype);
553+
READ_NODE_FIELD(aggargtypes);
552554
READ_NODE_FIELD(aggdirectargs);
553555
READ_NODE_FIELD(args);
554556
READ_NODE_FIELD(aggorder);

‎src/backend/optimizer/plan/setrefs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2085,6 +2085,7 @@ search_indexed_tlist_for_partial_aggref(Aggref *aggref, indexed_tlist *itlist,
20852085
continue;
20862086
if (aggref->inputcollid!=tlistaggref->inputcollid)
20872087
continue;
2088+
/* ignore aggtranstype and aggargtypes, should be redundant */
20882089
if (!equal(aggref->aggdirectargs,tlistaggref->aggdirectargs))
20892090
continue;
20902091
if (!equal(aggref->args,tlistaggref->args))

‎src/backend/optimizer/util/clauses.c

Lines changed: 35 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -523,7 +523,7 @@ contain_agg_clause_walker(Node *node, void *context)
523523
/*
524524
* count_agg_clauses
525525
* Recursively count the Aggref nodes in an expression tree, and
526-
* accumulate othercostinformation about them too.
526+
* accumulate other information about them too.
527527
*
528528
* Note: this also checks for nested aggregates, which are an error.
529529
*
@@ -532,6 +532,10 @@ contain_agg_clause_walker(Node *node, void *context)
532532
* values if all are evaluated in parallel (as would be done in a HashAgg
533533
* plan). See AggClauseCosts for the exact set of statistics collected.
534534
*
535+
* In addition, we mark Aggref nodes with the correct aggtranstype, so
536+
* that that doesn't need to be done repeatedly. (That makes this function's
537+
* name a bit of a misnomer.)
538+
*
535539
* NOTE that the counts/costs are ADDED to those already in *costs ... so
536540
* the caller is responsible for zeroing the struct initially.
537541
*
@@ -572,8 +576,6 @@ count_agg_clauses_walker(Node *node, count_agg_clauses_context *context)
572576
Oidaggtranstype;
573577
int32aggtransspace;
574578
QualCostargcosts;
575-
OidinputTypes[FUNC_MAX_ARGS];
576-
intnumArguments;
577579

578580
Assert(aggref->agglevelsup==0);
579581

@@ -597,6 +599,28 @@ count_agg_clauses_walker(Node *node, count_agg_clauses_context *context)
597599
aggtransspace=aggform->aggtransspace;
598600
ReleaseSysCache(aggTuple);
599601

602+
/*
603+
* Resolve the possibly-polymorphic aggregate transition type, unless
604+
* already done in a previous pass over the expression.
605+
*/
606+
if (OidIsValid(aggref->aggtranstype))
607+
aggtranstype=aggref->aggtranstype;
608+
else
609+
{
610+
OidinputTypes[FUNC_MAX_ARGS];
611+
intnumArguments;
612+
613+
/* extract argument types (ignoring any ORDER BY expressions) */
614+
numArguments=get_aggregate_argtypes(aggref,inputTypes);
615+
616+
/* resolve actual type of transition state, if polymorphic */
617+
aggtranstype=resolve_aggregate_transtype(aggref->aggfnoid,
618+
aggtranstype,
619+
inputTypes,
620+
numArguments);
621+
aggref->aggtranstype=aggtranstype;
622+
}
623+
600624
/* count it; note ordered-set aggs always have nonempty aggorder */
601625
costs->numAggs++;
602626
if (aggref->aggorder!=NIL||aggref->aggdistinct!=NIL)
@@ -668,15 +692,6 @@ count_agg_clauses_walker(Node *node, count_agg_clauses_context *context)
668692
costs->finalCost+=argcosts.per_tuple;
669693
}
670694

671-
/* extract argument types (ignoring any ORDER BY expressions) */
672-
numArguments=get_aggregate_argtypes(aggref,inputTypes);
673-
674-
/* resolve actual type of transition state, if polymorphic */
675-
aggtranstype=resolve_aggregate_transtype(aggref->aggfnoid,
676-
aggtranstype,
677-
inputTypes,
678-
numArguments);
679-
680695
/*
681696
* If the transition type is pass-by-value then it doesn't add
682697
* anything to the required size of the hashtable. If it is
@@ -698,14 +713,15 @@ count_agg_clauses_walker(Node *node, count_agg_clauses_context *context)
698713
* This works for cases like MAX/MIN and is probably somewhat
699714
* reasonable otherwise.
700715
*/
701-
intnumdirectargs=list_length(aggref->aggdirectargs);
702-
int32aggtranstypmod;
716+
int32aggtranstypmod=-1;
703717

704-
if (numArguments>numdirectargs&&
705-
aggtranstype==inputTypes[numdirectargs])
706-
aggtranstypmod=exprTypmod((Node*)linitial(aggref->args));
707-
else
708-
aggtranstypmod=-1;
718+
if (aggref->args)
719+
{
720+
TargetEntry*tle= (TargetEntry*)linitial(aggref->args);
721+
722+
if (aggtranstype==exprType((Node*)tle->expr))
723+
aggtranstypmod=exprTypmod((Node*)tle->expr);
724+
}
709725

710726
avgwidth=get_typavgwidth(aggtranstype,aggtranstypmod);
711727
}

‎src/backend/optimizer/util/tlist.c

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -797,11 +797,20 @@ apply_partialaggref_adjustment(PathTarget *target)
797797

798798
newaggref= (Aggref*)copyObject(aggref);
799799

800-
/* use the serialization type, if one exists */
800+
/*
801+
* Use the serialization type, if one exists. Note that we don't
802+
* support it being a polymorphic type. (XXX really we ought to
803+
* hardwire this as INTERNAL -> BYTEA, and avoid a catalog lookup
804+
* here altogether?)
805+
*/
801806
if (OidIsValid(aggform->aggserialtype))
802807
newaggref->aggoutputtype=aggform->aggserialtype;
803808
else
804-
newaggref->aggoutputtype=aggform->aggtranstype;
809+
{
810+
/* Otherwise, we return the aggregate's transition type */
811+
Assert(OidIsValid(newaggref->aggtranstype));
812+
newaggref->aggoutputtype=newaggref->aggtranstype;
813+
}
805814

806815
/* flag it as partial */
807816
newaggref->aggpartial= true;

‎src/backend/parser/parse_agg.c

Lines changed: 22 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -77,10 +77,10 @@ static List *expand_groupingset_node(GroupingSet *gs);
7777
*Finish initial transformation of an aggregate call
7878
*
7979
* parse_func.c has recognized the function as an aggregate, and has set up
80-
* all the fields of the Aggref exceptaggdirectargs, args, aggorder,
81-
* aggdistinct and agglevelsup. The passed-in args list has been through
82-
* standard expression transformation and type coercion to match the agg's
83-
* declared arg types, while the passed-in aggorder list hasn't been
80+
* all the fields of the Aggref exceptaggargtypes, aggdirectargs, args,
81+
*aggorder,aggdistinct and agglevelsup. The passed-in args list has been
82+
*throughstandard expression transformation and type coercion to match the
83+
*agg'sdeclared arg types, while the passed-in aggorder list hasn't been
8484
* transformed at all.
8585
*
8686
* Here we separate the args list into direct and aggregated args, storing the
@@ -101,13 +101,26 @@ void
101101
transformAggregateCall(ParseState*pstate,Aggref*agg,
102102
List*args,List*aggorder,boolagg_distinct)
103103
{
104+
List*argtypes=NIL;
104105
List*tlist=NIL;
105106
List*torder=NIL;
106107
List*tdistinct=NIL;
107108
AttrNumberattno=1;
108109
intsave_next_resno;
109110
ListCell*lc;
110111

112+
/*
113+
* Before separating the args into direct and aggregated args, make a list
114+
* of their data type OIDs for use later.
115+
*/
116+
foreach(lc,args)
117+
{
118+
Expr*arg= (Expr*)lfirst(lc);
119+
120+
argtypes=lappend_oid(argtypes,exprType((Node*)arg));
121+
}
122+
agg->aggargtypes=argtypes;
123+
111124
if (AGGKIND_IS_ORDERED_SET(agg->aggkind))
112125
{
113126
/*
@@ -1763,26 +1776,11 @@ get_aggregate_argtypes(Aggref *aggref, Oid *inputTypes)
17631776
intnumArguments=0;
17641777
ListCell*lc;
17651778

1766-
/* Any direct arguments of an ordered-set aggregate come first */
1767-
foreach(lc,aggref->aggdirectargs)
1768-
{
1769-
Node*expr= (Node*)lfirst(lc);
1770-
1771-
inputTypes[numArguments]=exprType(expr);
1772-
numArguments++;
1773-
}
1779+
Assert(list_length(aggref->aggargtypes) <=FUNC_MAX_ARGS);
17741780

1775-
/* Now get the regular (aggregated) arguments */
1776-
foreach(lc,aggref->args)
1781+
foreach(lc,aggref->aggargtypes)
17771782
{
1778-
TargetEntry*tle= (TargetEntry*)lfirst(lc);
1779-
1780-
/* Ignore ordering columns of a plain aggregate */
1781-
if (tle->resjunk)
1782-
continue;
1783-
1784-
inputTypes[numArguments]=exprType((Node*)tle->expr);
1785-
numArguments++;
1783+
inputTypes[numArguments++]=lfirst_oid(lc);
17861784
}
17871785

17881786
returnnumArguments;
@@ -1795,8 +1793,8 @@ get_aggregate_argtypes(Aggref *aggref, Oid *inputTypes)
17951793
* This function resolves a polymorphic aggregate's state datatype.
17961794
* It must be passed the aggtranstype from the aggregate's catalog entry,
17971795
* as well as the actual argument types extracted by get_aggregate_argtypes.
1798-
* (We could fetchthese valuesinternally, butforall existing callers that
1799-
*would just duplicate workthecaller has to do too, so wepass themin.)
1796+
* (We could fetchpg_aggregate.aggtranstypeinternally, but all existing
1797+
*callers already havethevalue at hand, so wemake thempass it.)
18001798
*/
18011799
Oid
18021800
resolve_aggregate_transtype(Oidaggfuncid,

‎src/backend/parser/parse_func.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -650,11 +650,15 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
650650
/* default the outputtype to be the same as aggtype */
651651
aggref->aggtype=aggref->aggoutputtype=rettype;
652652
/* aggcollid and inputcollid will be set by parse_collate.c */
653+
aggref->aggtranstype=InvalidOid;/* will be set by planner */
654+
/* aggargtypes will be set by transformAggregateCall */
653655
/* aggdirectargs and args will be set by transformAggregateCall */
654656
/* aggorder and aggdistinct will be set by transformAggregateCall */
655657
aggref->aggfilter=agg_filter;
656658
aggref->aggstar=agg_star;
657659
aggref->aggvariadic=func_variadic;
660+
/* at this point, the Aggref is never partial or combining */
661+
aggref->aggcombine=aggref->aggpartial= false;
658662
aggref->aggkind=aggkind;
659663
/* agglevelsup will be set by transformAggregateCall */
660664
aggref->location=location;

‎src/include/catalog/catversion.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,6 @@
5353
*/
5454

5555
/*yyyymmddN */
56-
#defineCATALOG_VERSION_NO201606151
56+
#defineCATALOG_VERSION_NO201606171
5757

5858
#endif

‎src/include/nodes/primnodes.h

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,18 @@ typedef struct Param
256256
* The direct arguments appear in aggdirectargs (as a list of plain
257257
* expressions, not TargetEntry nodes).
258258
*
259+
* aggtranstype is the data type of the state transition values for this
260+
* aggregate (resolved to an actual type, if agg's transtype is polymorphic).
261+
* This is determined during planning and is InvalidOid before that.
262+
*
263+
* aggargtypes is an OID list of the data types of the direct and regular
264+
* arguments. Normally it's redundant with the aggdirectargs and args lists,
265+
* but in a combining aggregate, it's not because the args list has been
266+
* replaced with a single argument representing the partial-aggregate
267+
* transition values.
268+
*
269+
* XXX need more documentation about partial aggregation here
270+
*
259271
* 'aggtype' and 'aggoutputtype' are the same except when we're performing
260272
* partal aggregation; in that case, we output transition states. Nothing
261273
* interesting happens in the Aggref itself, but we must set the output data
@@ -272,6 +284,8 @@ typedef struct Aggref
272284
Oidaggoutputtype;/* type Oid of result of this aggregate */
273285
Oidaggcollid;/* OID of collation of result */
274286
Oidinputcollid;/* OID of collation that function should use */
287+
Oidaggtranstype;/* type Oid of aggregate's transition value */
288+
List*aggargtypes;/* type Oids of direct and aggregated args */
275289
List*aggdirectargs;/* direct arguments, if an ordered-set agg */
276290
List*args;/* aggregated arguments and sort expressions */
277291
List*aggorder;/* ORDER BY (list of SortGroupClause) */

0 commit comments

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