pg_dump vs malloc

Magnus Hagander <magnus@hagander.net> writes:

I came across a situation today with a pretty bad crash of pg_dump,
due to not checking the return code from malloc(). When looking
through the code, it seems there are a *lot* of places in pg_dump that
doesn't check the malloc return code.

But we do have a pg_malloc() function in there - but from what I can
tell it's only used very sparsely?

Shouldn't we be using that one more or less everywhere

Yup. Have at it.

regards, tom lane

On Fri, Jun 10, 2011 at 21:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

I came across a situation today with a pretty bad crash of pg_dump,
due to not checking the return code from malloc(). When looking
through the code, it seems there are a *lot* of places in pg_dump that
doesn't check the malloc return code.

But we do have a pg_malloc() function in there - but from what I can
tell it's only used very sparsely?

Shouldn't we be using that one more or less everywhere

Yup.  Have at it.

Something along the line of this?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus@hagander.net> writes:

Something along the line of this?

I think this is a seriously, seriously bad idea:

+#define strdup(x) pg_strdup(x)
+#define malloc(x) pg_malloc(x)
+#define calloc(x,y) pg_calloc(x, y)
+#define realloc(x,y) pg_realloc(x, y)

as it will render the code unreadable to people expecting the normal
behavior of these fundamental functions; not to mention break any
call sites that have some other means of dealing with an alloc failure
besides going belly-up. Please take the trouble to do
s/malloc/pg_malloc/g and so on, instead.

regards, tom lane

On 22 June 2011 16:25, Magnus Hagander <magnus@hagander.net> wrote:

Something along the line of this?

IMHO the redefinition of malloc() looks a bit hairy...can't you just
make the callers use the functions directly?

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On Wed, Jun 22, 2011 at 17:48, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

Something along the line of this?

I think this is a seriously, seriously bad idea:

+#define strdup(x) pg_strdup(x)
+#define malloc(x) pg_malloc(x)
+#define calloc(x,y) pg_calloc(x, y)
+#define realloc(x,y) pg_realloc(x, y)

as it will render the code unreadable to people expecting the normal
behavior of these fundamental functions; not to mention break any
call sites that have some other means of dealing with an alloc failure
besides going belly-up.  Please take the trouble to do
s/malloc/pg_malloc/g and so on, instead.

Ok, I'll try that approach. This seemed like a "nicer" approach, but I
think once written out, i agree with your arguments :-)

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Excerpts from Magnus Hagander's message of mié jun 22 11:25:43 -0400 2011:

On Fri, Jun 10, 2011 at 21:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

I came across a situation today with a pretty bad crash of pg_dump,
due to not checking the return code from malloc(). When looking
through the code, it seems there are a *lot* of places in pg_dump that
doesn't check the malloc return code.

But we do have a pg_malloc() function in there - but from what I can
tell it's only used very sparsely?

Shouldn't we be using that one more or less everywhere

Yup.  Have at it.

Something along the line of this?

Huh, do you really need a new file for the four new functions? What's
wrong with common.c?

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Magnus Hagander wrote:

On Wed, Jun 22, 2011 at 17:48, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

Something along the line of this?

I think this is a seriously, seriously bad idea:

+#define strdup(x) pg_strdup(x)
+#define malloc(x) pg_malloc(x)
+#define calloc(x,y) pg_calloc(x, y)
+#define realloc(x,y) pg_realloc(x, y)

as it will render the code unreadable to people expecting the normal
behavior of these fundamental functions; not to mention break any
call sites that have some other means of dealing with an alloc failure
besides going belly-up. ?Please take the trouble to do
s/malloc/pg_malloc/g and so on, instead.

Ok, I'll try that approach. This seemed like a "nicer" approach, but I
think once written out, i agree with your arguments :-)

Where are we on this?

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

On Fri, Oct 14, 2011 at 21:11, Bruce Momjian <bruce@momjian.us> wrote:

Magnus Hagander wrote:

On Wed, Jun 22, 2011 at 17:48, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

Something along the line of this?

I think this is a seriously, seriously bad idea:

+#define strdup(x) pg_strdup(x)
+#define malloc(x) pg_malloc(x)
+#define calloc(x,y) pg_calloc(x, y)
+#define realloc(x,y) pg_realloc(x, y)

as it will render the code unreadable to people expecting the normal
behavior of these fundamental functions; not to mention break any
call sites that have some other means of dealing with an alloc failure
besides going belly-up. ?Please take the trouble to do
s/malloc/pg_malloc/g and so on, instead.

Ok, I'll try that approach. This seemed like a "nicer" approach, but I
think once written out, i agree with your arguments :-)

Where are we on this?

It's still sitting on my personal TODO list, just not with a really
high priority.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Alvaro Herrera wrote:

Excerpts from Magnus Hagander's message of mi�� jun 22 11:25:43 -0400 2011:

On Fri, Jun 10, 2011 at 21:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Magnus Hagander <magnus@hagander.net> writes:

I came across a situation today with a pretty bad crash of pg_dump,
due to not checking the return code from malloc(). When looking
through the code, it seems there are a *lot* of places in pg_dump that
doesn't check the malloc return code.

But we do have a pg_malloc() function in there - but from what I can
tell it's only used very sparsely?

Shouldn't we be using that one more or less everywhere

Yup. ��Have at it.

Something along the line of this?

Huh, do you really need a new file for the four new functions? What's
wrong with common.c?

I developed the attached patch to handle this. I moved the catalog code
from common.c into dumpcatalog.c, so there are just memory routines now
in common.c. I created new memory routines in pg_dumpall.c because
there is no AH structure in pg_dumpall.c. I then modified all the calls
to use the new routines, and removed the NULL return checks that were no
longer necessary.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian wrote:

I developed the attached patch to handle this. I moved the catalog code
from common.c into dumpcatalog.c, so there are just memory routines now
in common.c. I created new memory routines in pg_dumpall.c because
there is no AH structure in pg_dumpall.c. I then modified all the calls
to use the new routines, and removed the NULL return checks that were no
longer necessary.

Applied.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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