diff --git a/doc/src/sgml/xaggr.sgml b/doc/src/sgml/xaggr.sgml index ef7cff4879..e91f400403 100644 --- a/doc/src/sgml/xaggr.sgml +++ b/doc/src/sgml/xaggr.sgml @@ -530,7 +530,7 @@ if (AggCheckCallContext(fcinfo, NULL)) 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. + 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 @@ -538,6 +538,20 @@ if (AggCheckCallContext(fcinfo, NULL)) re-executed on the same final state value.) + + 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 state 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 diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c index 4c82176671..fa26bd8c35 100644 --- a/src/backend/executor/nodeAgg.c +++ b/src/backend/executor/nodeAgg.c @@ -72,10 +72,13 @@ * 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. + * 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), @@ -694,8 +697,10 @@ advance_transition_function(AggState *aggstate, /* * 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. + * 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 (!peraggstate->transtypeByVal && DatumGetPointer(newVal) != DatumGetPointer(pergroupstate->transValue)) @@ -703,12 +708,25 @@ advance_transition_function(AggState *aggstate, if (!fcinfo->isnull) { MemoryContextSwitchTo(aggstate->aggcontexts[aggstate->current_set]->ecxt_per_tuple_memory); - newVal = datumCopy(newVal, - peraggstate->transtypeByVal, - peraggstate->transtypeLen); + if (DatumIsReadWriteExpandedObject(newVal, + false, + peraggstate->transtypeLen) && + MemoryContextGetParent(DatumGetEOHP(newVal)->eoh_context) == CurrentMemoryContext) + /* do nothing */ ; + else + newVal = datumCopy(newVal, + peraggstate->transtypeByVal, + peraggstate->transtypeLen); } if (!pergroupstate->transValueIsNull) - pfree(DatumGetPointer(pergroupstate->transValue)); + { + if (DatumIsReadWriteExpandedObject(pergroupstate->transValue, + false, + peraggstate->transtypeLen)) + DeleteExpandedObject(pergroupstate->transValue); + else + pfree(DatumGetPointer(pergroupstate->transValue)); + } } pergroupstate->transValue = newVal; @@ -1059,7 +1077,9 @@ finalize_aggregate(AggState *aggstate, (void *) aggstate, NULL); /* Fill in the transition state value */ - fcinfo.arg[0] = pergroupstate->transValue; + fcinfo.arg[0] = MakeExpandedObjectReadOnly(pergroupstate->transValue, + pergroupstate->transValueIsNull, + peraggstate->transtypeLen); fcinfo.argnull[0] = pergroupstate->transValueIsNull; anynull |= pergroupstate->transValueIsNull; @@ -1086,6 +1106,7 @@ finalize_aggregate(AggState *aggstate, } else { + /* Don't need MakeExpandedObjectReadOnly; datumCopy will copy it */ *resultVal = pergroupstate->transValue; *resultIsNull = pergroupstate->transValueIsNull; } diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c index ecf96f8c19..a405a31f18 100644 --- a/src/backend/executor/nodeWindowAgg.c +++ b/src/backend/executor/nodeWindowAgg.c @@ -362,8 +362,10 @@ advance_windowaggregate(WindowAggState *winstate, /* * 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. + * 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 (!peraggstate->transtypeByVal && DatumGetPointer(newVal) != DatumGetPointer(peraggstate->transValue)) @@ -371,12 +373,25 @@ advance_windowaggregate(WindowAggState *winstate, if (!fcinfo->isnull) { MemoryContextSwitchTo(peraggstate->aggcontext); - newVal = datumCopy(newVal, - peraggstate->transtypeByVal, - peraggstate->transtypeLen); + if (DatumIsReadWriteExpandedObject(newVal, + false, + peraggstate->transtypeLen) && + MemoryContextGetParent(DatumGetEOHP(newVal)->eoh_context) == CurrentMemoryContext) + /* do nothing */ ; + else + newVal = datumCopy(newVal, + peraggstate->transtypeByVal, + peraggstate->transtypeLen); } if (!peraggstate->transValueIsNull) - pfree(DatumGetPointer(peraggstate->transValue)); + { + if (DatumIsReadWriteExpandedObject(peraggstate->transValue, + false, + peraggstate->transtypeLen)) + DeleteExpandedObject(peraggstate->transValue); + else + pfree(DatumGetPointer(peraggstate->transValue)); + } } MemoryContextSwitchTo(oldContext); @@ -513,8 +528,10 @@ advance_windowaggregate_base(WindowAggState *winstate, /* * If pass-by-ref datatype, must copy the new value into aggcontext and - * pfree the prior transValue. But if invtransfn returned a pointer to - * its first input, we don't need to do anything. + * free the prior transValue. But if invtransfn returned a pointer to its + * first input, we don't need to do anything. Also, if invtransfn + * 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. * * Note: the checks for null values here will never fire, but it seems * best to have this stanza look just like advance_windowaggregate. @@ -525,12 +542,25 @@ advance_windowaggregate_base(WindowAggState *winstate, if (!fcinfo->isnull) { MemoryContextSwitchTo(peraggstate->aggcontext); - newVal = datumCopy(newVal, - peraggstate->transtypeByVal, - peraggstate->transtypeLen); + if (DatumIsReadWriteExpandedObject(newVal, + false, + peraggstate->transtypeLen) && + MemoryContextGetParent(DatumGetEOHP(newVal)->eoh_context) == CurrentMemoryContext) + /* do nothing */ ; + else + newVal = datumCopy(newVal, + peraggstate->transtypeByVal, + peraggstate->transtypeLen); } if (!peraggstate->transValueIsNull) - pfree(DatumGetPointer(peraggstate->transValue)); + { + if (DatumIsReadWriteExpandedObject(peraggstate->transValue, + false, + peraggstate->transtypeLen)) + DeleteExpandedObject(peraggstate->transValue); + else + pfree(DatumGetPointer(peraggstate->transValue)); + } } MemoryContextSwitchTo(oldContext); @@ -568,7 +598,9 @@ finalize_windowaggregate(WindowAggState *winstate, numFinalArgs, perfuncstate->winCollation, (void *) winstate, NULL); - fcinfo.arg[0] = peraggstate->transValue; + fcinfo.arg[0] = MakeExpandedObjectReadOnly(peraggstate->transValue, + peraggstate->transValueIsNull, + peraggstate->transtypeLen); fcinfo.argnull[0] = peraggstate->transValueIsNull; anynull = peraggstate->transValueIsNull; @@ -596,6 +628,7 @@ finalize_windowaggregate(WindowAggState *winstate, } else { + /* Don't need MakeExpandedObjectReadOnly; datumCopy will copy it */ *result = peraggstate->transValue; *isnull = peraggstate->transValueIsNull; } diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 18a2b10c70..a5f4761011 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -558,6 +558,16 @@ count_agg_clauses_walker(Node *node, count_agg_clauses_context *context) /* Use average width if aggregate definition gave one */ if (aggtransspace > 0) avgwidth = aggtransspace; + else if (aggtransfn == F_ARRAY_APPEND) + { + /* + * If the transition function is array_append(), it'll use an + * expanded array as transvalue, which will occupy at least + * ALLOCSET_SMALL_INITSIZE and possibly more. Use that as the + * estimate for lack of a better idea. + */ + avgwidth = ALLOCSET_SMALL_INITSIZE; + } else { /* diff --git a/src/backend/utils/adt/array_userfuncs.c b/src/backend/utils/adt/array_userfuncs.c index d0e0dfdae9..58a176fe81 100644 --- a/src/backend/utils/adt/array_userfuncs.c +++ b/src/backend/utils/adt/array_userfuncs.c @@ -32,6 +32,10 @@ static Datum array_position_common(FunctionCallInfo fcinfo); * Caution: if the input is a read/write pointer, this returns the input * argument; so callers must be sure that their changes are "safe", that is * they cannot leave the array in a corrupt state. + * + * If we're being called as an aggregate function, make sure any newly-made + * expanded array is allocated in the aggregate state context, so as to save + * copying operations. */ static ExpandedArrayHeader * fetch_array_arg_replace_nulls(FunctionCallInfo fcinfo, int argno) @@ -39,6 +43,7 @@ fetch_array_arg_replace_nulls(FunctionCallInfo fcinfo, int argno) ExpandedArrayHeader *eah; Oid element_type; ArrayMetaState *my_extra; + MemoryContext resultcxt; /* If first time through, create datatype cache struct */ my_extra = (ArrayMetaState *) fcinfo->flinfo->fn_extra; @@ -51,10 +56,17 @@ fetch_array_arg_replace_nulls(FunctionCallInfo fcinfo, int argno) fcinfo->flinfo->fn_extra = my_extra; } + /* Figure out which context we want the result in */ + if (!AggCheckCallContext(fcinfo, &resultcxt)) + resultcxt = CurrentMemoryContext; + /* Now collect the array value */ if (!PG_ARGISNULL(argno)) { + MemoryContext oldcxt = MemoryContextSwitchTo(resultcxt); + eah = PG_GETARG_EXPANDED_ARRAYX(argno, my_extra); + MemoryContextSwitchTo(oldcxt); } else { @@ -72,7 +84,7 @@ fetch_array_arg_replace_nulls(FunctionCallInfo fcinfo, int argno) errmsg("input data type is not an array"))); eah = construct_empty_expanded_array(element_type, - CurrentMemoryContext, + resultcxt, my_extra); }