2009/11/19 Andrew Gierth <and...@tao11.riddles.org.uk>: > 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.
OK. It's tough for me to rewrite that big part of comment but I'll try it. > 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. A_Const/Const will be replace by Expr, to cover any expression without local Var. > > - 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.) Hmm, good point. Though I doubt we need two contexts for this because we have not so far (and we already have tmpcontext for that purpose), memory leakage probably seems to happen. I'll check it out. Thanks for your elaborate review anyway. All I was worried about is now clear. It will be lucky if I can update my patch until next week. So please keep it "Waiting on Author". 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
