Thread: Re: [HACKERS] [COMMITTERS] pgsql: Add new function dsa_allocate0.
On Thu, Feb 16, 2017 at 10:09 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Fri, Feb 17, 2017 at 12:03 PM, Thomas Munro > <thomas.munro@enterprisedb.com> wrote: >> On Fri, Feb 17, 2017 at 11:34 AM, Thomas Munro >> <thomas.munro@enterprisedb.com> wrote: >>> On Fri, Feb 17, 2017 at 7:02 AM, Robert Haas <rhaas@postgresql.org> 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
Robert Haas <robertmhaas@gmail.com> 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
On Sat, Feb 18, 2017 at 5:41 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Thomas Munro <thomas.munro@enterprisedb.com> writes:
> On Sat, Feb 18, 2017 at 5:41 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Robert Haas <robertmhaas@gmail.com> 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
On Sun, Feb 19, 2017 at 11:27 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Thomas Munro <thomas.munro@enterprisedb.com> writes: >> On Sat, Feb 18, 2017 at 5:41 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> Robert Haas <robertmhaas@gmail.com> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On Sun, Feb 19, 2017 at 12:27 PM, Thomas Munro <thomas.munro@enterprisedb.com> wrote: > On Sun, Feb 19, 2017 at 11:27 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Thomas Munro <thomas.munro@enterprisedb.com> writes: >>> On Sat, Feb 18, 2017 at 5:41 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>>> Robert Haas <robertmhaas@gmail.com> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On Sat, Feb 18, 2017 at 6:31 PM, Thomas Munro <thomas.munro@enterprisedb.com> 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