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

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