Re: xmalloc => pg_malloc

While looking around to fix the pg_malloc(0) issue, I noticed that
various other pieces of code such as pg_basebackup have essentially
identical functions, except they're called xmalloc(). I propose to
standardize all these things on this set of names:

pg_malloc
pg_malloc0 (for malloc-and-zero behavior)
pg_calloc (randomly different API for pg_malloc0)
pg_realloc
pg_free
pg_strdup

Any objections?

regards, tom lane

On Tuesday, October 02, 2012 06:02:13 PM Tom Lane wrote:
> While looking around to fix the pg_malloc(0) issue, I noticed that
> various other pieces of code such as pg_basebackup have essentially
> identical functions, except they're called xmalloc(). I propose to
> standardize all these things on this set of names:
>
> pg_malloc
> pg_malloc0 (for malloc-and-zero behavior)
> pg_calloc (randomly different API for pg_malloc0)
Do we need this?

> pg_realloc
> pg_free
> pg_strdup
I wonder whether the same set of functions should also be available in the
backend with ereport(EC_OUT_OF_MEMORY, ...) behaviour as well. As noted before
the are quite some malloc() calls around. Not all of them should be replaced,
but I think quite some could.

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Oct 2, 2012 at 12:02:13PM -0400, Tom Lane wrote:
> While looking around to fix the pg_malloc(0) issue, I noticed that
> various other pieces of code such as pg_basebackup have essentially
> identical functions, except they're called xmalloc(). I propose to
> standardize all these things on this set of names:
>
> pg_malloc
> pg_malloc0 (for malloc-and-zero behavior)
> pg_calloc (randomly different API for pg_malloc0)
> pg_realloc
> pg_free
> pg_strdup
>
> Any objections?

Please standarize. I am totally confused by the many memory
implementations we have. (Pg_upgrade uses pg_malloc().)

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

Andres Freund <andres(at)2ndquadrant(dot)com> writes:
>> pg_calloc (randomly different API for pg_malloc0)

> Do we need this?

I thought about getting rid of it, but there are some dozens of calls
scattered across several files, so I wasn't sure it was worth it.
Anybody else have an opinion?

> I wonder whether the same set of functions should also be available in the
> backend with ereport(EC_OUT_OF_MEMORY, ...) behaviour as well.

In the backend, you almost always ought to be using palloc instead.
The only places where it's really appropriate to be using malloc
directly are where you don't want an error thrown for out-of-memory.
So I think providing these in the backend would do little except to
encourage bad programming.

regards, tom lane

On Tue, Oct 2, 2012 at 12:30 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
>>> pg_calloc (randomly different API for pg_malloc0)
>
>> Do we need this?
>
> I thought about getting rid of it, but there are some dozens of calls
> scattered across several files, so I wasn't sure it was worth it.
> Anybody else have an opinion?

I think having more than 1 function that does the same thing is
generally a bad idea. It sounds like it is going to cause confusion
and provide no real benefit.

>
>> I wonder whether the same set of functions should also be available in the
>> backend with ereport(EC_OUT_OF_MEMORY, ...) behaviour as well.
>
> In the backend, you almost always ought to be using palloc instead.
> The only places where it's really appropriate to be using malloc
> directly are where you don't want an error thrown for out-of-memory.
> So I think providing these in the backend would do little except to
> encourage bad programming.
>
> regards, tom lane
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

On Tuesday, October 02, 2012 06:30:33 PM Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> >> pg_calloc (randomly different API for pg_malloc0)
> >
> > Do we need this?
>
> I thought about getting rid of it, but there are some dozens of calls
> scattered across several files, so I wasn't sure it was worth it.
I always found calloc to be a pointless API but by now I have learned what it
means so I don't have a strong opinion.

> > I wonder whether the same set of functions should also be available in
> > the backend with ereport(EC_OUT_OF_MEMORY, ...) behaviour as well.
>
> In the backend, you almost always ought to be using palloc instead.
> The only places where it's really appropriate to be using malloc
> directly are where you don't want an error thrown for out-of-memory.
> So I think providing these in the backend would do little except to
> encourage bad programming.
We seem to have 100+ usages of malloc() anyway. Several of those are in helper
libraries like regex/* though. A quick grep shows most of the others to be
valid in my opinion. Mostly its allocating memory which is never deallocated
but to big to unconditionally allocated.

The quick grep showed that at least src/backend/utils/misc/ps_status.c doesn't
properly check the return value. Others (mctx.c) use Asserts...

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, 2012-10-02 at 12:02 -0400, Tom Lane wrote:
> While looking around to fix the pg_malloc(0) issue, I noticed that
> various other pieces of code such as pg_basebackup have essentially
> identical functions, except they're called xmalloc(). I propose to
> standardize all these things on this set of names:
>
> pg_malloc
> pg_malloc0 (for malloc-and-zero behavior)
> pg_calloc (randomly different API for pg_malloc0)
> pg_realloc
> pg_free
> pg_strdup
>
> Any objections?

xmalloc, xstrdup, etc. are pretty common names for functions that do
alloc-or-die (another possible naming scheme ;-) ). The naming
pg_malloc etc. on the other hand suggests that the allocation is being
done in a PostgreSQL-specific way, and anyway sounds too close to
palloc.

So I'd be more in favor of xmalloc <= pg_malloc.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> xmalloc, xstrdup, etc. are pretty common names for functions that do
> alloc-or-die (another possible naming scheme ;-) ). The naming
> pg_malloc etc. on the other hand suggests that the allocation is being
> done in a PostgreSQL-specific way, and anyway sounds too close to
> palloc.

> So I'd be more in favor of xmalloc <= pg_malloc.

Meh. The fact that other people use that name is not really an
advantage from where I sit. I'm concerned about possible name
collisions, eg in libraries loaded into the backend.

There are probably not any actual risks of collision right now, given
that all these functions are currently in our client-side programs ---
but it's foreseeable that we might use this same naming convention in
more-exposed places in future. In fact, somebody was already proposing
creating such functions in the core backend.

But having said that, I'm not absolutely wedded to these names; they
were just the majority of existing cases.

regards, tom lane

On Wed, Oct 3, 2012 at 11:36 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>> xmalloc, xstrdup, etc. are pretty common names for functions that do
>> alloc-or-die (another possible naming scheme ;-) ). The naming
>> pg_malloc etc. on the other hand suggests that the allocation is being
>> done in a PostgreSQL-specific way, and anyway sounds too close to
>> palloc.
>
>> So I'd be more in favor of xmalloc <= pg_malloc.
>
> Meh. The fact that other people use that name is not really an
> advantage from where I sit. I'm concerned about possible name
> collisions, eg in libraries loaded into the backend.
>
> There are probably not any actual risks of collision right now, given
> that all these functions are currently in our client-side programs ---
> but it's foreseeable that we might use this same naming convention in
> more-exposed places in future. In fact, somebody was already proposing
> creating such functions in the core backend.
>
> But having said that, I'm not absolutely wedded to these names; they
> were just the majority of existing cases.

Why not split the difference and use pg_xmalloc?
As in: "PostgreSQL-special malloc that dies on failure."

--
Jon