Re: [HACKERS] add more frame types in window functions (ROWS)
Hitoshi == Hitoshi Harada umi.tan...@gmail.com writes: Hitoshi Attached is updated version. I added AggGetMemoryContext() Hitoshi in executor/nodeAgg.h (though I'm not sure where to go...) Hitoshi and its second argument iswindowagg is output parameter to Hitoshi know whether the call context is Agg or WindowAgg. Your Hitoshi proposal of APIs to know whether the function is called as Hitoshi Aggregate or not is also a candidate to be, but it seems out Hitoshi of this patch scope, so it doesn't touch anything. I don't really like the extra argument; aggregate functions should almost never have to care about whether they're being called as window functions rather than aggregate functions. And if it does care, I don't see why this is the appropriate function for it. At the very least the function should accept NULL for the iswindowagg pointer to avoid useless variables in the caller. So for this and the regression test problem mentioned in the other mail, I'm setting this back to waiting on author. -- Andrew. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] add more frame types in window functions (ROWS)
2009/12/7 Andrew Gierth and...@tao11.riddles.org.uk: Hitoshi == Hitoshi Harada umi.tan...@gmail.com writes: Hitoshi Attached is updated version. I added AggGetMemoryContext() Hitoshi in executor/nodeAgg.h (though I'm not sure where to go...) Hitoshi and its second argument iswindowagg is output parameter to Hitoshi know whether the call context is Agg or WindowAgg. Your Hitoshi proposal of APIs to know whether the function is called as Hitoshi Aggregate or not is also a candidate to be, but it seems out Hitoshi of this patch scope, so it doesn't touch anything. I don't really like the extra argument; aggregate functions should almost never have to care about whether they're being called as window functions rather than aggregate functions. And if it does care, I don't see why this is the appropriate function for it. At the very least the function should accept NULL for the iswindowagg pointer to avoid useless variables in the caller. I've just remembered such situation before, around here: http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/utils/adt/array_userfuncs.c.diff?r1=1.30;r2=1.31 Of course it is fixed now. Still thinking about error reporting or something, there should be a way to know which context you are called. OK, I agree iswindowagg can be nullable, though most isnull parameter cannot be NULL and forcing caller to put boolean stack value don't seem a hard work. So for this and the regression test problem mentioned in the other mail, I'm setting this back to waiting on author. In my humble opinion, view regression test is not necessary in both my patch and yours. At least window test has not been testing views so far since it was introduced. Am I missing? 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
Re: [HACKERS] add more frame types in window functions (ROWS)
Hitoshi == Hitoshi Harada umi.tan...@gmail.com writes: So for this and the regression test problem mentioned in the other mail, I'm setting this back to waiting on author. Hitoshi In my humble opinion, view regression test is not necessary Hitoshi in both my patch and yours. At least window test has not Hitoshi been testing views so far since it was introduced. Am I Hitoshi missing? If you don't want to include views in the regression test, then take them out; I will object to that, but I'll leave it up to the committer to decide whether it is appropriate, provided the other concerns are addressed. But what you have in the regression tests _now_ is, as far as I can tell, only working by chance; with rules and window being in the same parallel group, and window.sql creating a view, you have an obvious race condition, unless I'm missing something. I included view tests in my own patch for the very simple reason that I found actual bugs that way during development. -- 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
Re: [HACKERS] add more frame types in window functions (ROWS)
Andrew Gierth wrote: But what you have in the regression tests _now_ is, as far as I can tell, only working by chance; with rules and window being in the same parallel group, and window.sql creating a view, you have an obvious race condition, unless I'm missing something. You're right. rules.sql does this, among other things that might get impacted: SELECT viewname, definition FROM pg_views WHERE schemaname 'information_schema' ORDER BY viewname; Both rules and window are part of the same parallel group: test: select_views portals_p2 rules foreign_key cluster dependency guc bitmapops combocid tsearch tsdicts foreign_data window xmlmap Which means that views created in the window test could absolutely cause the rules test to fail given a bad race condition. Either rules or window needs to be moved to another section of the test schedule. (I guess you could cut down the scope of rules to avoid this particular problem, but hacking other people's regression tests to work around issues caused by yours is never good practice). I also agree with Andrew's sentiment that including a view on top of the new window implementations is good practice, just for general robustness. Also, while not a strict requirement, I personally hate seeing things with simple names used in the regression tests, like how v is used in this patch. The better written regression tests use a preface for all their names to decrease the chance of a conflict here; for example, the rules test names all of its views with names like rtest_v1 so they're very clearly not going to conflict with views created by other tests. No cleverness there can eliminate the conflict with rules though, when it's looking at every view in the system. It looks like a lot of progress has been made on this patch through its review. But there's enough open issues still that I think it could use a bit more time to mature before we try to get it committed--the fact that it's been getting bounced around for weeks now and the regression tests aren't even completely settled down yet is telling. The feature seems complete, useful, and functionally solid, but still in need of some general cleanup and re-testing afterwards. I'm going to mark this one Returned with Feedback for now. Please continue to work on knocking all these issues out, this should be a lot easier to get committed in our next CF. -- Greg Smith2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support g...@2ndquadrant.com www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] add more frame types in window functions (ROWS)
2009/12/7 Greg Smith g...@2ndquadrant.com: Which means that views created in the window test could absolutely cause the rules test to fail given a bad race condition. Either rules or window needs to be moved to another section of the test schedule. (I guess you could cut down the scope of rules to avoid this particular problem, but hacking other people's regression tests to work around issues caused by yours is never good practice). I also agree with Andrew's sentiment that including a view on top of the new window implementations is good practice, just for general robustness. I've agreed window and rules cannot be in the same group if window has view test. It looks like a lot of progress has been made on this patch through its review. But there's enough open issues still that I think it could use a bit more time to mature before we try to get it committed--the fact that it's been getting bounced around for weeks now and the regression tests aren't even completely settled down yet is telling. The feature seems complete, useful, and functionally solid, but still in need of some general cleanup and re-testing afterwards. I'm going to mark this one Returned with Feedback for now. Please continue to work on knocking all these issues out, this should be a lot easier to get committed in our next CF. OK, Andrew, thank you for collaborating on this. This fest made my patch to progress greatly. More comments from others on the memory leakage issue are welcome yet. 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
Re: [HACKERS] add more frame types in window functions (ROWS)
2009/12/5 Andrew Gierth and...@tao11.riddles.org.uk: 1) Memory context handling for aggregate calls aggcontext = AggGetMemoryContext(fcinfo-context); if (!aggcontext) ereport(...); For completeness, there should be two other functions: one to simply return whether we are in fact being called as an aggregate, and another one to return whether it's safe to scribble on the first argument (while it's currently the case that these two are equivalent, it would be good not to assume that). Comments? This is the most significant issue as I see it. Yep, I agree. The most essential point on this is that external functions refer to the struct members directly; we should provide kinds of API. (Also, a function in contrib/tsearch2 that accesses wincontext wasn't updated by the patch.) Thanks for noticing. I didn't know it. 2) Keywords The discussion is: http://archives.postgresql.org/message-id/e08cc0400911241703u4bf53ek1c3910605a3d8...@mail.gmail.com and ideas are extracted from Tom's mail in the last year just before committing the first window function patch, except for changing to COL_NAME_KEYWORD rather than RESERVED_KEYWORD as suggested. The reason I picked it up was only that it works. I cannot tell the side effect of COL_NAME_KEYWORD but I guess we tend to avoid RESERVED_KEYWORD as far as possible. And the reason BETWEEN cannot work as function name any more is based on bison's output shift/reduce conflict when SCONST follows BETWEEN in frame_extent. I don't have clear example that makes this happen, though. 3) Regression tests Testing that views work is OK as far as it goes, but I think that view definition should be left in place rather than dropped (possibly with even more variants) so that the deparse code gets properly tested too. (see the rules test) OK, 4) Deparse output The code is forcing explicit casting on the offset expressions, i.e. the deparsed code looks like ... ROWS BETWEEN 1::bigint PRECEDING AND 1::bigint FOLLOWING ... This looks a bit ugly; is it avoidable? At least for ROWS it should be, I think, since the type is known; even for RANGE, the type would be determined by the sort column. Hmm, I'll change it as LIMIT clause does. Pass false as showimplicit to get_rule_expr() maybe? 5) Documentation issues The entry for BETWEEN in the keywords appendix isn't updated. (Wouldn't it make more sense for this to be generated from the keyword list somehow?) I heard that you don't need to send keywords appendix in the patch because it is auto-generated, if I remember correctly. Spelling: - current row. In literalROWS/ mode this value means phisical row + current row. In literalROWS/ mode this value means physical row (this error appears twice) Oops, thanks. I'm now reworking as reviewed. The last issue is whether we accept extension of frame types without RANGE support. 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
Re: [HACKERS] add more frame types in window functions (ROWS)
2009/12/5 Hitoshi Harada umi.tan...@gmail.com: I'm now reworking as reviewed. The last issue is whether we accept extension of frame types without RANGE support. Attached is updated version. I added AggGetMemoryContext() in executor/nodeAgg.h (though I'm not sure where to go...) and its second argument iswindowagg is output parameter to know whether the call context is Agg or WindowAgg. Your proposal of APIs to know whether the function is called as Aggregate or not is also a candidate to be, but it seems out of this patch scope, so it doesn't touch anything. Fix tsearch function is also included, as well as typo phisical - physical. Pass false to get_rule_expr() of value in PRECEDING/FOLLOWING. One thing for rule test, I checked existing regression test cases and concluded DROP VIEW is necessary, or even VIEW test for a specific feature is not needed. I remember your aggregate ORDER BY patch contains rules test changes. However, since processing order of regression tests is not predictable and may change AFAIK, I guess it shouldn't add those changes in rules.out. Regards. -- Hitoshi Harada rows_frame_types.20091205.patch.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] add more frame types in window functions (ROWS)
Hitoshi == Hitoshi Harada umi.tan...@gmail.com writes: Hitoshi One thing for rule test, I checked existing regression test Hitoshi cases and concluded DROP VIEW is necessary, or even VIEW Hitoshi test for a specific feature is not needed. I remember your Hitoshi aggregate ORDER BY patch contains rules test Hitoshi changes. However, since processing order of regression tests Hitoshi is not predictable and may change AFAIK, I guess it Hitoshi shouldn't add those changes in rules.out. Actually, looking more closely, the way you have it currently works only by chance - rules and window are running in parallel, therefore the view creation in window can break the output of rules. The order of regression tests is set in parallel_schedule and serial_schedule; it's unpredictable only for tests within the same parallel group. I think a modification of the schedule is needed here; the only other option would be to move the view creation into a different test. -- Andrew. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] add more frame types in window functions (ROWS)
Functionally this patch looks excellent; correct format, applies cleanly, passes regression, and I've been unable to find any issues with the code itself. But I still have a concern over the interface change, so I'm setting this back to waiting on author for now even though it's really a matter for general discussion on -hackers. To take the outstanding issues in descending order of importance: 1) Memory context handling for aggregate calls Having thought about this carefully, I think the solution used in this patch has to be rejected for one specific reason: if you compile existing code (i.e. aggregates that use WindowAggState.wincontext) written for 8.4 against the patched server, the code compiles successfully and appears to run, but leaks memory at runtime. (And I've confirmed that there do exist external modules that would be affected.) If we're going to change the interface in this way, there should, IMO, be enough of a change that old code fails to compile; e.g. by renaming wincontext to partition_context or some equivalent change. But in my opinion we should go further than this: since there's obviously a need for aggregates to be able to get at a suitable memory context, I think we should formalize this and actually define some interface functions for them to use, so that something like if (fcinfo-context IsA(fcinfo-context, AggState)) aggcontext = ((AggState *) fcinfo-context)-aggcontext; else if (fcinfo-context IsA(fcinfo-context, WindowAggState)) aggcontext = ((WindowAggState *) fcinfo-context)-aggcontext; else ereport(...); becomes something like aggcontext = AggGetMemoryContext(fcinfo-context); if (!aggcontext) ereport(...); For completeness, there should be two other functions: one to simply return whether we are in fact being called as an aggregate, and another one to return whether it's safe to scribble on the first argument (while it's currently the case that these two are equivalent, it would be good not to assume that). Comments? This is the most significant issue as I see it. (Also, a function in contrib/tsearch2 that accesses wincontext wasn't updated by the patch.) 2) Keywords I didn't find any discussion of this previously; did I miss it? The patch changes BETWEEN from TYPE_FUNC_NAME_KEYWORD to COL_NAME_KEYWORD, so it's now allowed as a column name (which it previously wasn't), but now not allowed as a function. I get why it can't be a function now, but if it didn't work as a column name before, why does it now? (UNBOUNDED remains an unreserved word, and seems to behave in a reasonable fashion even if used as an upper-level var in the query; the interpretation of a bare UNBOUNDED has the standard behaviour as far as I can see.) 3) Regression tests Testing that views work is OK as far as it goes, but I think that view definition should be left in place rather than dropped (possibly with even more variants) so that the deparse code gets properly tested too. (see the rules test) 4) Deparse output The code is forcing explicit casting on the offset expressions, i.e. the deparsed code looks like ... ROWS BETWEEN 1::bigint PRECEDING AND 1::bigint FOLLOWING ... This looks a bit ugly; is it avoidable? At least for ROWS it should be, I think, since the type is known; even for RANGE, the type would be determined by the sort column. 5) Documentation issues The entry for BETWEEN in the keywords appendix isn't updated. (Wouldn't it make more sense for this to be generated from the keyword list somehow?) Spelling: - current row. In literalROWS/ mode this value means phisical row + current row. In literalROWS/ mode this value means physical row (this error appears twice) The doc could probably do with some wordsmithing but this probably shouldn't block the patch on its own; that's something that could be handled separately I guess. -- Andrew. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] add more frame types in window functions (ROWS)
Andrew Gierth and...@tao11.riddles.org.uk writes: If we're going to change the interface in this way, there should, IMO, be enough of a change that old code fails to compile; e.g. by renaming wincontext to partition_context or some equivalent change. Agreed --- if we have to break things, break them obviously not silently. I don't have time right now to think about this issue in detail, but if those are the alternatives I think the choice is clear. Quietly adding a memory leak to code that used to work well is not acceptable. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] add more frame types in window functions (ROWS)
Hitoshi == Hitoshi Harada umi.tan...@gmail.com writes: Hitoshi As earlier mail, I added aggcontext to WindowAggState. This issue (as detailed in this post): http://archives.postgresql.org/pgsql-hackers/2009-11/msg01871.php is currently the only significant outstanding issue in my review of this patch. I think we need to see more feedback on whether it is acceptable to change the aggregate function API again (and if so, what to do with it) before I can post a final review on this and mark it ready for committer (or not). -- Andrew. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] add more frame types in window functions (ROWS)
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,100) 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,100) 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
Re: [HACKERS] add more frame types in window functions (ROWS)
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
Re: [HACKERS] add more frame types in window functions (ROWS)
2009/11/15 Tom Lane t...@sss.pgh.pa.us: Andrew Gierth and...@tao11.riddles.org.uk writes: (A question here: the spec allows (by my reading) the use of parameters in the window frame clause, i.e. BETWEEN $1 PRECEDING AND $2 FOLLOWING. Wouldn't it therefore make more sense to treat the values as Exprs, albeit very limited ones, and eval them at startup rather than assuming we know the node type and digging down into it all over the place?) Seems like you might as well allow any expression not containing local Vars. Compare the handling of LIMIT. Hmm, I've read it wrong, was assuming a constant for unsigned value specification which actually includes any expression. But it's a fixed value during execution, right? Otherwise, we cannot predicate frame boundary. 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
Re: [HACKERS] add more frame types in window functions (ROWS)
Thanks for your review. 2009/11/15 Andrew Gierth and...@tao11.riddles.org.uk: Hi, I've started reviewing your patch. I've already found some things that need work: - missing _readWindowFrameDef function (all nodes that are output from parse analysis must have both _read and _out functions, otherwise views can't work) I added _outWindowFramedef() but seem to forget _read one. Will add it. - 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. Thanks for this comment. I hadn't determined which node should be used as a value node passed to executor. Including Tom's comment, I must consider which should be again. 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
Re: [HACKERS] add more frame types in window functions (ROWS)
Hitoshi == Hitoshi Harada umi.tan...@gmail.com writes: (A question here: the spec allows (by my reading) the use of parameters in the window frame clause, i.e. BETWEEN $1 PRECEDING AND $2 FOLLOWING. Wouldn't it therefore make more sense to treat the values as Exprs, albeit very limited ones, and eval them at startup rather than assuming we know the node type and digging down into it all over the place?) Seems like you might as well allow any expression not containing local Vars. Compare the handling of LIMIT. Hitoshi Hmm, I've read it wrong, was assuming a constant for unsigned value Hitoshi specification which actually includes any expression. But it's a Hitoshi fixed value during execution, right? Otherwise, we cannot predicate Hitoshi frame boundary. The spec doesn't allow arbitrary expressions there, only literals and parameters. Allowing more than that would be going beyond the spec, but as with LIMIT, could be useful nonetheless. For it to be a fixed value during execution, the same rules apply as for LIMIT; it can't contain Vars of the current query level. My thinking is that the executor definitely shouldn't be relying on it being a specific node type, but should just ExecEvalExpr it on the first call and store the result; then you don't have to know whether it's a Const or Param node or a more complex expression. -- Andrew. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] add more frame types in window functions (ROWS)
2009/11/15 Andrew Gierth and...@tao11.riddles.org.uk: My thinking is that the executor definitely shouldn't be relying on it being a specific node type, but should just ExecEvalExpr it on the first call and store the result; then you don't have to know whether it's a Const or Param node or a more complex expression. Yeah, so that we allow something like ROWS BETWEEN 1 + $1 PRECEDING AND ... And to support RANGE BETWEEN n PRECEDING ..., we should make datum to add or to subtract from current value on initial call anyway. 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
Re: [HACKERS] add more frame types in window functions (ROWS)
Hi, I've started reviewing your patch. I've already found some things that need work: - 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 question here: the spec allows (by my reading) the use of parameters in the window frame clause, i.e. BETWEEN $1 PRECEDING AND $2 FOLLOWING. Wouldn't it therefore make more sense to treat the values as Exprs, albeit very limited ones, and eval them at startup rather than assuming we know the node type and digging down into it all over the place?) You can see the problem here if you do this: create view v as select i,sum(i) over (order by i rows between 1 preceding and 1 following) from generate_series(1,10) i; select * from v; (A regression test for this would probably be good, which reminds me that I need to add one of those myself in the aggregate order by stuff.) Also, you're going to need to do some work in ruleutils.c (get_rule_windowspec) in order to be able to deparse your new frame definitions. I'll continue reviewing the stuff that does work, so I'm not marking this as waiting for author yet. -- 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
Re: [HACKERS] add more frame types in window functions (ROWS)
Andrew Gierth and...@tao11.riddles.org.uk writes: (A question here: the spec allows (by my reading) the use of parameters in the window frame clause, i.e. BETWEEN $1 PRECEDING AND $2 FOLLOWING. Wouldn't it therefore make more sense to treat the values as Exprs, albeit very limited ones, and eval them at startup rather than assuming we know the node type and digging down into it all over the place?) Seems like you might as well allow any expression not containing local Vars. Compare the handling of LIMIT. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
