Thread: Re: mcxt.c
"Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> "Mendola Gaetano" <mendola@bigfoot.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@bigfoot.com> writes:
> "Tom Lane" <tgl@sss.pgh.pa.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@sss.pgh.pa.us> wrote: > "Gaetano Mendola" <mendola@bigfoot.com> writes: > > "Tom Lane" <tgl@sss.pgh.pa.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@sss.pgh.pa.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@samurai.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@mit.edu> writes:
> Neil Conway <neilc@samurai.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
invaluablefor code development purposes, but the tests slow things down a little. Also, having the tests
turnedon won't necessarily enhance the stability of your server! The assertion checks are not categorized
forseverity, 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
onfor 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@mit.edu> writes: > > >>Neil Conway <neilc@samurai.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@dunslane.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@dunslane.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[@]dcc.uchile.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@dunslane.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