Now that I recognize the memory issue Andrew noted in his previous
review, I think we need to make a decision on this.
http://archives.postgresql.org/message-id/876397c68t.fsf@news-spur.riddles.org.uk
The problem is that for current eval_windowaggregate the node holds
wincontext to save the trans value, but since WindowAgg now supports
moving frame head the aggregates are initialized step-by-step as well
as the trans value should be discarded. This leads us to reset
wincontext multiple times within a partition. However, wincontext
saves partition local memory of individual window functions such like
rank() as well and if we reset wincontext on frame moving, rank() in
the same node doesn't return the correct answer.
So I added aggcontext to WindowAggState parallely to wincontext to
store aggregate trans values and to reset memory on frame moving, and
found that doesn't solve the problem I noted above, because
array_agg() uses winstate->wincontext in it directly.
Here the ways are:
1. Rewrite array_agg to use aggcontext instead of wincontext, or add
some kind of API such like GetWindowFrameContext() to avoid similar
future problem. This breaks array_agg() and I concern about the third
party aggregates that followed array_agg's behavior in 8.4. Change
twice?
2. Use wincontext as frame local context and add another
partition-level context to WindowAggState. Since rank() calls
WinGetPartitionLocalMemory(), this doesn't cause any friction in
array_agg and third parties. But the semantics of "wincontext" is
broken.
3. Cheat aggregate functions. Actually if we create aggcontext for
frame-level and set winstate->wincontext = winstate->aggcontext just
before calling state functions, problem is solved although the code
looks kludge. This is like below in advance_windowaggregate():/* * cheat aggregate functions to keep
backward-compatibility.*/saved_wincontext = winstate->wincontext;winstate->wincontext = winstate->aggcontext;
/* * OK to call the transition function */InitFunctionCallInfoData(*fcinfo, &(peraggstate->transfn),
numArguments + 1, (void *) winstate, NULL);fcinfo->arg[0] =
peraggstate->transValue;fcinfo->argnull[0]= peraggstate->transValueIsNull;newVal = FunctionCallInvoke(fcinfo);
winstate->wincontext = saved_wincontext;
My idea is 1. because 2. breaks "wincontext" semantics and somebody in
the future will be confused even though we leave comments and 3.
doesn't sound like production code. The only problem of 1. is third
party aggregate which I guess doesn't cause serious things anyway. Not
crash, nor return wrong answer. For 1. way, the ideal situation is
compiler error on building such custom aggregates for 8.5, which tells
the developer that the API had changed.
Comments?
Regards,
--
Hitoshi Harada