Neil Conway <[EMAIL PROTECTED]> writes:
> The attached patch invokes the transition function in the current memory
> context. I don't think that's right: a memory leak in an aggregate's
> transition function would be problematic when we're invoked from a
> per-query memory context, as is the case with advance_aggregates().
Agreed. You can't really assume that the transition function is
leak-free, particularly not when it's returning a pass-by-ref datatype.
> Perhaps we need an additional short-lived memory context in
> AggStatePerAggData: we could invoke the transition function in that
> context, and reset it once per, say, 1000 tuples.
This seems like it might work. Instead of copying the result into the
aggcontext on every cycle, we could copy it only when we intend to reset
the working context. However there is still a risk: the function might
return a pointer into the source tuple, not something that's in the
working context at all. (See text_smaller() as one example.) This is
problematic since the source tuple will go away on the next cycle.
The existing copying code takes care of this case as well as the one
where the result is in per_tuple_context.
I'm a bit surprised that you measured a significant performance speedup
from removing the copying step. Awhile ago I experimented with hacking
the count() aggregate function internally to avoid pallocs, and was
disappointed to measure no noticeable speedup at all. Digging in the
list archives, it looks like I neglected to report that failure, but
I do still have the patch and I attach it here. This was from late
Dec '03 so it'd be against just-past-7.4 sources. I'd be interested
to hear how this compares to what you did under your test conditions;
maybe I was using a bogus test case.
regards, tom lane
*** src/backend/executor/nodeAgg.c.orig Sat Nov 29 14:51:48 2003
--- src/backend/executor/nodeAgg.c Sun Dec 21 16:02:25 2003
***************
*** 350,356 ****
*/
/* MemSet(&fcinfo, 0, sizeof(fcinfo)); */
! fcinfo.context = NULL;
fcinfo.resultinfo = NULL;
fcinfo.isnull = false;
--- 350,356 ----
*/
/* MemSet(&fcinfo, 0, sizeof(fcinfo)); */
! fcinfo.context = (void *) aggstate;
fcinfo.resultinfo = NULL;
fcinfo.isnull = false;
***************
*** 528,533 ****
--- 528,534 ----
FunctionCallInfoData fcinfo;
MemSet(&fcinfo, 0, sizeof(fcinfo));
+ fcinfo.context = (void *) aggstate;
fcinfo.flinfo = &peraggstate->finalfn;
fcinfo.nargs = 1;
fcinfo.arg[0] = pergroupstate->transValue;
*** /home/postgres/pgsql/src/backend/utils/adt/int8.c.orig Mon Dec 1
17:29:19 2003
--- /home/postgres/pgsql/src/backend/utils/adt/int8.c Sun Dec 21 16:03:43 2003
***************
*** 17,22 ****
--- 17,23 ----
#include <math.h>
#include "libpq/pqformat.h"
+ #include "nodes/nodes.h"
#include "utils/int8.h"
***************
*** 565,573 ****
Datum
int8inc(PG_FUNCTION_ARGS)
{
! int64 arg = PG_GETARG_INT64(0);
! PG_RETURN_INT64(arg + 1);
}
Datum
--- 566,594 ----
Datum
int8inc(PG_FUNCTION_ARGS)
{
! if (fcinfo->context != NULL && IsA(fcinfo->context, AggState))
! {
! /*
! * Special case to avoid palloc overhead for COUNT(). Note:
this
! * assumes int8 is a pass-by-ref type; if we ever support
pass-by-val
! * int8, this should be ifdef'd out when int8 is pass-by-val.
! *
! * When called from nodeAgg, we know that the argument is
modifiable
! * local storage, so just update it in-place.
! */
! int64 *arg = (int64 *) PG_GETARG_POINTER(0);
!
! (*arg)++;
! PG_RETURN_POINTER(arg);
! }
! else
! {
! /* Not called by nodeAgg, so just do it the dumb way */
! int64 arg = PG_GETARG_INT64(0);
!
! PG_RETURN_INT64(arg + 1);
! }
}
Datum
---------------------------(end of broadcast)---------------------------
TIP 7: don't forget to increase your free space map settings
