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

I gave this a quick look.  It took me a while to figure out how to apply
it -- turns out you used the "ignore whitespace" option to diff, so the
only way to apply it was with patch -p1 --ignore-whitespace.  Please
don't do that.

I ran pgindent over the patched code and there were a number of changes.
I suggest you run that over your local copy before your next submission,
to avoid the next official run to mangle your stuff in unforeseen ways.
For instance, the comment starting with "A slight edge case" would be
mangled; I suggest enclosing that in /*---- to avoid the problem.
(TBH that's the only interesting thing, but avoiding that kind of
breakage is worth it IMHO.)

First thing I noticed was the funky bms_initialize thingy.  There was
some controversy upthread about the use of bitmapsets, and it seems you
opted for not using them for the constant case as suggested by Jeff; but
apparently the other comment by Robert about the custom bms initializer
went largely ignored.  I agree with him that this is grotty.  However,
the current API to get partition-local memory is too limited as there's
no way to change to the partition's context; instead you only get the
option to allocate a certain amount of memory and return that.  I think
the easiest way to get around this problem is to create a new
windowapi.h function which returns the MemoryContext for the partition.
Then you can just allocate the BMS in that context.

But how do we ensure that the BMS is allocated in a context?  You'd have
to switch contexts each time you call bms_add_member.  I don't have a
good answer to this.  I used this code in another project:
   /*    * grow the "visited" bitmapset to the index' current size, to avoid    * repeated repalloc's    */   {
BlockNumber    lastblock;
 
       lastblock = RelationGetNumberOfBlocks(rel);       visited = bms_add_member(visited, lastblock);       visited =
bms_del_member(visited,lastblock);   }
 

This way, I know the bitmapset already has enough space for all the bits
I need and there will be no further allocation.  But this is also
grotty.  Maybe we should have a new entry point in bitmapset.h, say
"bms_grow" that ensures you have enough space for that many bits.  Or
perhaps add a MemoryContext member to struct Bitmapset, so that all
allocations occur therein.

I'm not too sure I follow the parse_agg.c changes, but don't you need to
free the clone in the branch that finds a duplicate window spec?  Is
this parent/child relationship thingy a preexisting concept, or are you
just coming up with it?  It seems a bit unfamiliar to me.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services