Hackers,

I think there a bug in the GUC mechanism.  The custom variables patch
added several malloc() and a strdup() call, and they are never checked
for an out of memory condition.

-- 
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"El que vive para el futuro es un iluso, y el que vive para el pasado,
un imbï¿½cil" (Luis Adler, "Los tripulantes de la noche")


Thu, 24 Jun 2004 01:27:12 -0400
Alvaro Herrera <alvherre@dcc.uchile.cl>
bug in GUC
I'll look into that. Thanks for pointing it out.

Kind regards,

Thomas Hallgren

"Alvaro Herrera" <alvherre@dcc.uchile.cl> wrote in message
news:20040624052712.GA7158@dcc.uchile.cl...
> Hackers,
>
> I think there a bug in the GUC mechanism.  The custom variables patch
> added several malloc() and a strdup() call, and they are never checked
> for an out of memory condition.
>
> -- 
> Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
> "El que vive para el futuro es un iluso, y el que vive para el pasado,
> un imbï¿½cil" (Luis Adler, "Los tripulantes de la noche")
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
>                http://archives.postgresql.org
>


Thu, 24 Jun 2004 08:49:21 +0200
"Thomas Hallgren" <thhal@mailblocks.com>
Re: bug in GUC
Rather than clutter the code with the same ereport over and over again (I
count 12 malloc's in guc.c alone), I'd like something like this:

void* malloc_or_fail(int elevel, size_t sz)
{
    void* result;
    if(sz < 1)
        /*
         * Make sure we have something that can be passed to free() even
         * when the size is zero.
         */
        sz = 1;

    result = malloc(sz);
    if(result == NULL)
    {
        ereport(elevel,
            (errcode(ERRCODE_OUT_OF_MEMORY),
                errmsg("out of memory")));

        /* Oops, ereport returned! Who called us with elevel < ERROR? */
        exit(EXIT_FAILURE);
    }
    return result;
}

void* malloc_or_error(size_t sz)
{
    return malloc_or_fail(ERROR, sz);
}


void* malloc_or_fatal(size_t sz)
{
    return malloc_or_fail(FATAL, sz);
}

I search the code but the only thing I find that comes close is pq_malloc.
But that's a different thing altogether since it doesn't use ereport. I'm
sure I missed something somewhere but if not, perhaps the above functions
would make sense? If so, what's the best name for them and where should they
reside?

Kind regards,

Thomas Hallgren

"Alvaro Herrera" <alvherre@dcc.uchile.cl> wrote in message
news:20040624052712.GA7158@dcc.uchile.cl...
> Hackers,
>
> I think there a bug in the GUC mechanism.  The custom variables patch
> added several malloc() and a strdup() call, and they are never checked
> for an out of memory condition.
>
> -- 
> Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
> "El que vive para el futuro es un iluso, y el que vive para el pasado,
> un imbï¿½cil" (Luis Adler, "Los tripulantes de la noche")
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
>                http://archives.postgresql.org
>


Thu, 24 Jun 2004 11:06:25 +0200
"Thomas Hallgren" <thhal@mailblocks.com>
Re: bug in GUC
"Thomas Hallgren" <thhal@mailblocks.com> writes:
> Rather than clutter the code with the same ereport over and over again (I
> count 12 malloc's in guc.c alone), I'd like something like this:

The larger question is why it contains even one.  In general, use of
malloc in the backend is the mark of a newbie.  I'd think palloc in
TopMemoryContext would be a more suitable approach.

			regards, tom lane

Thu, 24 Jun 2004 10:02:08 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: bug in GUC 
Ok, so I'm a newbie. To my defence I'll say that I made an effort to follow
the style previously used in guc.c. The unchecked mallocs I added where not
the first ;-)

So, what you are saying is that there's no need for the functions I
suggested and that a palloc using the TopMemoryContext will guarantee
correct behavior on "out of memory"?

Kind regards,

Thomas Hallgren

"Tom Lane" <tgl@sss.pgh.pa.us> wrote in message
news:13534.1088085728@sss.pgh.pa.us...
> "Thomas Hallgren" <thhal@mailblocks.com> writes:
> > Rather than clutter the code with the same ereport over and over again
(I
> > count 12 malloc's in guc.c alone), I'd like something like this:
>
> The larger question is why it contains even one.  In general, use of
> malloc in the backend is the mark of a newbie.  I'd think palloc in
> TopMemoryContext would be a more suitable approach.
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
>                http://archives.postgresql.org
>


Thu, 24 Jun 2004 16:45:31 +0200
"Thomas Hallgren" <thhal@mailblocks.com>
Re: bug in GUC

On Jun 24, 2004, at 10:45 AM, Thomas Hallgren wrote:

> So, what you are saying is that there's no need for the functions I
> suggested and that a palloc using the TopMemoryContext will guarantee
> correct behavior on "out of memory"?

Perhaps a section regarding proper memory management code in the 
backend could be written , say, somewhere in the internals document 
around the coding conventions chapter:

	http://developer.postgresql.org/docs/postgres/source.html

I myself don't have a clue, not being a backend hacker, so I'll just 
slink back to my cave.

----
James Robinson
Socialserve.com


Thu, 24 Jun 2004 11:59:59 -0400
James Robinson <jlrobins@socialserve.com>
Re: [HACKERS] bug in GUC
On Thu, Jun 24, 2004 at 04:45:31PM +0200, Thomas Hallgren wrote:
> Ok, so I'm a newbie. To my defence I'll say that I made an effort to follow
> the style previously used in guc.c. The unchecked mallocs I added were not
> the first ;-)

Apparently Peter thought it was a good idea *not* to use palloc and
friends, and documented it.  The rationale seems to be "we have more
control over out-of-memory conditions", and if you look closely, the
out-of-memory is handled at a lower level than ERROR if it's not
processing interactively.  For example, when reading the config file,
the ereport is DEBUG2.

I'm not sure exactly why this is a good idea.  After all, if the systems
runs out of memory while starting up, what can be expected later?  Not a
lot is going to work.

> So, what you are saying is that there's no need for the functions I
> suggested and that a palloc using the TopMemoryContext will guarantee
> correct behavior on "out of memory"?

IMO yes and yes.

-- 
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"La felicidad no es maï¿½ana. La felicidad es ahora"


Thu, 24 Jun 2004 12:55:42 -0400
Alvaro Herrera <alvherre@dcc.uchile.cl>
Re: bug in GUC
Alvaro Herrera <alvherre@dcc.uchile.cl> writes:
> I'm not sure exactly why this is a good idea.  After all, if the systems
> runs out of memory while starting up, what can be expected later?

The issue isn't with startup, but with re-reading postgresql.conf due to
SIGHUP later on.  We don't want to elog(ERROR) partway through that
process.  Especially not in the postmaster, where elog(ERROR) is
tantamount to elog(FATAL).  (But of course the postmaster shouldn't ever
run out of memory anyway...)

It's possible that this should all be rethought, but it would be a much
more wide-ranging change than we've been discussing.

			regards, tom lane

Thu, 24 Jun 2004 13:29:13 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: bug in GUC 
Tom Lane wrote:
> Alvaro Herrera <alvherre@dcc.uchile.cl> writes:
> 
> ... We don't want to elog(ERROR) partway through that
> process.  Especially not in the postmaster, where elog(ERROR) is
> tantamount to elog(FATAL).  (But of course the postmaster shouldn't ever
> run out of memory anyway...)
> 
> It's possible that this should all be rethought, but it would be a much
> more wide-ranging change than we've been discussing.
> 
> 			regards, tom lane
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 7: don't forget to increase your free space map settings
> 

Here's a patch dealing with the unchecked mallocs and strdups in guc.c. 
Rather than mixing in palloc and TopMemoryContext into the code (would 
be rather messy given that most allocs actually falls into the category 
of not always being permitted to do elog(ERROR/FATAL)), I added a couple 
of simple static functions for guc_alloc, guc_realloc, and guc_strdup 
that are used throughout the guc.c file.

Kind regards,

Thomas Hallgren
Sun, 27 Jun 2004 20:45:40 +0200
Thomas Hallgren <thhal@mailblocks.com>
Re: [HACKERS] bug in GUC
Thomas Hallgren <thhal@mailblocks.com> writes:
> Here's a patch dealing with the unchecked mallocs and strdups in guc.c. 

Applied with minor editorialization.  Thanks.

			regards, tom lane

Mon, 05 Jul 2004 19:15:39 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: [HACKERS] bug in GUC 