"Allow usage of huge maintenance_work_mem for GIN build" patch - Mailing list pgsql-hackers

From Peter Geoghegan
Subject "Allow usage of huge maintenance_work_mem for GIN build" patch
Date
Msg-id CAM3SWZRp+n50_xTi_KdUtgd8_84ui3oJwNGGP6kg3CRdK7mF+g@mail.gmail.com
Whole thread Raw
Responses Re: "Allow usage of huge maintenance_work_mem for GIN build" patch  (Robert Haas <robertmhaas@gmail.com>)
Re: "Allow usage of huge maintenance_work_mem for GIN build" patch  (Teodor Sigaev <teodor@sigaev.ru>)
List pgsql-hackers
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 GinEntryAccumulatortypedef 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.

Thanks
-- 
Peter Geoghegan



pgsql-hackers by date:

Previous
From: Tom Lane
Date:
Subject: First-draft release notes for next week's back-branch releases
Next
From: Thomas Munro
Date:
Subject: A population of population counts