Re: faulty error handling around pgstat_count_io_op_time()

Hi,

On Fri, Jun 12, 2026 at 06:01:45PM +0200, Peter Eisentraut wrote:
> 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.

Nice catch!

> In the attached patch, I have marked up those places.

I agree with those places and did not find others.

> 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.

Sounds reasonable to me and done that way in the attached.

> 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.

What about keeping the intent (record short reads) by discarding r <= 0? (done
in the attached).

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com