Re: review: More frame options in window functions

Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> writes:
> [ more_frame_options patch ]

Committed after rather extensive revisions.

I removed the RANGE value PRECEDING/FOLLOWING support as per discussion,
which was probably a good thing anyway from an incremental-development
standpoint; it made it easier to see what was going on in nodeWindowAgg.

I'm not terribly happy with the changes you made in WinGetFuncArgInPartition
and WinGetFuncArgInFrame to force the window function mark to not go
past frame start in some modes. Not only is that pretty ugly, but I
think it can mask bugs in window functions: it's an error for a window
function to fetch a row before what it has set its mark to be, but in
some cases that wouldn't be detected because of this change. I think
it would be better to revert those changes and find another method of
protecting fetches needed to determine the frame head. One idea is
to create a separate read pointer that tracks the frame head whenever
actual fetches of the frame head might be needed by update_frameheadpos.
I committed it without changing that, but I think this should be
revisited before trying to add the RANGE value PRECEDING/FOLLOWING
options, because those will substantially expand the number of cases
where that hack affects the behavior.

I found one actual bug, which was indeed exposed by the submitted
regression tests:

SELECT sum(unique1) over (rows between 1 following and 3 following),
unique1, four
FROM tenk1 WHERE unique1 < 10;
sum | unique1 | four
-----+---------+------
9 | 4 | 0
16 | 2 | 2
23 | 1 | 1
22 | 6 | 2
16 | 9 | 1
15 | 8 | 0
10 | 5 | 1
7 | 3 | 3
0 | 7 | 3
0 | 0 | 0
(10 rows)

The last row's SUM ought to be null because there are no rows left in
the frame. The cause of the bug was that update_frameheadpos was
forcing frameheadpos to not go past the last partition row, but this
has to be allowed so that the frame can be empty at need.

regards, tom lane