Re: nodeAgg perf tweak
От | Tom Lane |
---|---|
Тема | Re: nodeAgg perf tweak |
Дата | |
Msg-id | 18956.1101934451@sss.pgh.pa.us обсуждение исходный текст |
Ответ на | nodeAgg perf tweak (Neil Conway <neilc@samurai.com>) |
Ответы |
Re: nodeAgg perf tweak
|
Список | pgsql-hackers |
Neil Conway <neilc@samurai.com> 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
В списке pgsql-hackers по дате отправления: