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

On Fri, May 6, 2016 at 7:58 PM, Peter Geoghegan <pg@heroku.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