Re: faulty error handling around pgstat_count_io_op_time()

Hi,

On Wed, 17 Jun 2026 at 05:56, Michael Paquier <michael(at)paquier(dot)xyz> wrote:

> On Mon, Jun 15, 2026 at 06:26:00AM +0000, Bertrand Drouvot wrote:
> > On Fri, Jun 12, 2026 at 06:01:45PM +0200, Peter Eisentraut wrote:
>
>
> >> 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.
>

The "if (r > 0)" guard to keep counting short reads in xlogrecovery.c,
looks correct.

In the xlogrecovery.c case, we should care about the short read case.
> What you are doing here looks OK for this path.
>
> In XLogFileInitInternal(), the first pgstat_count_io_op_time() is not
> completely right, no? pg_pwrite_zeros() or pg_pwrite() could fail,
> and it does not make sense to me to count data if we have a
> save_errno, and the files are unlinked in the error path. I'd propose
> to delay the count() call to happen after the error check is done.
>

I agree on this change, but the original placement was also recording
the I/O timing of the attempted write, not just byte count, so moving
it post save_errno drops that. (But even the ordinary write/fsync
paths ereport before reaching their pgstat_count_io_op_time() call).

Regards,
Ayush