Re: Windowing Function Patch Review -> Standard Conformance

2008/11/25 Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>:
> Hitoshi Harada wrote:
>>
>> If you keep this in your mind, please don't be annoyed but the current
>> frame concept is wrong.
>>
>> ...
>>
>> Note that on empno=4 then last_value=4(yours)/3(mine), which means the
>> frame is applied to last_value() as well as nth_value() and
>> first_value(). Not only to aggregates. So these lines in nodeWindow.c,
>
> Yes, you're right.
>
> Hmm, I wonder if we should implement first_value, last_value and nth_value
> as regular aggregates. That way, all the window functions would operate on
> the partition, not caring about the frame, and all the aggregates would
> operate on the frame.

No way. Current specification that we don't get other frames than
with/without ORDER BY doesn't have conflict with your proposal.
However, thinking about the future, we decided to add window
aggregates with wfunc='t', with subfunc for shrinking frame
performance up. The direction we should head for is convert aggregates
to subset of window functions, not vice versa.


>> Then window_gettupleslot looks like killing performance in some cases.
>> What if the partition has millions of rows and wfunc1 seeks the head
>> and wfunc2 seeks the tail?
>
> Yeah, we probably should do something about that. You used several different
> read pointers to the tuplestore in your patch, one for frame head, another
> for frame tail, another for partition head etc., but I think that
> potentially suffers from the same problem. Perhaps we should have one read
> pointer for each window function, plus one reading the current row? It seems
> likely that one window function is not going to hop around wildly, and the
> behavior wouldn't depend so much on what other window functions are being
> used.

Yes. My patch also has performance problem in seeking but is better
than one read pointer. So I like your proposal to attach a read
pointer with each wfuncs. I am not sure what kind of window functions
the user will define in the future, but for current use cases it comes
reasonable. Anyway, only one read pointer will make problems, I guess.

>> So as you point it is possible to define frame and partition while
>> scanning current rows rather than before scanning, I felt it'd cost
>> more. But I know this is work in progress, so you probably think about
>> the solutions.  What do you think?
>
> Here's an updated patch, where the rows are fetched on-demand.

Good! And I like the fetching args by number better. Let me take more
time to look in detail...

Regards,


-- 
Hitoshi Harada