On Wed, Mar 08, 2006 at 06:42:45PM -0500, Greg Stark wrote:
> Ben Chelf <ben@coverity.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@svana.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.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFED/1XIB7bNG8LQkwRApV6AJ41JMnOVIHMFISeYrdOU7Xh71zDrQCf T3QI
y4g60xEe6DB9rKbuzH8yI9o=
=RVP8
-----END PGP SIGNATURE-----

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

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

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@coverity.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@hub.org Yahoo!: yscrappy ICQ: 7615664

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

"Marc G. Fournier" <scrappy@postgresql.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

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster

Tom Lane wrote:
> "Marc G. Fournier" <scrappy@postgresql.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. +

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings

* Bruce Momjian (pgman@candle.pha.pa.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

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2 (GNU/Linux)

iD8DBQFEEKMcrzgMPqB3kigRAoOrAJ4nyuZI38d9jH8xjD89TP c9F0dLxwCgk6DQ
yaSyzU2BDIzNWr1sQ7FbV6A=
=QA3K
-----END PGP SIGNATURE-----

On Thu, 9 Mar 2006, Tom Lane wrote:

> "Marc G. Fournier" <scrappy@postgresql.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@hub.org Yahoo!: yscrappy ICQ: 7615664

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

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/cvsw...log.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@svana.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.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFEEU61IB7bNG8LQkwRAs6cAJ9wrv9t4cDXTZLN1WNQcQ qyR+LZlACeLNPC
qGmmm0eoNHBCZYKy1B47iDs=
=9LcA
-----END PGP SIGNATURE-----