Re: EXPLAIN: showing ReadStream / prefetch stats
Tomas Vondra <tomas@vondra.me>
From: Tomas Vondra <tomas@vondra.me>
To: Andres Freund <andres@anarazel.de>
Cc: Lukas Fittl <lukas@fittl.com>,
Melanie Plageman <melanieplageman@gmail.com>,
PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2026-03-31T13:16:57Z
Lists: pgsql-hackers
Attachments
- v6-0005-show-prefetch-stats-for-TidRangeScan.patch (text/x-patch) patch v6-0005
- v6-0004-show-prefetch-stats-for-SeqScan.patch (text/x-patch) patch v6-0004
- v6-0003-explain-show-prefetch-stats-in-EXPLAIN-ANALYZE.patch (text/x-patch) patch v6-0003
- v6-0002-switch-explain-to-unaligned-for-json-xml-yaml.patch (text/x-patch) patch v6-0002
- v6-0001-bufmgr-Return-whether-WaitReadBuffers-needed-to-w.patch (text/x-patch) patch v6-0001
Here's v6, addressing most of the review comments. Most of this is pretty simple / non-contentious. The remaining questions are in the last two items (phs_len field, initializing the instrumentation pointers): - 0001 tweaks WaitReadBuffers to return if a wait was needed. As pointed out, this does not include some changes needed for io_uring, I don't want to make this larger. So test with "worker". - 0002 switches a couple explain tests to unaligned output. This makes it easier to understand output changes with yaml/json/xml output. - I kept the three scans in separate changes. It makes it easier to review etc. and to me it seems easier to review. I'd keep it like this for commit, but if someone thinks we should squash it into a single commit that's possible too. - I kept the IO option enabled by default. I think it makes sense to keep this aligned with BUFFERS, but on the other hand the explain should not be overwhelming. Various other explain options are similarly helpful, and yet we left them OFF by default. - Cleanup of the explain formatting code (merging appendString lines), and so on. One question is whether to keep printing the information only when (stats->prefetch_count > 0), especially in the non-text cases. With the three scans this is not an issue, because the read stream is initialized right away. But with index scans we delay that a bit, so I wonder if this might cause some test instability. Probably not. - I switched to %.2f format for printing the averages etc. instead of %.3f. Not a huge change, we could probably go even to %.1f ... - Replaced the ACCUMULATE_IO_STATS macro with a static inline. - Renamed "stalls" to "waits". - Fixed a couple obsolete comments, and the bug in dereferencing NULL when the instrumentation was not initialized. - Added the pscan_len length (returned by table_parallelscan_estimate) to ParallelTableScanDesc, so that the workers can use this to calculate the pointer of shared instrumentation without calling the _estimate again (which I agree seems wrong and grotty). This only matters for SeqScan and TidRangeScan, and it makes it quite a bit cleaner. I don't see a better way. Right now the nodes set the new field right after calling shm_toc_allocate, but I suppose it'd be more correct to pass it to table_parallelscan_initialize() and set it from there. Barring objections I'll adjust this for v7. - I still don't understand how could the leader calculate the instrumentation pointers for workers, so it's done the same way as in BHS (except that we need to use the new phs_len field to calculate the initial offset, of course). regards -- Tomas Vondra
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
bufmgr: Return whether WaitReadBuffers() needed to wait
- 513374a47a71 19 (unreleased) landed