Re: Windowing Function Patch Review -> Standard Conformance

Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)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