add more frame types in window functions (ROWS)
in [PgSql]
|
From: Hitoshi Harada on 6 Dec 2009 20:04 2009/12/7 Andrew Gierth <andrew(a)tao11.riddles.org.uk>: >>>>>> "Hitoshi" == Hitoshi Harada <umi.tanuki(a)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(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Andrew Gierth on 6 Dec 2009 20:48 >>>>> "Hitoshi" == Hitoshi Harada <umi.tanuki(a)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(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Greg Smith on 6 Dec 2009 21:53 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 Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg(a)2ndQuadrant.com www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Hitoshi Harada on 6 Dec 2009 22:11 2009/12/7 Greg Smith <greg(a)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(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
First
|
Prev
|
Pages: 1 2 3 4 Prev: patch - per-tablespace random_page_cost/seq_page_cost Next: named parameters in SQL functions |