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

On Tue, Jul 21, 2026 at 11:03 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> On 2026-07-16 15:59:12 -0400, Melanie Plageman wrote:
>
> > 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...

That's a great point I hadn't thought about.

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

That makes sense. What I'm actually about to commit to 19 and master
is just to remove the restriction.

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

I don't know if that buys us anything because the validation/assertion
in question happens at flush time not when counting the IO. Adding a
separate function would make it harder for the dev to accidentally
pass 0 for count when they didn't mean to (because we could then
assert count > 0 in the current function). But you could already pass
some other bogus value for "count" with no consequences.

That being said, if we add waiting for an AIO handle to master and it
needs a way to count accumulated time (as the comment says), we'll
need a new function anyway. I don't quite understand why you would
need a different pg_stat_io interface for this case, though. It seems
like you would just get the start time before the blocking
pgaio_io_acquire() and then call pgstat_count_io_op_time() after...

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

That all sounds like a good idea for someone to do.

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

I'm interested if you found that you wanted more wait event
granularity and specificity when you are developing AIO.

- Melanie