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....@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
