Hi, On Wed, 17 Jun 2026 at 05:56, Michael Paquier <[email protected]> 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
