Re: Windowing Function Patch Review -> Standard Conformance - Mailing list pgsql-hackers

I wrote:
> All,
>
> This is my first patch review for PostgreSQL. I did submit a patch last
> commit fest (Boyer-Moore) so I feel I should review one this commit fest.
> I'm quite new to PostgreSQL so please don't rely on me totally. I'll do my
> best. Heikki is also reviewing this patch which makes me feel better.
>
> My aim is to get the author has much feed back as quickly as possible.
> For this reason I'm going to be breaking down my reviews into the
> following topics.
>
> 1. Does patch apply cleanly?
>
> 2. Any compiler warnings?
>
> 3. Do the results follow the SQL standard?
>
> 4. Performance Comparison, does it perform better than alternate ways of
> doing things. Self joins, sub queries etc.
>
> 5. Performance, no comparison. How does it perform with larger tables?

I regret starting the individual thread for each function now. I was
expecting them to get longer. So I'll use this one for the remainder of the
standard conformance tests.

I covered all of the non-aggregate functions in previous tests. I will do
more final tests on them soon. Tonight I started testing the aggregate
functions with NULLable columns and I've found a small problem. I'll just
post my test script to make it easy for you to see what I mean.

BEGIN WORK;

CREATE TABLE auction_items ( auctionid INT NOT NULL, category VARCHAR(32) NOT NULL, description VARCHAR(128) NOT NULL,
highestbidINT NULL, -- NULL when no bids reserve INT NULL, -- NULL when no reserve started TIMESTAMP NOT NULL, -- When
theaction opened days INT NOT NULL, -- how many days the auction runs.  CHECK(days > 0), PRIMARY KEY (auctionid) 
);

INSERT INTO auction_items VALUES(1,'BIKE','Broken Bicycle',NULL,100,NOW(),10);
INSERT INTO auction_items VALUES(2,'BIKE','Mountain Bike',120,NULL,NOW(),10);
INSERT INTO auction_items VALUES(3,'BIKE','Racer',230,NULL,NOW(),7);

INSERT INTO auction_items VALUES(4,'CAR','Bmw M3',5000,6000,NOW(),7);
INSERT INTO auction_items VALUES(5,'CAR','Audi S3',NULL,6500,NOW(),7);
INSERT INTO auction_items VALUES(6,'CAR','Holden Kingswood',NULL,2000,NOW(),7);

COMMIT;

-- The following query gives incorrect results on the
-- maxhighbid column

SELECT auctionid,      category,      description,      highestbid,      reserve,      MAX(highestbid) OVER (ORDER BY
auctionid)AS maxhighbid,      MAX(reserve) OVER() AS highest_reserve 
FROM auction_items;

If you remove the highest_reserve column you get the correct results.

The bug also affects MIN, AVG, COUNT, STDDEV but not SUM.

I've also had a little look at the documentation included in the patch.
At first scan the only thing I can see that is wrong is

+
+    <para>
+     Window functions are put in the <command>SELECT</command> list.
+     It is forbidden anywhere else such as <literal>GROUP BY</literal>,

I think this needs to be rewritten as they are allowed in the ORDER BY
clause, works in patch and spec says the same.

Maybe it should read something more like:

"Window functions are only permitted in the <command>SELECT</command> and
the <command>ORDER BY</command> clause of the query. They are forbidden
anywhere else..." etc.

I won't be around until Monday evening (Tuesday morning JST). I'll pickup
the review again there. It's really time for me to push on with this review.
The patch is getting closer to getting the thumbs up from me. I'm really
hoping that can happen on Monday. Then it's over to Heikki for more code
feedback.

Keep up the good work.

David.