Re: WITHIN GROUP patch - Mailing list pgsql-hackers

I checked a conformance with ANSI SQL - and I didn't find any issue.

I found so following error message is not too friendly (mainly because this functionality will be new)

postgres=# select dense_rank(3,3,2) within group (order by num desc, odd) from test4;
ERROR:  Incorrect number of arguments for hypothetical set function
LINE 1: select dense_rank(3,3,2) within group (order by num desc, od...
               ^
postgres=# select dense_rank(3,3,2) within group (order by num desc) from test4;
ERROR:  Incorrect number of arguments for hypothetical set function
LINE 1: select dense_rank(3,3,2) within group (order by num desc) fr...
               ^
postgres=# select dense_rank(3,3) within group (order by num desc) from test4;
ERROR:  Incorrect number of arguments for hypothetical set function
LINE 1: select dense_rank(3,3) within group (order by num desc) from...
               ^
postgres=# select dense_rank(3,3) within group (order by num desc, num) from test4;
 dense_rank
------------
          3
(1 row)

Probably some hint should be there?

In addition to Pavel's review, I have finally finished reading the patch. Here are some notes, mainly on style:

First of all, it no longer compiles on HEAD because commit 4d212bac1752e1bad6f3aa6242061c393ae93a0a stole oid 3968.  I modified that locally to be able to continue my review.

Some of the error messages do not comply with project style.  That is, they begin with a capital letter.

Ordered set functions cannot have transition functions
Ordered set functions must have final functions
Invalid argument types for hypothetical set function
Invalid argument types for ordered set function
Incompatible change to aggregate definition
Too many arguments to ordered set function
Ordered set finalfns must not be strict
Cannot have multiple ORDER BY clauses with WITHIN GROUP
Cannot have DISTINCT and WITHIN GROUP together
Incorrect number of arguments for hypothetical set function
Incorrect number of direct arguments to ordered set function %s

And in pg_aggregate.c I found a comment with a similar problem that doesn't match its surrounding code:
Oid            transsortop = InvalidOid;  /* Can be omitted */

I didn't find any more examples like that, but I did see several block comments that weren't complete sentences whereas I think they should be.  Also a lot of the code comments say "I" and I don't recall seeing that elsewhere.  I may be wrong, but I would prefer if they were more neutral.

The documentation has a number of issues.

collateindex.pl complains of duplicated index entries for PERCENTILE CONTINUOUS and PERCENTILE DISCRETE.  This is because the index markup is used for both overloaded versions.  This is the same mistake Bruce made and then corrected in commit 5dcc48c2c76cf4b2b17c8e14fe3e588ae0c8eff3.

"if there are multiple equally good result" should have an s on the end in func.sgml.

Table 9-49 has an extra empty column.  That should either be removed, or filled in with some kind of comment text like other similar tables.

Apart from that, it looks good.  There is some mismatched coding styles in there but the next run of pgindent should catch them so it's no big deal.

I haven't yet exercised the actual functionality of the new functions, nor have I tried to create my own.  Andrew alerted me to a division by zero bug in one of them, so I'll be looking forward to catching that.

So, more review to come.

-- 
Vik