Re: [PATCHES] mcxt.c

A test for null string is missing here:

*** pgsql/src/backend/utils/mmgr/mcxt.c 2003-09-02 13:30:05.000000000 +0200
--- pgsql.old/src/backend/utils/mmgr/mcxt.c 2003-08-04 04:40:08.000000000
+0200
*************** char *
*** 620,632 ****
MemoryContextStrdup(MemoryContext context, const char *string)
{
char *nstr;
-
- if ( !string )
- {
- elog(ERROR, "MemoryContextStrdup called with a NULL pointer");
- return NULL;
- }
-
Size len = strlen(string) + 1;
nstr = (char *) MemoryContextAlloc(context, len);
--- 620,625 ----

"Mendola Gaetano" <mendola(at)bigfoot(dot)com> writes:
> A test for null string is missing here:

> MemoryContextStrdup(MemoryContext context, const char *string)
> {
> char *nstr;
> -
> - if ( !string )
> - {
> - elog(ERROR, "MemoryContextStrdup called with a NULL pointer");
> - return NULL;
> - }

This seems inappropriate to me. Are you going to suggest that every
routine that takes a pointer parameter needs to explicitly test for
null? We could bloat the code a great deal that way, and slow it down,
without gaining anything at all in debuggability (IMHO anyway).

If there's a reason for pstrdup in particular to do this, what is it?

regards, tom lane

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Mendola Gaetano" <mendola(at)bigfoot(dot)com> writes:
> > A test for null string is missing here:
>
> > MemoryContextStrdup(MemoryContext context, const char *string)
> > {
> > char *nstr;
> > -
> > - if ( !string )
> > - {
> > - elog(ERROR, "MemoryContextStrdup called with a NULL pointer");
> > - return NULL;
> > - }
>
> This seems inappropriate to me. Are you going to suggest that every
> routine that takes a pointer parameter needs to explicitly test for
> null? We could bloat the code a great deal that way, and slow it down,
> without gaining anything at all in debuggability (IMHO anyway).

Of course I'm not suggesting this, what I'm suggesting is put an
assert( ) if the test can slow down the performances and an "if ( ) "
in places that are not going to touch the performances.

I think that is reasonable.

Regards
Gaetano Mendola

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Mendola Gaetano" <mendola(at)bigfoot(dot)com> writes:
> > A test for null string is missing here:
>
> > MemoryContextStrdup(MemoryContext context, const char *string)
> > {
> > char *nstr;
> > -
> > - if ( !string )
> > - {
> > - elog(ERROR, "MemoryContextStrdup called with a NULL pointer");
> > - return NULL;
> > - }
>
> This seems inappropriate to me. Are you going to suggest that every
> routine that takes a pointer parameter needs to explicitly test for
> null? We could bloat the code a great deal that way, and slow it down,
> without gaining anything at all in debuggability (IMHO anyway).

Of course I'm not suggesting this, what I'm suggesting is put an
assert( ) if the test can slow down the performances and an "if ( ) "
in places that are not going to touch the performances.

I think that is reasonable.

Regards
Gaetano Mendola

"Gaetano Mendola" <mendola(at)bigfoot(dot)com> writes:
> "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> This seems inappropriate to me. Are you going to suggest that every
>> routine that takes a pointer parameter needs to explicitly test for
>> null?

> Of course I'm not suggesting this, what I'm suggesting is put an
> assert( ) if the test can slow down the performances and an "if ( ) "
> in places that are not going to touch the performances.

I see no value at all in an assert. The code will dump core just fine
with or without an assert ...

regards, tom lane

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Gaetano Mendola" <mendola(at)bigfoot(dot)com> writes:
> > "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> This seems inappropriate to me. Are you going to suggest that every
> >> routine that takes a pointer parameter needs to explicitly test for
> >> null?
>
> > Of course I'm not suggesting this, what I'm suggesting is put an
> > assert( ) if the test can slow down the performances and an "if ( ) "
> > in places that are not going to touch the performances.
>
> I see no value at all in an assert. The code will dump core just fine
> with or without an assert ...

Right but an assert can display information about the file and line number
without debug the application, without mention that reading the code with
the assert is clear what are the precondictions for a call function.

Regards
Gaetano Mendola

On Mon, 2003-09-08 at 11:09, Gaetano Mendola wrote:
> "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > I see no value at all in an assert. The code will dump core just fine
> > with or without an assert ...
>
> Right but an assert can display information about the file and line number
> without debug the application

I think the percentage of deployments that enable assertions (which
causes a runtime performance hit) but NOT debugging info (which does
not) is pretty small.

ISTM that checking for non-NULL pointers is pretty pointless: just
because a pointer happens to be non-NULL doesn't mean it is any more
valid, and dereferencing a NULL pointer is easy enough to track down in
any case.

-Neil

"Neil Conway" <neilc(at)samurai(dot)com> wrote:
> On Mon, 2003-09-08 at 11:09, Gaetano Mendola wrote:
> > "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > > I see no value at all in an assert. The code will dump core just fine
> > > with or without an assert ...
> >
> > Right but an assert can display information about the file and line
number
> > without debug the application
>
> I think the percentage of deployments that enable assertions (which
> causes a runtime performance hit) but NOT debugging info (which does
> not) is pretty small.
>
> ISTM that checking for non-NULL pointers is pretty pointless: just
> because a pointer happens to be non-NULL doesn't mean it is any more
> valid, and dereferencing a NULL pointer is easy enough to track down in
> any case.

I'm not speaking only about to test if pointer is null or not but also do
some
assert to better understand wich condition shall be verified in some part of
the
code also to have clear what is going on inside code that may be after years
and years is not clear anymore.

May be I'm not clear enough.
Please tell me when and were an assert shall be used.

Regards
Gaetano Mendola

Neil Conway <neilc(at)samurai(dot)com> writes:

> I think the percentage of deployments that enable assertions (which
> causes a runtime performance hit) but NOT debugging info (which does
> not) is pretty small.

How big a penalty is it? If it's small, or if it could be made small by making
a few assertions require an extra extra-assertions option, then perhaps it
would make more sense to ship with it enabled?

I know the number of times I received ORA-600 (oracle's way of spelling
"assertion failed") I sure wouldn't have wanted the database to continue
processing based on invalid data.

> ISTM that checking for non-NULL pointers is pretty pointless: just
> because a pointer happens to be non-NULL doesn't mean it is any more
> valid, and dereferencing a NULL pointer is easy enough to track down in
> any case.

That would depend a lot on the scenario. Often code doesn't crash right at
that point but stores the data causes a crash elsewhere. Or perhaps even
causes corrupted data on disk.

Probably the most useful side-effect of checking for null pointers is that
programmers get in the habit of checking all their arguments...

--
greg

Greg Stark <gsstark(at)mit(dot)edu> writes:
> Neil Conway <neilc(at)samurai(dot)com> writes:
>> I think the percentage of deployments that enable assertions (which
>> causes a runtime performance hit) but NOT debugging info (which does
>> not) is pretty small.

> How big a penalty is it? If it's small, or if it could be made small by making
> a few assertions require an extra extra-assertions option, then perhaps it
> would make more sense to ship with it enabled?

We generally don't recommend enabling assertions in production
installations, because it's not clear that there is any net gain in
stability from doing so. Per the manual:

--enable-cassert

Enables assertion checks in the server, which test for many
"can't happen" conditions. This is invaluable for code
development purposes, but the tests slow things down a
little. Also, having the tests turned on won't necessarily
enhance the stability of your server! The assertion checks are
not categorized for severity, and so what might be a relatively
harmless bug will still lead to server restarts if it triggers
an assertion failure. Currently, this option is not
recommended for production use, but you should have it on for
development work or when running a beta version.

Obviously this does not apply to cases where the assert is testing
for something that will cause a core dump anyway, like an improperly
NULL pointer. But there are many, many asserts for things that are
probably not serious bugs (at worst they might deserve a FATAL exit,
rather than a system-wide PANIC).

Peter E. has speculated about improving the Assert facility to allow
categorization along this line, but I dunno when it will happen.

As far as your original question goes, I find that
MEMORY_CONTEXT_CHECKING and CLOBBER_FREED_MEMORY are quite expensive,
and presently --enable-cassert turns these on. But of course we could
decouple that if we were going to encourage people to run with asserts
enabled in production. I don't think asserts are hugely expensive
otherwise (though that might change if we sprinkle them as liberally
as Gaetano's proposal implies...)

regards, tom lane

The particular assertion that was proposed doesn't strike me as terribly
useful - It should be checked at the point of call rather than inside
pstrdup, I should have thought.

Of course, that would make for lots of code bloat ... cases like this
are when gdb is your friend.

cheers

andrew

Tom Lane wrote:

>Greg Stark <gsstark(at)mit(dot)edu> writes:
>
>
>>Neil Conway <neilc(at)samurai(dot)com> writes:
>>
>>
>>>I think the percentage of deployments that enable assertions (which
>>>causes a runtime performance hit) but NOT debugging info (which does
>>>not) is pretty small.
>>>
>>>
>
>
>
>>How big a penalty is it? If it's small, or if it could be made small by making
>>a few assertions require an extra extra-assertions option, then perhaps it
>>would make more sense to ship with it enabled?
>>
>>
>
>We generally don't recommend enabling assertions in production
>installations, because it's not clear that there is any net gain in
>stability from doing so. Per the manual:
>
> --enable-cassert
>
> Enables assertion checks in the server, which test for many
> "can't happen" conditions. This is invaluable for code
> development purposes, but the tests slow things down a
> little. Also, having the tests turned on won't necessarily
> enhance the stability of your server! The assertion checks are
> not categorized for severity, and so what might be a relatively
> harmless bug will still lead to server restarts if it triggers
> an assertion failure. Currently, this option is not
> recommended for production use, but you should have it on for
> development work or when running a beta version.
>
>Obviously this does not apply to cases where the assert is testing
>for something that will cause a core dump anyway, like an improperly
>NULL pointer. But there are many, many asserts for things that are
>probably not serious bugs (at worst they might deserve a FATAL exit,
>rather than a system-wide PANIC).
>
>Peter E. has speculated about improving the Assert facility to allow
>categorization along this line, but I dunno when it will happen.
>
>As far as your original question goes, I find that
>MEMORY_CONTEXT_CHECKING and CLOBBER_FREED_MEMORY are quite expensive,
>and presently --enable-cassert turns these on. But of course we could
>decouple that if we were going to encourage people to run with asserts
>enabled in production. I don't think asserts are hugely expensive
>otherwise (though that might change if we sprinkle them as liberally
>as Gaetano's proposal implies...)
>
> regards, tom lane
>
>
>

"Andrew Dunstan" <andrew(at)dunslane(dot)net> wrote:
> The particular assertion that was proposed doesn't strike me as terribly
> useful - It should be checked at the point of call rather than inside
> pstrdup, I should have thought.

Are you going to trust the client of that function ?
Here the question is not if insert a check/assert there but write a general
rule if insert and where check/assert

Regards
Gaetano Mendola

On Tue, Sep 09, 2003 at 04:53:06PM +0200, Gaetano Mendola wrote:
> "Andrew Dunstan" <andrew(at)dunslane(dot)net> wrote:
> > The particular assertion that was proposed doesn't strike me as terribly
> > useful - It should be checked at the point of call rather than inside
> > pstrdup, I should have thought.
>
> Are you going to trust the client of that function ?

Yes, because it can only used in backend code and C functions, which
can be written only by a trusted user ('cause C is an untrusted language).

(I might be wrong...)

--
Alvaro Herrera (<alvherre[(at)]dcc(dot)uchile(dot)cl>)
"Use it up, wear it out, make it do, or do without"

Alvaro Herrera Munoz wrote:

>On Tue, Sep 09, 2003 at 04:53:06PM +0200, Gaetano Mendola wrote:
>
>
>>"Andrew Dunstan" <andrew(at)dunslane(dot)net> wrote:
>>
>>
>>>The particular assertion that was proposed doesn't strike me as terribly
>>>useful - It should be checked at the point of call rather than inside
>>>pstrdup, I should have thought.
>>>
>>>
>>Are you going to trust the client of that function ?
>>
>>
>
>Yes, because it can only used in backend code and C functions, which
>can be written only by a trusted user ('cause C is an untrusted language).
>
>(I might be wrong...)
>
>
>
Besides that, trust isn't the issue, but rather what useful information
can be gathered. How useful is it to know "someone called pstrdup() with
a null pointer"? Not very, IMNSHO.

cheers

andrew