| 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-07 00:19:06 |
| Message-ID: | CAAKRu_by3fu=VsRmHjRMi9sF6Uy8mSruApyvzRm+O_=E1-ViAA@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Mon, Apr 6, 2026 at 4:39 PM Tomas Vondra <tomas(at)vondra(dot)me> wrote:
>
> On 4/6/26 18:50, Melanie Plageman wrote:
>
> > 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.
>
<--snip-->
>
> I think this is ready to go, with a tiny amount of polishing.
>
> 1) "amount of DSM" sounds a bit strange to me. The wording "amount of
> space in ..." from the other nodes seems better to me. Or maybe I'm just
> used to it, not sure.
Fixed that before pushing.
> 2) I wonder if maybe PARALLEL_KEY_SCAN_INSTRUMENT_OFFSET should be
> placed in plannodes.h, because that's where Plan->plan_node_id is
> defined. instrument_node.h works too, but the places accessing the
> plan_node_id already have to include plannodes.h.
I don't think it really belongs there. It is very specific to
execution. We use the plan node id as the key for convenience, but it
isn't the purpose of plan_node_id.
PARALLEL_KEY_SCAN_INSTRUMENT_OFFSET's only purpose is to be used
during execution for instrumentation, so it feels like it belongs in
node_instrument.h. And we don't need a separate include for it either.
It goes with the other stuff being defined in instrument_node.h (like
SharedIndexScanInstrumentation) and being used by those callers. I
admit the comment above it is a bit odd, but I think it is ultimately
okay.
> We need to decide whether to push this into PG19. This was primarily
> motivated by the index prefetching work, but we now know that won't
> happen in PG19 :-( But the instrumentation is useful even for the three
> scans using read streams, so I think it's a meaningful improvement.
I think it is a meaningful improvement too. I think we should do it.
> If you think you can get this pushed, I'll do my best to finalize the
> instrumentation and SeqScan/TidRangeScan parts.
I've pushed the first patch for index/index-only scans. Attached is
the BHS fix that uses the new pattern. It needs at least one review
before pushing. While I was polishing it, I realized I neglected to
use add_size()/mul_size() in the index-only/index scan patches. So,
0002 is just a fix commit to do that. Feel free to push these if you
think they're ready. Otherwise, I'll do so pending your review in my
morning.
- Melanie
| Attachment | Content-Type | Size |
|---|---|---|
| v11-0001-Fix-BitmapHeapScan-non-parallel-aware-EXPLAIN-AN.patch | text/x-patch | 8.8 KB |
| v11-0002-Use-add_size-mul_size-for-index-instrumentation-.patch | text/x-patch | 3.1 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Alexander Korotkov | 2026-04-07 00:19:56 | Re: Asynchronous MergeAppend |
| Previous Message | Heikki Linnakangas | 2026-04-07 00:18:05 | Re: Don't use the deprecated and insecure PQcancel in our frontend tools anymore |