new compiler warnings

On tis, 2011-10-18 at 01:00 -0700, Jeff Davis wrote:

I'm not sure if these can/should be fixed or not, but here are the
compiler warnings I'm getting on gcc and clang on ubuntu 11.10 with -O2.
The gcc ones are mostly new.

They are expected with gcc 4.6. There isn't anything we can do about
them.

The clang warnings are also expected. I understand the next clang
version will address them.

Jeff Davis wrote:

I'm not sure if these can/should be fixed or not, but here are the
compiler warnings I'm getting on gcc and clang on ubuntu 11.10 with
-O2.

elog.c: In function ‘write_pipe_chunks’:
elog.c:2479:8: warning: ignoring return value of ‘write’, declared
with attribute warn_unused_result [-Wunused-result]
elog.c:2488:7: warning: ignoring return value of ‘write’, declared
with attribute warn_unused_result [-Wunused-result]
elog.c: In function ‘write_console’:
elog.c:1797:7: warning: ignoring return value of ‘write’, declared
with attribute warn_unused_result [-Wunused-result]

common.c: In function ‘handle_sigint’:
common.c:247:4: warning: ignoring return value of ‘write’, declared
with attribute warn_unused_result [-Wunused-result]
common.c:250:4: warning: ignoring return value of ‘write’, declared
with attribute warn_unused_result [-Wunused-result]
common.c:251:4: warning: ignoring return value of ‘write’, declared
with attribute warn_unused_result [-Wunused-result]
In file included from mainloop.c:425:0:

These we are getting only because of a stubborn insistence on coding
to the current implementation rather than the API. It's not that
much code to code to the API instead. I've already offered to
provide the (trivial) patch for this if there is buy-in on the idea
of coding to the API.

The argument against is that no implementer of the API would ever
exercise the freedom the documented API gives them to do *part* of a
write to disk and return to the caller the number of bytes written
and then allow a subsequent write request to continue the output. I
think that the rise of virtual machine environments in big shops
provides a fairly obvious reason someone might want to do that.

-Kevin

On Tue, Oct 18, 2011 at 9:03 AM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:

Jeff Davis  wrote:

I'm not sure if these can/should be fixed or not, but here are the
compiler warnings I'm getting on gcc and clang on ubuntu 11.10 with
-O2.

elog.c: In function ‘write_pipe_chunks’:
elog.c:2479:8: warning: ignoring return value of ‘write’, declared
with attribute warn_unused_result [-Wunused-result]
elog.c:2488:7: warning: ignoring return value of ‘write’, declared
with attribute warn_unused_result [-Wunused-result]
elog.c: In function ‘write_console’:
elog.c:1797:7: warning: ignoring return value of ‘write’, declared
with attribute warn_unused_result [-Wunused-result]

common.c: In function ‘handle_sigint’:
common.c:247:4: warning: ignoring return value of ‘write’, declared
with attribute warn_unused_result [-Wunused-result]
common.c:250:4: warning: ignoring return value of ‘write’, declared
with attribute warn_unused_result [-Wunused-result]
common.c:251:4: warning: ignoring return value of ‘write’, declared
with attribute warn_unused_result [-Wunused-result]
In file included from mainloop.c:425:0:

These we are getting only because of a stubborn insistence on coding
to the current implementation rather than the API.  It's not that
much code to code to the API instead.  I've already offered to
provide the (trivial) patch for this if there is buy-in on the idea
of coding to the API.

The argument against is that no implementer of the API would ever
exercise the freedom the documented API gives them to do *part* of a
write to disk and return to the caller the number of bytes written
and then allow a subsequent write request to continue the output.  I
think that the rise of virtual machine environments in big shops
provides a fairly obvious reason someone might want to do that.

Even if all we got out of it was that the compiler warnings went away,
I think that would still be a sufficient reason to do it. And I tend
to agree with you that the warnings are legit; and defending against
them is virtually free.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 10/18/2011 09:03 AM, Kevin Grittner wrote:

Jeff Davis wrote:

I'm not sure if these can/should be fixed or not, but here are the
compiler warnings I'm getting on gcc and clang on ubuntu 11.10 with
-O2.

elog.c: In function ‘write_pipe_chunks’:
elog.c:2479:8: warning: ignoring return value of ‘write’, declared
with attribute warn_unused_result [-Wunused-result]
elog.c:2488:7: warning: ignoring return value of ‘write’, declared
with attribute warn_unused_result [-Wunused-result]
elog.c: In function ‘write_console’:
elog.c:1797:7: warning: ignoring return value of ‘write’, declared
with attribute warn_unused_result [-Wunused-result]

common.c: In function ‘handle_sigint’:
common.c:247:4: warning: ignoring return value of ‘write’, declared
with attribute warn_unused_result [-Wunused-result]
common.c:250:4: warning: ignoring return value of ‘write’, declared
with attribute warn_unused_result [-Wunused-result]
common.c:251:4: warning: ignoring return value of ‘write’, declared
with attribute warn_unused_result [-Wunused-result]
In file included from mainloop.c:425:0:

These we are getting only because of a stubborn insistence on coding
to the current implementation rather than the API. It's not that
much code to code to the API instead. I've already offered to
provide the (trivial) patch for this if there is buy-in on the idea
of coding to the API.

The argument against is that no implementer of the API would ever
exercise the freedom the documented API gives them to do *part* of a
write to disk and return to the caller the number of bytes written
and then allow a subsequent write request to continue the output. I
think that the rise of virtual machine environments in big shops
provides a fairly obvious reason someone might want to do that.

There are non-disk uses of write() where partial writes are legitimate
(e.g. pipes under some circumstances on Linux).

It is a pity we can't just tell the compiler to turn off the warning in
a particular case.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

It is a pity we can't just tell the compiler to turn off the warning in
a particular case.

I haven't tested, but won't an explicit cast to void silence the
warning?

(void) fwrite(...);

There are places, notably the calls in elog.c, where ignoring write
failures is the right thing. I think that what Kevin was on about
was something else entirely, namely whether we need to retry writes
to disk. I would hope that we're not simply not bothering to check
in any cases where it matters.

regards, tom lane

Peter Eisentraut <peter_e@gmx.net> writes:

On tis, 2011-10-18 at 01:00 -0700, Jeff Davis wrote:

I'm not sure if these can/should be fixed or not, but here are the
compiler warnings I'm getting on gcc and clang on ubuntu 11.10 with -O2.
The gcc ones are mostly new.

They are expected with gcc 4.6. There isn't anything we can do about
them.

Well, we're going to have to think of something, because as more of us
move onto the newer gcc releases the annoyance level is going to become
intolerable.

I think a large fraction of the -Waddress warnings are coming from
this line in the heap_getattr macro:

AssertMacro((tup) != NULL), \

Seems to me we could just lose that test and be no worse off, since
the macro is surely gonna dump core anyway on a null pointer.

But some of the remaining -Waddress warnings are not so painless to
get rid of. Ultimately we might have to add -Wno-address to the
default CFLAGS.

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think that what Kevin was on about was something else entirely,
namely whether we need to retry writes to disk.

I would phrase it that we need to *continue* a write to disk if the
OS chooses to write a portion of it and return to the caller with
the number actually written. If the first write, or any later
write, actually gets an error or fails to make progress, *then* we
should consider the attempt to be done. I don't understand the
point of not coding to the API.

-Kevin

On tis, 2011-10-18 at 09:32 -0400, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

It is a pity we can't just tell the compiler to turn off the warning in
a particular case.

I haven't tested, but won't an explicit cast to void silence the
warning?

(void) fwrite(...);

No, tried that already. You could try

rc = write(...);
(void) rc;

There are places, notably the calls in elog.c, where ignoring write
failures is the right thing. I think that what Kevin was on about
was something else entirely, namely whether we need to retry writes
to disk. I would hope that we're not simply not bothering to check
in any cases where it matters.

No, I believe we are OK everywhere else. We are only ignoring the
result in cases where we are trying to report errors in the first place.

On Tue, Oct 18, 2011 at 10:06 AM, Peter Eisentraut <peter_e@gmx.net> wrote:

No, I believe we are OK everywhere else.  We are only ignoring the
result in cases where we are trying to report errors in the first place.

The relevant code is:

while (len > PIPE_MAX_PAYLOAD)
{
p.proto.is_last = (dest == LOG_DESTINATION_CSVLOG ? 'F' : 'f');
p.proto.len = PIPE_MAX_PAYLOAD;
memcpy(p.proto.data, data, PIPE_MAX_PAYLOAD);
write(fd, &p, PIPE_HEADER_SIZE + PIPE_MAX_PAYLOAD);
data += PIPE_MAX_PAYLOAD;
len -= PIPE_MAX_PAYLOAD;
}

Which it seems to me we could change by doing rc = write(). Then if
rc <= 0, we bail out. If not, we add and subtract rc, rather than
PIPE_MAX_PAYLOAD. That would be barely more code, probably safer, and
would silence the warning.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> wrote:

Which it seems to me we could change by doing rc = write(). Then
if rc <= 0, we bail out. If not, we add and subtract rc, rather
than PIPE_MAX_PAYLOAD.

Something along the general lines of this?:

http://archives.postgresql.org/pgsql-hackers/2011-02/msg01719.php

That would be barely more code, probably safer, and would silence
the warning.

Exactly.

-Kevin

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> wrote:

http://archives.postgresql.org/pgsql-hackers/2011-02/msg01719.php

Although, it being a quick example of the general idea, I have an
obvious bug there -- the write location would have to be "buffer +
t".

I think Noah might have also posted some example code a month or two
back.

-Kevin

Robert Haas <robertmhaas@gmail.com> writes:

On Tue, Oct 18, 2011 at 10:06 AM, Peter Eisentraut <peter_e@gmx.net> wrote:

No, I believe we are OK everywhere else. �We are only ignoring the
result in cases where we are trying to report errors in the first place.

The relevant code is:

while (len > PIPE_MAX_PAYLOAD)
{
p.proto.is_last = (dest == LOG_DESTINATION_CSVLOG ? 'F' : 'f');
p.proto.len = PIPE_MAX_PAYLOAD;
memcpy(p.proto.data, data, PIPE_MAX_PAYLOAD);
write(fd, &p, PIPE_HEADER_SIZE + PIPE_MAX_PAYLOAD);
data += PIPE_MAX_PAYLOAD;
len -= PIPE_MAX_PAYLOAD;
}

Which it seems to me we could change by doing rc = write(). Then if
rc <= 0, we bail out. If not, we add and subtract rc, rather than
PIPE_MAX_PAYLOAD. That would be barely more code, probably safer, and
would silence the warning.

And it would break the code. The whole point here is that the message
must be sent indivisibly.

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> wrote:

And it would break the code. The whole point here is that the
message must be sent indivisibly.

If the new code splits the message, it would previously have been
truncated. Is that less broken?

-Kevin

On Tue, Oct 18, 2011 at 12:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Tue, Oct 18, 2011 at 10:06 AM, Peter Eisentraut <peter_e@gmx.net> wrote:

No, I believe we are OK everywhere else.  We are only ignoring the
result in cases where we are trying to report errors in the first place.

The relevant code is:

    while (len > PIPE_MAX_PAYLOAD)
    {
        p.proto.is_last = (dest == LOG_DESTINATION_CSVLOG ? 'F' : 'f');
        p.proto.len = PIPE_MAX_PAYLOAD;
        memcpy(p.proto.data, data, PIPE_MAX_PAYLOAD);
        write(fd, &p, PIPE_HEADER_SIZE + PIPE_MAX_PAYLOAD);
        data += PIPE_MAX_PAYLOAD;
        len -= PIPE_MAX_PAYLOAD;
    }

Which it seems to me we could change by doing rc = write().  Then if
rc <= 0, we bail out.  If not, we add and subtract rc, rather than
PIPE_MAX_PAYLOAD.  That would be barely more code, probably safer, and
would silence the warning.

And it would break the code.  The whole point here is that the message
must be sent indivisibly.

How is that different than the chunking that the while loop is already doing?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:

On Tue, Oct 18, 2011 at 12:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

And it would break the code. �The whole point here is that the message
must be sent indivisibly.

How is that different than the chunking that the while loop is already doing?

The chunks are sent indivisibly, because they are less than the pipe
buffer size. Read the pipe man page. It's guaranteed that the write
will either succeed or fail as a whole, not write a partial message.
If we cared to retry a failure, there would be some point in checking
the return code.

regards, tom lane

On Tue, Oct 18, 2011 at 1:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Tue, Oct 18, 2011 at 12:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

And it would break the code.  The whole point here is that the message
must be sent indivisibly.

How is that different than the chunking that the while loop is already doing?

The chunks are sent indivisibly, because they are less than the pipe
buffer size.  Read the pipe man page.  It's guaranteed that the write
will either succeed or fail as a whole, not write a partial message.
If we cared to retry a failure, there would be some point in checking
the return code.

On MacOS X v10.6.8, I see no such guarantee in the pipe(2) man page.
The man page for read(2) says:

Upon successful completion, read(), readv(), and pread() return
the number of
bytes actually read and placed in the buffer. The system
guarantees to read the
number of bytes requested if the descriptor references a normal
file that has
that many bytes left before the end-of-file, but in no other case.

In any event, whether or not it's *possible* to have a short read is a
separate question from what we should do if it happens. Retrying an
error doesn't seem practical, because in all likelihood the error will
recur forever and we'll go into an infinite loop. But if we do
somehow get a short write, sending the rest of the current chunk in
the next write() does not seem materially worse than sending the next
chunk.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think that what Kevin was on about was something else entirely,
namely whether we need to retry writes to disk.

I would phrase it that we need to *continue* a write to disk if the
OS chooses to write a portion of it and return to the caller with
the number actually written. If the first write, or any later
write, actually gets an error or fails to make progress, *then* we
should consider the attempt to be done. I don't understand the
point of not coding to the API.

My point here is just that that's a different discussion. You are not
talking about places where this new compiler warning is getting raised.

regards, tom lane

On tis, 2011-10-18 at 09:36 -0400, Tom Lane wrote:

But some of the remaining -Waddress warnings are not so painless to
get rid of. Ultimately we might have to add -Wno-address to the
default CFLAGS.

Here is the bug report to gcc on this issue:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48778

FWIW, I've been building with -Wno-error=address for months, ever since
gcc 4.6 because the default on my machine. I don't know what other
issues one might be missing that way.

Robert Haas <robertmhaas@gmail.com> writes:

On Tue, Oct 18, 2011 at 1:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

The chunks are sent indivisibly, because they are less than the pipe
buffer size. �Read the pipe man page. �It's guaranteed that the write
will either succeed or fail as a whole, not write a partial message.
If we cared to retry a failure, there would be some point in checking
the return code.

On MacOS X v10.6.8, I see no such guarantee in the pipe(2) man page.

Sorry, maybe write(2) is the place to look. The Single Unix Spec quoth
(at http://pubs.opengroup.org/onlinepubs/9699919799/):

Write requests to a pipe or FIFO shall be handled in the same
way as a regular file with the following exceptions:

There is no file offset associated with a pipe, hence each write
request shall append to the end of the pipe.

Write requests of {PIPE_BUF} bytes or less shall not be
interleaved with data from other processes doing writes on the
same pipe. Writes of greater than {PIPE_BUF} bytes may have data
interleaved, on arbitrary boundaries, with writes by other
processes, whether or not the O_NONBLOCK flag of the file status
flags is set.

If the O_NONBLOCK flag is clear, a write request may cause the
thread to block, but on normal completion it shall return nbyte.

Note the last in particular. Short writes are specifically disallowed
on pipes.

If this were not the case, the logging collector protocol would be
useless.

regards, tom lane