| From: | Melanie Plageman <melanieplageman(at)gmail(dot)com> |
|---|---|
| To: | Tomas Vondra <tomas(at)vondra(dot)me> |
| Cc: | Andres Freund <andres(at)anarazel(dot)de>, Lukas Fittl <lukas(at)fittl(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: EXPLAIN: showing ReadStream / prefetch stats |
| Date: | 2026-04-06 16:50:58 |
| Message-ID: | CAAKRu_ZQ_yapZoMWt-WuUfwcZyw4hek81Oo7x2_yTqLodzCjCQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Mon, Apr 6, 2026 at 10:01 AM Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
>
> On Mon, Apr 6, 2026 at 5:15 AM Tomas Vondra <tomas(at)vondra(dot)me> wrote:
> >
> > Anyway, that doesn't matter much, because the more I look at the
> > approach with having a separate chunk of shared memory, the more I like
> > it. It seems much simpler, more elegant, etc. I really disliked how
> > unreadable the code got with the parallel_aware/instrument checks in
> > multiple places, and this just gets rid of that. I like that.
> >
> <--snip-->
> >
> > Is execParallel.h the right place to define the offset? It means the
> > various nodes (like nodeBitmapHeapScan) now have to include this header,
> > and it seems a bit suspicious. I can't think of a better .h file, and
> > maybe I'm wrong and it's perfectly fine.
>
> It could go in src/include/executor is instrument_node.h. It's where
> the structs for the shared instrumentation go. It's probably cheaper
> to include also because it doesn't include anything.
I cleaned up the first patch in the set that refactors index-only and
index scan to use this pattern and realized that I wasn't sure what to
do about the duplication between index-only and index scans for these
functions.
ExecIndexScanInstrumentEstimate() and
ExecIndexOnlyScanInstrumentEstimate() are the same code except they
take different node types to do the check for intsrumentation
if (!node->ss.ps.instrument || pcxt->nworkers == 0)
return;
The size estimation for SharedIndexScanIntrumentation could be common
to the two but it is only three lines and doesn't seem worth another
function call. ExecIndex[Only]ScanEstimate() use a common helper
index_parallelscan_estimate() but that is more code.
We could just have a common helper and only call it if
node->ss.ps.instrument && pcxt->nworkers > 0 but I don't see anybody
else in ExecParallelEstimate() checking for instrumentation being
enabled.
Besides that, the function names are very long already but don't
include the word parallel (though ExecIndexScanEstimate() is for
parallel scans only and doesn't include "parallel").
ExecIndex[Only]ScanInstrumentInitDSM() is the same between the two
except the node type it takes but three different members of node are
used here, so I'm not sure how much sense a common helper makes here.
I have a feeling the above stuff isn't okay how it is now, but I'm not
sure which direction to go.
I think ExecIndex[Only]ScanInstrumentInitWorker() is fine the way it
is because it is only 1 line and requires a different node for
index/index-only.
- Melanie
| Attachment | Content-Type | Size |
|---|---|---|
| v9-0001-Allocate-separate-DSM-chunk-for-parallel-Index-On.patch | text/x-patch | 23.1 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Andrew Dunstan | 2026-04-06 16:51:40 | Re: Add errdetail() with PID and UID about source of termination signal |
| Previous Message | Robert Haas | 2026-04-06 16:46:45 | Re: Add custom EXPLAIN options support to auto_explain |