Re: The Axe list
От | Ian Caulfield |
---|---|
Тема | Re: The Axe list |
Дата | |
Msg-id | 27bbfebe0810151418j1b567baah5c41d279aace75d0@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: The Axe list (Greg Stark <greg.stark@enterprisedb.com>) |
Список | pgsql-hackers |
2008/10/15 Greg Stark <greg.stark@enterprisedb.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.caulfield@gmail.com> > wrote: > >> 2008/10/14 Robert Treat <xzilla@users.sourceforge.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@postgresql.org) >> To make changes to your subscription: >> http://www.postgresql.org/mailpref/pgsql-hackers >
В списке pgsql-hackers по дате отправления: