Re: Patch: propose to include 3 new functions into intarray and intagg

Markus Wanner <markus(at)bluegap(dot)ch> writes:

> Hi,
>
> Gregory Stark wrote:
>> Regarding the patch listed on the commitfest "3 new functions into intarray
>> and intagg" (which I just noticed has a reviewer listed -- doh):
>
> ..well, just add your name as well, no?

Yeah, people should feel free to comment on items even if other people have or
are as well. It would have just been more useful for me to pick one that
didn't already have someone reading it is all.

>> As far as detailed code commentary the only thing which jumps out at me is
>> that it's using MemoryContextAlloc to grow the array instead of repalloc which
>> seems like a waste. This isn't a new thing though, it was how intagg was
>> written and this patch just didn't change it.
>
> Oh, good catch.

Actually on further thought what's going on is that it's resizing the array by
doubling its size when necessary. palloc/repalloc does that as well, so you're
getting two layers of extra space to handle reallocations. I think it's
simpler and cleaner to just repalloc just enough space at each growth and let
repalloc handle allocating extra space to handle future growth. I think that
would be necessary for handling varlenas also.

> The naming 'bidx' seems a bit weired to me, but otherwise I'm also optimistic
> about it.

If we wanted to put that in core it would make more sense to have a flag on
the array indicating whether it's sorted or not which is maintained whenever
we construct or alter an array. Then just have the regular _int_contains()
(which is an operator @>) take advantage of it if it's set and the data type
is fixed-size.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's On-Demand Production Tuning