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
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 |