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

> --- 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.
It could not be negative - subtruction is doing only around repalloc call, in 
all other places it only grows.

>
> 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.
It could be an int64 without any regressions or problems. I choose Size type 
because variable allocatedMemory actually stores a size of piece of memory and 
it's the same type as returned by GetMemoryChunkSpace()

AFAIK, size_t type (type Size is a just typedef alias) is a part of C99 and it 
should be unsigned and large enough to store size of any chunk of avaliable RAM. 
And it at least twice larger than allowed all memory-related GUC variables.

I don't see a reason to use int64 except, may be, general practice in pgsql. But 
then why we keep Size type - let us just use int64.


-- 
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
  WWW: http://www.sigaev.ru/