[9.0beta5/cvs head] build failure due to unchecked results

On fre, 2010-04-30 at 12:55 +0200, Martin Pitt wrote:

Hello PostgreSQL developers,

9.0beta5 seems to enable -Werror by default (which is a good thing,
thanks!).

You probably mean alpha5, unless you come from the future. ;-) That was
actually a mistake in the packaging, which is why there is a -revised
tarball available.

I suggest you hold off for a day or two and wait for beta1.

Peter Eisentraut [2010-04-30 14:56 +0300]:

You probably mean alpha5, unless you come from the future. ;-)

FYI, those are next week's lottery numbers: 12, 19, ...

Right, of course I mean alpha-5, sorry.

That was actually a mistake in the packaging

Oh, I see. Well, for a mistake the code is surprisingly well-behaved.
Those three or four patch hunks is all it takes to make it build, and
sometimes those warnings are indeed useful; if for nothing else, then
to make you think explicitly why it's okay to ignore a short or failed
write().

I suggest you hold off for a day or two and wait for beta1.

Sure, thank you!

Martin
--
Martin Pitt | http://www.piware.de
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)

On Friday 30 April 2010 13:56:11 Peter Eisentraut wrote:

On fre, 2010-04-30 at 12:55 +0200, Martin Pitt wrote:

Hello PostgreSQL developers,

9.0beta5 seems to enable -Werror by default (which is a good thing,
thanks!).

You probably mean alpha5, unless you come from the future. ;-) That was
actually a mistake in the packaging, which is why there is a -revised
tarball available.

Isnt it a good idea to set this in the pre-release versions? It might catch
potential problems and people trying to compile unreleased software from
source should be able to change it/report a bug if necessary.

I wouldnt think of considering it for a release version, but thats a different
thing (as you will very likely use it with newer compilers and stuff)

Andres

On fre, 2010-04-30 at 14:43 +0200, Andres Freund wrote:

On Friday 30 April 2010 13:56:11 Peter Eisentraut wrote:

You probably mean alpha5, unless you come from the future. ;-) That was
actually a mistake in the packaging, which is why there is a -revised
tarball available.

Isnt it a good idea to set this in the pre-release versions? It might catch
potential problems and people trying to compile unreleased software from
source should be able to change it/report a bug if necessary.

In practice it might prevent more people from building and thus testing
PostgreSQL than it would catch actually interesting problems. You also
can't assume that the same warning-freeness applies to all platforms.

On Fri, Apr 30, 2010 at 9:14 AM, Peter Eisentraut <peter_e@gmx.net> wrote:

On fre, 2010-04-30 at 14:43 +0200, Andres Freund wrote:

On Friday 30 April 2010 13:56:11 Peter Eisentraut wrote:

You probably mean alpha5, unless you come from the future. ;-)  That was
actually a mistake in the packaging, which is why there is a -revised
tarball available.

Isnt it a good idea to set this in the pre-release versions? It might catch
potential problems and people trying to compile unreleased software from
source should be able to change it/report a bug if necessary.

In practice it might prevent more people from building and thus testing
PostgreSQL than it would catch actually interesting problems.  You also
can't assume that the same warning-freeness applies to all platforms.

Yeah, that's the real problem - one doesn't get the same warnings
everywhere. Still, I think we should consider applying the portion of
the proposed patch that avoid relying on the contents of the fgets()
buffer after fgets() returns NULL, because at least on MacOS the man
page for gets/fgets says:

If an error occurs, they return NULL and the buffer contents are indeterminate.

Indeterminate is bad. The actual chance of a crash seems low, and I
notice that the Linux man page has no such caveat, but still.

...Robert

Robert Haas <robertmhaas@gmail.com> writes:

Still, I think we should consider applying the portion of
the proposed patch that avoid relying on the contents of the fgets()
buffer after fgets() returns NULL,

I concur, those two changes look worthwhile. The proposed Assert()
additions are right out, though, as they would turn write failures
into database crashes. The current code doesn't even think that such
a failure is worth testing for, so that's surely an overreaction.
(And in any case, if Asserts are disabled, this change would fail to
suppress the warning, no?)

regards, tom lane

Tom Lane [2010-04-30 12:51 -0400]:

I concur, those two changes look worthwhile. The proposed Assert()
additions are right out, though, as they would turn write failures
into database crashes.

Right, that might be too strong.

The current code doesn't even think that such a failure is worth
testing for, so that's surely an overreaction. (And in any case, if
Asserts are disabled, this change would fail to suppress the
warning, no?)

It seems gcc is happy enough if you assign the returned value to a
variable. At least I have done a build without --enable-cassert (where
the entire Assert() was thrown away), and it didn't complain about the
unchecked result any more. I guess that heuristics gets it only so
far..

Thanks,

Martin

--
Martin Pitt | http://www.piware.de
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)