Re: [HACKERS] [COMMITTERS] pgsql: Add new function dsa_allocate0.
On Sat, Feb 18, 2017 at 6:31 PM, Thomas Munro wrote: >> Agreed. Here's a patch like that. > > Oops, that had a typo in a comment. Here's a better one. This looked fine, except that the new comment in dsa_allocate contained an extremely long sentence which happened to contradict the last sentence of the existing comment. Committed after frobbing the comment to fix those two issues. -- 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
Re: [HACKERS] [COMMITTERS] pgsql: Add new function dsa_allocate0.
On Sun, Feb 19, 2017 at 12:27 PM, Thomas Munro wrote: > On Sun, Feb 19, 2017 at 11:27 AM, Tom Lane wrote: >> Thomas Munro writes: >>> On Sat, Feb 18, 2017 at 5:41 AM, Tom Lane wrote: Robert Haas writes: > I'm thinking we should change this to look more like the > MemoryContextAlloc interface. >> +1 >> >>> Maybe something like the attached? I didn't add DSA_ALLOC_HUGE >>> because there is currently no limit on allocation size (other than the >>> limit on total size which you can set with dsa_set_size_limit, but >>> that causes allocation failure, not a separate kind of error). Should >>> there be a per-allocation size sanity check of 1GB like palloc? >> >> I think it's not a bad idea. It could help catch faulty allocation >> requests (since I'd bet very few call sites actually intend to allocate >> gigabytes in one go), and as Robert says, there is substantial value in >> the semantics being as much like palloc() as possible. People are >> likely to assume that even if it isn't true. > > Agreed. Here's a patch like that. Oops, that had a typo in a comment. Here's a better one. -- Thomas Munro http://www.enterprisedb.com dsa-extended-v3.patch Description: Binary data -- 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] [COMMITTERS] pgsql: Add new function dsa_allocate0.
On Sun, Feb 19, 2017 at 11:27 AM, Tom Lane wrote: > Thomas Munro writes: >> On Sat, Feb 18, 2017 at 5:41 AM, Tom Lane wrote: >>> Robert Haas writes: I'm thinking we should change this to look more like the MemoryContextAlloc interface. > >>> +1 > >> Maybe something like the attached? I didn't add DSA_ALLOC_HUGE >> because there is currently no limit on allocation size (other than the >> limit on total size which you can set with dsa_set_size_limit, but >> that causes allocation failure, not a separate kind of error). Should >> there be a per-allocation size sanity check of 1GB like palloc? > > I think it's not a bad idea. It could help catch faulty allocation > requests (since I'd bet very few call sites actually intend to allocate > gigabytes in one go), and as Robert says, there is substantial value in > the semantics being as much like palloc() as possible. People are > likely to assume that even if it isn't true. Agreed. Here's a patch like that. -- Thomas Munro http://www.enterprisedb.com dsa-extended-v2.patch Description: Binary data -- 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] [COMMITTERS] pgsql: Add new function dsa_allocate0.
Thomas Munro writes: > On Sat, Feb 18, 2017 at 5:41 AM, Tom Lane wrote: >> Robert Haas writes: >>> I'm thinking we should change this to look more like the >>> MemoryContextAlloc interface. >> +1 > Maybe something like the attached? I didn't add DSA_ALLOC_HUGE > because there is currently no limit on allocation size (other than the > limit on total size which you can set with dsa_set_size_limit, but > that causes allocation failure, not a separate kind of error). Should > there be a per-allocation size sanity check of 1GB like palloc? I think it's not a bad idea. It could help catch faulty allocation requests (since I'd bet very few call sites actually intend to allocate gigabytes in one go), and as Robert says, there is substantial value in the semantics being as much like palloc() as possible. People are likely to assume that even if it isn't true. 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] [COMMITTERS] pgsql: Add new function dsa_allocate0.
On Sat, Feb 18, 2017 at 5:41 AM, Tom Lane wrote: > Robert Haas writes: >> I'm thinking we should change this to look more like the >> MemoryContextAlloc interface. Let's have DSA_ALLOC_HUGE, >> DSA_ALLOC_NO_OOM, and DSA_ALLOC_ZERO, just like the corresponding >> MCXT_* flags, and a function dsa_allocate_extended() that takes a >> flags argument. Then, dsa_allocate(x,y) can be a macro for >> dsa_allocate_extended(x,y,0) and dsa_allocate0(x,y) can be a macro for >> dsa_allocate_extended(x,y,DSA_ALLOC_ZERO). What this goof on my (and >> Dilip's) part illustrates to me is that having this interface behave >> significantly differently from the MemoryContextAlloc interface is >> going to cause mistakes. > > +1 Maybe something like the attached? I didn't add DSA_ALLOC_HUGE because there is currently no limit on allocation size (other than the limit on total size which you can set with dsa_set_size_limit, but that causes allocation failure, not a separate kind of error). Should there be a per-allocation size sanity check of 1GB like palloc? -- Thomas Munro http://www.enterprisedb.com dsa-extended.patch Description: Binary data -- 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] [COMMITTERS] pgsql: Add new function dsa_allocate0.
Robert Haas writes: > I'm thinking we should change this to look more like the > MemoryContextAlloc interface. Let's have DSA_ALLOC_HUGE, > DSA_ALLOC_NO_OOM, and DSA_ALLOC_ZERO, just like the corresponding > MCXT_* flags, and a function dsa_allocate_extended() that takes a > flags argument. Then, dsa_allocate(x,y) can be a macro for > dsa_allocate_extended(x,y,0) and dsa_allocate0(x,y) can be a macro for > dsa_allocate_extended(x,y,DSA_ALLOC_ZERO). What this goof on my (and > Dilip's) part illustrates to me is that having this interface behave > significantly differently from the MemoryContextAlloc interface is > going to cause mistakes. +1 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] [COMMITTERS] pgsql: Add new function dsa_allocate0.
On Thu, Feb 16, 2017 at 10:09 PM, Michael Paquier wrote: > On Fri, Feb 17, 2017 at 12:03 PM, Thomas Munro > wrote: >> On Fri, Feb 17, 2017 at 11:34 AM, Thomas Munro >> wrote: >>> On Fri, Feb 17, 2017 at 7:02 AM, Robert Haas wrote: http://git.postgresql.org/pg/commitdiff/9acb85597f1223ac26a5b19a9345849c43d0ff54 >>> Hmm. This will segfault if you're out of memory. >> >> Or to provide a more useful response... maybe this should be like the >> attached? Or maybe people think that dsa_allocate should throw on >> failure to allocate, like palloc? > > dp = dsa_allocate(area, size); > -object = dsa_get_address(area, dp); > -memset(object, 0, size); > +if (DsaPointerIsValid(dp)) > +memset(dsa_get_address(area, dp), 0, size); > What you are proposing here looks like the right answer to me. Like > dsa_allocate, dsa_allocate0 should allow users to fallback to other > methods if what is returned is InvalidDsaPointer for consistency. I'm thinking we should change this to look more like the MemoryContextAlloc interface. Let's have DSA_ALLOC_HUGE, DSA_ALLOC_NO_OOM, and DSA_ALLOC_ZERO, just like the corresponding MCXT_* flags, and a function dsa_allocate_extended() that takes a flags argument. Then, dsa_allocate(x,y) can be a macro for dsa_allocate_extended(x,y,0) and dsa_allocate0(x,y) can be a macro for dsa_allocate_extended(x,y,DSA_ALLOC_ZERO). What this goof on my (and Dilip's) part illustrates to me is that having this interface behave significantly differently from the MemoryContextAlloc interface is going to cause mistakes. -- 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
