Horton

Thread

  1. pg_dump vs malloc

    Magnus Hagander <magnus@hagander.net> — 2011-06-10T17:58:13Z

    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, or even #define it?

Or am I missing something in the code here?

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

  2. Re: pg_dump vs malloc

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-06-10T19:07:00Z

    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

  3. Re: pg_dump vs malloc

    Magnus Hagander <magnus@hagander.net> — 2011-06-22T15:25:43Z

    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/

  4. Re: pg_dump vs malloc

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-06-22T15:48:56Z

    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

  5. Re: pg_dump vs malloc

    Peter Geoghegan <peter@2ndquadrant.com> — 2011-06-22T15:52:35Z

    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

  6. Re: pg_dump vs malloc

    Magnus Hagander <magnus@hagander.net> — 2011-06-22T15:55:06Z

    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/

  7. Re: pg_dump vs malloc

    Alvaro Herrera <alvherre@commandprompt.com> — 2011-06-22T19:26:44Z

    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

  8. Re: pg_dump vs malloc

    Bruce Momjian <bruce@momjian.us> — 2011-10-14T19:11:24Z

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

  9. Re: pg_dump vs malloc

    Magnus Hagander <magnus@hagander.net> — 2011-10-15T15:23:37Z

    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/

  10. Re: pg_dump vs malloc

    Bruce Momjian <bruce@momjian.us> — 2011-11-14T22:10:17Z

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

  11. Re: pg_dump vs malloc

    Bruce Momjian <bruce@momjian.us> — 2011-11-25T20:41:36Z

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