Re: [HACKERS] Speed of user-defined aggregates using array_append as transfn
I wrote: > I'm still worried about chewing up a kilobyte-at-least for each transition > value, but maybe that's something we could leave to fix later. Another > idea is that we could teach the planner to know about that in its hash > table size estimates. Here's a complete proposed patch for this. I decided to hard-wire the planner adjustment to apply to array_append specifically. One could consider doing it whenever the aggregate transtype is an array type, but that seems likely to be casting too wide a net for now. We can revisit it in the future if necessary. In any case, the estimate can be overridden per-aggregate using the aggtransspace parameter. Barring objections, I intend to back-patch this as far as 9.5. regards, tom lane diff --git a/doc/src/sgml/xaggr.sgml b/doc/src/sgml/xaggr.sgml index fa98572..d432c9a 100644 *** a/doc/src/sgml/xaggr.sgml --- b/doc/src/sgml/xaggr.sgml *** if (AggCheckCallContext(fcinfo, NULL)) *** 626,632 function, the first input must be a temporary state value and can therefore safely be modified in-place rather than allocating a new copy. !See int8inc() for an example. (This is the only case where it is safe for a function to modify a pass-by-reference input. In particular, final functions for normal aggregates must not --- 626,632 function, the first input must be a temporary state value and can therefore safely be modified in-place rather than allocating a new copy. !See int8inc() for an example. (This is the only case where it is safe for a function to modify a pass-by-reference input. In particular, final functions for normal aggregates must not *** if (AggCheckCallContext(fcinfo, NULL)) *** 635,640 --- 635,654 +The second argument of AggCheckCallContext can be used to +retrieve the memory context in which aggregate state values are being kept. +This is useful for transition functions that wish to use expanded +objects (see ) as their transition values. +On first call, the transition function should return an expanded object +whose memory context is a child of the aggregate state context, and then +keep returning the same expanded object on subsequent calls. See +array_append() for an example. (array_append() +is not the transition function of any built-in aggregate, but it is written +to behave efficiently when used as transition function of a custom +aggregate.) + + + Another support routine available to aggregate functions written in C is AggGetAggref, which returns the Aggref parse node that defines the aggregate call. This is mainly useful diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c index b06e1c1..28c15ba 100644 *** a/src/backend/executor/nodeAgg.c --- b/src/backend/executor/nodeAgg.c *** *** 91,100 * transition value or a previous function result, and in either case its * value need not be preserved. See int8inc() for an example. Notice that * advance_transition_function() is coded to avoid a data copy step when ! * the previous transition value pointer is returned. Also, some ! * transition functions want to store working state in addition to the ! * nominal transition value; they can use the memory context returned by ! * AggCheckCallContext() to do that. * * Note: AggCheckCallContext() is available as of PostgreSQL 9.0. The * AggState is available as context in earlier releases (back to 8.1), --- 91,103 * transition value or a previous function result, and in either case its * value need not be preserved. See int8inc() for an example. Notice that * advance_transition_function() is coded to avoid a data copy step when ! * the previous transition value pointer is returned. It is also possible ! * to avoid repeated data copying when the transition value is an expanded ! * object: to do that, the transition function must take care to return ! * an expanded object that is in a child context of the memory context ! * returned by AggCheckCallContext(). Also, some transition functions want ! * to store working state in addition to the nominal transition value; they ! * can use the memory context returned by AggCheckCallContext() to do that. * * Note: AggCheckCallContext() is available as of PostgreSQL 9.0. The * AggState is available as context in earlier releases (back to 8.1), *** advance_transition_function(AggState *ag *** 791,798 /* * If pass-by-ref datatype, must copy the new value into aggcontext and ! * pfree the prior transValue. But if transfn returned a pointer to its ! * first input, we don't need to do anything. */ if (!pertrans->transtypeByVal && DatumGetPointer(newVal) != DatumGetPointer(pergroupstate->transValue)) --- 794,803
Re: [HACKERS] Speed of user-defined aggregates using array_append as transfn
I wrote: > 3. We could try to fix it mostly from array_append's side, by teaching it > to return the expanded array in the aggregation context when it's being > called as an aggregate function, and making some > hopefully-not-performance-killing tweaks to nodeAgg to play along with > that. This would amount to additional complication in the API contract > for C-coded aggregate functions, but I think it wouldn't affect functions > that weren't attempting to invoke the optimization, so it should be OK. > A larger objection is that it still doesn't do anything for the memory > consumption angle. Hm, that actually seems to work, at least from a speed standpoint; see the attached not-terribly-well-documented patch. The additions to nodeAgg are only in places where we're going to be doing datum copying or freeing anyway. I'm still worried about chewing up a kilobyte-at-least for each transition value, but maybe that's something we could leave to fix later. Another idea is that we could teach the planner to know about that in its hash table size estimates. regards, tom lane diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c index b06e1c1..e0288ba 100644 *** a/src/backend/executor/nodeAgg.c --- b/src/backend/executor/nodeAgg.c *** advance_transition_function(AggState *ag *** 791,798 /* * If pass-by-ref datatype, must copy the new value into aggcontext and ! * pfree the prior transValue. But if transfn returned a pointer to its ! * first input, we don't need to do anything. */ if (!pertrans->transtypeByVal && DatumGetPointer(newVal) != DatumGetPointer(pergroupstate->transValue)) --- 791,800 /* * If pass-by-ref datatype, must copy the new value into aggcontext and ! * free the prior transValue. But if transfn returned a pointer to its ! * first input, we don't need to do anything. Also, if transfn returned a ! * pointer to a R/W expanded object that is already a child of the ! * aggcontext, assume we can adopt that value without copying it. */ if (!pertrans->transtypeByVal && DatumGetPointer(newVal) != DatumGetPointer(pergroupstate->transValue)) *** advance_transition_function(AggState *ag *** 800,811 if (!fcinfo->isnull) { MemoryContextSwitchTo(aggstate->aggcontexts[aggstate->current_set]->ecxt_per_tuple_memory); ! newVal = datumCopy(newVal, ! pertrans->transtypeByVal, ! pertrans->transtypeLen); } if (!pergroupstate->transValueIsNull) ! pfree(DatumGetPointer(pergroupstate->transValue)); } pergroupstate->transValue = newVal; --- 802,826 if (!fcinfo->isnull) { MemoryContextSwitchTo(aggstate->aggcontexts[aggstate->current_set]->ecxt_per_tuple_memory); ! if (DatumIsReadWriteExpandedObject(newVal, ! false, ! pertrans->transtypeLen) && ! MemoryContextGetParent(DatumGetEOHP(newVal)->eoh_context) == CurrentMemoryContext) ! /* do nothing */ ; ! else ! newVal = datumCopy(newVal, ! pertrans->transtypeByVal, ! pertrans->transtypeLen); } if (!pergroupstate->transValueIsNull) ! { ! if (DatumIsReadWriteExpandedObject(pergroupstate->transValue, ! false, ! pertrans->transtypeLen)) ! DeleteExpandedObject(pergroupstate->transValue); ! else ! pfree(DatumGetPointer(pergroupstate->transValue)); ! } } pergroupstate->transValue = newVal; *** advance_combine_function(AggState *aggst *** 1053,1060 /* * If pass-by-ref datatype, must copy the new value into aggcontext and ! * pfree the prior transValue. But if the combine function returned a ! * pointer to its first input, we don't need to do anything. */ if (!pertrans->transtypeByVal && DatumGetPointer(newVal) != DatumGetPointer(pergroupstate->transValue)) --- 1068,1078 /* * If pass-by-ref datatype, must copy the new value into aggcontext and ! * free the prior transValue. But if the combine function returned a ! * pointer to its first input, we don't need to do anything. Also, if the ! * combine function returned a pointer to a R/W expanded object that is ! * already a child of the aggcontext, assume we can adopt that value ! * without copying it. */ if (!pertrans->transtypeByVal && DatumGetPointer(newVal) != DatumGetPointer(pergroupstate->transValue)) *** advance_combine_function(AggState *aggst *** 1062,1073 if (!fcinfo->isnull) { MemoryContextSwitchTo(aggstate->aggcontexts[aggstate->current_set]->ecxt_per_tuple_memory); ! newVal = datumCopy(newVal, ! pertrans->transtypeByVal, ! pertrans->transtypeLen); } if (!pergroupstate->transValueIsNull) ! pfree(DatumGetPointer(pergroupstate->transValue)); } pergroupstate->transValue = newVal; --- 1080,1104 if (!fcinfo->isnull) {
Re: [HACKERS] Speed of user-defined aggregates using array_append as transfn
On 10/28/2016 03:14 PM, Tom Lane wrote: Andrew Dunstan writes: My initial admittedly ugly thought was why not have a second append function that doesn't use expanded arrays? That won't get us out of the O(N^2) behavior. Also I don't see what's better about it than my suggestion of making array_append itself do that when called as an aggregate function. Probably nothing, I was just thinking out loud. That suggestion seems like the most obviously back-patchable solution. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Speed of user-defined aggregates using array_append as transfn
Andrew Dunstan writes: > My initial admittedly ugly thought was why not have a second append > function that doesn't use expanded arrays? That won't get us out of the O(N^2) behavior. Also I don't see what's better about it than my suggestion of making array_append itself do that when called as an aggregate function. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Speed of user-defined aggregates using array_append as transfn
On 10/28/2016 02:04 PM, Tom Lane wrote: Comments, better ideas? My initial admittedly ugly thought was why not have a second append function that doesn't use expanded arrays? cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers