There are several places where the return value of pg_pread() or
pg_pwrite() is passed directly as the byte count to
pgstat_count_io_op_time(). The bytes argument of
pgstat_count_io_op_time() is of type uint64, and so error returns of -1
are going to passed as UINT64_MAX and added as such to the internal
statistics.
In the attached patch, I have marked up those places.
I think the correction here would be to move the
pgstat_count_io_op_time() calls to after the error returns are handled.
This is effectively how most other code already behaves. For example,
most smgr calls don't return on error, so you don't get a chance to make
any pgstat calls afterwards. It's only the open-coded places where we
can even do that.
However, XLogPageRead() even goes out of its way to make an explicit
pgstat_count_io_op_time() call in the error branch. I suppose this
could be useful to record short reads, but a) this particular instance
is still faulty regarding -1, and b) other places don't do that. So
it's a bit unclear what the preferred behavior on error should be.
An alternative would be to call pgstat_count_io_op_time() with like
Max(byteswritten, 0), but that seems kind of ugly.
Another alternative would be to change the bytes argument of
pgstat_count_io_op_time() to ssize_t. POSIX file system operations
can't operate on sizes larger than ssize_t, so this type should be
sufficient. And then error returns could be handled centrally in
pgstat_count_io_op_time(). (Record them, don't record them, or even
count errors separately, etc.)
Thoughts?
