Re: Coverity Open Source Defect Scan of PostgreSQL

Hello PostgreSQL Developers,

I'm the CTO of Coverity, Inc., a company that does static source code
analysis to look for defects in code. You may have heard of us or of our
technology from its days at Stanford (the "Stanford Checker"). The
reason I'm writing is because we have set up a framework internally to
continually scan open source projects and provide the results of our
analysis back to the developers of those projects. PostgreSQL is one of
the 32 projects currently scanned at:

http://scan.coverity.com

My belief is that we (Coverity) must reach out to the developers of
these packages (you) in order to make progress in actually fixing the
defects that we happen to find, so this is my first step in that
mission. Of course, I think Coverity technology is great, but I want to
hear what you think and that's why I worked with folks at Coverity to
put this infrastructure in place. The process is simple -- it checks out
your code each night from your repository and scans it so you can always
see the latest results.

Right now, we're guarding access to the actual defects that we report
for a couple of reasons: (1) We think that you, as developers of
PostgreSQL, should have the chance to look at the defects we find to
patch them before random other folks get to see what we found and (2)
From a support perspective, we want to make sure that we have the
appropriate time to engage with those who want to use the results to fix
the code. Because of this second point, I'd ask that if you are
interested in really digging into the results a bit further for your
project, please have a couple of core maintainers (or group nominated
individuals) reach out to me to request access. As this is a new process
for us and still involves a small number of packages, I want to make
sure that I personally can be involved with the activity that is
generated from this effort.

So I'm basically asking for people who want to play around with some
cool new technology to help make source code better. If this interests
you, please feel free to reach out to me directly. And of course, if
there are other packages you care about that aren't currently on the
list, I want to know about those too.

If this is the wrong list, my sincerest apologies and please let me
know where would be a more appropriate forum for this type of message.

Many thanks for reading this far...

-ben

Ben Chelf
Chief Technology Officer
Coverity, Inc.

Ben Chelf wrote:
> Hello PostgreSQL Developers,
>
> I'm the CTO of Coverity, Inc., a company that does static source code
> analysis to look for defects in code. You may have heard of us or of our
> technology from its days at Stanford (the "Stanford Checker"). The
> reason I'm writing is because we have set up a framework internally to
> continually scan open source projects and provide the results of our
> analysis back to the developers of those projects. PostgreSQL is one of
> the 32 projects currently scanned at:
>
> http://scan.coverity.com

Hm, interestingly and in contrast to some announcements, MySQL is not
included in this list. Did it blast the defects column ? :-)

Regards,
Andreas

Andreas Pflug wrote:
> Ben Chelf wrote:
> > Hello PostgreSQL Developers,
> >
> > I'm the CTO of Coverity, Inc., a company that does static source code
> > analysis to look for defects in code. You may have heard of us or of our
> > technology from its days at Stanford (the "Stanford Checker"). The
> > reason I'm writing is because we have set up a framework internally to
> > continually scan open source projects and provide the results of our
> > analysis back to the developers of those projects. PostgreSQL is one of
> > the 32 projects currently scanned at:
> >
> > http://scan.coverity.com
>
> Hm, interestingly and in contrast to some announcements, MySQL is not
> included in this list. Did it blast the defects column ? :-)

I thought we ran the Converity analysis a year ago and cleaned up the
warnings, so I am surprised at our high number, but I assume they are
mostly noise.

--
Bruce Momjian http://candle.pha.pa.us
SRA OSS, Inc. http://www.sraoss.com

+ If your life is a hard drive, Christ can be your backup. +

Andreas Pflug wrote:
> Ben Chelf wrote:
> >Hello PostgreSQL Developers,
> >
> > I'm the CTO of Coverity, Inc., a company that does static source code
> >analysis to look for defects in code. You may have heard of us or of our
> >technology from its days at Stanford (the "Stanford Checker"). The
> >reason I'm writing is because we have set up a framework internally to
> >continually scan open source projects and provide the results of our
> >analysis back to the developers of those projects. PostgreSQL is one of
> >the 32 projects currently scanned at:
> >
> >http://scan.coverity.com
>
> Hm, interestingly and in contrast to some announcements, MySQL is not
> included in this list. Did it blast the defects column ? :-)

AFAIR they got a private scan done and they fixed the reported defects.
After that they issued a press release telling how little defects they
got, or something ...

OTOH neither JBoss, BerkeleyDB, Qt are listed. Is there a pattern here?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:

> AFAIR they got a private scan done and they fixed the reported defects.
> After that they issued a press release telling how little defects they
> got, or something ...
>
> OTOH neither JBoss, BerkeleyDB, Qt are listed. Is there a pattern here?

I guess the pattern is obvious indeed:
Coverity probably wants a shot at selling their services to these
companies and rightly so imho.

regards,
Lukas

>
>OTOH neither JBoss, BerkeleyDB, Qt are listed. Is there a pattern here?
>
>
>
http://www.coverity.com/news/news_02_15_05_story_6.html

On Mon, 6 Mar 2006, Bruce Momjian wrote:

> Andreas Pflug wrote:
>> Ben Chelf wrote:
>>> Hello PostgreSQL Developers,
>>>
>>> I'm the CTO of Coverity, Inc., a company that does static source code
>>> analysis to look for defects in code. You may have heard of us or of our
>>> technology from its days at Stanford (the "Stanford Checker"). The
>>> reason I'm writing is because we have set up a framework internally to
>>> continually scan open source projects and provide the results of our
>>> analysis back to the developers of those projects. PostgreSQL is one of
>>> the 32 projects currently scanned at:
>>>
>>> http://scan.coverity.com
>>
>> Hm, interestingly and in contrast to some announcements, MySQL is not
>> included in this list. Did it blast the defects column ? :-)
>
> I thought we ran the Converity analysis a year ago and cleaned up the
> warnings, so I am surprised at our high number, but I assume they are
> mostly noise.

Got an account and will take a look at the details this evening ... :)

----
Marc G. Fournier Hub.Org Networking Services (http://www.hub.org)
Email: scrappy(at)hub(dot)org Yahoo!: yscrappy ICQ: 7615664

On Mon, 2006-03-06 at 11:55 -0300, Alvaro Herrera wrote:
> AFAIR they got a private scan done and they fixed the reported defects.

Indeed: EnterpriseDB paid for a license for the Coverity static analysis
tool, and then ran that tool on the open-source Postgres tree. One of
their engineers then worked with me to get a bunch of patches committed
to fix the issues the tool identified -- e.g.

http://archives.postgresql.org/pgsql-committers/2005-06/msg00428.php
http://archives.postgresql.org/pgsql-committers/2005-06/msg00314.php
http://archives.postgresql.org/pgsql-committers/2005-06/msg00315.php
http://archives.postgresql.org/pgsql-committers/2005-06/msg00298.php

The tool found a few significant bugs, but most of the fixes were
somewhat cosmetic. (Perhaps one reason for this is that the Stanford
checker was run on an earlier version of PostgreSQL by some grad
students at Stanford, who submitted patches / bug reports for the more
serious issues they found.)

I'm a bit surprised to see that there are ~300 unfixed defects: AFAIR I
fixed all the issues the EDB guys passed on to me, with the exception of
some false positives and a handful of minor issues in ECPG that I
couldn't be bothered fixing (frankly I would rather not touch the ECPG
code). I've requested access to the Coverity results -- I'll be curious
to see if we can get any more useful fixes from the tool.

-Neil

Neil Conway wrote:

>On Mon, 2006-03-06 at 11:55 -0300, Alvaro Herrera wrote:
>
>
>>AFAIR they got a private scan done and they fixed the reported defects.
>>
>>
>
>Indeed: EnterpriseDB paid for a license for the Coverity static analysis
>tool, and then ran that tool on the open-source Postgres tree. One of
>their engineers then worked with me to get a bunch of patches committed
>to fix the issues the tool identified -- e.g.
>
>http://archives.postgresql.org/pgsql-committers/2005-06/msg00428.php
>http://archives.postgresql.org/pgsql-committers/2005-06/msg00314.php
>http://archives.postgresql.org/pgsql-committers/2005-06/msg00315.php
>http://archives.postgresql.org/pgsql-committers/2005-06/msg00298.php
>
>The tool found a few significant bugs, but most of the fixes were
>somewhat cosmetic. (Perhaps one reason for this is that the Stanford
>checker was run on an earlier version of PostgreSQL by some grad
>students at Stanford, who submitted patches / bug reports for the more
>serious issues they found.)
>
>I'm a bit surprised to see that there are ~300 unfixed defects: AFAIR I
>fixed all the issues the EDB guys passed on to me, with the exception of
>some false positives and a handful of minor issues in ECPG that I
>couldn't be bothered fixing (frankly I would rather not touch the ECPG
>code). I've requested access to the Coverity results -- I'll be curious
>to see if we can get any more useful fixes from the tool.
>
>
>

For a short while EDB were pushing their Coverity results up to the
buildfarm server, too. But it didn't last long.

cheers

andrew

Ben,

> I'm the CTO of Coverity, Inc., a company that does static source code
> analysis to look for defects in code. You may have heard of us or of our
> technology from its days at Stanford (the "Stanford Checker"). The
> reason I'm writing is because we have set up a framework internally to
> continually scan open source projects and provide the results of our
> analysis back to the developers of those projects. PostgreSQL is one of
> the 32 projects currently scanned at:

Nice to hear from you! Coverity has previously worked with Sean
Chittenden, EnterpriseDB and Neil Conway. So we're glad to be continuing
our relationship with you.

--
--Josh

Josh Berkus
Aglio Database Solutions
San Francisco

On Mon, Mar 06, 2006 at 12:50:15PM -0400, Marc G. Fournier wrote:
> >I thought we ran the Converity analysis a year ago and cleaned up the
> >warnings, so I am surprised at our high number, but I assume they are
> >mostly noise.
>
> Got an account and will take a look at the details this evening ... :)

Are the people who look at the results allowed to tell the rest of us
what kind of bugs are involved (ie trivial or otherwise)? I mean,
presumably they can't cut and paste the results to the mailing list but
are we going to see anything other than a list of bug-fixes for
(apparently) unreported bugs?

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
> tool for doing 5% of the work and then sitting around waiting for someone
> else to do the other 95% so you can sue them.

Neil Conway wrote:

> I'm a bit surprised to see that there are ~300 unfixed defects: AFAIR I
> fixed all the issues the EDB guys passed on to me, with the exception of
> some false positives and a handful of minor issues in ECPG that I
> couldn't be bothered fixing (frankly I would rather not touch the ECPG
> code). I've requested access to the Coverity results -- I'll be curious
> to see if we can get any more useful fixes from the tool.

It's not unlikely that the checker got improved.

FWIW I don't see any report related to the MemoryContext stuff, which I
was expecting we would be flooded with. I haven't seen the entire list
yet, but they do make the mistake of not noticing that ereport(ERROR)
does not continue execution. For example:

-------->-------->-------->-------->-------->-------->-------->-------->
pgsql/src/backend/optimizer/plan/createplan.c

638 foreach(l, uniq_exprs)
639 {
640 Node *uniqexpr = lfirst(l);
641 TargetEntry *tle;
642
643 tle = tlist_member(uniqexpr, newtlist);

Event var_compare_op: Added "tle" due to comparison "tle == 0"
Also see events: [var_deref_op]
At conditional (1): "tle == 0" taking true path

644 if (!tle) /* shouldn't happen */
645 elog(ERROR, "failed to find unique expression in subplan tlist");

Event var_deref_op: Variable "tle" tracked as NULL was dereferenced.
Also see events: [var_compare_op]

646 groupColIdx[groupColPos++] = tle->resno;
647 }
-------->-------->-------->-------->-------->-------->-------->-------->

We all know that this is not a bug. There are lots of these, as you can
imagine.

There are lots of other "not a bugs". For example in initdb, there is a
lot of noise about how we don't free the resources.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:

> but they do make the mistake of not noticing that ereport(ERROR)
> does not continue execution.

There is a way in gcc to indicate that a function never returns. But in
Postgres it's a bit weird since elog()/ereport() sometimes return and
sometimes don't. Perhaps it would be better to make the forms that don't
return separate functions. Then hopefully they can be marked to not trigger
these kinds of warnings.

> We all know that this is not a bug. There are lots of these, as you can
> imagine.
>
> There are lots of other "not a bugs". For example in initdb, there is a
> lot of noise about how we don't free the resources.

Presumably the reason they don't immediately release the reports is for fear
of security holes revealed. Once somebody has checked the errors and checked
for any exploitable holes would it be possible to just open access to anyone
so mere mortals can clean up the mundane bugs?

--
greg

On Tue, Mar 07, 2006 at 05:10:44PM -0500, Greg Stark wrote:
>
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>
> > but they do make the mistake of not noticing that ereport(ERROR)
> > does not continue execution.
>
> There is a way in gcc to indicate that a function never returns. But in
> Postgres it's a bit weird since elog()/ereport() sometimes return and
> sometimes don't. Perhaps it would be better to make the forms that don't
> return separate functions. Then hopefully they can be marked to not trigger
> these kinds of warnings.

I think the problem is that both of those are macros that expand into
calls to errstart and errfinish. The error level is passed to errstart
but the actual exception is thrown in errfinish using the value stored
on the exception stack. For a static analysis tool to pick that up
would be quite a trick. For gcc I wouldn't bet on it.

One possibility would be to add code to the elog/ereport macros that is
only used when using one of these tools. For example:

#ifdef STATIC_ANALYSIS
#define ereport(elevel, rest) \
(errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO) ? \
(errfinish rest) : (void) 0), (elevel >= ERROR ? exit(0) : 0)
#else
/* Normal def */
#endif

The actual code never gets executed but it would give gcc and any other
tools the info they need to handle this situation.

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
> tool for doing 5% of the work and then sitting around waiting for someone
> else to do the other 95% so you can sue them.

Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
> #ifdef STATIC_ANALYSIS
> #define ereport(elevel, rest) \
> (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO) ? \
> (errfinish rest) : (void) 0), (elevel >= ERROR ? exit(0) : 0)
> #else
> /* Normal def */
> #endif

Hmm, neat idea ... though I wonder whether either gcc or Coverity's tool
is smart enough to draw the right conclusions from a conditional exit()
call ...

regards, tom lane

On Tue, Mar 07, 2006 at 05:39:18PM -0500, Tom Lane wrote:
> Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
> > #ifdef STATIC_ANALYSIS
> > #define ereport(elevel, rest) \
> > (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO) ? \
> > (errfinish rest) : (void) 0), (elevel >= ERROR ? exit(0) : 0)
> > #else
> > /* Normal def */
> > #endif
>
> Hmm, neat idea ... though I wonder whether either gcc or Coverity's tool
> is smart enough to draw the right conclusions from a conditional exit()
> call ...

Well, remember this is a macro so the conditional is known at compile
time and the optimiser should see that the exit is unconditional. A
quick test with the attached program shows that gcc does correctly
determine that the last few lines are unreachable and are optimised out
entirely (with -Wunreachable-code which is not the default).

I tried to create an empty static inline function with
attribute((noreturn)) to optimise out the call to exit(), but gcc
merely points out the function does actually return and proceeds to
assume that the rest of main() is also reachable.

Another possibility would be to create two versions of errfinish, one
marked (noreturn), and use a conditional on elevel to decide which to
use. However, then you get issues with multiple evaluation of macro
arguments...

gcc 3.3.5
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
> tool for doing 5% of the work and then sitting around waiting for someone
> else to do the other 95% so you can sue them.

Martijn van Oosterhout wrote:
> On Tue, Mar 07, 2006 at 05:39:18PM -0500, Tom Lane wrote:
>
>>Martijn van Oosterhout <kleptog(at)svana(dot)org> writes:
>>
>>>#ifdef STATIC_ANALYSIS
>>>#define ereport(elevel, rest) \
>>> (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO) ? \
>>> (errfinish rest) : (void) 0), (elevel >= ERROR ? exit(0) : 0)
>>>#else
>>>/* Normal def */
>>>#endif
>>
>>Hmm, neat idea ... though I wonder whether either gcc or Coverity's tool
>>is smart enough to draw the right conclusions from a conditional exit()
>>call ...
>

As for Coverity, if the elevel that's passed to the ereport is really a
constant, the above #ifdef should absolutely do the trick for us so we
know to stop analyzing on that path...Let me know if it doesn't actually
do that ;)

-ben

Folks,
> As for Coverity, if the elevel that's passed to the ereport is really a
> constant, the above #ifdef should absolutely do the trick for us so we
> know to stop analyzing on that path...Let me know if it doesn't actually
> do that ;)

Um, I think the answer is to train Coverity, not change our code to avoid
the false-positives. I know that Coverity is sophisticated enough to, for
example, be programed to understand that elog(ERROR) does not continue.

Actually, I thougth that Neil/eDB did this with their copy. Is there any
way to get a copy of that "training configuration"?

--
--Josh

Josh Berkus
Aglio Database Solutions
San Francisco

On 3/8/06, Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>
> Actually, I thougth that Neil/eDB did this with their copy. Is there any
> way to get a copy of that "training configuration"?

I think we have a backup of it somewhere. I'll look into it.

--
Jonah H. Harris, Database Internals Architect
EnterpriseDB Corporation
732.331.1324

Ben Chelf <ben(at)coverity(dot)com> writes:

> >>>#ifdef STATIC_ANALYSIS
> >>>#define ereport(elevel, rest) \
> >>> (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO) ? \
> >>> (errfinish rest) : (void) 0), (elevel >= ERROR ? exit(0) : 0)
> >>>#else
> >>>/* Normal def */
> >>>#endif
>
> As for Coverity, if the elevel that's passed to the ereport is really a
> constant, the above #ifdef should absolutely do the trick for us so we know to
> stop analyzing on that path...Let me know if it doesn't actually do that ;)

If you're willing to require elevel to always be a constant then why not just
tack on the (elevel >= ERROR ? exit(0) : 0) onto the end of the regular
definition of ereport instead of having an ifdef?

Incidentally, if it's not guaranteed to be a constant then the definition
above is wrong because it's missing parentheses around elevel at both
occurrences.

--
greg

On 3/8/06, Josh Berkus <josh ( at ) agliodbs ( dot ) com> wrote:

> Actually, I thougth that Neil/eDB did this with their copy. Is
> there any way to get a copy of that "training configuration"?

Just to jump in on this thread, we can absolutely configure elog -- if
you have the config already, great. If not, if you can just send me the
prototype/macro expansion for 'elog' and the constant values that are
passed in the case where it exits, I'll add that config. Thanks!

-ben

On Wed, Mar 08, 2006 at 06:42:45PM -0500, Greg Stark wrote:
> Ben Chelf <ben(at)coverity(dot)com> writes:
>
> > >>>#ifdef STATIC_ANALYSIS
> > >>>#define ereport(elevel, rest) \
> > >>> (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO) ? \
> > >>> (errfinish rest) : (void) 0), (elevel >= ERROR ? exit(0) : 0)
> > >>>#else
> > >>>/* Normal def */
> > >>>#endif
> >
> > As for Coverity, if the elevel that's passed to the ereport is really a
> > constant, the above #ifdef should absolutely do the trick for us so we know to
> > stop analyzing on that path...Let me know if it doesn't actually do that ;)
>
> If you're willing to require elevel to always be a constant then why not just
> tack on the (elevel >= ERROR ? exit(0) : 0) onto the end of the regular
> definition of ereport instead of having an ifdef?

Well, the only cost would be a useless call to exit() for each
elog/ereport with an elevel >= ERROR. It bloats the binary a bit. Not
sure whether people care enough about that.

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
> tool for doing 5% of the work and then sitting around waiting for someone
> else to do the other 95% so you can sue them.

Martijn van Oosterhout wrote:
-- Start of PGP signed section.
> On Wed, Mar 08, 2006 at 06:42:45PM -0500, Greg Stark wrote:
> > Ben Chelf <ben(at)coverity(dot)com> writes:
> >
> > > >>>#ifdef STATIC_ANALYSIS
> > > >>>#define ereport(elevel, rest) \
> > > >>> (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO) ? \
> > > >>> (errfinish rest) : (void) 0), (elevel >= ERROR ? exit(0) : 0)
> > > >>>#else
> > > >>>/* Normal def */
> > > >>>#endif
> > >
> > > As for Coverity, if the elevel that's passed to the ereport is really a
> > > constant, the above #ifdef should absolutely do the trick for us so we know to
> > > stop analyzing on that path...Let me know if it doesn't actually do that ;)
> >
> > If you're willing to require elevel to always be a constant then why not just
> > tack on the (elevel >= ERROR ? exit(0) : 0) onto the end of the regular
> > definition of ereport instead of having an ifdef?
>
> Well, the only cost would be a useless call to exit() for each
> elog/ereport with an elevel >= ERROR. It bloats the binary a bit. Not
> sure whether people care enough about that.

We care. :-)

--
Bruce Momjian http://candle.pha.pa.us
SRA OSS, Inc. http://www.sraoss.com

+ If your life is a hard drive, Christ can be your backup. +

On Thu, 9 Mar 2006, Bruce Momjian wrote:

> Martijn van Oosterhout wrote:
> -- Start of PGP signed section.
>> On Wed, Mar 08, 2006 at 06:42:45PM -0500, Greg Stark wrote:
>>> Ben Chelf <ben(at)coverity(dot)com> writes:
>>>
>>>>>>> #ifdef STATIC_ANALYSIS
>>>>>>> #define ereport(elevel, rest) \
>>>>>>> (errstart(elevel, __FILE__, __LINE__, PG_FUNCNAME_MACRO) ? \
>>>>>>> (errfinish rest) : (void) 0), (elevel >= ERROR ? exit(0) : 0)
>>>>>>> #else
>>>>>>> /* Normal def */
>>>>>>> #endif
>>>>
>>>> As for Coverity, if the elevel that's passed to the ereport is really a
>>>> constant, the above #ifdef should absolutely do the trick for us so we know to
>>>> stop analyzing on that path...Let me know if it doesn't actually do that ;)
>>>
>>> If you're willing to require elevel to always be a constant then why not just
>>> tack on the (elevel >= ERROR ? exit(0) : 0) onto the end of the regular
>>> definition of ereport instead of having an ifdef?
>>
>> Well, the only cost would be a useless call to exit() for each
>> elog/ereport with an elevel >= ERROR. It bloats the binary a bit. Not
>> sure whether people care enough about that.
>
> We care. :-)

Why? I don't think we are able to run 'embedded' now as it is, so its not
like we're dealign with system with small disk spaces :) how much bigger
would adding that exit() make the binary? Martijn, could you do a build
with/without it and compare sizes?

----
Marc G. Fournier Hub.Org Networking Services (http://www.hub.org)
Email: scrappy(at)hub(dot)org Yahoo!: yscrappy ICQ: 7615664

"Marc G. Fournier" <scrappy(at)postgresql(dot)org> writes:
> Why? I don't think we are able to run 'embedded' now as it is, so its not
> like we're dealign with system with small disk spaces :) how much bigger
> would adding that exit() make the binary?

It's not only the exit(), as the elevel parameter isn't always a
constant. The proposed patch would at a minimum expose us to
double-evaluation risks. I kinda doubt there are any cases where an
elevel parameter expression has side-effects, so that objection may be
mostly hypothetical, but nonetheless we are talking about more than just
wasting a few bytes. It's not impossible that the patch would introduce
outright bugs. Consider something like

/* ENOENT is expected, anything else is not */
elog(errno == ENOENT ? DEBUG : ERROR, ...)

By the time control comes back from elog, errno would likely be
different, and so this would result in an unexpected exit() call
if the patch is in place. I'd be the first to call the above poor
coding, but it wouldn't be a bug ... unless the errno is rechecked.

It's been asserted that Coverity can be taught to understand about
elog/ereport without this sort of hack, so I'd rather take that tack.

regards, tom lane

Tom Lane wrote:
> "Marc G. Fournier" <scrappy(at)postgresql(dot)org> writes:
> > Why? I don't think we are able to run 'embedded' now as it is, so its not
> > like we're dealign with system with small disk spaces :) how much bigger
> > would adding that exit() make the binary?
>
> It's not only the exit(), as the elevel parameter isn't always a
> constant. The proposed patch would at a minimum expose us to
> double-evaluation risks. I kinda doubt there are any cases where an
> elevel parameter expression has side-effects, so that objection may be
> mostly hypothetical, but nonetheless we are talking about more than just
> wasting a few bytes. It's not impossible that the patch would introduce
> outright bugs. Consider something like
>
> /* ENOENT is expected, anything else is not */
> elog(errno == ENOENT ? DEBUG : ERROR, ...)
>
> By the time control comes back from elog, errno would likely be
> different, and so this would result in an unexpected exit() call
> if the patch is in place. I'd be the first to call the above poor
> coding, but it wouldn't be a bug ... unless the errno is rechecked.
>
> It's been asserted that Coverity can be taught to understand about
> elog/ereport without this sort of hack, so I'd rather take that tack.

Agreed. The idea of modifying our binary in any way to help a sanity
tool not complain is totally backwards.

--
Bruce Momjian http://candle.pha.pa.us
SRA OSS, Inc. http://www.sraoss.com

+ If your life is a hard drive, Christ can be your backup. +

* Bruce Momjian (pgman(at)candle(dot)pha(dot)pa(dot)us) wrote:
> > It's been asserted that Coverity can be taught to understand about
> > elog/ereport without this sort of hack, so I'd rather take that tack.
>
> Agreed. The idea of modifying our binary in any way to help a sanity
> tool not complain is totally backwards.

This is very amusing. I have to agree w/ Tom in general, the code in
this case does the right thing and the Coverity tool should be able to
be told about that. However, for areas where the tool is actually right
and there's some bug in Postgres, well, I'd hope we'd modify Postgres to
fix the bug... ;)

Enjoy,

Stephen

On Thu, 9 Mar 2006, Tom Lane wrote:

> "Marc G. Fournier" <scrappy(at)postgresql(dot)org> writes:
>> Why? I don't think we are able to run 'embedded' now as it is, so its not
>> like we're dealign with system with small disk spaces :) how much bigger
>> would adding that exit() make the binary?
>
> It's not only the exit(), as the elevel parameter isn't always a
> constant. The proposed patch would at a minimum expose us to
> double-evaluation risks. I kinda doubt there are any cases where an
> elevel parameter expression has side-effects, so that objection may be
> mostly hypothetical, but nonetheless we are talking about more than just
> wasting a few bytes. It's not impossible that the patch would introduce
> outright bugs. Consider something like
>
> /* ENOENT is expected, anything else is not */
> elog(errno == ENOENT ? DEBUG : ERROR, ...)
>
> By the time control comes back from elog, errno would likely be
> different, and so this would result in an unexpected exit() call
> if the patch is in place. I'd be the first to call the above poor
> coding, but it wouldn't be a bug ... unless the errno is rechecked.
>
> It's been asserted that Coverity can be taught to understand about
> elog/ereport without this sort of hack, so I'd rather take that tack.

I realize that this might sound 'odd' ... but, would it maybe make sense
to document the code around stuff like this as to why we do it the way we
do? Basically, we're debating how we could change the code to clean
things up for Coverity's analysis, and by the fact that we're getting both
sides of the discussion, there are ppl that think that the code
could/should be changed ... the arguments against make sense, but instead
of coming back to revisit this some time in the future, documenting it in
the code as to why we are doing it this way in the first place might save
time?

----
Marc G. Fournier Hub.Org Networking Services (http://www.hub.org)
Email: scrappy(at)hub(dot)org Yahoo!: yscrappy ICQ: 7615664

On Wed, Mar 08, 2006 at 10:34:38PM -0800, Ben Chelf wrote:
> On 3/8/06, Josh Berkus <josh ( at ) agliodbs ( dot ) com> wrote:
>
> > Actually, I thougth that Neil/eDB did this with their copy. Is
> > there any way to get a copy of that "training configuration"?
>
>
> Just to jump in on this thread, we can absolutely configure elog -- if
> you have the config already, great. If not, if you can just send me the
> prototype/macro expansion for 'elog' and the constant values that are
> passed in the case where it exits, I'll add that config. Thanks!

I don't know it anyone has responded to this but it works as follows.
the actual source can be seen here:

http://developer.postgresql.org/cvsweb.cgi/pgsql/src/include/utils/elog.h?rev=1.82

elog expands to elog_finish, which doesn't return if the first argument
is >= ERROR (which is the number 20).

ereport(X) is more complex. You want the first argument of that but it
expands to something similar to:

errstart(X), errfinish()

if X >= ERROR in the call to errstart, errfinish doesn't return.

Hope this helps,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a
> tool for doing 5% of the work and then sitting around waiting for someone
> else to do the other 95% so you can sue them.