Thread: 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_mallocpg_malloc0 (for malloc-and-zero behavior)pg_calloc (randomly different API for
pg_malloc0)pg_reallocpg_freepg_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@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +
Andres Freund <andres@2ndquadrant.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@sss.pgh.pa.us> wrote: > Andres Freund <andres@2ndquadrant.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@postgresql.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@2ndquadrant.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@gmx.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@sss.pgh.pa.us> wrote: > Peter Eisentraut <peter_e@gmx.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