Re: pg19b1: TRAP: failed Assert("pgstat_bktype_io_stats_valid(bktype_shstats, MyBackendType)")

I tried implementing the approach Andres suggested, so that we keep
recording the foreign-IO wait time rather than dropping it.

The patch:
- adds pgstat_count_io_time() for timing-only updates (no operation count).
- switches WaitReadBuffers() to use pgstat_count_io_time().
- keeps pgstat_count_io_op_time() for counted operations, with Assert(cnt >
0).
- stops requiring a non-zero count whenever time is non-zero.
- briefly documents that per-backend IO stats may then show read_time
without matching reads.

---
Regards,
Rachitskiy Andrey

вт, 21 июл. 2026 г. в 20:03, Andres Freund <andres(at)anarazel(dot)de>:

> Hi,
>
> On 2026-07-16 15:59:12 -0400, Melanie Plageman wrote:
> > Thanks, Justin, for the report
> >
> > On Thu, Jul 16, 2026 at 11:03 AM Andrey Rachitskiy <pl0h0yp1(at)gmail(dot)com>
> wrote:
> > >
> > > With track_io_timing=on, foreign IO wait time is currently recorded:
> when WaitReadBuffers() waits on foreign_io, it calls
> pgstat_count_io_op_time() with cnt=0. So we do write non-zero IOOP_READ
> time into this backend's pending pg_stat_io stats, without incrementing the
> read count in this backend.
> > >
> > > Thus, this fix introduces a real change: it disables recording foreign
> IO wait time in pg_stat_io statistics for the backend that is performing
> the wait.
> > >
> > > What we lose is only that wait time. The initiating backend still
> records the read count and its own IO time, and the waiting backend still
> records a buffer hit once the IO completes.
> >
> > Yea, so, I don't think we can just not record the wait time for
> > foreign IOs.
>
> Agreed.
>
>
> > For asynchronous reads, read time in pg_stat_io is almost entirely wait
> > time.
>
> Right. And for some workloads a decent percentage of the waits can be due
> to
> foreign IOs (often started by ourselves). That's particularly true once
> index
> readahead goes in.
>
>
> > That's also why it doesn't make sense to have a separate IOOP_WAIT --
> unless
> > we start measuring and displaying read latency (how long the actual reads
> > took) in pg_stat_io and then you could separate that from wait time.
>
> My main problem with IOOP_WAIT would be that it would conflate waiting for
> different IO operations. Read, write, fsync latencies can be wildly
> different...
>
>
> > My first thought is that we should relax the restriction in
> > pgstat_bktype_io_stats_valid() that requires non-zero counts if time
> > is non-zero.
>
> I think that may be the way. There's other places where that assertion
> causes
> us to track less well than we otherwise could:
>
> /*
> * We must get an IO handle before StartBufferIO(), as
> pgaio_io_acquire()
> * might block, which we don't want after setting IO_IN_PROGRESS.
> If we
> * don't need to do the IO, we'll release the handle.
> *
> * If we need to wait for IO before we can get a handle, submit
> * already-staged IO first, so that other backends don't need to
> wait.
> * There wouldn't be a deadlock risk, as pgaio_io_acquire() just
> needs to
> * wait for already submitted IO, which doesn't require additional
> locks,
> * but it could still cause undesirable waits.
> *
> * A secondary benefit is that this would allow us to measure the
> time in
> * pgaio_io_acquire() without causing undue timer overhead in the
> common,
> * non-blocking, case. However, currently the pgstats
> infrastructure
> * doesn't really allow that, as it a) asserts that an operation
> can't
> * have time without operations b) doesn't have an API to report
> * "accumulated" time.
> */
>
> The a) in the last paragraph in particular.
>
>
> > But that seems silly: 1) we lose lots of validation for cases where this
> > would be bogus
>
> But how likely is that actually going to be a problem?
>
> Perhaps we should add a separate function that explicitly just counts
> timing
> (no operation count), and only allow that one to work with a 0 operations
> count?
>
>
> > and 2) anyone looking at the view that seems non-zero IO time and 0 count
> > will be very confused
>
> But I don't think you would really see that - normally the IO count will
> have
> been increemented by another operation.
>
>
> > and 3) it makes any math dividing read time by read operations
> meaningless
> > (though I don't know how meaningful that is now).
>
> Hm. The fix for that is to write read_time / NULLIF(reads, 0). But it's
> somewhat annoying to have to know to do that, as it will be quite rare to
> encounter cases where just the wait time, but no operation count will have
> been reported. At least for pg_stat_io, I guess pg_stat_get_backend_io()
> will
> make it a bit more common.
>
> But I just don't really see an alternative.
>
>
> > If read time is truly just wait time
>
> It's not always that way. For a fast IO subsystem with a deep enough queue
> most of the time will actually be in the submission path (since that's
> where
> you trigger interrupts and do kernel work like identifying the disk block
> the
> requested data is on etc).
>
>
> > we could just pass pg_stat_count_io_op_time() a count of 1 for foreign
> IO in
> > WaitReadBuffers(). In 19, we should likely either do this or do as Andrey
> > suggested and not count the wait time for foreign IOs.
> >
> > For master, I'd like to do something more satisfying, but I'm not sure
> what.
>
> I think medium term we might need to redesign where the pg_stat_io
> accounting
> happens. Not primarily because of this, but because there's at the moment
> sources of waits that are not properly accounted.
>
> Consider e.g. this:
> - io_max_concurrency=32
> - some read stream starts 32 reads
> - another part of the query tries to do a single AIO write
> - that needs to do a blocking pgaio_io_acquire(), as there are no unused
> handles
> - as part of that, pgaio_io_acquire() will wait on the oldest in-flight
> IO,
> which was a read
>
> Today the wait time for that (as explained in the comment above) is
> unattributed. If we were to account it were the comment suggests, we'd
> attribute it to the *write*, even though the waiting is actually unrelated
> to
> the write itself, its waiting for some old read to finish.
>
> The same thing could happen with reads that fall into different contexts,
> e.g. we could end up attributing waits triggered by relation/bulkread to
> relation/normal, just because the bulkread filled up the queue.
>
>
> Now, some fuzziness here is fundamental. Having N IOs in the queue before
> an
> additional IO X will increase the latency of X. If we only wait for X, not
> for
> any of the earlier IOs, the latency of those other IOs will be attributed
> to X
> to some degree. But I feel the case of waiting for specific earlier IOs to
> complete is different, as that's not an indirect increase in latency due to
> deeper queues, it's directly waiting for other IO operations - and those
> other
> IO operations might be much higher latency ones (the most extreme case is
> probably io_max_concurrency fsyncs queued when trying to start a small
> read).
>
>
> So a larger refactoring could be to add something like
> pgstat_io_set_stats_target(), which would then allow the pgstat reporting
> to
> happen within the AIO subsystem.
>
>
> > It did get me thinking that there is 0 way for the user (or developer)
> > to know about foreign IOs. Maybe that's okay...?
>
> I think it may be OK, we historically have not exposed having to wait in
> StartBufferIO() when the buffer is undergoing IO in another backend either.
>
> It's not entirely clear what the alternative is either. One relatively easy
> thing we could do is to have more separated out wait events. E.g.:
>
> 1) a direct wait for IO started by the current backend
>
> 2) a direct wait for an IO started by another backend
>
> 3) a wait for an IO started by the current backend, but as part of
> acquiring
> an IO handle.
>
> 4) a wait as part of pgaio_closing_fd()
>
> 5) a wait as part of pgaio_shutdown()
>
> With possible subdivisions for 1), to separate out "foreign" IOs that have
> been started by the current backend from plain waits on the IO itself. But
> that's a bit of a weird division, because for the AIO subsystem there's
> zero
> difference between those cases.
>
>
> It's not entirely trivial to introduce that separation, because
> pgaio_io_wait() already has different wait events, because you may need to
> wait for the IO to actually complete (WAIT_EVENT_AIO_IO_URING_EXECUTION) or
> you may need to wait for another backend to execute the IO completion
> (WAIT_EVENT_AIO_IO_COMPLETION).
>
> Which is not helped by WAIT_EVENT_AIO_IO_COMPLETION covering different
> things
> for different IO methods. For io_uring it will just be used when the kernel
> has executed the IO, but the callback hasn't yet been executed, whereas for
> sync/worker modes, it'll be used for the entire IO duration.
>
> Greetings,
>
> Andres Freund
>