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

I've attached another iteration of the lead-lag patch.

> I suggest you run that over your local copy before your next submission

I ran pgindent before generating my patch (without -w this time), and
I've got a few more whitespace differences in the files that I
touched. I hope that hasn't added too much noise.

> I suggest enclosing that in /*---- to avoid the problem.

Done

> create a new windowapi.h function which returns the MemoryContext for the partition
...
> But even if we did decide to switch memory contexts on every call, it would still be much cleaner than this.

I've removed all the bms_initalize code from the patch and am using
this solution. As the partition memory is zero-initialised I just
store a Bitmapset pointer in the WinGetPartitionLocalMemory. The
bms_add_member and bms_is_member functions behave sensibly for
null-pointer inputs (they return a bms_make_singleton in the current
memory context and false respectively). I've surrounded the calls to
bms_make_singleton with a memory context switch (to the partition's
context) so the Bitmapset stays in the partition's context.

> Maybe we should have a new entry point in bitmapset.h, say "bms_grow" that ensures you have enough space for that many bits

This would be useful, as currently n additions require O(n) repallocs,
especially as I'm iterating through the indices in ascending order.
However, I'd rather "cheat" as I know the number of bits I'll need up
front; I can just set the (n+1)-th bit to force a single repalloc to
the final size. It's worth noting that other Bitmap implementations
(e.g. Java's java.util.BitSet) try to minimise re-allocations by
increasing the size to (e.g.) Max(2 * current size, n) if a re-size is
needed.

> but don't you need to free the clone in the branch that finds a duplicate window spec?

Good catch - I've fixed that

> Is this parent/child relationship thingy a preexisting concept

Yes, although it's not very well documented. I've added a lot of
documentation to the WindowDef struct in
src/include/nodes/parsenodes.h to explain which of the struct's
members use this mechanism. The WindowDef is very much like an object
in a higher-level language, where some of the members are 'virtual',
so use the parent's version if they don't have a value, and some
members are 'final', so values in this member in child WindowDefs are
ignore (i.e. the parent WindowDef's value is always used). I don't
think this degree of complexity is necessary for the lead-lag patch
alone, but since it was there I decided to take advantage of it.

Thanks -

Nick