Here's the rest of the review, as far as I've taken it given the
problems with the code.
The patch applied cleanly and includes regression tests but not docs.
Small nitpicks: there are some comments not updated (e.g. the
big one at the start of eval_windowaggregates). A couple of lines are
commented-out using C++ comments.
The overall approach seems ok, and the parser stuff seems fine to me.
These are the issues I've found that make it not committable in its
present form (including the ones I mentioned before):
- missing _readWindowFrameDef function (all nodes that are output
from parse analysis must have both _read and _out functions,
otherwise views can't work)
- the A_Const nodes should probably be transformed to Const nodes in
parse analysis, since A_Const has no _read/_out functions, which
means changing the corresponding code in the executor.
- ruleutils.c not updated to deparse the newly added window options
- leaks memory like it's going out of style
The memory leakage is caused by not resetting any memory contexts when
throwing away all the aggregate state when advancing the start of the
window frame. This looks like it will require a rethink of the memory
management being used; it's not enough just to pfree copies of the
transition values (which you don't appear to be doing), you have to
reset the memory context that was exposed to the transition functions
via context->wincontext. So the current setup of a single long-lived
context won't work; you'll need a long-lived one, plus an additional
one that you can reset any time the aggregates need to be
re-initialized. (And if you're not going to break existing aggregate
functions, WindowAggState.wincontext needs to be the one that gets
reset.)
Tests for memory leaks:
-- tests for failure to free by-ref transition values
select count(*)
from (select i,max(repeat(i::text,100)) over (order by i rows between 1
preceding and current row)
from generate_series(1,1000000) i) s;
-- tests for failure to reset memory context on window advance
select count(*)
from (select i,array_agg(i) over (order by i rows between 1 preceding and
current row)
from generate_series(1,1000000) i) s;
--
Andrew (irc:RhodiumToad)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
