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

On 29.09.2013 23:32, Nicholas White wrote:
>> bms_add_member() is an accident waiting to happen
>
> I've attached a patch that makes it use repalloc as suggested

You'll have to zero out the extended portion.

I tried to demonstrate that by setting RANDOMIZE_ALLOCATED_MEMORY, but
surprisingly regression tests still passed. I guess the regression suite
doesn't use wide enough bitmapsets to exercise that. But this causes an
assertion failure, with RANDOMIZE_ALLOCATED_MEMORY:

create table t (i int4);
select * from t as t1, t as t2, t as t3, t as t4, t as t5, t as t6, t as
t7, t as t8, t as t9, t as t10, t as t11, t as t12, t as t13, t as t14,
t as t15, t as t16, t as t17, t as t18, t as t19, t as t20, t as t21, t
as t22, t as t23, t as t24, t as t25, t as t26, t as t27, t as t28, t as
t29, t as t30, t as t31, t as t32, t as t33, t as t34, t as t35, t as
t36, t as t37, t as t38, t as t39, t as t40;

> - is it OK to commit separately? I'll address the lead-lag patch
> comments in the next couple of days. Thanks

Yep, thanks. I committed the attached.

After thinking about this some more, I realized that bms_add_member() is
still sensitive to CurrentMemoryContext, if the 'a' argument is NULL.
That's probably OK for the lag&lead patch - I didn't check - but if
we're going to start relying on the fact that bms_add_member() no longer
allocates a new bms in CurrentMemoryContext, that needs to be
documented. bitmapset.c currently doesn't say a word about memory contexts.

So what needs to be done next is to document how the functions in
bitmapset.c work wrt. memory contexts. Then double-check that the
behavior of all the other "recycling" bms functions is consistent. (At
least bms_add_members() needs a similar change).

- Heikki