Re: The Axe list

2008/10/15 Greg Stark <greg(dot)stark(at)enterprisedb(dot)com>:
> Sorry for top posting - darn phone...
>
> 1) as I mentioned when I reviewed the patch in commitfest I don't see the
> point of the manual memory management. Palloc/realloc has just the same kind
> of doubling behaviour behind the scenes anyways. Just call realloc before
> adding every new element.

Does this work? If an aggregate returns a different pointer to the one
it was passed, nodeAgg.c will free the original - but if we've
actually realloc'ed it, won't this lead to a double free?

> 2) look at the aggregate for count() which avoids pallocing a new bigint
> using a similar trick. It protects against the bug you describe checking the
> fctxt to verify it's being called as an aggregate function and not a regular
> function. So as long as the aggregate has the right initial state it should
> be fine.

The right initial state bit is what I was worried about, though I
think I've thought of a way around for now...

> Come to think of it though... Do we require creators of new aggregates own
> the state transition function? If not we have a problem...

No idea...

Ian

> greg
>
> On 15 Oct 2008, at 07:08 PM, "Ian Caulfield" <ian(dot)caulfield(at)gmail(dot)com>
> wrote:
>
>> 2008/10/14 Robert Treat <xzilla(at)users(dot)sourceforge(dot)net>
>>>
>>> On Monday 13 October 2008 04:53:44 Markus Wanner wrote:
>>>
>>>> Having reviewed the last commit fest's intagg patch as well, I thought
>>>> we agreed that a more general functionality is wanted for core. But as
>>>> long as we don't have that, I'd like intagg to stay in contrib.
>>>>
>>>
>>> While I agree that the "right" solution would be to make this code work
>>> more
>>> generally for other aggregates, I also think that until someone is
>>> willing to
>>> do that work, this needs to stay in contrib, and that we ought to accept
>>> patches improving it.
>>
>> I started to look at implementing array_agg by making the existing
>> intagg stuff more generic, but I came across an issue (which might
>> also be a bug in intagg). Intagg relies on being able to stomp on the
>> input transition value, and basically allocates the working array as a
>> block, reallocating it when it becomes too small. The lower bound of
>> the array is (ab)used to keep track of how many items have been
>> allocated.
>>
>> For a generic aggregate, which accepts any types and NULL inputs, in
>> order to avoid a linear search through the data to find where to put
>> the next element, it would be useful to instead store the offset to
>> the next free byte in the lower pointer.
>>
>> The problem is that I can't see any way to guarantee that someone
>> wouldn't create another aggregate using the same transition function,
>> but giving an initial value to the lower bound which will cause the
>> transition function to do naughty things (as far as I can tell, this
>> is also true of intagg - giving an inital state value of
>> '[200000:200000]{1}' will cause it to happily write up to 200000
>> integers off the end of that one element array without allocating any
>> extra storage...
>>
>> I'm not sure what the best way around this is - it seems that
>> performancewise, avoiding an array_seek() call in the transition
>> function would be good. Is there some way that the transition function
>> can tell which context the state value was allocated in, and thus
>> whether it was supplied as an initial value or not?
>>
>> Ian
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>