Horton

Thread

Commits

  1. Make dsa_allocate interface more like MemoryContextAlloc.

  2. Add new function dsa_allocate0.

  1. pgsql: Add new function dsa_allocate0.

    Robert Haas <rhaas@postgresql.org> — 2017-02-16T18:02:51Z

    Add new function dsa_allocate0.

This does the same thing as dsa_allocate, except that the memory
is guaranteed to be zero-filled on return.

Dilip Kumar, adjusted by me.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/9acb85597f1223ac26a5b19a9345849c43d0ff54

Modified Files
--------------
src/backend/utils/mmgr/dsa.c | 16 ++++++++++++++++
src/include/utils/dsa.h      |  1 +
2 files changed, 17 insertions(+)

  2. Re: pgsql: Add new function dsa_allocate0.

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-02-16T22:34:45Z

    On Fri, Feb 17, 2017 at 7:02 AM, Robert Haas <rhaas@postgresql.org> wrote:
> Add new function dsa_allocate0.
>
> This does the same thing as dsa_allocate, except that the memory
> is guaranteed to be zero-filled on return.
>
> Dilip Kumar, adjusted by me.
>
> Branch
> ------
> master
>
> Details
> -------
> http://git.postgresql.org/pg/commitdiff/9acb85597f1223ac26a5b19a9345849c43d0ff54
>
> Modified Files
> --------------
> src/backend/utils/mmgr/dsa.c | 16 ++++++++++++++++
> src/include/utils/dsa.h      |  1 +
> 2 files changed, 17 insertions(+)

Hmm.  This will segfault if you're out of memory.

-- 
Thomas Munro
http://www.enterprisedb.com

  3. Re: pgsql: Add new function dsa_allocate0.

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-02-17T03:03:32Z

    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?

-- 
Thomas Munro
http://www.enterprisedb.com

  4. Re: pgsql: Add new function dsa_allocate0.

    Michael Paquier <michael.paquier@gmail.com> — 2017-02-17T03:09:20Z

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

  5. Re: [COMMITTERS] pgsql: Add new function dsa_allocate0.

    Robert Haas <robertmhaas@gmail.com> — 2017-02-17T13:17:17Z

    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

  6. Re: [COMMITTERS] pgsql: Add new function dsa_allocate0.

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-02-17T16:41:13Z

    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

  7. Re: [COMMITTERS] pgsql: Add new function dsa_allocate0.

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-02-18T21:06:41Z

    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

  8. Re: [COMMITTERS] pgsql: Add new function dsa_allocate0.

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-02-18T22:27:42Z

    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

  9. Re: [COMMITTERS] pgsql: Add new function dsa_allocate0.

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-02-18T23:27:59Z

    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

  10. Re: [COMMITTERS] pgsql: Add new function dsa_allocate0.

    Thomas Munro <thomas.munro@enterprisedb.com> — 2017-02-18T23:31:42Z

    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

  11. Re: [COMMITTERS] pgsql: Add new function dsa_allocate0.

    Robert Haas <robertmhaas@gmail.com> — 2017-02-19T08:34:46Z

    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