"Allow usage of huge maintenance_work_mem for GIN build" patch

From: Peter Geoghegan <pg(at)heroku(dot)com>
To: Teodor Sigaev <teodor(at)sigaev(dot)ru>
Cc: Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: "Allow usage of huge maintenance_work_mem for GIN build" patch
Date: 2016-05-06 23:58:39
Message-ID: CAM3SWZRp+n50_xTi_KdUtgd8_84ui3oJwNGGP6kg3CRdK7mF+g@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: 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 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.

Thanks
--
Peter Geoghegan

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Thomas Munro 2016-05-07 00:41:15 A population of population counts
Previous Message Tom Lane 2016-05-06 23:47:28 First-draft release notes for next week's back-branch releases