Re: Windowing Function Patch Review -> Standard Conformance

From: Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>
To: Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com>
Cc: David Rowley <dgrowley(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Windowing Function Patch Review -> Standard Conformance
Date: 2008-11-24 11:21:47
Message-ID: 492A8E4B.4050409@enterprisedb.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hitoshi Harada wrote:
> 2008/11/20 David Rowley <dgrowley(at)gmail(dot)com>:
>> 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.
>
> This time I only fixed some bugs and rebased against the HEAD, but did
> not refactored. I can figure out some part of what Heikki claimed but
> not all, so I'd like to see what he will send and post another patch
> if needed.

Thanks! Here's what I've got this far I merged your latest patch into
this, as well as latest changes from CVS HEAD.

It's a bit of a mess, but here's the big changes I've done:
- Removed all the iterator stuff. You can just call
WinGetPart/FrameGetArg repeatedly. It ought to be just as fast if done
right in nodeWindow.c.
- Removed all the Shrinked/Extended stuff, as it's not needed until we
have full support for window frames.
- Removed all the WinRow* functions, you can just call WinFrame/PartGet*
functions, using WINDOW_SEEK_CURRENT
- Removed WinFrame/PartGetTuple functions. They were only used for
determining if two tuples are peer with each other, so I added a
WinRowIsPeer function instead.
- Removed all the buffering strategy stuff. Currently, the whole
partition is always materialized. That's of course not optimal; I'm
still thinking we should just read the tuples from the outer node
lazily, on-demand, instead. To avoid keeping all tuples in the partition
in tuplestore, when not needed, should add an extra function to trim the
tuplestore.

There's now a lot less code in nodeWindow.c, and I'm starting to
understand most of what-s left :-).

TODO
- clean up the comments and other mess.
- Modify WinPart/FrameGetArg so that it's passed the parameter number
instead of an Expr node.

I'll continue working on the executor, but please let me know what you
think.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Eisentraut 2008-11-24 12:18:19 Re: Autoconf, libpq and replacement function
Previous Message Pavel Stehule 2008-11-24 11:16:33 Re: [Review] Grouping Sets patch