Re: Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls

On Mon, 2013-07-01 at 18:20 -0400, Nicholas White wrote:
> > pg_get_viewdef() needs to be updated
>
> Ah, good catch - I've fixed this in the attached. I also discovered
> that there's a parent-child hierarchy of WindowDefs (using
> relname->name), so instead of cloning the WindowDef (in parse_agg.c)
> if the frameOptions are different (e.g. by adding the ignore-nulls
> flag) I create a child of the WindowDef and override the frameOptions.
> This has the useful side-effect of making pg_get_viewdef work as
> expected (the previous iteration of the patch produced a copy of the
> window definintion, not the window name, as it was using a nameless
> clone), although the output has parentheses around the view name:
>
A couple comments:
* We shouldn't create an arbitrary number of duplicate windows when
many aggregates are specified with IGNORE NULLS.
* It's bad form to modify a list while iterating through it. This is
just a style issue because there's a break afterward, anyway.

Also, I'm concerned that we're changing a reference of the form:
OVER w
into:
OVER (w)
in a user-visible way. Is there a problem with having two windowdefs in
the p_windowdefs list with the same name and different frameOptions?

I think you could just change the matching criteria to be a matching
name and matching frameOptions. In the loop, if you find a matching name
but frameOptions doesn't match, keep a pointer to the windowdef and
create a new one at the end of the loop with the same name.

You'll have to be a little careful that any other code knows that names
can be duplicated in the list though.

Regards,
Jeff Davis