Re: [HACKERS] "Allow usage of huge maintenance_work_mem for GIN build" patch
Teodor Sigaev writes: >> 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. As long as we're certain of that, Size seems like the appropriate field type. But I wonder if it'd be worth adding an assert to the subtraction steps, ie Assert(accum->allocatedMemory >= delta); accum->allocatedMemory -= delta; regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] "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; - longallocatedMemory; + SizeallocatedMemory; 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: teo...@sigaev.ru WWW: http://www.sigaev.ru/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] "Allow usage of huge maintenance_work_mem for GIN build" patch
On Thu, May 12, 2016 at 11:33:24AM -0400, Robert Haas wrote: > On Fri, May 6, 2016 at 7:58 PM, Peter Geoghegan 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; > > - longallocatedMemory; > > + SizeallocatedMemory; > > 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. [This is a generic notification.] The above-described topic is currently a PostgreSQL 9.6 open item. Teodor, since you committed the patch believed to have created it, you own this open item. If some other commit is more relevant or if this does not belong as a 9.6 open item, please let us know. Otherwise, please observe the policy on open item ownership[1] and send a status update within 72 hours of this message. Include a date for your subsequent status update. Testers may discover new open items at any time, and I want to plan to get them all fixed well in advance of shipping 9.6rc1. Consequently, I will appreciate your efforts toward speedy resolution. Thanks. [1] http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] "Allow usage of huge maintenance_work_mem for GIN build" patch
On Fri, May 6, 2016 at 7:58 PM, Peter Geoghegan 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; > - longallocatedMemory; > + SizeallocatedMemory; > 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] "Allow usage of huge maintenance_work_mem for GIN build" patch
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; - longallocatedMemory; + SizeallocatedMemory; 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers