Re: Windowing Function Patch Review -> Standard Conformance
От | Tom Lane |
---|---|
Тема | Re: Windowing Function Patch Review -> Standard Conformance |
Дата | |
Msg-id | 23506.1227724677@sss.pgh.pa.us обсуждение исходный текст |
Ответ на | Re: Windowing Function Patch Review -> Standard Conformance (Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>) |
Ответы |
Re: Windowing Function Patch Review -> Standard Conformance
|
Список | pgsql-hackers |
Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes: > Here's another updated patch, including all your bug fixes. I did a very fast pass through this and have a few comments. * Don't bother manually updating keywords.sgml. As stated therein, that table is automatically generated. All you'll accomplish is to cause merge problems. (The effort would be a lot better spent on the non-boilerplate parts of the docs anyway.) * I assume there's going to be some way to create user-defined window functions? * It seems fairly unlikely that you can get away with not supporting any qual expression on a Window plan node. What will you do with HAVING qualifiers? * The find_aggref code added to planagg.c (where it doesn't belong anyway) doesn't seem to be used anywhere. * In the same vein, I'm unimpressed with moving GetAggInitVal into execGrouping.c, which it isn't at all related to. I'd just leave it alone and duplicate the code in nodeWindow.c --- it's not exactly large. If you did insist on sharing this code it would be appropriate to change the name and comments to reflect the fact that it's being used for more than just aggregates, anyhow. * And in the same vein. var.c is hardly the place to put a search-for-wfuncs routine. * It seems like a coin was flipped to determine whether struct and field names would use "window", "win", or just "w" (I find "WFunc" to be particularly unhelpful to a reader who doesn't already know what it is). Please try to reduce the surprise factor. I'd suggest consistently using "window" in type names, though "win" is an OK prefix for field names within window-related structs. * This is a bad idea: /* + * OrderClause - + * representation of ORDER BY in Window + */ + typedef SortGroupClause OrderClause; + + + /* + * PartitionClause - + * representaition of PATITION BY in Window + */ + typedef SortGroupClause PartitionClause; If they're just SortGroupClauses, call them that, don't invent an alias. (Yes, I know SortClause and GroupClause used to be aliases. That was a bad idea: it confused matters and required lots of useless duplicated code, except for the places where we didn't duplicate code because we were willing to assume struct equivalence. There's basically just nothing that wins about that approach.) In any case, "order" and "partition" are really bad names to be using here given the number of possible other meanings for those terms in a DBMS context. If you actually need separate struct types then names like WindowPartitionClause would be appropriate. * The API changes chosen for func_get_detail seem pretty bizarre. Why didn't you just add a new return code FUNCDETAIL_WINDOW? * The node support needs to be gone over more closely. I noticed just in random checking that WFunc is missing parse-location support in nodeFuncs.c and the Query.hasWindow field got added to copyfuncs, outfuncs, readfuncs, but not equalfuncs. * Please heed the comment at the top of parallel_schedule about the max number of tests per parallel group. * I don't find the test added to opr_sanity.sql to be particularly sane. We *will* have the ability to add window functions. But it might be helpful to check that proisagg and proiswfunc aren't both set. * errcodes.h is not the only place that has to be touched to add a new error code --- see also sgml/errcodes.sgml, plpgsql/src/plerrcodes.h. And what is your precedent for using 42813? I don't see that in the standard. If it's coming from DB2 it's okay, otherwise we should be using a private 'P' code. * Please try to eliminate random whitespace changes from the patch ... *particularly* in files that otherwise wouldn't be touched at all (there are at least two cases of that in this patch) That's all I have time for right now ... more to come no doubt. regards, tom lane
В списке pgsql-hackers по дате отправления:
Следующее
От: "Merlin Moncure"Дата:
Сообщение: Re: [bugfix] DISCARD ALL does not release advisory locks