Hi, On 2021-02-23 14:58:32 -0500, Greg Stark wrote: > So firstly this is all just awesome work and I have questions but I > don't want them to come across in any way as criticism or as a demand > for more work.
I posted it to get argued with ;). > The callbacks make me curious about two questions: > > 1) Is there a chance that a backend issues i/o, the i/o completes in > some other backend and by the time this backend gets around to looking > at the buffer it's already been overwritten again? Do we have to > initiate I/O again or have you found a way to arrange that this > backend has the buffer pinned from the time the i/o starts even though > it doesn't handle the comletion? The initiator of the IO can just keep a pin for the buffer to prevent that. There's a lot of complexity around how to handle pinning and locking around asynchronous buffer IO. I plan to send a separate email with a more details. In short: I made it so that for shared buffer IO holds a separate refcount for the duration of the IO - that way the issuer can release its own pin without causing a problem (consider e.g. an error while an IO is in flight). The pin held by the IO gets released in the completion callback. There's similar trickery with locks - but more about that later. > 2) Have you made (or considered making) things like sequential scans > (or more likely bitmap index scans) asynchronous at a higher level. > That is, issue a bunch of asynchronous i/o and then handle the pages > and return the tuples as the pages arrive. Since sequential scans and > bitmap scans don't guarantee to read the pages in order they're > generally free to return tuples from any page in any order. I'm not > sure how much of a win that would actually be since all the same i/o > would be getting executed and the savings in shared buffers would be > small but if there are mostly hot pages you could imagine interleaving > a lot of in-memory pages with the few i/os instead of sitting idle > waiting for the async i/o to return. I have not. Mostly because it'd basically break the entire regression test suite. And probably a lot of user expectations (yes, synchronize_seqscan exists, but it pretty rarely triggers). I'm not sure how big the win would be - the readahead for heapam.c that is in the patch set tries to keep ahead of the "current scan position" by a certain amount - so it'll issue the reads for the "missing" pages before they're needed. Hopefully early enough to avoid unnecessary stalls. But I'm pretty sure there'll be cases where that'd require a prohibitively long "readahead distance". I think there's a lot of interesting things along these lines that we could tackle, but since they involve changing results and/or larger changes to avoid those (e.g. detecting when sequential scan order isn't visible to the user) I think it'd make sense to separate them from the aio patchset itself. If we had the infrastructure to detect whether seqscan order matters, we could also switch to the tuple-iteration order to "backwards" in heapgetpage() - right now iterating forward in "itemid order" causes a lot of cache misses because the hardware prefetcher doesn't predict that the tuples are layed out "decreasing pointer order". > > ## Stats > > > > There are two new views: pg_stat_aios showing AIOs that are currently > > in-progress, pg_stat_aio_backends showing per-backend statistics about AIO. > > This is impressive. How easy is it to correlate with system aio stats? Could you say a bit more about what you are trying to correlate? Here's some example IOs from pg_stat_aios. ┌─[ RECORD 1 ]─┬───────────────────────────────────────────────────────────────────────────────────────────────────┐ │ backend_type │ client backend │ │ id │ 98 │ │ gen │ 13736 │ │ op │ write │ │ scb │ sb │ │ flags │ PGAIOIP_IN_PROGRESS | PGAIOIP_INFLIGHT │ │ ring │ 5 │ │ owner_pid │ 1866051 │ │ merge_with │ (null) │ │ result │ 0 │ │ desc │ fd: 38, offset: 329588736, nbytes: 8192, already_done: 0, release_lock: 1, buffid: 238576 │ ├─[ RECORD 24 ]┼───────────────────────────────────────────────────────────────────────────────────────────────┤ │ backend_type │ checkpointer │ │ id │ 1501 │ │ gen │ 15029 │ │ op │ write │ │ scb │ sb │ │ flags │ PGAIOIP_IN_PROGRESS | PGAIOIP_INFLIGHT │ │ ring │ 3 │ │ owner_pid │ 1865275 │ │ merge_with │ 1288 │ │ result │ 0 │ │ desc │ fd: 24, offset: 105136128, nbytes: 8192, already_done: 0, release_lock: 1, buffid: 202705 │ ├─[ RECORD 31 ]┼───────────────────────────────────────────────────────────────────────────────────────────────────┤ │ backend_type │ walwriter │ │ id │ 90 │ │ gen │ 26498 │ │ op │ write │ │ scb │ wal │ │ flags │ PGAIOIP_IN_PROGRESS | PGAIOIP_INFLIGHT │ │ ring │ 5 │ │ owner_pid │ 1865281 │ │ merge_with │ 181 │ │ result │ 0 │ │ desc │ write_no: 17, fd: 12, offset: 6307840, nbytes: 1048576, already_done: 0, bufdata: 0x7f94dd670000 │ └──────────────┴───────────────────────────────────────────────────────────────────────────────────────────────────┘ And the per-backend AIO view shows information like this (too wide to display all cols): SELECT pid, last_context, backend_type, executed_total_count,issued_total_count,submissions_total_count, inflight_count FROM pg_stat_aio_backends sab JOIN pg_stat_activity USING (pid); ┌─────────┬──────────────┬──────────────────────────────┬──────────────────────┬────────────────────┬─────────────────────────┬────────────────┐ │ pid │ last_context │ backend_type │ executed_total_count │ issued_total_count │ submissions_total_count │ inflight_count │ ├─────────┼──────────────┼──────────────────────────────┼──────────────────────┼────────────────────┼─────────────────────────┼────────────────┤ │ 1865291 │ 3 │ logical replication launcher │ 0 │ 0 │ 0 │ 0 │ │ 1865296 │ 0 │ client backend │ 85 │ 85 │ 85 │ 0 │ │ 1865341 │ 7 │ client backend │ 9574416 │ 2905321 │ 345642 │ 0 │ ... │ 1873501 │ 3 │ client backend │ 3565 │ 3565 │ 467 │ 10 │ ... │ 1865277 │ 0 │ background writer │ 695402 │ 575906 │ 13513 │ 0 │ │ 1865275 │ 3 │ checkpointer │ 4664110 │ 3488530 │ 1399896 │ 0 │ │ 1865281 │ 3 │ walwriter │ 77203 │ 7759 │ 7747 │ 3 │ └─────────┴──────────────┴──────────────────────────────┴──────────────────────┴────────────────────┴─────────────────────────┴────────────────┘ It's not super obvious at this point but executed_total_count / issued_total_count shows the rate at which IOs have been merged. issued_total_count / submissions_total_count shows how many (already merged) IOs were submitted together in one "submission batch" (for io_method=io_uring, io_uring_enter()). So pg_stat_aios should allow to enrich some system stats - e.g. by being able to split out WAL writes from data file writes. And - except that the current callbacks for that aren't great - it should even allow to split the IO by different relfilenodes etc. I assume pg_stat_aio_backends also can be helpful, e.g. by seeing which backends currently the deep IO queues that cause latency delays in other backends, and which backends do a lot of sequential IO (high executed_total_count / issued_total_count) and which only random... Greetings, Andres Freund