Re: "Allow usage of huge maintenance_work_mem for GIN build" patch

On Fri, May 6, 2016 at 7:58 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> I noticed that commit 30bb26b5 ("Allow usage of huge
> maintenance_work_mem for GIN build") made the following modification:
>
> --- a/src/include/access/gin_private.h
> +++ b/src/include/access/gin_private.h
> @@ -903,7 +903,7 @@ typedef struct GinEntryAccumulator
> typedef struct
> {
> GinState *ginstate;
> - long allocatedMemory;
> + Size allocatedMemory;
> GinEntryAccumulator *entryallocator;
> uint32 eas_used;
> RBTree *tree;
>
> Are you sure this is safe, Teodor? I don't have time to study the
> patch in detail, but offhand I think that it might have been better to
> make allocatedMemory of type int64, just like the tuplesort.c memory
> accounting variables are post-MaxAllocHuge. It's not obvious to me
> that this variable isn't allowed to occasionally become negative, just
> like in tuplesort.c. It looks like that *might* be true -- ginbulk.c
> may let allocatedMemory go negative for a period, which would now be
> broken.
>
> If you did make this exact error, you would not be the first. If it
> isn't actually broken, perhaps you should still make this change,
> simply on general principle. I'd like to hear other opinions on that,
> though.

I've added this to the open items list.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company